iwd.lists.linux.dev archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 1/3] wiphy: make wiphy work queue reentrant
@ 2023-05-11 18:52 James Prestwood
  2023-05-11 18:52 ` [PATCH v2 2/3] auto-t: modify PSK-roam test to use FT failure path James Prestwood
                   ` (2 more replies)
  0 siblings, 3 replies; 4+ messages in thread
From: James Prestwood @ 2023-05-11 18:52 UTC (permalink / raw)
  To: iwd; +Cc: James Prestwood

In some situations its convenient for the same work item to be
inserted (rescheduled) while its in progress. FT for example does
this now if a roam fails. The same ft_work item gets re-inserted
which, currently, is not safe to do since the item is modified
and removed once completed.

Currently FT reschedules the same work item in the 'do_work'
callback but this patch actually allows for rescheduling as long
as the item is currently running, even if do_work has returned and
waiting for wiphy_radio_work_done.

A few new flags were added to the radio work item. One which is
set when the item is running which prevents other items from being
inserted ahead (previously done with priority=INT_MIN). This is
needed since we cannot modify the priority anymore in case the item
is being rescheduled.

The another flag is set when the item was rescheduled. Once the
item is finished the 'rescheduled' flag is checked and the item
will be removed and re-inserted into the queue if needed.
---
 src/wiphy.c | 69 ++++++++++++++++++++++++++++++++++++++++++-----------
 src/wiphy.h |  2 ++
 2 files changed, 57 insertions(+), 14 deletions(-)

diff --git a/src/wiphy.c b/src/wiphy.c
index 2db2d2cd..e6ca3b76 100644
--- a/src/wiphy.c
+++ b/src/wiphy.c
@@ -2570,6 +2570,17 @@ static void wiphy_reg_notify(struct l_genl_msg *msg, void *user_data)
 	wiphy_dump_after_regdom(wiphy);
 }
 
+static int insert_by_priority(const void *a, const void *b, void *user_data)
+{
+	const struct wiphy_radio_work_item *new = a;
+	const struct wiphy_radio_work_item *work = b;
+
+	if (work->running || work->priority <= new->priority)
+		return 1;
+
+	return -1;
+}
+
 static void wiphy_radio_work_next(struct wiphy *wiphy)
 {
 	struct wiphy_radio_work_item *work;
@@ -2583,7 +2594,7 @@ static void wiphy_radio_work_next(struct wiphy *wiphy)
 	 * Ensures no other work item will get inserted before this one while
 	 * the work is being done.
 	 */
-	work->priority = INT_MIN;
+	work->running = true;
 
 	l_debug("Starting work item %u", work->id);
 
@@ -2592,7 +2603,12 @@ static void wiphy_radio_work_next(struct wiphy *wiphy)
 	wiphy->work_in_callback = false;
 
 	if (done) {
-		work->id = 0;
+		bool rescheduled = work->rescheduled;
+
+		work->running = false;
+
+		if (!rescheduled)
+			work->id = 0;
 
 		l_queue_remove(wiphy->work, work);
 
@@ -2600,26 +2616,49 @@ static void wiphy_radio_work_next(struct wiphy *wiphy)
 		destroy_work(work);
 		wiphy->work_in_callback = false;
 
+		/*
+		 * If the item was rescheduled inside do_work() we can safely
+		 * insert it here, otherwise destroy_work() could have freed it.
+		 * The item could have been re-inserted inside destroy_work()
+		 * but this is safe since the item was removed from the queue.
+		 */
+		if (rescheduled) {
+			work->rescheduled = false;
+			l_queue_insert(wiphy->work, work,
+					insert_by_priority, NULL);
+		}
+
 		wiphy_radio_work_next(wiphy);
 	}
 }
 
-static int insert_by_priority(const void *a, const void *b, void *user_data)
-{
-	const struct wiphy_radio_work_item *new = a;
-	const struct wiphy_radio_work_item *work = b;
-
-	if (work->priority <= new->priority)
-		return 1;
-
-	return -1;
-}
-
 uint32_t wiphy_radio_work_insert(struct wiphy *wiphy,
 				struct wiphy_radio_work_item *item,
 				int priority,
 				const struct wiphy_radio_work_item_ops *ops)
 {
+	/*
+	 * Handling the case of re-inserting the same work item that is in
+	 * progress. A non-started work item should never be re-inserted
+	 * into the queue. Keep the same ID, priority, and ops. If these somehow
+	 * are different the caller should really be using a separate work item.
+	 * Once the item is finished it will be removed and re-inserted based
+	 * on the rescheduled flag.
+	 */
+	if (item == l_queue_peek_head(wiphy->work)) {
+		/*
+		 * Shouldn't cause problems, but at least warn the caller they
+		 * should really be using a separate item
+		 */
+		L_WARN_ON(item->rescheduled || item->priority != priority ||
+				ops != item->ops);
+
+		item->rescheduled = true;
+		l_debug("Rescheduled work item %u", item->id);
+
+		return item->id;
+	}
+
 	item->priority = priority;
 	item->ops = ops;
 	item->id = ++work_ids;
@@ -2656,6 +2695,7 @@ void wiphy_radio_work_done(struct wiphy *wiphy, uint32_t id)
 	if (item->id == id) {
 		next = true;
 		l_queue_pop_head(wiphy->work);
+		item->running = false;
 	} else
 		item = l_queue_remove_if(wiphy->work, match_id,
 						L_UINT_TO_PTR(id));
@@ -2664,7 +2704,8 @@ void wiphy_radio_work_done(struct wiphy *wiphy, uint32_t id)
 
 	l_debug("Work item %u done", id);
 
-	item->id = 0;
+	if (!item->rescheduled)
+		item->id = 0;
 
 	wiphy->work_in_callback = true;
 	destroy_work(item);
diff --git a/src/wiphy.h b/src/wiphy.h
index d5d1cc8f..b6861f5f 100644
--- a/src/wiphy.h
+++ b/src/wiphy.h
@@ -45,6 +45,8 @@ struct wiphy_radio_work_item {
 	uint32_t id;
 	int priority;
 	const struct wiphy_radio_work_item_ops *ops;
+	bool rescheduled : 1;
+	bool running : 1;
 };
 
 enum {
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* [PATCH v2 2/3] auto-t: modify PSK-roam test to use FT failure path
  2023-05-11 18:52 [PATCH v2 1/3] wiphy: make wiphy work queue reentrant James Prestwood
@ 2023-05-11 18:52 ` James Prestwood
  2023-05-11 18:52 ` [PATCH v2 3/3] auto-t: increase timeout in testPSK-roam James Prestwood
  2023-05-17  1:16 ` [PATCH v2 1/3] wiphy: make wiphy work queue reentrant Denis Kenzior
  2 siblings, 0 replies; 4+ messages in thread
From: James Prestwood @ 2023-05-11 18:52 UTC (permalink / raw)
  To: iwd; +Cc: James Prestwood

This adds another radio so IWD hits the FT failure path after
authentication to the first BSS fails. This causes a wiphy work
item to be rescheduled which previously was unsafe.
---
 autotests/testPSK-roam/connection_test.py     | 13 ++++---
 autotests/testPSK-roam/failed_roam_test.py    | 33 ++++++++++------
 autotests/testPSK-roam/ft-psk-ccmp-3.conf     | 39 +++++++++++++++++++
 autotests/testPSK-roam/hw.conf                |  3 +-
 .../testPSK-roam/roam_ap_disconnect_test.py   |  4 +-
 5 files changed, 74 insertions(+), 18 deletions(-)
 create mode 100644 autotests/testPSK-roam/ft-psk-ccmp-3.conf

v2:
 * Added the hostapd configuration which was missing from the last
   patch set.

diff --git a/autotests/testPSK-roam/connection_test.py b/autotests/testPSK-roam/connection_test.py
index 7a135e95..459c25cf 100644
--- a/autotests/testPSK-roam/connection_test.py
+++ b/autotests/testPSK-roam/connection_test.py
@@ -48,7 +48,7 @@ class Test(unittest.TestCase):
             self.rule2.enabled = True
             # Send 100 packets (to be dropped), should trigger beacon loss
             testutil.tx_packets(device.name, self.bss_hostapd[0].ifname, 100)
-            device.wait_for_event('packet-loss-roam', timeout=30)
+            device.wait_for_event('packet-loss-roam', timeout=60)
         else:
             device.roam(self.bss_hostapd[1].bssid)
 
@@ -173,19 +173,22 @@ class Test(unittest.TestCase):
         IWD.copy_to_storage('TestFT.psk')
 
         cls.bss_hostapd = [ HostapdCLI(config='ft-psk-ccmp-1.conf'),
-                            HostapdCLI(config='ft-psk-ccmp-2.conf') ]
+                            HostapdCLI(config='ft-psk-ccmp-2.conf'),
+                            HostapdCLI(config='ft-psk-ccmp-3.conf') ]
+        cls.bss_hostapd[2].disable()
+
         rad0 = hwsim.get_radio('rad0')
-        rad2 = hwsim.get_radio('rad2')
+        rad3 = hwsim.get_radio('rad3')
 
         cls.rule0 = hwsim.rules.create()
-        cls.rule0.source = rad2.addresses[0]
+        cls.rule0.source = rad3.addresses[0]
         cls.rule0.bidirectional = True
         cls.rule0.signal = -2000
         cls.rule0.prefix = 'b0'
         cls.rule0.drop = True
 
         cls.rule1 = hwsim.rules.create()
-        cls.rule1.source = rad2.addresses[0]
+        cls.rule1.source = rad3.addresses[0]
         cls.rule1.prefix = '08'
         cls.rule1.drop = True
 
diff --git a/autotests/testPSK-roam/failed_roam_test.py b/autotests/testPSK-roam/failed_roam_test.py
index 360c1de5..eda2b4e2 100644
--- a/autotests/testPSK-roam/failed_roam_test.py
+++ b/autotests/testPSK-roam/failed_roam_test.py
@@ -62,21 +62,17 @@ class Test(unittest.TestCase):
 
         self.rule0.enabled = True
 
-        device.roam(self.bss_hostapd[1].bssid)
-
-        # Roam should fail...
-        device.wait_for_event('ft-roam-failed')
+        # IWD should connect, then attempt a roam to BSS 1, which should fail...
+        device.wait_for_event('ft-roam-failed', timeout=60)
         # ... but IWD should remain connected
         self.assertTrue(device.state == DeviceState.connected)
 
         self.rule0.enabled = False
 
-        # Try again once more
-        device.roam(self.bss_hostapd[1].bssid)
-
-        self.verify_roam(wd, device, self.bss_hostapd[0], self.bss_hostapd[1])
+        # IWD should then try BSS 2, and succeed
+        self.verify_roam(wd, device, self.bss_hostapd[0], self.bss_hostapd[2])
 
-        self.bss_hostapd[1].deauthenticate(device.address)
+        self.bss_hostapd[2].deauthenticate(device.address)
         condition = 'obj.state == DeviceState.disconnected'
         wd.wait_for_object_condition(device, condition)
 
@@ -88,6 +84,7 @@ class Test(unittest.TestCase):
 
         self.rule0.enabled = False
         self.rule1.enabled = False
+        self.rule2.enabled = False
 
     @classmethod
     def setUpClass(cls):
@@ -96,14 +93,16 @@ class Test(unittest.TestCase):
         IWD.copy_to_storage('TestFT.psk')
 
         cls.bss_hostapd = [ HostapdCLI(config='ft-psk-ccmp-1.conf'),
-                            HostapdCLI(config='ft-psk-ccmp-2.conf') ]
+                            HostapdCLI(config='ft-psk-ccmp-2.conf'),
+                            HostapdCLI(config='ft-psk-ccmp-3.conf') ]
 
         cls.bss_hostapd[0].set_address('12:00:00:00:00:01')
         cls.bss_hostapd[1].set_address('12:00:00:00:00:02')
+        cls.bss_hostapd[2].set_address('12:00:00:00:00:03')
 
         # Drop Authenticate frames
         cls.rule0 = hwsim.rules.create()
-        cls.rule0.bidirectional = True
+        cls.rule0.source = hwsim.get_radio('rad1').addresses[0]
         cls.rule0.prefix = 'b0'
         cls.rule0.drop = True
 
@@ -113,6 +112,18 @@ class Test(unittest.TestCase):
         cls.rule1.prefix = 'd0'
         cls.rule1.drop = True
 
+        # Causes IWD to immediately roam away from BSS 0
+        cls.rule2 = hwsim.rules.create()
+        cls.rule2.source = hwsim.get_radio('rad0').addresses[0]
+        cls.rule2.signal = -8000
+        cls.rule2.enabled = True
+
+        # Causes IWD to first prefer BSS 1 to roam, then BSS 2.
+        cls.rule3 = hwsim.rules.create()
+        cls.rule3.source = hwsim.get_radio('rad2').addresses[0]
+        cls.rule3.signal = -7000
+        cls.rule3.enabled = True
+
     @classmethod
     def tearDownClass(cls):
         IWD.clear_storage()
diff --git a/autotests/testPSK-roam/ft-psk-ccmp-3.conf b/autotests/testPSK-roam/ft-psk-ccmp-3.conf
new file mode 100644
index 00000000..a42e175d
--- /dev/null
+++ b/autotests/testPSK-roam/ft-psk-ccmp-3.conf
@@ -0,0 +1,39 @@
+hw_mode=g
+channel=2
+ssid=TestFT
+utf8_ssid=1
+ctrl_interface=/var/run/hostapd
+
+r1_key_holder=120000000002
+nas_identifier=dummy2
+
+wpa=2
+# Can support WPA-PSK and FT-PSK (space separated list) and/or EAP at the same
+# time but we want to force FT
+wpa_key_mgmt=FT-PSK
+wpa_pairwise=CCMP
+wpa_passphrase=EasilyGuessedPassword
+ieee80211w=1
+rsn_preauth=1
+rsn_preauth_interfaces=lo
+disable_pmksa_caching=0
+# Allow PMK cache to be shared opportunistically among configured interfaces
+# and BSSes (i.e., all configurations within a single hostapd process).
+okc=1
+mobility_domain=1234
+reassociation_deadline=60000
+r0kh=12:00:00:00:00:01 dummy1 000102030405060708090a0b0c0d0e0f
+r0kh=12:00:00:00:00:02 dummy2 000102030405060708090a0b0c0d0e0f
+r1kh=12:00:00:00:00:01 00:00:00:00:00:01 000102030405060708090a0b0c0d0e0f
+r1kh=12:00:00:00:00:02 00:00:00:00:00:02 000102030405060708090a0b0c0d0e0f
+# Push mode only needed for 8021x, not PSK mode since msk already known
+pmk_r1_push=0
+# Allow locally generated FT response so we don't have to configure push/pull
+# between BSSes running as separate hostapd processes as in the test-runner
+# case.  Only works with FT-PSK, otherwise brctl needs to be installed and
+# CONFIG_BRIDGE enabled in the kernel.
+ft_psk_generate_local=1
+ft_over_ds=0
+ap_table_expiration_time=36000
+ap_table_max_size=10
+rrm_neighbor_report=1
diff --git a/autotests/testPSK-roam/hw.conf b/autotests/testPSK-roam/hw.conf
index c2b35d6e..da9e385c 100644
--- a/autotests/testPSK-roam/hw.conf
+++ b/autotests/testPSK-roam/hw.conf
@@ -1,8 +1,9 @@
 [SETUP]
-num_radios=3
+num_radios=4
 start_iwd=0
 hwsim_medium=yes
 
 [HOSTAPD]
 rad0=ft-psk-ccmp-1.conf
 rad1=ft-psk-ccmp-2.conf
+rad2=ft-psk-ccmp-3.conf
diff --git a/autotests/testPSK-roam/roam_ap_disconnect_test.py b/autotests/testPSK-roam/roam_ap_disconnect_test.py
index 416541ce..130e1702 100644
--- a/autotests/testPSK-roam/roam_ap_disconnect_test.py
+++ b/autotests/testPSK-roam/roam_ap_disconnect_test.py
@@ -70,8 +70,10 @@ class Test(unittest.TestCase):
         IWD.copy_to_storage('TestFT.psk')
 
         cls.bss_hostapd = [ HostapdCLI(config='ft-psk-ccmp-1.conf'),
-                            HostapdCLI(config='ft-psk-ccmp-2.conf') ]
+                            HostapdCLI(config='ft-psk-ccmp-2.conf'),
+                            HostapdCLI(config='ft-psk-ccmp-3.conf') ]
         cls.bss_hostapd[1].disable()
+        cls.bss_hostapd[2].disable()
 
         cls.bss_hostapd[0].set_value('ocv', '0')
         cls.bss_hostapd[0].set_value('ieee80211w', '0')
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* [PATCH v2 3/3] auto-t: increase timeout in testPSK-roam
  2023-05-11 18:52 [PATCH v2 1/3] wiphy: make wiphy work queue reentrant James Prestwood
  2023-05-11 18:52 ` [PATCH v2 2/3] auto-t: modify PSK-roam test to use FT failure path James Prestwood
@ 2023-05-11 18:52 ` James Prestwood
  2023-05-17  1:16 ` [PATCH v2 1/3] wiphy: make wiphy work queue reentrant Denis Kenzior
  2 siblings, 0 replies; 4+ messages in thread
From: James Prestwood @ 2023-05-11 18:52 UTC (permalink / raw)
  To: iwd; +Cc: James Prestwood

This was causing random failures and increasing the timeout seems
to be a lot more reliable.
---
 autotests/testPSK-roam/roam_ap_disconnect_test.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/autotests/testPSK-roam/roam_ap_disconnect_test.py b/autotests/testPSK-roam/roam_ap_disconnect_test.py
index 130e1702..b5775990 100644
--- a/autotests/testPSK-roam/roam_ap_disconnect_test.py
+++ b/autotests/testPSK-roam/roam_ap_disconnect_test.py
@@ -33,7 +33,7 @@ class Test(unittest.TestCase):
         # Since both BSS's have low signal, the roam should fail and trigger
         # another roam scan.
         device.wait_for_event('roam-scan-triggered', timeout=30)
-        device.wait_for_event('no-roam-candidates', timeout=30)
+        device.wait_for_event('no-roam-candidates', timeout=60)
 
         # Hostapd sends disconnect
         self.bss_hostapd[0].disable()
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH v2 1/3] wiphy: make wiphy work queue reentrant
  2023-05-11 18:52 [PATCH v2 1/3] wiphy: make wiphy work queue reentrant James Prestwood
  2023-05-11 18:52 ` [PATCH v2 2/3] auto-t: modify PSK-roam test to use FT failure path James Prestwood
  2023-05-11 18:52 ` [PATCH v2 3/3] auto-t: increase timeout in testPSK-roam James Prestwood
@ 2023-05-17  1:16 ` Denis Kenzior
  2 siblings, 0 replies; 4+ messages in thread
From: Denis Kenzior @ 2023-05-17  1:16 UTC (permalink / raw)
  To: James Prestwood, iwd

Hi James,

On 5/11/23 13:52, James Prestwood wrote:
> In some situations its convenient for the same work item to be
> inserted (rescheduled) while its in progress. FT for example does
> this now if a roam fails. The same ft_work item gets re-inserted
> which, currently, is not safe to do since the item is modified
> and removed once completed.
> 
> Currently FT reschedules the same work item in the 'do_work'
> callback but this patch actually allows for rescheduling as long
> as the item is currently running, even if do_work has returned and
> waiting for wiphy_radio_work_done.
> 
> A few new flags were added to the radio work item. One which is
> set when the item is running which prevents other items from being
> inserted ahead (previously done with priority=INT_MIN). This is
> needed since we cannot modify the priority anymore in case the item
> is being rescheduled.

This seems way too complicated.

> 
> The another flag is set when the item was rescheduled. Once the
> item is finished the 'rescheduled' flag is checked and the item
> will be removed and re-inserted into the queue if needed.
> ---
>   src/wiphy.c | 69 ++++++++++++++++++++++++++++++++++++++++++-----------
>   src/wiphy.h |  2 ++
>   2 files changed, 57 insertions(+), 14 deletions(-)
> 

<snip>

> @@ -2583,7 +2594,7 @@ static void wiphy_radio_work_next(struct wiphy *wiphy)
>   	 * Ensures no other work item will get inserted before this one while
>   	 * the work is being done.
>   	 */
> -	work->priority = INT_MIN;
> +	work->running = true;
>   
>   	l_debug("Starting work item %u", work->id);
>   

Can't you simply take note of the current item id, set the priority, take it off 
the queue here, prior to starting.

> @@ -2592,7 +2603,12 @@ static void wiphy_radio_work_next(struct wiphy *wiphy)
>   	wiphy->work_in_callback = false;
>   

If done and work id changed, there's nothing to do.

If done, then proceed as now.

If not done, reinsert the work to the head.

>   	if (done) {
> -		work->id = 0;
> +		bool rescheduled = work->rescheduled;
> +
> +		work->running = false;
> +
> +		if (!rescheduled)
> +			work->id = 0;
>   
>   		l_queue_remove(wiphy->work, work);
>   
> @@ -2600,26 +2616,49 @@ static void wiphy_radio_work_next(struct wiphy *wiphy)
>   		destroy_work(work);

I'm really not sure this makes sense.  You are calling destroy on an item you 
just potentially reinserted.  I think a reinserted item should not trigger the 
destroy.  The caller has full control over the item anyway, so they can take 
care of this on their end.  In fact, I'd still think that the caller should use 
a different API for this to be absolutely clear, even if it is just 
wiphy_radio_work_insert() with some extra checks.

Regards,
-Denis

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2023-05-17  1:16 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-05-11 18:52 [PATCH v2 1/3] wiphy: make wiphy work queue reentrant James Prestwood
2023-05-11 18:52 ` [PATCH v2 2/3] auto-t: modify PSK-roam test to use FT failure path James Prestwood
2023-05-11 18:52 ` [PATCH v2 3/3] auto-t: increase timeout in testPSK-roam James Prestwood
2023-05-17  1:16 ` [PATCH v2 1/3] wiphy: make wiphy work queue reentrant Denis Kenzior

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).