Gregor,

Thank you for working on this. I have one comment about your code,
comments inlined in your below diff.

Here is debug output of one day session, with suspend in the middle
when going home, back to work and home again.

# grep -E 'type=0x(15|e)' wpa_supplicant.log
1544550580.320374: XXX evnt_rcv: name=iwn0, type=0x15
1544550580.321089: XXX evnt_rcv: name=iwn0, type=0xe
1544558742.024184: XXX evnt_rcv: name=iwn0, type=0xe
1544558753.880521: XXX evnt_rcv: name=iwn0, type=0x15
1544558753.883372: XXX evnt_rcv: name=iwn0, type=0xe
1544612208.200216: XXX evnt_rcv: name=iwn0, type=0xe
1544612217.053147: XXX evnt_rcv: name=iwn0, type=0x15
1544612217.053676: XXX evnt_rcv: name=iwn0, type=0xe
1544648478.023587: XXX evnt_rcv: name=iwn0, type=0xe

Above is while being sationary and not going around in the office.
This means that I switched only between AP closest to my desk and
to home AP with resume in between. Interestingly it seems that
each RTM_80211INFO is also accompanied with RTM_IFINFO, in random
order.

Tomorrow I may give you example of above log when walking around the
office, to compare events while switching APs. The other day I did
testing by walking around and it works most of the time, however there
is a race somewhere, as while I was walking around I sometimes ended up
without connection and ifconfig(8) at the same time was showing:

        status: active
        ieee80211: join lighthouse chan 100 bssid <random-ap-close> <level>dBm 
wpaprotos wpa2 wpaakms 802.1x wpaciphers ccmp wpagroupcipher ccmp

like network should work, but it didn't and connection was totally
offline. This is very empirical, but it looked to me, when I walked
slower to give wpa_supplicant time to reassoc with each access point on
the way it all worked. When I got bored and walked faster, passing by
multiple APs on the way and with reassocs happening one after another,
before previous assoc was able to finish and me finally stopping, I
ended up with iwn0 card with status active, but no connectivity.
Restarting slaacd(8) or dhclient(8) was not helping as problem was layer
below them.

All in all, I would need to test all of this more and collect more debug
information. Unfortunately this is my last week with my current employer,
so in practice tomorrow is my last day I can test all of this :)

I think your diff makes sense and with handling of RTM_IFINFO it's good
progress in correct direction. I even took your diff and tested with
latest wpa_supplicant 2.7 and it works there too. However I cannot
dive into this more, as I have too many changes in my personal and
professional life in coming days.

If you would like to see any specific info tomorrow, please let me know
I will try to arrange something. After that I'll go offline for family
time.

See comments inline your diff below.

Regards,
 Mikolaj

On Wed, Dec 12, 2018 at 09:44:24PM +0100, Gregor Best wrote:
> 
> Hi Edd, Raf, Mikolaj, ports@
> 
> I just updated my proposed patch with Mikolaj's suggestion of also
> listening to RTM_IFINFO instead of just RTM_80211INFO. The patch is
> attached below the signature.
> 
> It makes reassociation over suspend/resume work (if I resume in the
> same spot I suspended in, haven't checked going out of range). What's
> noteworthy is that it takes a while (in the area of 10 - 15 seconds)
> for wpa_supplicant to re-establish a connection.
> 
> As for Edd's report that moving out of range with the device on doesn't
> cause re-association (did I understand that correctly?), I think the
> output of `route monitor` and `ifconfig $dev scan` before and after
> loss of signal (and going into range of the other AP) might be
> interesting to see if there's another routing message that we have to
> consider.
> 
> Maybe the attached patch also helps Edd's situation. As always, I'm
> grateful for testing and feedback.
> 
> -- 
>       Gregor
> 
> Index: patches/patch-src_drivers_driver_openbsd_c
> ===================================================================
> RCS file: 
> /home/cvs/ports/security/wpa_supplicant/patches/patch-src_drivers_driver_openbsd_c,v
> retrieving revision 1.5
> diff -u -p -r1.5 patch-src_drivers_driver_openbsd_c
> --- patches/patch-src_drivers_driver_openbsd_c        17 May 2016 08:29:27 
> -0000      1.5
> +++ patches/patch-src_drivers_driver_openbsd_c        12 Dec 2018 20:18:19 
> -0000
> @@ -1,24 +1,151 @@
>  $OpenBSD: patch-src_drivers_driver_openbsd_c,v 1.5 2016/05/17 08:29:27 
> dcoppa Exp $
>  
> -Fix includes
> +Fix includes and react to NWID changes and suspend/resume.
>  
> ---- src/drivers/driver_openbsd.c.orig        Sun Sep 27 21:02:05 2015
> -+++ src/drivers/driver_openbsd.c     Mon Sep 28 09:51:53 2015
> -@@ -9,13 +9,14 @@
> +Index: src/drivers/driver_openbsd.c
> +--- src/drivers/driver_openbsd.c.orig
> ++++ src/drivers/driver_openbsd.c
> +@@ -9,19 +9,34 @@
>   #include "includes.h"
>   #include <sys/ioctl.h>
> - 
> +
>  +#include "common.h"
>  +#include "driver.h"
> ++#include "eloop.h"
>  +
> ++#include <sys/socket.h>
>   #include <net/if.h>
>  +#include <net/if_var.h>
> ++#include <net/route.h>
>   #include <net80211/ieee80211.h>
>   #include <net80211/ieee80211_crypto.h>
>   #include <net80211/ieee80211_ioctl.h>
> --
> +
>  -#include "common.h"
>  -#include "driver.h"
> - 
> ++#define RTM_READSZ 2048
> +
>   struct openbsd_driver_data {
> -     char ifname[IFNAMSIZ + 1];
> +-    char ifname[IFNAMSIZ + 1];
> +     void *ctx;
> +
> +-    int sock;                       /* open socket for 802.11 ioctls */
> ++    char ifname[IFNAMSIZ + 1];
> ++    int ifindex;  /* Ifindex of the configured interface */
> ++
> ++    int sock;     /* open socket for 802.11 ioctls */
> ++    int rtsock;   /* routing socket for interface state messages */
> ++
> ++    /* These fields are used to track the last seen (and associated) access 
> point
> ++       to determine whether we should kick off an association event */
> ++    int nwid_len; /* Length of last seen SSID (as per routing message) */
> ++    char nwid[IEEE80211_NWID_LEN]; /* Last seen SSID (as per routing 
> message) */
> ++    char addr[IEEE80211_ADDR_LEN]; /* Last seen BSSID (as per routing 
> message) */
> + };
> +
> +
> +@@ -90,6 +105,71 @@ wpa_driver_openbsd_set_key(const char *ifname, void *p
> +     return 0;
> + }
> +
> ++static void
> ++wpa_driver_openbsd_event_receive(int sock, void *global, void *sock_ctx)
> ++{
> ++    struct openbsd_driver_data *drv = sock_ctx;
> ++    struct rt_msghdr *rtm;
> ++    struct if_ieee80211_data *ifie;
> ++    char *rtmmsg;
> ++    ssize_t n;
> ++
> ++    rtmmsg = os_zalloc(RTM_READSZ);
> ++    if (rtmmsg == NULL) {
> ++            wpa_printf(MSG_ERROR, "Can't allocate space for routing 
> message");
> ++            return;
> ++    }
> ++
> ++    do {
> ++            n = read(sock, rtmmsg, RTM_READSZ);
> ++    } while (n == -1 && errno == EINTR);
> ++
> ++    if (n == -1)
> ++            goto done;
> ++
> ++    rtm = (struct rt_msghdr *)rtmmsg;
> ++
> ++    if ((size_t)n < sizeof(rtm->rtm_msglen) ||
> ++        n < rtm->rtm_msglen ||
> ++        rtm->rtm_version != RTM_VERSION)
> ++            goto done;
> ++
> ++  if (rtm->rtm_index != drv->ifindex)
> ++    goto done;
> ++
> ++  if (rtm->rtm_type == RTM_80211INFO) {
> ++    printf("RTM_80211INFO received\n");

This printf(3) should be removed or changed into wpa_printf(). For my tests
I've used wpa_printf(MSG_DEBUG, ...), which I think is good enough balance
of being not noisy in logs by default, but still providing info when there
is a need via debug flag to the cli. However this is your call and giving
the current sparsity of debug log lines in this c-file, I would just remove
it.

> ++    ifie = &((struct if_ieee80211_msghdr *)rtm)->ifim_ifie;
> ++
> ++    if ((ifie->ifie_nwid_len != drv->nwid_len) ||
> ++        (os_memcmp(drv->nwid, ifie->ifie_nwid, ifie->ifie_nwid_len) != 0) ||
> ++        (os_memcmp(drv->addr, ifie->ifie_addr, IEEE80211_ADDR_LEN) != 0)) {
> ++      os_memcpy(drv->addr, ifie->ifie_addr, IEEE80211_ADDR_LEN);
> ++
> ++      os_memcpy(drv->nwid, ifie->ifie_nwid, ifie->ifie_nwid_len);
> ++      drv->nwid_len = ifie->ifie_nwid_len;
> ++
> ++      /* Emit ASSOC event */
> ++      wpa_supplicant_event(drv->ctx, EVENT_ASSOC, NULL);
> ++    }
> ++    } else if (rtm->rtm_type == RTM_IFINFO) {
> ++    /* This is here so we can react to suspend/resume.
> ++
> ++       This is a bit rough, sometimes there are two or more IFINFOs 
> notifying
> ++       us that the device just got "up" again. It doesn't seem to hurt to
> ++       issue multiple EVENT_ASSOC in those cases though.
> ++    */
> ++
> ++    if (rtm->rtm_flags & RTF_UP) {
> ++      /* Emit ASSOC event */
> ++      wpa_supplicant_event(drv->ctx, EVENT_ASSOC, NULL);
> ++    }
> ++  }
> ++
> ++done:
> ++    os_free(rtmmsg);
> ++}
> ++
> + static void *
> + wpa_driver_openbsd_init(void *ctx, const char *ifname)
> + {
> +@@ -103,9 +183,21 @@ wpa_driver_openbsd_init(void *ctx, const char *ifname)
> +     if (drv->sock < 0)
> +             goto fail;
> +
> ++    drv->rtsock = socket(PF_ROUTE, SOCK_RAW, AF_UNSPEC);
> ++    if (drv->rtsock < 0)
> ++            goto fail;
> ++
> +     drv->ctx = ctx;
> +     os_strlcpy(drv->ifname, ifname, sizeof(drv->ifname));
> +
> ++    drv->ifindex = if_nametoindex(drv->ifname);
> ++    if (drv->ifindex == 0) /* No interface with that name */
> ++            goto fail;
> ++
> ++    drv->nwid_len = wpa_driver_openbsd_get_ssid(drv, drv->nwid);
> ++    wpa_driver_openbsd_get_bssid(drv, drv->addr);
> ++
> ++    eloop_register_read_sock(drv->rtsock, wpa_driver_openbsd_event_receive, 
> NULL, drv);
> +     return drv;
> +
> + fail:
> +@@ -119,7 +211,11 @@ wpa_driver_openbsd_deinit(void *priv)
> + {
> +     struct openbsd_driver_data *drv = priv;
> +
> ++    eloop_unregister_read_sock(drv->rtsock);
> ++
> +     close(drv->sock);
> ++    close(drv->rtsock);
> ++
> +     os_free(drv);
> + }


Reply via email to