[PATCH net-next 0/2] Converting pernet_operations (part #3) (wireless, mac80211_hwsim)

[PATCH net-next 0/2] Converting pernet_operations (part #3) (wireless, mac80211_hwsim)

[Kirill Tkhai]

These series fixes ID overflow of hwsim_netgroup [1/2],
and reviews and makes hwsim_net_ops as async [2/2].

The first pernet_operations requiring changes to make
the converting safe!

([1/2] may be need to [net], but better people see it's really need).

Thanks,
Kirill

---

Kirill Tkhai (2):
      mac80211_hwsim: Make hwsim_netgroup IDA
      net: Convert hwsim_net_ops


 drivers/net/wireless/mac80211_hwsim.c |   15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

--
Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>

[PATCH net-next 1/2] mac80211_hwsim: Make hwsim_netgroup IDA

[Kirill Tkhai]

hwsim_netgroup counter is declarated as int, and it is incremented
every time a new net is created. After sizeof(int) net are created,
it will overflow, and different net namespaces will have the same
identifier. This patch fixes the problem by introducing IDA instead
of int counter. IDA guarantees, all the net namespaces have the uniq
identifier.

Note, that after we do ida_simple_remove() in hwsim_exit_net(),
and we destroy the ID, later there may be executed destroy_radio()
from the workqueue. But destroy_radio() does not use the ID, so it's OK.

Out of bounds of this patch, just as a report to wireless subsystem
maintainer, destroy_radio() increaments hwsim_radios_generation
without hwsim_radio_lock, so this may need one more patch to fix.

Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
---
 drivers/net/wireless/mac80211_hwsim.c |   14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c
index 3c64afa161bf..45ba846bc285 100644
--- a/drivers/net/wireless/mac80211_hwsim.c
+++ b/drivers/net/wireless/mac80211_hwsim.c
@@ -253,7 +253,7 @@ static inline void hwsim_clear_chanctx_magic(struct ieee80211_chanctx_conf *c)
 
 static unsigned int hwsim_net_id;
 
-static int hwsim_netgroup;
+static struct ida hwsim_netgroup_ida = IDA_INIT;
 
 struct hwsim_net {
 	int netgroup;
@@ -267,11 +267,13 @@ static inline int hwsim_net_get_netgroup(struct net *net)
 	return hwsim_net->netgroup;
 }
 
-static inline void hwsim_net_set_netgroup(struct net *net)
+static inline int hwsim_net_set_netgroup(struct net *net)
 {
 	struct hwsim_net *hwsim_net = net_generic(net, hwsim_net_id);
 
-	hwsim_net->netgroup = hwsim_netgroup++;
+	hwsim_net->netgroup = ida_simple_get(&hwsim_netgroup_ida,
+					     0, 0, GFP_KERNEL);
+	return hwsim_net->netgroup >= 0 ? 0 : -ENOMEM;
 }
 
 static inline u32 hwsim_net_get_wmediumd(struct net *net)
@@ -3507,9 +3509,7 @@ static int __init hwsim_init_netlink(void)
 
 static __net_init int hwsim_init_net(struct net *net)
 {
-	hwsim_net_set_netgroup(net);
-
-	return 0;
+	return hwsim_net_set_netgroup(net);
 }
 
 static void __net_exit hwsim_exit_net(struct net *net)
@@ -3532,6 +3532,8 @@ static void __net_exit hwsim_exit_net(struct net *net)
 		queue_work(hwsim_wq, &data->destroy_work);
 	}
 	spin_unlock_bh(&hwsim_radio_lock);
+
+	ida_simple_remove(&hwsim_netgroup_ida, hwsim_net_get_netgroup(net));
 }
 
 static struct pernet_operations hwsim_net_ops = {

Re: [PATCH net-next 1/2] mac80211_hwsim: Make hwsim_netgroup IDA

[Benjamin Beichler]

Am 01.03.2018 um 12:30 schrieb Kirill Tkhai:

> Out of bounds of this patch, just as a report to wireless subsystem
> maintainer, destroy_radio() increaments hwsim_radios_generation
> without hwsim_radio_lock, so this may need one more patch to fix.
>
The lock is here implicit, because the value only could change at init
(where netlink callbacks are deactivated and always happens sequential)
or in netlink callbacks. The only reader of this variable is the dump
callback, and the only other writers are also netlink callbacks and
because they are implicitly not parallel (because the parallel flag is
not set), there could be no race condition. Maybe this should be
documented somehow, especially if somebody got the idea to allow
parallel callbacks :-)

-- 
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/

Re: [PATCH net-next 1/2] mac80211_hwsim: Make hwsim_netgroup IDA

[Kirill Tkhai]

On 01.03.2018 20:22, Benjamin Beichler wrote:
> Am 01.03.2018 um 12:30 schrieb Kirill Tkhai:
> 
>> Out of bounds of this patch, just as a report to wireless subsystem
>> maintainer, destroy_radio() increaments hwsim_radios_generation
>> without hwsim_radio_lock, so this may need one more patch to fix.
>>
> The lock is here implicit, because the value only could change at init
> (where netlink callbacks are deactivated and always happens sequential)
> or in netlink callbacks. The only reader of this variable is the dump
> callback, and the only other writers are also netlink callbacks and
> because they are implicitly not parallel (because the parallel flag is
> not set), there could be no race condition. Maybe this should be
> documented somehow, especially if somebody got the idea to allow
> parallel callbacks :-)


static void hwsim_exit_net(...)
{
	...
	INIT_WORK(&data->destroy_work, destroy_radio);
        queue_work(hwsim_wq, &data->destroy_work);
	...
}

destroy_radio() may be executed in parallel with everything above you wrote,
doesn't it? There may be several network namespaces, and destroy_radio()
queued from one net namespace may race with mac80211_hwsim_new_radio()
or hwsim_del_radio_nl() for another net namespace. I don't see, how netlink
locking can act on synchronization with a work. This is what I mention.

Thanks,
Kirill

Re: [PATCH net-next 1/2] mac80211_hwsim: Make hwsim_netgroup IDA

[Benjamin Beichler]

Am 2. März 2018 12:37:25 MEZ schrieb Kirill Tkhai <ktkhai@virtuozzo.com>:
>destroy_radio() may be executed in parallel with everything above you
>wrote,
>doesn't it? There may be several network namespaces, and
>destroy_radio()
>queued from one net namespace may race with mac80211_hwsim_new_radio()
>or hwsim_del_radio_nl() for another net namespace. I don't see, how
>netlink
>locking can act on synchronization with a work. This is what I mention.
>
I see, you are right. Nonetheless, this value is pretty uncritical, since the user (the netlink dump) only checks whether it changes within a dump and even if there would be race conditions, e.g. some generations would be skipped caused by parallel writing, it would also set the dump interrupted flag, and the user space program knows, if it needs exact results, it needs to dump again. I'm unsure about things like caching of this variable. Maybe it needs a volatile flag to work always as expected.

Unfortunately, currently the code triggers a dump interrupted also when the interfaces of the current namespace didn't change, but I think that is acceptable. Otherwise we need a per namespace generation and I think all this happens really rare and it's not worth the effort.


>Thanks,
>Kirill

-- 
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/

[PATCH net-next 2/2] net: Convert hwsim_net_ops

[Kirill Tkhai]

These pernet_operations allocate and destroy IDA identifier,
and these actions are synchronized by IDA subsystem locks.
Exit method removes mac80211_hwsim_data enteries from the lists,
and this is synchronized by hwsim_radio_lock with the rest
parallel pernet_operations. Also it queues destroy_radio()
work, and these work already may be executed in parallel
with any pernet_operations (as it's a work :). So, we may
mark these pernet_operations as async.

Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
---
 drivers/net/wireless/mac80211_hwsim.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c
index 45ba846bc285..7b6c3640a94f 100644
--- a/drivers/net/wireless/mac80211_hwsim.c
+++ b/drivers/net/wireless/mac80211_hwsim.c
@@ -3541,6 +3541,7 @@ static struct pernet_operations hwsim_net_ops = {
 	.exit = hwsim_exit_net,
 	.id   = &hwsim_net_id,
 	.size = sizeof(struct hwsim_net),
+	.async = true,
 };
 
 static void hwsim_exit_netlink(void)