[PATCH 1/2] station: Fix autoconnect loops

[PATCH 1/2] station: Fix autoconnect loops

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 2476 bytes --]

Make sure we process the result of a connect attempt both in a D-Bus
triggered connection and during autoconnect.  Until now we'd only call
station_connect_cb() on NETDEV_EVENT_DISCONNECT_BY_{AP,SME} if
station->connect_pending was non-NULL, i.e. only in the D-Bus method.
As a result we were never blacklisting BSSes and never calling
network_connect_failed() (to set ask_passphrase) during autoconnect.

Use station->netdev_connected to keep track of whether the event
actually happens in the handshake (as opposed to netconfig for example)
to avoid calling network_connect_failed() with the "in_handshake"
parameter set to true.  Arguably we might want to call
netdev_connect_failed() or at least temporarily blacklist/downrank
the bss if the connection breaks during netconfig but I kept the
current logic in this commit.

We might also want to call station_reassociate_cb() if we're in a
reassociation but this wouldn't currently make much difference (and we
don't seem to have any flag to know that we're in a reassociation right
now.)
---
 src/station.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/src/station.c b/src/station.c
index 479f81f5..e503f636 100644
--- a/src/station.c
+++ b/src/station.c
@@ -113,6 +113,7 @@ struct station {
 	bool ap_directed_roaming : 1;
 	bool scanning : 1;
 	bool autoconnect : 1;
+	bool netdev_connected : 1;
 };
 
 struct anqp_entry {
@@ -1339,6 +1340,7 @@ static void station_reset_connection_state(struct station *station)
 	l_queue_insert(station->networks_sorted, station->connected_network,
 				network_rank_compare, NULL);
 
+	station->netdev_connected = false;
 	station->connected_bss = NULL;
 	station->connected_network = NULL;
 
@@ -1370,7 +1372,9 @@ static void station_disconnect_event(struct station *station, void *event_data)
 {
 	l_debug("%u", netdev_get_ifindex(station->netdev));
 
-	if (station->connect_pending)
+	if (station->connect_pending ||
+			(station->state == STATION_STATE_CONNECTING &&
+			 !station->netdev_connected))
 		station_connect_cb(station->netdev,
 					NETDEV_RESULT_HANDSHAKE_FAILED,
 					event_data, station);
@@ -2554,6 +2558,7 @@ static void station_connect_cb(struct netdev *netdev, enum netdev_result result,
 			l_warn("Could not request neighbor report");
 	}
 
+	station->netdev_connected = true;
 	network_connected(station->connected_network);
 
 	if (station->netconfig)
-- 
2.27.0

[PATCH 2/2] netdev: Pass a reason code with NETDEV_EVENT_DISCONNECT_BY_*

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 1225 bytes --]

Station callbacks expect a reason code (as opposed to status codes) with
this event type.  This shouldn't matter a lot because
station_disconnect_event() only actually looks at the value during
connection establishment so usually the "result" parameter is going to
be HANDSHAKE_FAILED meaning that we'd already be getting a reason code
in status_or_reason, but there could be corner cases where we got a
different "result".
---
 src/netdev.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/src/netdev.c b/src/netdev.c
index 4fbe813a..6c063df5 100644
--- a/src/netdev.c
+++ b/src/netdev.c
@@ -771,10 +771,15 @@ static void netdev_connect_failed(struct netdev *netdev,
 
 	if (connect_cb)
 		connect_cb(netdev, result, &status_or_reason, connect_data);
-	else if (event_filter)
+	else if (event_filter) {
+		/* NETDEV_EVENT_DISCONNECT_BY_SME expects a reason code */
+		if (result != NETDEV_RESULT_HANDSHAKE_FAILED)
+			status_or_reason = MMPDU_REASON_CODE_UNSPECIFIED;
+
 		event_filter(netdev, NETDEV_EVENT_DISCONNECT_BY_SME,
 				&status_or_reason,
 				connect_data);
+	}
 }
 
 static void netdev_disconnect_cb(struct l_genl_msg *msg, void *user_data)
-- 
2.27.0

Re: [PATCH 2/2] netdev: Pass a reason code with NETDEV_EVENT_DISCONNECT_BY_*

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1879 bytes --]

Hi Andrew,

On 5/10/21 5:12 AM, Andrew Zaborowski wrote:
> Station callbacks expect a reason code (as opposed to status codes) with
> this event type.  This shouldn't matter a lot because
> station_disconnect_event() only actually looks at the value during

I'm not sure why station is written this way.  I'm pretty sure that calling 
station_disconnect_event() for NETDEV_EVENT_DISCONNECT_BY_SME() is the wrong 
thing to do.

> connection establishment so usually the "result" parameter is going to
> be HANDSHAKE_FAILED meaning that we'd already be getting a reason code
> in status_or_reason, but there could be corner cases where we got a
> different "result".

That is correct for AP disconnections during handshaking, but that is signaled 
by NETDEV_EVENT_DISCONNECT_BY_AP...

> ---
>   src/netdev.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/src/netdev.c b/src/netdev.c
> index 4fbe813a..6c063df5 100644
> --- a/src/netdev.c
> +++ b/src/netdev.c
> @@ -771,10 +771,15 @@ static void netdev_connect_failed(struct netdev *netdev,
>   
>   	if (connect_cb)
>   		connect_cb(netdev, result, &status_or_reason, connect_data);
> -	else if (event_filter)
> +	else if (event_filter) {
> +		/* NETDEV_EVENT_DISCONNECT_BY_SME expects a reason code */
> +		if (result != NETDEV_RESULT_HANDSHAKE_FAILED)
> +			status_or_reason = MMPDU_REASON_CODE_UNSPECIFIED;
> +

 From what I remember, the only time we can get here is if a rekey somehow 
fails, or setting keys during a rekey fails.  Are there other cases?

>   		event_filter(netdev, NETDEV_EVENT_DISCONNECT_BY_SME,
>   				&status_or_reason,
>   				connect_data);
> +	}
>   }
>   
>   static void netdev_disconnect_cb(struct l_genl_msg *msg, void *user_data)
> 

Anyway, I applied this with a modified commit description.

Regards,
-Denis

Re: [PATCH 2/2] netdev: Pass a reason code with NETDEV_EVENT_DISCONNECT_BY_*

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 2817 bytes --]

On Tue, 11 May 2021 at 18:36, Denis Kenzior <denkenz@gmail.com> wrote:
> On 5/10/21 5:12 AM, Andrew Zaborowski wrote:
> > Station callbacks expect a reason code (as opposed to status codes) with
> > this event type.  This shouldn't matter a lot because
> > station_disconnect_event() only actually looks at the value during
>
> I'm not sure why station is written this way.  I'm pretty sure that calling
> station_disconnect_event() for NETDEV_EVENT_DISCONNECT_BY_SME() is the wrong
> thing to do.

That is currently the only use of that function though.

>
> > connection establishment so usually the "result" parameter is going to
> > be HANDSHAKE_FAILED meaning that we'd already be getting a reason code
> > in status_or_reason, but there could be corner cases where we got a
> > different "result".
>
> That is correct for AP disconnections during handshaking, but that is signaled
> by NETDEV_EVENT_DISCONNECT_BY_AP...
>
> > ---
> >   src/netdev.c | 7 ++++++-
> >   1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/netdev.c b/src/netdev.c
> > index 4fbe813a..6c063df5 100644
> > --- a/src/netdev.c
> > +++ b/src/netdev.c
> > @@ -771,10 +771,15 @@ static void netdev_connect_failed(struct netdev *netdev,
> >
> >       if (connect_cb)
> >               connect_cb(netdev, result, &status_or_reason, connect_data);
> > -     else if (event_filter)
> > +     else if (event_filter) {
> > +             /* NETDEV_EVENT_DISCONNECT_BY_SME expects a reason code */
> > +             if (result != NETDEV_RESULT_HANDSHAKE_FAILED)
> > +                     status_or_reason = MMPDU_REASON_CODE_UNSPECIFIED;
> > +
>
>  From what I remember, the only time we can get here is if a rekey somehow
> fails, or setting keys during a rekey fails.  Are there other cases?

In theory you can pass a null connect_cb to netdev_connect_common(),
but in practice we don't do this so you could trigger this only for:

NETDEV_RESULT_HANDSHAKE_FAILED,
NETDEV_RESULT_KEY_SETTING_FAILED,
NETDEV_RESULT_ABORTED.

I realize I was confused, I thought that netdev_connect_failed() was
actually sending NETDEV_EVENT_DISCONNECT_BY_SME to station and
triggering my autoconnect loop, but actually it was
netdev_disconnect_event() which always calls netdev->event_filter and
never uses netdev->connect_cb.  Perhaps netdev_disconnect_event()
should use netdev_connect_failed() instead.

>
> >               event_filter(netdev, NETDEV_EVENT_DISCONNECT_BY_SME,
> >                               &status_or_reason,
> >                               connect_data);
> > +     }
> >   }
> >
> >   static void netdev_disconnect_cb(struct l_genl_msg *msg, void *user_data)
> >
>
> Anyway, I applied this with a modified commit description.

Thanks.

Best regards

Re: [PATCH 2/2] netdev: Pass a reason code with NETDEV_EVENT_DISCONNECT_BY_*

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 3606 bytes --]

On 5/11/21 4:56 PM, Andrew Zaborowski wrote:
> On Tue, 11 May 2021 at 18:36, Denis Kenzior <denkenz@gmail.com> wrote:
>> On 5/10/21 5:12 AM, Andrew Zaborowski wrote:
>>> Station callbacks expect a reason code (as opposed to status codes) with
>>> this event type.  This shouldn't matter a lot because
>>> station_disconnect_event() only actually looks at the value during
>>
>> I'm not sure why station is written this way.  I'm pretty sure that calling
>> station_disconnect_event() for NETDEV_EVENT_DISCONNECT_BY_SME() is the wrong
>> thing to do.
> 
> That is currently the only use of that function though.

No, we have DISCONNECT_BY_AP and DISCONNECT_BY_SME.  DISCONNECT_BY_SME should 
pretty much imply station_disassociated() should be called since we should never 
receive this event when we're in the connecting state.

Receiving DISCONNECT_BY_AP needs to be treated differently as well, depending on 
whether we're connected or connecting, regardless of connect_pending state (i.e. 
during autoconnect).  Or have netdev.c invoke connect_cb if we're still 
connecting instead of sending this event.

> 
>>
>>> connection establishment so usually the "result" parameter is going to
>>> be HANDSHAKE_FAILED meaning that we'd already be getting a reason code
>>> in status_or_reason, but there could be corner cases where we got a
>>> different "result".
>>
>> That is correct for AP disconnections during handshaking, but that is signaled
>> by NETDEV_EVENT_DISCONNECT_BY_AP...
>>
>>> ---
>>>    src/netdev.c | 7 ++++++-
>>>    1 file changed, 6 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/netdev.c b/src/netdev.c
>>> index 4fbe813a..6c063df5 100644
>>> --- a/src/netdev.c
>>> +++ b/src/netdev.c
>>> @@ -771,10 +771,15 @@ static void netdev_connect_failed(struct netdev *netdev,
>>>
>>>        if (connect_cb)
>>>                connect_cb(netdev, result, &status_or_reason, connect_data);
>>> -     else if (event_filter)
>>> +     else if (event_filter) {
>>> +             /* NETDEV_EVENT_DISCONNECT_BY_SME expects a reason code */
>>> +             if (result != NETDEV_RESULT_HANDSHAKE_FAILED)
>>> +                     status_or_reason = MMPDU_REASON_CODE_UNSPECIFIED;
>>> +
>>
>>   From what I remember, the only time we can get here is if a rekey somehow
>> fails, or setting keys during a rekey fails.  Are there other cases?
> 
> In theory you can pass a null connect_cb to netdev_connect_common(),

Yes, so either netdev should handle this specially, or we should make it an 
error condition during netdev_connect().

> but in practice we don't do this so you could trigger this only for:
> 
> NETDEV_RESULT_HANDSHAKE_FAILED,
> NETDEV_RESULT_KEY_SETTING_FAILED,
> NETDEV_RESULT_ABORTED.

NETDEV_ABORTED should only be invoked in the connecting phase, no?  I suppose 
there may be some bizarre situations, like we're rekeying and the device goes down.

> 
> I realize I was confused, I thought that netdev_connect_failed() was
> actually sending NETDEV_EVENT_DISCONNECT_BY_SME to station and
> triggering my autoconnect loop, but actually it was
> netdev_disconnect_event() which always calls netdev->event_filter and

This confirms my suspicions that we're not handling DISCONNECT_BY_AP completely 
correctly.

> never uses netdev->connect_cb.  Perhaps netdev_disconnect_event()
> should use netdev_connect_failed() instead.

They're different conditions though.  DISCONNECT_BY_AP tells us the AP sent a 
Deauthenticate.  DISCONNECT_BY_SME says that we did (actual or implied).

Regards,
-Denis

Re: [PATCH 2/2] netdev: Pass a reason code with NETDEV_EVENT_DISCONNECT_BY_*

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 1260 bytes --]

On Wed, 12 May 2021 at 00:34, Denis Kenzior <denkenz@gmail.com> wrote:
> On 5/11/21 4:56 PM, Andrew Zaborowski wrote:
> > On Tue, 11 May 2021 at 18:36, Denis Kenzior <denkenz@gmail.com> wrote:
> >> On 5/10/21 5:12 AM, Andrew Zaborowski wrote:
> >>> Station callbacks expect a reason code (as opposed to status codes) with
> >>> this event type.  This shouldn't matter a lot because
> >>> station_disconnect_event() only actually looks at the value during
> >>
> >> I'm not sure why station is written this way.  I'm pretty sure that calling
> >> station_disconnect_event() for NETDEV_EVENT_DISCONNECT_BY_SME() is the wrong
> >> thing to do.
> >
> > That is currently the only use of that function though.
>
> No, we have DISCONNECT_BY_AP and DISCONNECT_BY_SME.  DISCONNECT_BY_SME should
> pretty much imply station_disassociated() should be called since we should never
> receive this event when we're in the connecting state.

I wouldn't rely on that, if the kernel doesn't pass
NL80211_ATTR_DISCONNECTED_BY_AP it could mean that the AP sent
something that triggered the kernel to disconnect (possibly before
handshake complete).  For us we mainly care that we can't use this BSS
and should try a different one next time.

Best regards

Re: [PATCH 2/2] netdev: Pass a reason code with NETDEV_EVENT_DISCONNECT_BY_*

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1696 bytes --]

On 5/11/21 5:57 PM, Andrew Zaborowski wrote:
> On Wed, 12 May 2021 at 00:34, Denis Kenzior <denkenz@gmail.com> wrote:
>> On 5/11/21 4:56 PM, Andrew Zaborowski wrote:
>>> On Tue, 11 May 2021 at 18:36, Denis Kenzior <denkenz@gmail.com> wrote:
>>>> On 5/10/21 5:12 AM, Andrew Zaborowski wrote:
>>>>> Station callbacks expect a reason code (as opposed to status codes) with
>>>>> this event type.  This shouldn't matter a lot because
>>>>> station_disconnect_event() only actually looks at the value during
>>>>
>>>> I'm not sure why station is written this way.  I'm pretty sure that calling
>>>> station_disconnect_event() for NETDEV_EVENT_DISCONNECT_BY_SME() is the wrong
>>>> thing to do.
>>>
>>> That is currently the only use of that function though.
>>
>> No, we have DISCONNECT_BY_AP and DISCONNECT_BY_SME.  DISCONNECT_BY_SME should
>> pretty much imply station_disassociated() should be called since we should never
>> receive this event when we're in the connecting state.
> 
> I wouldn't rely on that, if the kernel doesn't pass
> NL80211_ATTR_DISCONNECTED_BY_AP it could mean that the AP sent
> something that triggered the kernel to disconnect (possibly before

I suppose it might be possible, but wouldn't you treat it as a connect failed 
and not a disconnect_by_sme?  That is what we do if Auth/Assoc times out for 
example.

> handshake complete).  For us we mainly care that we can't use this BSS
> and should try a different one next time.

But we need to make a best attempt to guess as to why it failed.

 From what I gather, in this particular case, the AP just sends a Deauth after 
PTK2/4 packet doesn't pass the MIC check.

Regards,
-Denis

Re: [PATCH 1/2] station: Fix autoconnect loops

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 3593 bytes --]

Hi Andrew,

On Mon, 2021-05-10 at 12:12 +0200, Andrew Zaborowski wrote:
> Make sure we process the result of a connect attempt both in a D-Bus
> triggered connection and during autoconnect.  Until now we'd only
> call
> station_connect_cb() on NETDEV_EVENT_DISCONNECT_BY_{AP,SME} if
> station->connect_pending was non-NULL, i.e. only in the D-Bus method.
> As a result we were never blacklisting BSSes and never calling
> network_connect_failed() (to set ask_passphrase) during autoconnect.

Is the purpose of this to avoid re-trying the same network over and
over? Relying on network_autoconnect() to fail due to ask_passphrase
being true?

Seems like it would be better to re-work autoconnect to actually pop
the list when autoconnect fails, and immediately move onto the next (no
scanning).

> 
> Use station->netdev_connected to keep track of whether the event
> actually happens in the handshake (as opposed to netconfig for
> example)
> to avoid calling network_connect_failed() with the "in_handshake"
> parameter set to true.  Arguably we might want to call
> netdev_connect_failed() or at least temporarily blacklist/downrank
> the bss if the connection breaks during netconfig but I kept the
> current logic in this commit.
> 
> We might also want to call station_reassociate_cb() if we're in a
> reassociation but this wouldn't currently make much difference (and
> we
> don't seem to have any flag to know that we're in a reassociation
> right
> now.)
> ---
>  src/station.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/src/station.c b/src/station.c
> index 479f81f5..e503f636 100644
> --- a/src/station.c
> +++ b/src/station.c
> @@ -113,6 +113,7 @@ struct station {
>         bool ap_directed_roaming : 1;
>         bool scanning : 1;
>         bool autoconnect : 1;
> +       bool netdev_connected : 1;
>  };
>  
>  struct anqp_entry {
> @@ -1339,6 +1340,7 @@ static void
> station_reset_connection_state(struct station *station)
>         l_queue_insert(station->networks_sorted, station-
> >connected_network,
>                                 network_rank_compare, NULL);
>  
> +       station->netdev_connected = false;
>         station->connected_bss = NULL;
>         station->connected_network = NULL;
>  
> @@ -1370,7 +1372,9 @@ static void station_disconnect_event(struct
> station *station, void *event_data)
>  {
>         l_debug("%u", netdev_get_ifindex(station->netdev));
>  
> -       if (station->connect_pending)
> +       if (station->connect_pending ||
> +                       (station->state == STATION_STATE_CONNECTING
> &&
> +                        !station->netdev_connected))
>                 station_connect_cb(station->netdev,
>                                         NETDEV_RESULT_HANDSHAKE_FAILE
> D,
>                                         event_data, station);
> @@ -2554,6 +2558,7 @@ static void station_connect_cb(struct netdev
> *netdev, enum netdev_result result,
>                         l_warn("Could not request neighbor report");
>         }
>  
> +       station->netdev_connected = true;
>         network_connected(station->connected_network);
>  
>         if (station->netconfig)

Re: [PATCH 1/2] station: Fix autoconnect loops

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 2342 bytes --]

Hi James,

On Tue, 11 May 2021 at 18:32, James Prestwood <prestwoj@gmail.com> wrote:
> On Mon, 2021-05-10 at 12:12 +0200, Andrew Zaborowski wrote:
> > Make sure we process the result of a connect attempt both in a D-Bus
> > triggered connection and during autoconnect.  Until now we'd only
> > call
> > station_connect_cb() on NETDEV_EVENT_DISCONNECT_BY_{AP,SME} if
> > station->connect_pending was non-NULL, i.e. only in the D-Bus method.
> > As a result we were never blacklisting BSSes and never calling
> > network_connect_failed() (to set ask_passphrase) during autoconnect.
>
> Is the purpose of this to avoid re-trying the same network over and
> over? Relying on network_autoconnect() to fail due to ask_passphrase
> being true?

Either this, or getting the BSS blacklisted.  Oherwise autoconnect is
working around the blacklist logic and has no chance to try another
BSS or another network.

I don't think the current blacklist and ask_passphrase logic is very
good, but it could be misleading that station_connect_cb is called
only in D-Bus triggered connections.  station_connect_cb is already
handling NETDEV_RESULT_HANDSHAKE_FAILED for both manual- and
auto-connect, it should also handle NETDEV_EVENT_DISCONNECT_BY_* for
both cases.  Perhaps station_connect_cb should then implement a better
policy that looks at whether we're in autoconnect or not, and handles
NETDEV_RESULT_HANDSHAKE_FAILED and NETDEV_EVENT_DISCONNECT_BY_* in the
same way.

>
> Seems like it would be better to re-work autoconnect to actually pop
> the list when autoconnect fails, and immediately move onto the next (no
> scanning).

Yep, using autoconnect_list would at least give justification for
having a list, as it is now we could just as well select a single
network/bss (if ask_passphrase is true there's no point adding it to
the list in the first place).

Ideally I think this should happen:

* the blacklist can be dropped completely,

* a connection failure at a given timestamp causes the BSS rank and
the network's rank to be penalized and recover to its normal value
with time, independent of auto- vs. manual- connect.

* ask_password should be set based on the failure reason, either
always or only for manual connect (IIRC Denis thinks it should be only
for manual connect)

Best regards