[PATCH 0/3] mac80211_hwsim: radio destruction fixes

[PATCH 0/3] mac80211_hwsim: radio destruction fixes

[Martin Willi]

This small series fixes two issues for cleaning up hwsim radios. The
first one is rather easy to hit when terminating namespaces with many
hwsim radios. The second one affects destroy-on-close users only.

Given that the use of a work-queue for deferred cleanup with namespaces
has been and still is tricky to get right, this series switches these
users to a synchronous cleanup in hwsim; The removal of that work-queue
is in a dedicated commit in case we want to skip that in backports.

Martin Willi (3):
  mac80211_hwsim: fix locking when iterating radios during ns exit
  mac80211_hwsim: fix race in radio destruction from netlink notifier
  mac80211_hwsim: drop now unused work-queue from hwsim

 drivers/net/wireless/mac80211_hwsim.c | 44 +++++++++++----------------
 1 file changed, 17 insertions(+), 27 deletions(-)

-- 
2.17.1

[PATCH 0/3] mac80211_hwsim: radio destruction fixes

[Martin Willi]

This small series fixes two issues for cleaning up hwsim radios. The
first one is rather easy to hit when terminating namespaces with many
hwsim radios. The second one affects destroy-on-close users only.

Given that the use of a work-queue for deferred cleanup with namespaces
has been and still is tricky to get right, this series switches these
users to a synchronous cleanup in hwsim; The removal of that work-queue
is in a dedicated commit in case we want to skip that in backports.

Martin Willi (3):
  mac80211_hwsim: fix locking when iterating radios during ns exit
  mac80211_hwsim: fix race in radio destruction from netlink notifier
  mac80211_hwsim: drop now unused work-queue from hwsim

 drivers/net/wireless/mac80211_hwsim.c | 44 +++++++++++----------------
 1 file changed, 17 insertions(+), 27 deletions(-)

-- 
2.17.1

[PATCH 1/3] mac80211_hwsim: fix locking when iterating radios during ns exit

[Martin Willi]

The cleanup of radios during namespace exit has recently been reworked
to directly delete a radio while temporarily releasing the spinlock,
fixing a race condition between the work-queue execution and namespace
exits. However, the temporary unlock allows unsafe modifications on the
iterated list, resulting in a potential crash when continuing the
iteration of additional radios.

Move radios about to destroy to a temporary list, and clean that up
after releasing the spinlock once iteration is complete.

Fixes: 8cfd36a0b53a ("mac80211_hwsim: fix use-after-free bug in hwsim_exit_net")
Signed-off-by: Martin Willi <martin@strongswan.org>
---
 drivers/net/wireless/mac80211_hwsim.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c
index f3863101af78..8c76e9a3499a 100644
--- a/drivers/net/wireless/mac80211_hwsim.c
+++ b/drivers/net/wireless/mac80211_hwsim.c
@@ -3642,6 +3642,7 @@ static __net_init int hwsim_init_net(struct net *net)
 static void __net_exit hwsim_exit_net(struct net *net)
 {
 	struct mac80211_hwsim_data *data, *tmp;
+	LIST_HEAD(list);
 
 	spin_lock_bh(&hwsim_radio_lock);
 	list_for_each_entry_safe(data, tmp, &hwsim_radios, list) {
@@ -3652,17 +3653,19 @@ static void __net_exit hwsim_exit_net(struct net *net)
 		if (data->netgroup == hwsim_net_get_netgroup(&init_net))
 			continue;
 
-		list_del(&data->list);
+		list_move(&data->list, &list);
 		rhashtable_remove_fast(&hwsim_radios_rht, &data->rht,
 				       hwsim_rht_params);
 		hwsim_radios_generation++;
-		spin_unlock_bh(&hwsim_radio_lock);
+	}
+	spin_unlock_bh(&hwsim_radio_lock);
+
+	list_for_each_entry_safe(data, tmp, &list, list) {
+		list_del(&data->list);
 		mac80211_hwsim_del_radio(data,
 					 wiphy_name(data->hw->wiphy),
 					 NULL);
-		spin_lock_bh(&hwsim_radio_lock);
 	}
-	spin_unlock_bh(&hwsim_radio_lock);
 
 	ida_simple_remove(&hwsim_netgroup_ida, hwsim_net_get_netgroup(net));
 }
-- 
2.17.1

[PATCH 2/3] mac80211_hwsim: fix race in radio destruction from netlink notifier

[Martin Willi]

The asynchronous destruction from a work-queue of radios tagged with
destroy-on-close may race with the owning namespace about to exit,
resulting in potential use-after-free of that namespace.

Instead of using a work-queue, move radios about to destroy to a
temporary list, which can be worked on synchronously after releasing
the lock. This should be safe to do from the netlink socket notifier,
as the namespace is guaranteed to not get released.

Signed-off-by: Martin Willi <martin@strongswan.org>
---
 drivers/net/wireless/mac80211_hwsim.c | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c
index 8c76e9a3499a..09fcd6911020 100644
--- a/drivers/net/wireless/mac80211_hwsim.c
+++ b/drivers/net/wireless/mac80211_hwsim.c
@@ -521,7 +521,6 @@ struct mac80211_hwsim_data {
 	int channels, idx;
 	bool use_chanctx;
 	bool destroy_on_close;
-	struct work_struct destroy_work;
 	u32 portid;
 	char alpha2[2];
 	const struct ieee80211_regdomain *regd;
@@ -3561,30 +3560,27 @@ static struct genl_family hwsim_genl_family __ro_after_init = {
 	.n_mcgrps = ARRAY_SIZE(hwsim_mcgrps),
 };
 
-static void destroy_radio(struct work_struct *work)
-{
-	struct mac80211_hwsim_data *data =
-		container_of(work, struct mac80211_hwsim_data, destroy_work);
-
-	hwsim_radios_generation++;
-	mac80211_hwsim_del_radio(data, wiphy_name(data->hw->wiphy), NULL);
-}
-
 static void remove_user_radios(u32 portid)
 {
 	struct mac80211_hwsim_data *entry, *tmp;
+	LIST_HEAD(list);
 
 	spin_lock_bh(&hwsim_radio_lock);
 	list_for_each_entry_safe(entry, tmp, &hwsim_radios, list) {
 		if (entry->destroy_on_close && entry->portid == portid) {
-			list_del(&entry->list);
+			list_move(&entry->list, &list);
 			rhashtable_remove_fast(&hwsim_radios_rht, &entry->rht,
 					       hwsim_rht_params);
-			INIT_WORK(&entry->destroy_work, destroy_radio);
-			queue_work(hwsim_wq, &entry->destroy_work);
+			hwsim_radios_generation++;
 		}
 	}
 	spin_unlock_bh(&hwsim_radio_lock);
+
+	list_for_each_entry_safe(entry, tmp, &list, list) {
+		list_del(&entry->list);
+		mac80211_hwsim_del_radio(entry, wiphy_name(entry->hw->wiphy),
+					 NULL);
+	}
 }
 
 static int mac80211_hwsim_netlink_notify(struct notifier_block *nb,
-- 
2.17.1

[PATCH 3/3] mac80211_hwsim: drop now unused work-queue from hwsim

[Martin Willi]

The work-queue was used for deferred destruction of hwsim radios;
this does not work well with namespaces about to exit. The one
remaining user has been migrated, so drop the now unused work-queue
instance.

Signed-off-by: Martin Willi <martin@strongswan.org>
---
 drivers/net/wireless/mac80211_hwsim.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c
index 09fcd6911020..70229a839c84 100644
--- a/drivers/net/wireless/mac80211_hwsim.c
+++ b/drivers/net/wireless/mac80211_hwsim.c
@@ -495,7 +495,6 @@ static const struct ieee80211_iface_combination hwsim_if_comb_p2p_dev[] = {
 
 static spinlock_t hwsim_radio_lock;
 static LIST_HEAD(hwsim_radios);
-static struct workqueue_struct *hwsim_wq;
 static struct rhashtable hwsim_radios_rht;
 static int hwsim_radio_idx;
 static int hwsim_radios_generation = 1;
@@ -3693,13 +3692,9 @@ static int __init init_mac80211_hwsim(void)
 
 	spin_lock_init(&hwsim_radio_lock);
 
-	hwsim_wq = alloc_workqueue("hwsim_wq", 0, 0);
-	if (!hwsim_wq)
-		return -ENOMEM;
-
 	err = rhashtable_init(&hwsim_radios_rht, &hwsim_rht_params);
 	if (err)
-		goto out_free_wq;
+		return err;
 
 	err = register_pernet_device(&hwsim_net_ops);
 	if (err)
@@ -3830,8 +3825,6 @@ static int __init init_mac80211_hwsim(void)
 	unregister_pernet_device(&hwsim_net_ops);
 out_free_rht:
 	rhashtable_destroy(&hwsim_radios_rht);
-out_free_wq:
-	destroy_workqueue(hwsim_wq);
 	return err;
 }
 module_init(init_mac80211_hwsim);
@@ -3843,12 +3836,10 @@ static void __exit exit_mac80211_hwsim(void)
 	hwsim_exit_netlink();
 
 	mac80211_hwsim_free();
-	flush_workqueue(hwsim_wq);
 
 	rhashtable_destroy(&hwsim_radios_rht);
 	unregister_netdev(hwsim_mon);
 	platform_driver_unregister(&mac80211_hwsim_driver);
 	unregister_pernet_device(&hwsim_net_ops);
-	destroy_workqueue(hwsim_wq);
 }
 module_exit(exit_mac80211_hwsim);
-- 
2.17.1

Re: [PATCH 0/3] mac80211_hwsim: radio destruction fixes

[Johannes Berg]

On Tue, 2018-09-25 at 09:41 +0200, Martin Willi wrote:
> This small series fixes two issues for cleaning up hwsim radios. The
> first one is rather easy to hit when terminating namespaces with many
> hwsim radios. The second one affects destroy-on-close users only.
> 
> Given that the use of a work-queue for deferred cleanup with namespaces
> has been and still is tricky to get right, this series switches these
> users to a synchronous cleanup in hwsim;

Cool, thanks. I wasn't really even aware of this, most our usage is with
VMs, not namespaces.

> The removal of that work-queue
> is in a dedicated commit in case we want to skip that in backports.

Yeah, also I'll probably stick that into -next only.

johannes

Re: [PATCH 0/3] mac80211_hwsim: radio destruction fixes

[Benjamin Beichler]

Am 25.09.2018 um 09:41 schrieb Martin Willi:
> This small series fixes two issues for cleaning up hwsim radios. The
> first one is rather easy to hit when terminating namespaces with many
> hwsim radios. The second one affects destroy-on-close users only.
>
> Given that the use of a work-queue for deferred cleanup with namespaces
> has been and still is tricky to get right, this series switches these
> users to a synchronous cleanup in hwsim; The removal of that work-queue
> is in a dedicated commit in case we want to skip that in backports.

Indeed, this is a much better solution. I wasn't that happy with the
workqueue thing, but I didn't know a better solution :-)

I see no errors at review, but I currently cannot test it quickly.

> Martin Willi (3):
>   mac80211_hwsim: fix locking when iterating radios during ns exit
>   mac80211_hwsim: fix race in radio destruction from netlink notifier
>   mac80211_hwsim: drop now unused work-queue from hwsim
>
>  drivers/net/wireless/mac80211_hwsim.c | 44 +++++++++++----------------
>  1 file changed, 17 insertions(+), 27 deletions(-)
>
kind regards

Benjamin

--
M.Sc. Benjamin Beichler

Universität Rostock, Fakultät für Informatik und Elektrotechnik
Institut für Angewandte Mikroelektronik und Datentechnik

University of Rostock, Department of CS and EE
Institute of Applied Microelectronics and CE

Richard-Wagner-Straße 31
18119 Rostock
Deutschland/Germany

phone: +49 (0) 381 498 - 7278
email: Benjamin.Beichler@uni-rostock.de
www: http://www.imd.uni-rostock.de/