Re: SMCCC 1.1 support for psci(4)

[Jonathan Gray]

On Tue, May 01, 2018 at 11:55:00AM +0200, Mark Kettenis wrote:
> So after adding a quick hack to mitigate Spectre variant 2 to ARM
> Trusted Firmware (ATF), ARM actually designed a proper solution that
> minimizes the performance loss and makes the presence of the
> workaround detectable.  This is all documented in an update of the SMC
> Calling Convention (SMCCC) standard.
> 
> The diff below implements support for this solution while keeping
> support for the hack.  While ARM strongly suggests vendors to update
> to a version of ATF that implements SMCCC 1.1 the current ATF for the
> Marvell ARMADA 8040 hasn't been updated yet (but does include the
> initial hack).
> 
> Unfortunately the SMCCC 1.1 implementation in ATF doesn't quite
> implement the spec.  As a result we have to check whether the
> workaround is implemented by issuing the relevant calls on each of the
> CPUs that might be affected.  This is important for big.LITTLE designs
> such as the RK3399 that include both Cortex-A53 cores that aren't
> vulnerable and Cortex-A72 cores that are.
> 
> ok?
> 
> 
> Index: dev/fdt/psci.c
> ===
> RCS file: /cvs/src/sys/dev/fdt/psci.c,v
> retrieving revision 1.6
> diff -u -p -r1.6 psci.c
> --- dev/fdt/psci.c23 Feb 2018 19:08:56 -  1.6
> +++ dev/fdt/psci.c1 May 2018 09:35:14 -

> +int32_t
> +smccc_version(void)
> +{
> + struct psci_softc *sc = psci_sc;
> +
> + if (sc && sc->sc_callfn)
> + return (*sc->sc_callfn)(SMCCC_VERSION, 0, 0, 0);

According to
https://developer.arm.com/-/media/developer/pdf/ARM_DEN_0070A_Firmware_interfaces_for_mitigating_CVE-2017-5715.pdf?revision=14854bab-e163-479e-b65b-05b491b31736=en

SMCCC_VERSION returning NOT_SUPPORTED/-1 should be treated as 1.0.
Should we return 0x1 here or in the callers?

Though it seems all the callers just test for >= 1.1 at the moment
so I'm ok with this version going in.

Re: in6_ioctl(): use read lock for SIOCGIF*_IN6

[Theo Buehler]

On Wed, May 02, 2018 at 04:25:20PM +0200, Hrvoje Popovski wrote:
> On 2.5.2018. 12:16, Theo Buehler wrote:
> > Here's a further refactoring of in6_ioctl() that splits out the
> > SIOCGIF*_IN6 cases into the new function in6_ioctl_get(), where we only
> > need a read lock, similar to ifioctl() and the pending patch for
> > in_ioctl(). We get rid of one of the four switches in the function body
> > and error out early on on unknown ioctls before grabbing a lock instead
> > of doing nothing until the end of the function.
> > 
> > After grabbing the lock in the body of in6_ioctl(), we now only deal
> > with SIOCAIFADDR_IN6 and SIOCDIFADDR_IN6. This will need more cleanup
> > and streamlining in a later step.
> > 
> > I didn't really know what to do with the big comment. I left it
> > essentially where it was. Suggestions welcome.
> 
> Hi,
> 
> i'm testing this diff on top on -current tree fetched hour ago. i'm
> forwarding ip6 traffic over vlan on trunk and doing ip6 client server
> with iperf3 while destroying and recreating pseudo interfaces
> 
> for now everything seems stable
> 

Thanks, that's good to know.  However, I overlooked a shadowing issue.
There was a local re-declaration of error, making the function succeed
more often than it should. This additional piece is needed:

switch(cmd) {
case SIOCAIFADDR_IN6:
{
-   int plen, error = 0, newifaddr = 0;
+   int plen, newifaddr = 0;

This is the only change to the previous submission.

Index: sys/netinet6/in6.c
===
RCS file: /var/cvs/src/sys/netinet6/in6.c,v
retrieving revision 1.223
diff -u -p -r1.223 in6.c
--- sys/netinet6/in6.c  2 May 2018 07:19:45 -   1.223
+++ sys/netinet6/in6.c  2 May 2018 23:10:05 -
@@ -116,6 +116,7 @@ const struct in6_addr in6mask96 = IN6MAS
 const struct in6_addr in6mask128 = IN6MASK128;
 
 int in6_ioctl(u_long, caddr_t, struct ifnet *, int);
+int in6_ioctl_get(u_long, caddr_t, struct ifnet *);
 int in6_ifinit(struct ifnet *, struct in6_ifaddr *, int);
 void in6_unlink_ifa(struct in6_ifaddr *, struct ifnet *);
 
@@ -218,29 +219,15 @@ in6_ioctl(u_long cmd, caddr_t data, stru
case SIOCGIFINFO_IN6:
case SIOCGNBRINFO_IN6:
return (nd6_ioctl(cmd, data, ifp));
-   }
-
-   /*
-* Find address for this interface, if it exists.
-*
-* In netinet code, we have checked ifra_addr in SIOCSIF*ADDR operation
-* only, and used the first interface address as the target of other
-* operations (without checking ifra_addr).  This was because netinet
-* code/API assumed at most 1 interface address per interface.
-* Since IPv6 allows a node to assign multiple addresses
-* on a single interface, we almost always look and check the
-* presence of ifra_addr, and reject invalid ones here.
-* It also decreases duplicated code among SIOC*_IN6 operations.
-*/
-   switch (cmd) {
-   case SIOCAIFADDR_IN6:
-   sa6 = >ifra_addr;
-   break;
case SIOCGIFDSTADDR_IN6:
case SIOCGIFNETMASK_IN6:
-   case SIOCDIFADDR_IN6:
case SIOCGIFAFLAG_IN6:
case SIOCGIFALIFETIME_IN6:
+   return (in6_ioctl_get(cmd, data, ifp));
+   case SIOCAIFADDR_IN6:
+   sa6 = >ifra_addr;
+   break;
+   case SIOCDIFADDR_IN6:
sa6 = >ifr_addr;
break;
case SIOCSIFADDR:
@@ -253,10 +240,22 @@ in6_ioctl(u_long cmd, caddr_t data, stru
 */
return (EINVAL);
default:
-   sa6 = NULL;
-   break;
+   return (EOPNOTSUPP);
}
 
+   /*
+* Find address for this interface, if it exists.
+*
+* In netinet code, we have checked ifra_addr in SIOCSIF*ADDR operation
+* only, and used the first interface address as the target of other
+* operations (without checking ifra_addr).  This was because netinet
+* code/API assumed at most 1 interface address per interface.
+* Since IPv6 allows a node to assign multiple addresses
+* on a single interface, we almost always look and check the
+* presence of ifra_addr, and reject invalid ones here.
+* It also decreases duplicated code among SIOC*_IN6 operations.
+*/
+
NET_LOCK();
 
if (sa6 && sa6->sin6_family == AF_INET6) {
@@ -312,93 +311,12 @@ in6_ioctl(u_long cmd, caddr_t data, stru
goto err;
}
break;
-
-   case SIOCGIFAFLAG_IN6:
-   case SIOCGIFNETMASK_IN6:
-   case SIOCGIFDSTADDR_IN6:
-   case SIOCGIFALIFETIME_IN6:
-   /* must think again about its semantics */
-   if (ia6 == NULL) {
-   error = EADDRNOTAVAIL;
-   goto err;

splice out a knock function for alps touchpads in pms.c

[Ryan Lennox]

Hi,

Elantech has elantech_knock()
Synaptics has synaptics_knock()
Alps should have alps_knock()

I'd ask for ok, but I don't have an Alps touchpad to test it.


​Index: src/sys/dev/pckbc/pms.c
===
RCS file: /cvs/src/sys/dev/pckbc/pms.c,v
retrieving revision 1.86
diff -u -p -r1.86 pms.c
--- src/sys/dev/pckbc/pms.c 29 Apr 2018 08:50:04 -  1.86
+++ src/sys/dev/pckbc/pms.c 2 May 2018 22:58:03 -
@@ -307,6 +307,7 @@ int synaptics_query(struct pms_softc *, 
 intsynaptics_get_hwinfo(struct pms_softc *);
 void   synaptics_sec_proc(struct pms_softc *);
 
+intalps_knock(struct pms_softc *);
 intalps_sec_proc(struct pms_softc *);
 intalps_get_hwinfo(struct pms_softc *);
 
@@ -1432,10 +1433,8 @@ alps_get_hwinfo(struct pms_softc *sc)
 }
 
 int
-pms_enable_alps(struct pms_softc *sc)
+alps_knock(struct pms_softc *sc)
 {
-   struct alps_softc *alps = sc->alps;
-   struct wsmousedev_attach_args a;
u_char resp[3];
 
if (pms_set_resolution(sc, 0) ||
@@ -1445,8 +1444,21 @@ pms_enable_alps(struct pms_softc *sc)
pms_get_status(sc, resp) ||
resp[0] != PMS_ALPS_MAGIC1 ||
resp[1] != PMS_ALPS_MAGIC2 ||
-   (resp[2] != PMS_ALPS_MAGIC3_1 && resp[2] != PMS_ALPS_MAGIC3_2 &&
-   resp[2] != PMS_ALPS_MAGIC3_3))
+   (resp[2] != PMS_ALPS_MAGIC3_1 &&
+resp[2] != PMS_ALPS_MAGIC3_2 &&
+resp[2] != PMS_ALPS_MAGIC3_3))
+   return (-1);
+
+   return (0);
+}
+
+int
+pms_enable_alps(struct pms_softc *sc)
+{
+   struct alps_softc *alps = sc->alps;
+   struct wsmousedev_attach_args a;
+
+   if (alps_knock(sc))
goto err;
 
if (sc->alps == NULL) {

Re: in6_ioctl(): use read lock for SIOCGIF*_IN6

[Hrvoje Popovski]

On 2.5.2018. 12:16, Theo Buehler wrote:
> Here's a further refactoring of in6_ioctl() that splits out the
> SIOCGIF*_IN6 cases into the new function in6_ioctl_get(), where we only
> need a read lock, similar to ifioctl() and the pending patch for
> in_ioctl(). We get rid of one of the four switches in the function body
> and error out early on on unknown ioctls before grabbing a lock instead
> of doing nothing until the end of the function.
> 
> After grabbing the lock in the body of in6_ioctl(), we now only deal
> with SIOCAIFADDR_IN6 and SIOCDIFADDR_IN6. This will need more cleanup
> and streamlining in a later step.
> 
> I didn't really know what to do with the big comment. I left it
> essentially where it was. Suggestions welcome.

Hi,

i'm testing this diff on top on -current tree fetched hour ago. i'm
forwarding ip6 traffic over vlan on trunk and doing ip6 client server
with iperf3 while destroying and recreating pseudo interfaces

for now everything seems stable

Re: libkvm requires kvm_getargv before kvm_getenv when both needed

[Todd C. Miller]

On Tue, 01 May 2018 13:35:54 -0600, "Theo de Raadt" wrote:

> >   b) Their working space should be independent of each other. This
> >  isn't hard, just splitting kd->argbuf into kd->argbuf and
> >  kd->envbuf. Seems a bit saner.
> >
>
> I think (b) would be the better solution, this seems very fragile.
>
> Todd and Guenther -- what do you think?

I'd prefer separate buffer spaces as well, the current situation is
fragile as we've seen.

 - todd

Re: reduce hppa interrupt guts awfulness

[Mark Kettenis]

> Date: Tue, 1 May 2018 21:22:12 +
> From: Miod Vallat 
> 
> B2000 and C3650 test have been done with sys/dev/ic/com.c downgraded to
> 1.166 in order to avoid freezes (bug resolution ongoing and not related
> to this diff).

So that's why my C3000 didn't come back after a reboot...

The problem is fairly obvious.  The probe for the XR17V35X that was
added in 1.167 reads from a register that is outside the standard
range of 8250 UART registers.  This obviously can't be done, so I've
reverted the com(4)-related bits of that commit.

Regarding your diff.  The approach seems reasonable to me.  The magic
number in the gscbus.c doesn't fill me with joy.

> + r[4] = cpu_gethpa(0) | (31 - irqbit);   /* iar */

I realise you're following existing practice, but maybe you could you
add a gscbus_reg struct similar to what was used in the asp/lasi/wax
code and use that?

Wither way, ok kettenis@

> $ cat levandes.vari
> Index: conf/GENERIC
> ===
> RCS file: /OpenBSD/src/sys/arch/hppa/conf/GENERIC,v
> retrieving revision 1.175
> diff -u -p -r1.175 GENERIC
> --- conf/GENERIC  14 Feb 2018 23:51:49 -  1.175
> +++ conf/GENERIC  1 May 2018 20:03:14 -
> @@ -46,21 +46,19 @@ uturn0at mainbus0 # U2/UTurn Runway IO
>  uturn1   at mainbus0
>  astro*   at mainbus0 # Astro memory & I/O controller
>  
> -lasi0at mainbus0 offset 0x10 irq 28  # LASI host adapter
> -lasi0at mainbus0 offset 0xfd0 irq 28 # LASI on C1[01]0, 
> J2[01]0
> -lasi0at phantomas0 offset 0xfd0 irq 28   # LASI on [AB]*
> -lasi0at uturn? offset 0xfd0 irq 28   # LASI on [CJ]*
> -lasi1at mainbus0 offset 0x50 irq 27  # 712 GIO card
> -asp* at mainbus0 irq 28  # this one comes w/ Viper and LEDs
> -wax* at mainbus0 irq 24  # Wax may host EISA as well
> -wax* at phantomas0 irq 24# Wax on [AB]*
> -wax* at uturn? irq 24# Wax on C*
> +lasi0at mainbus0 offset 0x10 # LASI host adapter
> +lasi0at mainbus0 offset 0xfd0# LASI on C1[01]0, 
> J2[01]0
> +lasi0at phantomas0 offset 0xfd0  # LASI on [AB]*
> +lasi0at uturn? offset 0xfd0  # LASI on [CJ]*
> +lasi1at mainbus0 offset 0x50 # 712 GIO card
> +asp* at mainbus0 # this one comes w/ Viper and LEDs
> +wax* at mainbus0 # Wax may host EISA as well
> +wax* at phantomas0   # Wax on [AB]*
> +wax* at uturn?   # Wax on C*
>  mongoose* at mainbus0 irq 17 # EISA Bus Adapter (i82350 or TI???)
>  #vmeb*   at mainbus0 irq ?   # VME bus adapter
> -dino0at phantomas? irq 26# PCI bus bridge on [AB]*
> -dino1at phantomas? irq 25
> -dino0at uturn0 irq 26# PCI bus bridge on [CJ]*
> -dino1at uturn1 irq 25
> +dino*at phantomas?   # PCI bus bridge on [AB]*
> +dino*at uturn?   # PCI bus bridge on [CJ]*
>  pci* at dino?
>  option   PCIVERBOSE
>  #pckbc0  at dino? irq 9
> @@ -176,12 +174,9 @@ onewire* at uow?
>  udl* at uhub?# DisplayLink USB displays
>  wsdisplay* at udl?
>  
> -sti0 at mainbus0 irq 11  # [H]CRX-{8,24,48}[Z] graphics
> -sti0 at phantomas0 irq 11# builtin graphics on BC*
> -sti0 at uturn? irq 11
> -sti1 at mainbus0 irq 12
> -sti1 at phantomas0 irq 12
> -sti1 at uturn? irq 12
> +sti* at mainbus0 # [H]CRX-{8,24,48}[Z] graphics
> +sti* at phantomas0   # builtin graphics on BC*
> +sti* at uturn?
>  sti* at pci? # EG-PCI, FX*
>  
>  # internal i/o space
> Index: conf/RAMDISK
> ===
> RCS file: /OpenBSD/src/sys/arch/hppa/conf/RAMDISK,v
> retrieving revision 1.109
> diff -u -p -r1.109 RAMDISK
> --- conf/RAMDISK  30 Dec 2016 22:36:07 -  1.109
> +++ conf/RAMDISK  1 May 2018 20:03:14 -
> @@ -48,20 +48,18 @@ uturn0at mainbus0 # U2/UTurn 
> Runway I
>  uturn1   at mainbus0
>  astro*   at mainbus0 # Astro memory & I/O controller
>  
> -lasi0at mainbus0 offset 0x10 irq 28  # LASI host 
> adapter
> -lasi0at mainbus0 offset 0xfd0 irq 28 # LASI on 
> C1[01]0, J2[01]0
> -lasi0at phantomas0 offset 0xfd0 irq 28   # LASI on [AB]*
> -lasi0at uturn? offset 0xfd0 irq 28   # LASI on [CJ]*
> -lasi1at mainbus0 offset 0x50 irq 27  # 712 GIO card
> -asp* at mainbus0 irq 28  # this one comes w/ Viper and LEDs
> -wax* at mainbus0 irq 24  # Wax may host EISA as well
> -wax* at phantomas0 irq 24# Wax on [AB]*
> -wax* at uturn? irq 24# Wax on C*
> +lasi0at mainbus0 offset 0x10 # LASI host 

Re: Don't send icmp redirect to the same interface a forwarded packet came in on

[Christopher Zimmermann]

On 2018-05-02 Martin Pieuchot  wrote:
> On 02/05/18(Wed) 11:47, Christopher Zimmermann wrote:
> > I just want to bring this up again. Can some network guru give me an ok
> > or some feedback please?  
> 
> Can you explain with words why we shouldn't send a redirect?  The
> comment above your diff states clearly:
> 
>   "If forwarding packet using same interface that it came in on,
>   perhaps should send a redirect to sender to shortcut a hop."
> 
> So you're suggesting no to do that, why?

That's not exactly what I'm suggesting.

In this setting:

A 192.168.4.7 <--> 192.168.4.1 Gateway 192.168.1.1 <--> 192.168.1.2 B

I observed this senseless redirect:

192.168.4.1 > 192.168.4.7: icmp: redirect 192.168.1.2 to host 192.168.4.1
in plain language it means:
"Hi 192.168.4.7, I'm 192.168.4.1. You sent me a packet for 192.168.1.2.
I'm not the best route, next time send it to 192.168.4.1."
So the gateway is instructing host 192.168.4.7 to use gateway
192.168.4.1 instead of 192.168.4.1. (this is not a typo!)

I'm suggesting to skip the redirect when we would redirect the sender
to an(other) address of the very interface the original packet came in
on.

I hope this explanation is reasonable.


Christopher


-- 
http://gmerlin.de
OpenPGP: http://gmerlin.de/christopher.pub
2779 7F73 44FD 0736 B67A  C410 69EC 7922 34B4 2566

Add support note for Theobroma RK3399-Q7 module

[Karel Gardas]

Hi,

Mark Kettenis confirmed on -arm mailing list support of -current for
Theobroma's RK3399-Q7 module together with Haikou Q7 Dev Kit. Patch
below fixes this omission on arm64 platform page.

Thanks,
Karel

? papers/eurobsdcon_2013_kde4/.new.img69.png
Index: arm64.html
===
RCS file: /cvs/www/arm64.html,v
retrieving revision 1.18
diff -u -p -u -r1.18 arm64.html
--- arm64.html  2 Apr 2018 18:02:36 -   1.18
+++ arm64.html  2 May 2018 12:18:11 -
@@ -107,6 +107,7 @@ OpenBSD/arm64 runs on the following hard

Pine64 Rock64
Firefly-RK3399
+   Theobroma RK3399-Q7 (Q7 Module) together with Haikou Q7 
Development Kit

 
 

Re: Don't send icmp redirect to the same interface a forwarded packet came in on

[Martin Pieuchot]

On 02/05/18(Wed) 11:47, Christopher Zimmermann wrote:
> I just want to bring this up again. Can some network guru give me an ok
> or some feedback please?

Can you explain with words why we shouldn't send a redirect?  The
comment above your diff states clearly:

  "If forwarding packet using same interface that it came in on,
  perhaps should send a redirect to sender to shortcut a hop."

So you're suggesting no to do that, why?

Re: Don't send icmp redirect to the same interface a forwarded packet came in on

[Christopher Zimmermann]

Hi,

I just want to bring this up again. Can some network guru give me an ok
or some feedback please?

Christopher


On 2017-12-01 Christopher Zimmermann  wrote:
> Hi,
> 
> by accident I discovered this rather senseless redirect:
> 
> $ doas tcpdump -eptni vlan2 icmp 
> tcpdump: listening on vlan2, link-type EN10MB
> 11:11:11:11:11:11 22:22:22:22:22 0800 98: 192.168.1.2 > 192.168.4.7: icmp: 
> echo request
> 22:22:22:22:22 11:11:11:11:11:11 0800 98: 192.168.4.7 > 192.168.1.2: icmp: 
> echo reply
> 11:11:11:11:11:11 22:22:22:22:22 0800 70: 192.168.4.1 > 192.168.4.7: icmp: 
> redirect 192.168.1.2 to host 192.168.4.1
> ^C
> 110 packets received by filter
> 0 packets dropped by kernel


[...]


> In any case I'd propose the following diff which will check whether the
> redirect target is an address of the interface the forwarded packet came in 
> on.
> 
> 
> Christopher
> 
> 
> Index: ip_input.c
> ===
> RCS file: /cvs/src/sys/netinet/ip_input.c,v
> retrieving revision 1.322
> diff -u -p -r1.322 ip_input.c
> --- ip_input.c7 Sep 2017 10:54:49 -   1.322
> +++ ip_input.c1 Dec 2017 18:00:53 -
> @@ -1514,16 +1514,27 @@ ip_forward(struct mbuf *m, struct ifnet 
>   (rt->rt_flags & (RTF_DYNAMIC|RTF_MODIFIED)) == 0 &&
>   satosin(rt_key(rt))->sin_addr.s_addr != 0 &&
>   ipsendredirects && !srcrt &&
> - !arpproxy(satosin(rt_key(rt))->sin_addr, m->m_pkthdr.ph_rtableid)) {
> - if ((ip->ip_src.s_addr & ifatoia(rt->rt_ifa)->ia_netmask) ==
> - ifatoia(rt->rt_ifa)->ia_net) {
> - if (rt->rt_flags & RTF_GATEWAY)
> + !arpproxy(satosin(rt_key(rt))->sin_addr, m->m_pkthdr.ph_rtableid) &&
> + (ip->ip_src.s_addr & ifatoia(rt->rt_ifa)->ia_netmask) ==
> + ifatoia(rt->rt_ifa)->ia_net) {
> + struct ifaddr *ifa;
> +
> + if (rt->rt_flags & RTF_GATEWAY)
>   dest = satosin(rt->rt_gateway)->sin_addr.s_addr;
> - else
> + else
>   dest = ip->ip_dst.s_addr;
> - /* Router requirements says to only send host redirects */
> - type = ICMP_REDIRECT;
> - code = ICMP_REDIRECT_HOST;
> +
> + /* don't redirect to the interface the packet came in on. */
> + TAILQ_FOREACH(ifa, >if_addrlist, ifa_list) {
> + if (ifa->ifa_addr->sa_family == AF_INET &&
> + satosin(ifa->ifa_addr)->sin_addr.s_addr == dest)
> + dest = 0;
> + }
> +
> + /* Router requirements says to only send host redirects */
> + if (dest != 0) {
> + type = ICMP_REDIRECT;
> + code = ICMP_REDIRECT_HOST;
>   }
>   }
> 
>  



-- 
http://gmerlin.de
OpenPGP: http://gmerlin.de/christopher.pub
2779 7F73 44FD 0736 B67A  C410 69EC 7922 34B4 2566

Re: 5GHz AP RSSI measurement problem

[Paul Irofti]

On Wed, May 02, 2018 at 11:52:18AM +0200, Stefan Sperling wrote:
> On Wed, May 02, 2018 at 12:30:30PM +0300, Paul Irofti wrote:
> > On Mon, Apr 30, 2018 at 10:55:22AM +0200, Stefan Sperling wrote:
> > > + /*
> > > +  * During a scan on 5Ghz, prefer RSSI measured for probe
> > > +  * response frames. i.e. don't allow beacons to lower the
> > > +  * measured RSSI. Some 5GHz APs send beacons with much
> > > +  * less Tx power than they use for probe responses.
> > > +  */
> > > +  if (isprobe)
> > 
> > Properly indent the if clause here
> > 
> > > + ni->ni_rssi = rxi->rxi_rssi;
> > > + else if (ni->ni_rssi < rxi->rxi_rssi)
> > 
> > Can't this be an OR in the former if clause?
> 
> Yes it could.
> 
> > > + ni->ni_rssi = rxi->rxi_rssi;
> > > + } else
> > > + ni->ni_rssi = rxi->rxi_rssi;
> > 
> > And actually, can't all of this be turned into a single if clause? :)
> > Maybe I am reading this wrong, but aren't you setting everywhere
> > ni->ni_rssi to rxi->rxi_rssi?
> 
> No. I don't update ni_rssi if the frame is a beacon (isprobe == false)
> which reduces an already recorded rssi value.
> 
> > I am a bit confused why this did not work before (when you were setting
> > the value to rxi_rssi no matter what) and why this extra checking fixed
> > it.
> 
> It didn't always work before because a beacon is not a probe response.
> Both types of frames contain the same data but semantics are different:
> The AP sends beacons at a fixed interval, and a probe response only
> when it has received a probe request from us.
> Sending probe requests during a scan is also known as an "active scan"
> (client performs an active tranmission), as opposed to a "passive scan"
> which only listens for beacons (client makes no tranmission).
> 
> The issue observed is that this AP is sending beacons with a much
> lower amount of transmit power than probe responses.

Got it, thanks!

I tried to contract the if clauses,

   if (!(ic->ic_state == IEEE80211_S_SCAN &&
   IEEE80211_IS_CHAN_5GHZ(ni->ni_chan) &&
   (isprobe || ni->ni_rssi < rxi->rxi_rssi)))
ni->ni_rssi = rxi->rxi_rssi;

but I think what you have now is most readable.

OK pirofti@

in6_ioctl(): use read lock for SIOCGIF*_IN6

[Theo Buehler]

Here's a further refactoring of in6_ioctl() that splits out the
SIOCGIF*_IN6 cases into the new function in6_ioctl_get(), where we only
need a read lock, similar to ifioctl() and the pending patch for
in_ioctl(). We get rid of one of the four switches in the function body
and error out early on on unknown ioctls before grabbing a lock instead
of doing nothing until the end of the function.

After grabbing the lock in the body of in6_ioctl(), we now only deal
with SIOCAIFADDR_IN6 and SIOCDIFADDR_IN6. This will need more cleanup
and streamlining in a later step.

I didn't really know what to do with the big comment. I left it
essentially where it was. Suggestions welcome.

Index: sys/netinet6/in6.c
===
RCS file: /var/cvs/src/sys/netinet6/in6.c,v
retrieving revision 1.223
diff -u -p -r1.223 in6.c
--- sys/netinet6/in6.c  2 May 2018 07:19:45 -   1.223
+++ sys/netinet6/in6.c  2 May 2018 10:05:13 -
@@ -116,6 +116,7 @@ const struct in6_addr in6mask96 = IN6MAS
 const struct in6_addr in6mask128 = IN6MASK128;
 
 int in6_ioctl(u_long, caddr_t, struct ifnet *, int);
+int in6_ioctl_get(u_long, caddr_t, struct ifnet *);
 int in6_ifinit(struct ifnet *, struct in6_ifaddr *, int);
 void in6_unlink_ifa(struct in6_ifaddr *, struct ifnet *);
 
@@ -218,29 +219,15 @@ in6_ioctl(u_long cmd, caddr_t data, stru
case SIOCGIFINFO_IN6:
case SIOCGNBRINFO_IN6:
return (nd6_ioctl(cmd, data, ifp));
-   }
-
-   /*
-* Find address for this interface, if it exists.
-*
-* In netinet code, we have checked ifra_addr in SIOCSIF*ADDR operation
-* only, and used the first interface address as the target of other
-* operations (without checking ifra_addr).  This was because netinet
-* code/API assumed at most 1 interface address per interface.
-* Since IPv6 allows a node to assign multiple addresses
-* on a single interface, we almost always look and check the
-* presence of ifra_addr, and reject invalid ones here.
-* It also decreases duplicated code among SIOC*_IN6 operations.
-*/
-   switch (cmd) {
-   case SIOCAIFADDR_IN6:
-   sa6 = >ifra_addr;
-   break;
case SIOCGIFDSTADDR_IN6:
case SIOCGIFNETMASK_IN6:
-   case SIOCDIFADDR_IN6:
case SIOCGIFAFLAG_IN6:
case SIOCGIFALIFETIME_IN6:
+   return (in6_ioctl_get(cmd, data, ifp));
+   case SIOCAIFADDR_IN6:
+   sa6 = >ifra_addr;
+   break;
+   case SIOCDIFADDR_IN6:
sa6 = >ifr_addr;
break;
case SIOCSIFADDR:
@@ -253,10 +240,22 @@ in6_ioctl(u_long cmd, caddr_t data, stru
 */
return (EINVAL);
default:
-   sa6 = NULL;
-   break;
+   return (EOPNOTSUPP);
}
 
+   /*
+* Find address for this interface, if it exists.
+*
+* In netinet code, we have checked ifra_addr in SIOCSIF*ADDR operation
+* only, and used the first interface address as the target of other
+* operations (without checking ifra_addr).  This was because netinet
+* code/API assumed at most 1 interface address per interface.
+* Since IPv6 allows a node to assign multiple addresses
+* on a single interface, we almost always look and check the
+* presence of ifra_addr, and reject invalid ones here.
+* It also decreases duplicated code among SIOC*_IN6 operations.
+*/
+
NET_LOCK();
 
if (sa6 && sa6->sin6_family == AF_INET6) {
@@ -312,90 +311,9 @@ in6_ioctl(u_long cmd, caddr_t data, stru
goto err;
}
break;
-
-   case SIOCGIFAFLAG_IN6:
-   case SIOCGIFNETMASK_IN6:
-   case SIOCGIFDSTADDR_IN6:
-   case SIOCGIFALIFETIME_IN6:
-   /* must think again about its semantics */
-   if (ia6 == NULL) {
-   error = EADDRNOTAVAIL;
-   goto err;
-   }
-   break;
}
 
switch (cmd) {
-   case SIOCGIFDSTADDR_IN6:
-   if ((ifp->if_flags & IFF_POINTOPOINT) == 0) {
-   error = EINVAL;
-   break;
-   }
-   /*
-* XXX: should we check if ifa_dstaddr is NULL and return
-* an error?
-*/
-   ifr->ifr_dstaddr = ia6->ia_dstaddr;
-   break;
-
-   case SIOCGIFNETMASK_IN6:
-   ifr->ifr_addr = ia6->ia_prefixmask;
-   break;
-
-   case SIOCGIFAFLAG_IN6:
-   ifr->ifr_ifru.ifru_flags6 = ia6->ia6_flags;
-   break;
-
-   case SIOCGIFALIFETIME_IN6:
-   ifr->ifr_ifru.ifru_lifetime = ia6->ia6_lifetime;
-   if (ia6->ia6_lifetime.ia6t_vltime != 

Re: 5GHz AP RSSI measurement problem

[Stefan Sperling]

On Wed, May 02, 2018 at 12:30:30PM +0300, Paul Irofti wrote:
> On Mon, Apr 30, 2018 at 10:55:22AM +0200, Stefan Sperling wrote:
> > +   /*
> > +* During a scan on 5Ghz, prefer RSSI measured for probe
> > +* response frames. i.e. don't allow beacons to lower the
> > +* measured RSSI. Some 5GHz APs send beacons with much
> > +* less Tx power than they use for probe responses.
> > +*/
> > +if (isprobe)
> 
> Properly indent the if clause here
> 
> > +   ni->ni_rssi = rxi->rxi_rssi;
> > +   else if (ni->ni_rssi < rxi->rxi_rssi)
> 
> Can't this be an OR in the former if clause?

Yes it could.

> > +   ni->ni_rssi = rxi->rxi_rssi;
> > +   } else
> > +   ni->ni_rssi = rxi->rxi_rssi;
> 
> And actually, can't all of this be turned into a single if clause? :)
> Maybe I am reading this wrong, but aren't you setting everywhere
> ni->ni_rssi to rxi->rxi_rssi?

No. I don't update ni_rssi if the frame is a beacon (isprobe == false)
which reduces an already recorded rssi value.

> I am a bit confused why this did not work before (when you were setting
> the value to rxi_rssi no matter what) and why this extra checking fixed
> it.

It didn't always work before because a beacon is not a probe response.
Both types of frames contain the same data but semantics are different:
The AP sends beacons at a fixed interval, and a probe response only
when it has received a probe request from us.
Sending probe requests during a scan is also known as an "active scan"
(client performs an active tranmission), as opposed to a "passive scan"
which only listens for beacons (client makes no tranmission).

The issue observed is that this AP is sending beacons with a much
lower amount of transmit power than probe responses.

Re: 5GHz AP RSSI measurement problem

[Paul Irofti]

On Mon, Apr 30, 2018 at 10:55:22AM +0200, Stefan Sperling wrote:
> I've ran into what seems to be a fairly modern dual-band AP (issued
> by a French ISP). This AP camps on channel 112. This channel requires
> radar detection which may explain the behaviour described below.
> 
> The AP sends 5GHz beacons with a ridicously low RSSI while no client
> is connected, and OpenBSD prefers the 2GHz band...
> 
>   + b8:ee:0e:cb:b3:081   +58 54M   ess  privacy   rsn  "ESSID"
>   + b8:ee:0e:cb:b3:09  112+6 54M   ess  privacy   rsn  "ESSID"
> 
> ... unless we get lucky and the most recently measured RSSI is obtained
> from a probe response instead of a beacon:
> 
>   + b8:ee:0e:cb:b3:081   +58 54M   ess  privacy   rsn  "ESSID"
>   + b8:ee:0e:cb:b3:09  112   +61 54M   ess  privacy   rsn  "ESSID"
> 
> tcpdump confirms that beacons are received with a low RSSI of -10dBm
> as long as no other client is connected (nevermind that the positive
> dBm values shown here are bogus; that is a separate issue):
> 
> 10:18:59.773885 802.11 flags=0<>: beacon, timestamp 974393344059, interval 
> 100,
> caps=2421, ssid (ESSID), rates 6M* 
> 9M
> 12M* 18M 24M* 36M 48M 54M, ds (chan 112), tim 0x0003, country 'FR ',
> channel 36 limit 23dB, channel 40 limit 23dB, channel 44 limit 23dB, channel 
> 48
> limit 23dB, channel 52 limit 23dB, channel 56 limit 23dB, channel 60 limit
> 23dB, channel 64 limit 23dB, channel 100 limit 23dB, channel 104 limit 23dB,
> channel 108 limit 23dB, channel 112 limit 23dB, channel 116 limit 23dB, 
> channel
> 132 limit 23dB, channel 136 limit 23dB, channel 140 limit 23dB, power
> constraint 0dB,  noise -127dBm>
> 
> Whereas probe responses consistently arrive with much more promising
> RSSI values of about -60dBm:
> 
> 10:18:58.889338 802.11 flags=8: probe response, timestamp 974392459305,
> interval 100, caps=2421, ssid
> (ESSID), rates 6M* 9M 12M* 18M 24M* 36M 48M 54M, ds (chan 112), country 'FR ',
> channel 36 limit 23dB, channel 40 limit 23dB, channel 44 limit 23dB, channel 
> 48
> limit 23dB, channel 52 limit 23dB, channel 56 limit 23dB, channel 60 limit
> 23dB, channel 64 limit 23dB, channel 100 limit 23dB, channel 104 limit 23dB,
> channel 108 limit 23dB, channel 112 limit 23dB, channel 116 limit 23dB, 
> channel
> 132 limit 23dB, channel 136 limit 23dB, channel 140 limit 23dB, power
> constraint 0dB,  noise -127dBm>
> 
> I have no idea if and where the 802.11 standard describes this behaviour.
> Maybe there is a way to tell the real RSSI even from beacon frames?
> Does anyone know more about this?
> 
> Setting aside concerns about my lack of understanding of the underlying
> reason for this behaviour, the hack below is sufficient to make this AP
> show up as a strong contender in the candidate list and be preferred
> over 2GHz as it should be.
> 
> Should I commit this hack? I don't see any downsides.
> 
> Index: ieee80211_input.c
> ===
> RCS file: /cvs/src/sys/net80211/ieee80211_input.c,v
> retrieving revision 1.200
> diff -u -p -r1.200 ieee80211_input.c
> --- ieee80211_input.c 29 Apr 2018 12:11:48 -  1.200
> +++ ieee80211_input.c 30 Apr 2018 08:32:58 -
> @@ -1689,13 +1689,26 @@ ieee80211_recv_probe_resp(struct ieee802
>   memcpy(ni->ni_essid, [2], ssid[1]);
>   }
>   IEEE80211_ADDR_COPY(ni->ni_bssid, wh->i_addr3);
> - ni->ni_rssi = rxi->rxi_rssi;
> + /* XXX validate channel # */
> + ni->ni_chan = >ic_channels[chan];
> + if (ic->ic_state == IEEE80211_S_SCAN &&
> + IEEE80211_IS_CHAN_5GHZ(ni->ni_chan)) {
> + /*
> +  * During a scan on 5Ghz, prefer RSSI measured for probe
> +  * response frames. i.e. don't allow beacons to lower the
> +  * measured RSSI. Some 5GHz APs send beacons with much
> +  * less Tx power than they use for probe responses.
> +  */
> +  if (isprobe)

Properly indent the if clause here

> + ni->ni_rssi = rxi->rxi_rssi;
> + else if (ni->ni_rssi < rxi->rxi_rssi)

Can't this be an OR in the former if clause?

> + ni->ni_rssi = rxi->rxi_rssi;
> + } else
> + ni->ni_rssi = rxi->rxi_rssi;

And actually, can't all of this be turned into a single if clause? :)
Maybe I am reading this wrong, but aren't you setting everywhere
ni->ni_rssi to rxi->rxi_rssi?

I am a bit confused why this did not work before (when you were setting
the value to rxi_rssi no matter what) and why this extra checking fixed
it.

Maybe I need another cup of coffee to understand this...

>   ni->ni_rstamp = rxi->rxi_tstamp;
>   memcpy(ni->ni_tstamp, tstamp, sizeof(ni->ni_tstamp));
>   ni->ni_intval = bintval;
>   ni->ni_capinfo = capinfo;
> - /* XXX validate channel # */
> - ni->ni_chan = >ic_channels[chan];
>   ni->ni_erp = erp;

Re: 5GHz AP RSSI measurement problem

[Peter Hessler]

On 2018 Apr 30 (Mon) at 10:55:22 +0200 (+0200), Stefan Sperling wrote:
:Setting aside concerns about my lack of understanding of the underlying
:reason for this behaviour, the hack below is sufficient to make this AP
:show up as a strong contender in the candidate list and be preferred
:over 2GHz as it should be.
:
:Should I commit this hack? I don't see any downsides.
:

OK

:Index: ieee80211_input.c
:===
:RCS file: /cvs/src/sys/net80211/ieee80211_input.c,v
:retrieving revision 1.200
:diff -u -p -r1.200 ieee80211_input.c
:--- ieee80211_input.c  29 Apr 2018 12:11:48 -  1.200
:+++ ieee80211_input.c  30 Apr 2018 08:32:58 -
:@@ -1689,13 +1689,26 @@ ieee80211_recv_probe_resp(struct ieee802
:   memcpy(ni->ni_essid, [2], ssid[1]);
:   }
:   IEEE80211_ADDR_COPY(ni->ni_bssid, wh->i_addr3);
:-  ni->ni_rssi = rxi->rxi_rssi;
:+  /* XXX validate channel # */
:+  ni->ni_chan = >ic_channels[chan];
:+  if (ic->ic_state == IEEE80211_S_SCAN &&
:+  IEEE80211_IS_CHAN_5GHZ(ni->ni_chan)) {
:+  /*
:+   * During a scan on 5Ghz, prefer RSSI measured for probe
:+   * response frames. i.e. don't allow beacons to lower the
:+   * measured RSSI. Some 5GHz APs send beacons with much
:+   * less Tx power than they use for probe responses.
:+   */
:+   if (isprobe)
:+  ni->ni_rssi = rxi->rxi_rssi;
:+  else if (ni->ni_rssi < rxi->rxi_rssi)
:+  ni->ni_rssi = rxi->rxi_rssi;
:+  } else
:+  ni->ni_rssi = rxi->rxi_rssi;
:   ni->ni_rstamp = rxi->rxi_tstamp;
:   memcpy(ni->ni_tstamp, tstamp, sizeof(ni->ni_tstamp));
:   ni->ni_intval = bintval;
:   ni->ni_capinfo = capinfo;
:-  /* XXX validate channel # */
:-  ni->ni_chan = >ic_channels[chan];
:   ni->ni_erp = erp;
:   /* NB: must be after ni_chan is setup */
:   ieee80211_setup_rates(ic, ni, rates, xrates, IEEE80211_F_DOSORT);
:

-- 
I doubt, therefore I might be.

Re: 5GHz AP RSSI measurement problem

[Stefan Sperling]

On Tue, May 01, 2018 at 10:06:54PM -0500, Patrick Dohman wrote:
> I believe you are correct to correlate RSSI & probe response.
> My understanding is that 802.11 beacon is a management frame to synchronize 
> networks.
> Essentially the beacon interval determines the multiple for the DTIM & may 
> include additional capability frames.
> The beacon blinks at a configurable interval (100 MS) & establishes a 
> countdown for broadcast eligibility.
> In some implementations the reception of blinks may indicate congestion or 
> interference & perhaps infer signal strength. 
> Regards
> Patrick

Thank you for your feedback! I am also quite confident
that this is the right thing to do.
I am still waiting for an OK from another developer before
I'll commit the change.