[1/2] dpp: relax requirement on IWD being in a disconnected state

Commit Message

James Prestwood July 19, 2024, 6:19 p.m. UTC

Initially this made sense and for a completely unconfigured device
there is little to no downside to limiting IWD's state to
disconnected (or autoconnect states).

Where this poses a problem is if the device has been configured
incorrectly, for example with an invalid PSK. IWD will continue to
try and connect to the network, blocking DPP from ever being
started and always returning busy. Even if you timed it perfectly
to start DP between connection attempts it would be canceled right
away when IWD tried connecting again.

Instead we can relax this requirement by allowing DPP to start even
if IWD is connecting, and only cancel DPP when IWD's state
transitions to connnected. Since DPP effectively blocks the wiphy
work queue once it is started it will prevent any further connection
attempts until it completes.
---
 src/dpp.c | 48 +++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 43 insertions(+), 5 deletions(-)

Comments

Denis Kenzior July 19, 2024, 8:33 p.m. UTC | #1

Hi James,

On 7/19/24 1:19 PM, James Prestwood wrote:
> Initially this made sense and for a completely unconfigured device
> there is little to no downside to limiting IWD's state to
> disconnected (or autoconnect states).
>

What about periodic scanning?

> Where this poses a problem is if the device has been configured
> incorrectly, for example with an invalid PSK. IWD will continue to
> try and connect to the network, blocking DPP from ever being
> started and always returning busy. Even if you timed it perfectly
> to start DP between connection attempts it would be canceled right
> away when IWD tried connecting again.

As in station_get_connected_network() returns non NULL?

> 
> Instead we can relax this requirement by allowing DPP to start even
> if IWD is connecting, and only cancel DPP when IWD's state
> transitions to connnected. Since DPP effectively blocks the wiphy
> work queue once it is started it will prevent any further connection
> attempts until it completes.

But if iwd is connecting, station_get_connected_network() is true and we've 
already sent the connection request?  So, you're inserting DPP work which will 
run after the connection attempt fails?

Have you looked into how WSC does this?

> ---
>   src/dpp.c | 48 +++++++++++++++++++++++++++++++++++++++++++-----
>   1 file changed, 43 insertions(+), 5 deletions(-)

FYI, CI is failing the DPP test for this set.

Regards,
-Denis

James Prestwood July 19, 2024, 8:42 p.m. UTC | #2

Hi Denis,

On 7/19/24 1:33 PM, Denis Kenzior wrote:
> Hi James,
>
> On 7/19/24 1:19 PM, James Prestwood wrote:
>> Initially this made sense and for a completely unconfigured device
>> there is little to no downside to limiting IWD's state to
>> disconnected (or autoconnect states).
>>
>
> What about periodic scanning?
If a periodic scan was queued prior to DPP starting, then yeah it would 
run. But DPP queue's its next offchannel request prior to completing its 
current one, so once DPP starts nothing else can scan/connect until DPP 
is done.
>
>> Where this poses a problem is if the device has been configured
>> incorrectly, for example with an invalid PSK. IWD will continue to
>> try and connect to the network, blocking DPP from ever being
>> started and always returning busy. Even if you timed it perfectly
>> to start DP between connection attempts it would be canceled right
>> away when IWD tried connecting again.
>
> As in station_get_connected_network() returns non NULL?
Yep, since the connected network gets set before we're really "connected".
>
>>
>> Instead we can relax this requirement by allowing DPP to start even
>> if IWD is connecting, and only cancel DPP when IWD's state
>> transitions to connnected. Since DPP effectively blocks the wiphy
>> work queue once it is started it will prevent any further connection
>> attempts until it completes.
>
> But if iwd is connecting, station_get_connected_network() is true and 
> we've already sent the connection request?  So, you're inserting DPP 
> work which will run after the connection attempt fails?
Yes exactly, and if the connection succeeds DPP will be canceled by the 
existing logic.
>
> Have you looked into how WSC does this?
I hadn't, but looks like it waits for a disconnected state before 
starting? We could do this too in DPP I guess. Just check if station is 
in any pending connecting states (connecting/connecting_auto), set a 
flag, and start DPP when station signals its disconnected.
>
>> ---
>>   src/dpp.c | 48 +++++++++++++++++++++++++++++++++++++++++++-----
>>   1 file changed, 43 insertions(+), 5 deletions(-)
>
> FYI, CI is failing the DPP test for this set.
>
> Regards,
> -Denis

Denis Kenzior July 19, 2024, 9 p.m. UTC | #3

Hi James,

>>
>> Have you looked into how WSC does this?
> I hadn't, but looks like it waits for a disconnected state before starting? We 
> could do this too in DPP I guess. Just check if station is in any pending 
> connecting states (connecting/connecting_auto), set a flag, and start DPP when 
> station signals its disconnected.

My view on this is that the user has explicitly indicated that they want to 
connect to a new network, so any ongoing autoconnect in station should be canceled.

Also, I think it would be good to be consistent between DPP and WSC.  In the end 
they implement the same use case.  Whatever we do for one should be the same for 
the other.

Regards,
-Denis

James Prestwood July 22, 2024, 11:23 a.m. UTC | #4

On 7/19/24 2:00 PM, Denis Kenzior wrote:
> Hi James,
>
>>>
>>> Have you looked into how WSC does this?
>> I hadn't, but looks like it waits for a disconnected state before 
>> starting? We could do this too in DPP I guess. Just check if station 
>> is in any pending connecting states (connecting/connecting_auto), set 
>> a flag, and start DPP when station signals its disconnected.
>
> My view on this is that the user has explicitly indicated that they 
> want to connect to a new network, so any ongoing autoconnect in 
> station should be canceled.
>
> Also, I think it would be good to be consistent between DPP and WSC.  
> In the end they implement the same use case.  Whatever we do for one 
> should be the same for the other.

Ok we can do this instead. I see that WSC will actually disconnect 
explicitly when its begins, and manages stations autoconnect state. This 
was my only concern with DPP as I didn't want starting an enrollee to 
cancel autoconnect in general in case of failure.

Thanks,

James

>
> Regards,
> -Denis

Patch

diff --git a/src/dpp.c b/src/dpp.c
index 567fe8d2..650e1e96 100644
--- a/src/dpp.c
+++ b/src/dpp.c
@@ -3744,14 +3744,14 @@  static void dpp_station_state_watch(enum station_state state, void *user_data)
 		return;
 
 	switch (state) {
-	case STATION_STATE_DISCONNECTED:
-	case STATION_STATE_DISCONNECTING:
 	case STATION_STATE_ROAMING:
 	case STATION_STATE_FT_ROAMING:
 	case STATION_STATE_FW_ROAMING:
 		if (L_WARN_ON(dpp->role == DPP_CAPABILITY_ENROLLEE))
 			dpp_reset(dpp);
-
+		/* fall through */
+	case STATION_STATE_DISCONNECTED:
+	case STATION_STATE_DISCONNECTING:
 		if (dpp->role == DPP_CAPABILITY_CONFIGURATOR) {
 			l_debug("Disconnected while configuring, stopping DPP");
 			dpp_reset(dpp);
@@ -3927,6 +3927,44 @@  static void dpp_start_presence(struct dpp_sm *dpp, uint32_t *limit_freqs,
 	dpp_start_offchannel(dpp, dpp->current_freq);
 }
 
+static bool dpp_can_start(struct station *station)
+{
+	enum station_state state = station_get_state(station);
+
+	/*
+	 * The obvious cases where DPP can start without a second thought are
+	 * disconnected/disconnecting/autoconnecting.
+	 *
+	 * The reason DPP is allowed to start while connecting, and even
+	 * netconfig is to handle misconfigured profiles e.g. an incorrect
+	 * passphrase. If IWD has been misconfigured either by a prior DPP run
+	 * or other means it remains in a connecting state failing repeatedly.
+	 * This will prevent DPP from being started, or cancel it each time a
+	 * connection is attempted. This will render the device unconfigurable
+	 * via DPP until that invalid profile is removed. Since all offchannel
+	 * work is gated by the wiphy work queue there should be no contention
+	 * between station trying to connect and DPP trying to configure.
+	 */
+
+	switch (state) {
+	case STATION_STATE_DISCONNECTED:
+	case STATION_STATE_DISCONNECTING:
+	case STATION_STATE_AUTOCONNECT_QUICK:
+	case STATION_STATE_AUTOCONNECT_FULL:
+	case STATION_STATE_CONNECTING:
+	case STATION_STATE_CONNECTING_AUTO:
+	case STATION_STATE_NETCONFIG:
+		return true;
+	case STATION_STATE_CONNECTED:
+	case STATION_STATE_ROAMING:
+	case STATION_STATE_FT_ROAMING:
+	case STATION_STATE_FW_ROAMING:
+		return false;
+	default:
+		return false;
+	}
+}
+
 static struct l_dbus_message *dpp_dbus_start_enrollee(struct l_dbus *dbus,
 						struct l_dbus_message *message,
 						void *user_data)
@@ -3943,7 +3981,7 @@  static struct l_dbus_message *dpp_dbus_start_enrollee(struct l_dbus *dbus,
 	 * Station isn't actually required for DPP itself, although this will
 	 * prevent connecting to the network once configured.
 	 */
-	if (station && station_get_connected_network(station)) {
+	if (station && !dpp_can_start(station)) {
 		l_warn("cannot be enrollee while connected, please disconnect");
 		return dbus_error_busy(message);
 	} else if (!station)
@@ -4370,7 +4408,7 @@  static struct l_dbus_message *dpp_dbus_pkex_start_enrollee(struct l_dbus *dbus,
 				dpp->interface != DPP_INTERFACE_UNBOUND)
 		return dbus_error_busy(message);
 
-	if (station && station_get_connected_network(station))
+	if (station && !dpp_can_start(station))
 		return dbus_error_busy(message);
 
 	if (!dpp_parse_pkex_args(message, &key, &id))