[PATCH v2 1/5] mac80211_hwsim: wait for deferred radio deletion on mod unload

[PATCH v2 1/5] mac80211_hwsim: wait for deferred radio deletion on mod unload

[Benjamin Beichler]

When closing multiple wmediumd instances with many radios and try to
unload the mac80211_hwsim module it may happen that the work items live
longer than the module. This patch does not mitigate completely the
problem, since we need to delete hwsim_data struct from delete queue
(since afterwards the reference is not valid anymore) at the beginning of
the work function and it may be interrupted in between. But this problem
only occurs for the last (or only) item of the delete list. We could either
create a dedicated work queue or call flush_scheduled_work, but both
introduce unnecessary overhead.

Signed-off-by: Benjamin Beichler <benjamin.beichler@uni-rostock.de>
---
 drivers/net/wireless/mac80211_hwsim.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c
index ec2f4c31425a..d35dc6b2a733 100644
--- a/drivers/net/wireless/mac80211_hwsim.c
+++ b/drivers/net/wireless/mac80211_hwsim.c
@@ -489,6 +489,8 @@ static const struct ieee80211_iface_combination hwsim_if_comb_p2p_dev[] = {
 
 static spinlock_t hwsim_radio_lock;
 static LIST_HEAD(hwsim_radios);
+static spinlock_t hwsim_delete_lock;
+static LIST_HEAD(delete_radios);
 static int hwsim_radio_idx;
 
 static struct platform_driver mac80211_hwsim_driver = {
@@ -3326,7 +3328,11 @@ static void destroy_radio(struct work_struct *work)
 	struct mac80211_hwsim_data *data =
 		container_of(work, struct mac80211_hwsim_data, destroy_work);
 
+	spin_lock_bh(&hwsim_delete_lock);
+	list_del(&data->list);
+	spin_unlock_bh(&hwsim_delete_lock);
 	mac80211_hwsim_del_radio(data, wiphy_name(data->hw->wiphy), NULL);
+
 }
 
 static void remove_user_radios(u32 portid)
@@ -3337,6 +3343,11 @@ static void remove_user_radios(u32 portid)
 	list_for_each_entry_safe(entry, tmp, &hwsim_radios, list) {
 		if (entry->destroy_on_close && entry->portid == portid) {
 			list_del(&entry->list);
+
+			spin_lock_bh(&hwsim_delete_lock);
+			list_add(&entry->list, &delete_radios);
+			spin_unlock_bh(&hwsim_delete_lock);
+
 			INIT_WORK(&entry->destroy_work, destroy_radio);
 			schedule_work(&entry->destroy_work);
 		}
@@ -3412,6 +3423,11 @@ static void __net_exit hwsim_exit_net(struct net *net)
 			continue;
 
 		list_del(&data->list);
+
+		spin_lock_bh(&hwsim_delete_lock);
+		list_add(&data->list, &delete_radios);
+		spin_unlock_bh(&hwsim_delete_lock);
+
 		INIT_WORK(&data->destroy_work, destroy_radio);
 		schedule_work(&data->destroy_work);
 	}
@@ -3444,6 +3460,7 @@ static int __init init_mac80211_hwsim(void)
 		return -EINVAL;
 
 	spin_lock_init(&hwsim_radio_lock);
+	spin_lock_init(&hwsim_delete_lock);
 
 	err = register_pernet_device(&hwsim_net_ops);
 	if (err)
@@ -3583,6 +3600,19 @@ static void __exit exit_mac80211_hwsim(void)
 	hwsim_exit_netlink();
 
 	mac80211_hwsim_free();
+
+	/*wait for radios with deferred delete*/
+	spin_lock_bh(&hwsim_delete_lock);
+	while (!list_empty(&delete_radios)) {
+		pr_debug("mac80211_hwsim: wait for deferred radio remove\n");
+		spin_unlock_bh(&hwsim_delete_lock);
+		flush_work(&list_entry(&delete_radios,
+				       struct mac80211_hwsim_data, list)
+			   ->destroy_work);
+		spin_lock_bh(&hwsim_delete_lock);
+	}
+	spin_unlock_bh(&hwsim_delete_lock);
+
 	unregister_netdev(hwsim_mon);
 	platform_driver_unregister(&mac80211_hwsim_driver);
 	unregister_pernet_device(&hwsim_net_ops);
-- 
2.15.0

Re: [PATCH v2 1/5] mac80211_hwsim: wait for deferred radio deletion on mod unload

[Johannes Berg]

On Tue, 2017-11-21 at 13:17 +0100, Benjamin Beichler wrote:
> 
> +	/*wait for radios with deferred delete*/

please add spaces there

> +	spin_lock_bh(&hwsim_delete_lock);
> +	while (!list_empty(&delete_radios)) {
> +		pr_debug("mac80211_hwsim: wait for deferred radio remove\n");
> +		spin_unlock_bh(&hwsim_delete_lock);
> +		flush_work(&list_entry(&delete_radios,
> +				       struct mac80211_hwsim_data, list)
> +			   ->destroy_work);

This can't possibly be right ... you're locking the list_empty which is
a trivial pointer comparison, but not the actual list_entry() ...

I'd also prefer you actually didn't leave the problem in part as you
describe - and a new workqueue probably isn't that much overhead and
should introduce *less* new code than this, so IMHO that's worth it.

johannes

Re: [PATCH v2 1/5] mac80211_hwsim: wait for deferred radio deletion on mod unload

[Benjamin Beichler]

Am 11.12.2017 um 12:46 schrieb Johannes Berg:
>
>> +	spin_lock_bh(&hwsim_delete_lock);
>> +	while (!list_empty(&delete_radios)) {
>> +		pr_debug("mac80211_hwsim: wait for deferred radio remove\n");
>> +		spin_unlock_bh(&hwsim_delete_lock);
>> +		flush_work(&list_entry(&delete_radios,
>> +				       struct mac80211_hwsim_data, list)
>> +			   ->destroy_work);
> This can't possibly be right ... you're locking the list_empty which is=

> a trivial pointer comparison, but not the actual list_entry() ...
Maybe the first spin_lock is not needed, but since flush_work wait for
the completion of the task, which at the end also deletes the item from
the list, holding any spin_lock would be wrong.
>
> I'd also prefer you actually didn't leave the problem in part as you
> describe - and a new workqueue probably isn't that much overhead and
> should introduce *less* new code than this, so IMHO that's worth it.
I totally agree with you, but I don't know the actual policy for
creating workqeues. I could also simply call flush_scheduled_work,
because we may have enough time at module unload. Some modules also do
it, but the description an some mailing threads mark it like
evil/deprecated. Which solution do you prefer?

kind regards

Benjamin

--=20
M.Sc. Benjamin Beichler

Universit=C3=A4t Rostock, Fakult=C3=A4t f=C3=BCr Informatik und Elektrote=
chnik
Institut f=C3=BCr Angewandte Mikroelektronik und Datentechnik

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

Richard-Wagner-Stra=C3=9Fe 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 v2 1/5] mac80211_hwsim: wait for deferred radio deletion on mod unload

[Johannes Berg]

On Mon, 2017-12-11 at 13:54 +0100, Benjamin Beichler wrote:
> Am 11.12.2017 um 12:46 schrieb Johannes Berg:
> > 
> > > +	spin_lock_bh(&hwsim_delete_lock);
> > > +	while (!list_empty(&delete_radios)) {
> > > +		pr_debug("mac80211_hwsim: wait for deferred radio remove\n");
> > > +		spin_unlock_bh(&hwsim_delete_lock);
> > > +		flush_work(&list_entry(&delete_radios,
> > > +				       struct mac80211_hwsim_data, list)
> > > +			   ->destroy_work);
> > 
> > This can't possibly be right ... you're locking the list_empty which is
> > a trivial pointer comparison, but not the actual list_entry() ...
> 
> Maybe the first spin_lock is not needed, but since flush_work wait for
> the completion of the task, which at the end also deletes the item from
> the list, holding any spin_lock would be wrong.

But not holding it while taking things that are on the list also seems
wrong.

> > I'd also prefer you actually didn't leave the problem in part as you
> > describe - and a new workqueue probably isn't that much overhead and
> > should introduce *less* new code than this, so IMHO that's worth it.
> 
> I totally agree with you, but I don't know the actual policy for
> creating workqeues. I could also simply call flush_scheduled_work,
> because we may have enough time at module unload. Some modules also do
> it, but the description an some mailing threads mark it like
> evil/deprecated. Which solution do you prefer?

Let's go with a new workqueue.

johannes

Re: [PATCH v2 1/5] mac80211_hwsim: wait for deferred radio deletion on mod unload

[Benjamin Beichler]

Am 11.12.2017 um 13:57 schrieb Johannes Berg:
> On Mon, 2017-12-11 at 13:54 +0100, Benjamin Beichler wrote:
>> Am 11.12.2017 um 12:46 schrieb Johannes Berg:
>>>> +	spin=5Flock=5Fbh(&hwsim=5Fdelete=5Flock);
>>>> +	while (!list=5Fempty(&delete=5Fradios)) {
>>>> +		pr=5Fdebug("mac80211=5Fhwsim: wait for deferred radio remove\n");
>>>> +		spin=5Funlock=5Fbh(&hwsim=5Fdelete=5Flock);
>>>> +		flush=5Fwork(&list=5Fentry(&delete=5Fradios,
>>>> +				       struct mac80211=5Fhwsim=5Fdata, list)
>>>> +			   ->destroy=5Fwork);
>>> This can't possibly be right ... you're locking the list=5Fempty which is
>>> a trivial pointer comparison, but not the actual list=5Fentry() ...
>> Maybe the first spin=5Flock is not needed, but since flush=5Fwork wait for
>> the completion of the task, which at the end also deletes the item from
>> the list, holding any spin=5Flock would be wrong.
> But not holding it while taking things that are on the list also seems
> wrong.
Since at this place (Netlink is already unregistered so no new deletion
requests) the only user of the delete list is the code deferred work
items (which take itself the lock). Nonetheless, with a separate
workqueue this is no problem anymore.
>
>>> I'd also prefer you actually didn't leave the problem in part as you
>>> describe - and a new workqueue probably isn't that much overhead and
>>> should introduce *less* new code than this, so IMHO that's worth it.
>> I totally agree with you, but I don't know the actual policy for
>> creating workqeues. I could also simply call flush=5Fscheduled=5Fwork,
>> because we may have enough time at module unload. Some modules also do
>> it, but the description an some mailing threads mark it like
>> evil/deprecated. Which solution do you prefer=3F
> Let's go with a new workqueue.
Ok, I prepare a patch in the next few days.

>
> johannes
>

-- 
M.Sc. Benjamin Beichler

Universit=C3=A4t Rostock, Fakult=C3=A4t f=C3=BCr Informatik und Elektrotechnik
Institut f=C3=BCr Angewandte Mikroelektronik und Datentechnik

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

Richard-Wagner-Stra=C3=9Fe 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 v2 2/5] mac80211_hwsim: add hashtable with mac address keys for faster lookup

[Benjamin Beichler]

This patch adds a rhastable for mac address lookup of hwsim radios. This
especially improve the speed on reception of a netlink message with a new
frame. Although redundant, we keep holding a normal list for all radios,
since the rhashtable_walk interface adds a lot of overhead for iterating
over all radios and the doc of rhashtable recommend a redundant structure
for stable walks in such situations.

Since rhashtable is rcu protected we do not need a lock for delivering
frames and thus improving this scenario.

Signed-off-by: Benjamin Beichler <benjamin.beichler@uni-rostock.de>
---
 drivers/net/wireless/mac80211_hwsim.c | 56 +++++++++++++++++++++++++----------
 1 file changed, 40 insertions(+), 16 deletions(-)

diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c
index d35dc6b2a733..48b9efed725e 100644
--- a/drivers/net/wireless/mac80211_hwsim.c
+++ b/drivers/net/wireless/mac80211_hwsim.c
@@ -32,6 +32,7 @@
 #include <net/genetlink.h>
 #include <net/net_namespace.h>
 #include <net/netns/generic.h>
+#include <linux/rhashtable.h>
 #include "mac80211_hwsim.h"
 
 #define WARN_QUEUE 100
@@ -489,6 +490,7 @@ static const struct ieee80211_iface_combination hwsim_if_comb_p2p_dev[] = {
 
 static spinlock_t hwsim_radio_lock;
 static LIST_HEAD(hwsim_radios);
+static struct rhashtable hwsim_radios_rht;
 static spinlock_t hwsim_delete_lock;
 static LIST_HEAD(delete_radios);
 static int hwsim_radio_idx;
@@ -501,6 +503,7 @@ static struct platform_driver mac80211_hwsim_driver = {
 
 struct mac80211_hwsim_data {
 	struct list_head list;
+	struct rhash_head rht;
 	struct ieee80211_hw *hw;
 	struct device *dev;
 	struct ieee80211_supported_band bands[NUM_NL80211_BANDS];
@@ -575,6 +578,20 @@ struct mac80211_hwsim_data {
 	u64 tx_failed;
 };
 
+static u32 hwsim_table_hash(const void *addr, u32 len, u32 seed)
+{
+	/* Use last four bytes of hw addr as hash index */
+	return jhash_1word(*(u32 *)(addr + 2), seed);
+}
+
+static const struct rhashtable_params hwsim_rht_params = {
+	.nelem_hint = 2,
+	.automatic_shrinking = true,
+	.key_len = ETH_ALEN,
+	.key_offset = offsetof(struct mac80211_hwsim_data, addresses[1]),
+	.head_offset = offsetof(struct mac80211_hwsim_data, rht),
+	.hashfn = hwsim_table_hash,
+};
 
 struct hwsim_radiotap_hdr {
 	struct ieee80211_radiotap_header hdr;
@@ -2727,6 +2744,15 @@ static int mac80211_hwsim_new_radio(struct genl_info *info,
 			     CLOCK_MONOTONIC, HRTIMER_MODE_ABS);
 
 	spin_lock_bh(&hwsim_radio_lock);
+	err = rhashtable_insert_fast(&hwsim_radios_rht, &data->rht,
+				     hwsim_rht_params);
+	if (err < 0) {
+		pr_debug("mac80211_hwsim: radio index %d already present\n",
+			 idx);
+		spin_unlock_bh(&hwsim_radio_lock);
+		goto failed_final_insert;
+	}
+
 	list_add_tail(&data->list, &hwsim_radios);
 	spin_unlock_bh(&hwsim_radio_lock);
 
@@ -2735,6 +2761,9 @@ static int mac80211_hwsim_new_radio(struct genl_info *info,
 
 	return idx;
 
+failed_final_insert:
+	debugfs_remove_recursive(data->debugfs);
+	ieee80211_unregister_hw(data->hw);
 failed_hw:
 	device_release_driver(data->dev);
 failed_bind:
@@ -2870,22 +2899,9 @@ static void hwsim_mon_setup(struct net_device *dev)
 
 static struct mac80211_hwsim_data *get_hwsim_data_ref_from_addr(const u8 *addr)
 {
-	struct mac80211_hwsim_data *data;
-	bool _found = false;
-
-	spin_lock_bh(&hwsim_radio_lock);
-	list_for_each_entry(data, &hwsim_radios, list) {
-		if (memcmp(data->addresses[1].addr, addr, ETH_ALEN) == 0) {
-			_found = true;
-			break;
-		}
-	}
-	spin_unlock_bh(&hwsim_radio_lock);
-
-	if (!_found)
-		return NULL;
-
-	return data;
+	return rhashtable_lookup_fast(&hwsim_radios_rht,
+				      addr,
+				      hwsim_rht_params);
 }
 
 static void hwsim_register_wmediumd(struct net *net, u32 portid)
@@ -3184,6 +3200,8 @@ static int hwsim_del_radio_nl(struct sk_buff *msg, struct genl_info *info)
 			continue;
 
 		list_del(&data->list);
+		rhashtable_remove_fast(&hwsim_radios_rht, &data->rht,
+				       hwsim_rht_params);
 		spin_unlock_bh(&hwsim_radio_lock);
 		mac80211_hwsim_del_radio(data, wiphy_name(data->hw->wiphy),
 					 info);
@@ -3343,6 +3361,8 @@ static void remove_user_radios(u32 portid)
 	list_for_each_entry_safe(entry, tmp, &hwsim_radios, list) {
 		if (entry->destroy_on_close && entry->portid == portid) {
 			list_del(&entry->list);
+			rhashtable_remove_fast(&hwsim_radios_rht, &entry->rht,
+					       hwsim_rht_params);
 
 			spin_lock_bh(&hwsim_delete_lock);
 			list_add(&entry->list, &delete_radios);
@@ -3423,6 +3443,8 @@ static void __net_exit hwsim_exit_net(struct net *net)
 			continue;
 
 		list_del(&data->list);
+		rhashtable_remove_fast(&hwsim_radios_rht, &data->rht,
+				       hwsim_rht_params);
 
 		spin_lock_bh(&hwsim_delete_lock);
 		list_add(&data->list, &delete_radios);
@@ -3461,6 +3483,7 @@ static int __init init_mac80211_hwsim(void)
 
 	spin_lock_init(&hwsim_radio_lock);
 	spin_lock_init(&hwsim_delete_lock);
+	rhashtable_init(&hwsim_radios_rht, &hwsim_rht_params);
 
 	err = register_pernet_device(&hwsim_net_ops);
 	if (err)
@@ -3613,6 +3636,7 @@ static void __exit exit_mac80211_hwsim(void)
 	}
 	spin_unlock_bh(&hwsim_delete_lock);
 
+	rhashtable_destroy(&hwsim_radios_rht);
 	unregister_netdev(hwsim_mon);
 	platform_driver_unregister(&mac80211_hwsim_driver);
 	unregister_pernet_device(&hwsim_net_ops);
-- 
2.15.0

[PATCH v2 3/5] mac80211_hwsim: add generation count for netlink dump operation

[Benjamin Beichler]

Make the dump operation aware of changes on radio list and corresponding
inconsistent dumps. Change the dump iteration to be independent from
increasing radio indices on radio list.

Signed-off-by: Benjamin Beichler <benjamin.beichler@uni-rostock.de>
---
 drivers/net/wireless/mac80211_hwsim.c | 30 +++++++++++++++++++-----------
 1 file changed, 19 insertions(+), 11 deletions(-)

diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c
index 48b9efed725e..fc8d9664cbfa 100644
--- a/drivers/net/wireless/mac80211_hwsim.c
+++ b/drivers/net/wireless/mac80211_hwsim.c
@@ -494,6 +494,7 @@ static struct rhashtable hwsim_radios_rht;
 static spinlock_t hwsim_delete_lock;
 static LIST_HEAD(delete_radios);
 static int hwsim_radio_idx;
+static int hwsim_radios_generation = 1;
 
 static struct platform_driver mac80211_hwsim_driver = {
 	.driver = {
@@ -2754,6 +2755,7 @@ static int mac80211_hwsim_new_radio(struct genl_info *info,
 	}
 
 	list_add_tail(&data->list, &hwsim_radios);
+	hwsim_radios_generation++;
 	spin_unlock_bh(&hwsim_radio_lock);
 
 	if (idx > 0)
@@ -3202,6 +3204,7 @@ static int hwsim_del_radio_nl(struct sk_buff *msg, struct genl_info *info)
 		list_del(&data->list);
 		rhashtable_remove_fast(&hwsim_radios_rht, &data->rht,
 				       hwsim_rht_params);
+		hwsim_radios_generation++;
 		spin_unlock_bh(&hwsim_radio_lock);
 		mac80211_hwsim_del_radio(data, wiphy_name(data->hw->wiphy),
 					 info);
@@ -3258,19 +3261,25 @@ static int hwsim_get_radio_nl(struct sk_buff *msg, struct genl_info *info)
 static int hwsim_dump_radio_nl(struct sk_buff *skb,
 			       struct netlink_callback *cb)
 {
-	int idx = cb->args[0];
-	struct mac80211_hwsim_data *data = NULL;
+	struct mac80211_hwsim_data *data =
+			(struct mac80211_hwsim_data *)cb->args[0];
 	int res;
 
 	spin_lock_bh(&hwsim_radio_lock);
+	cb->seq = hwsim_radios_generation;
 
-	if (idx == hwsim_radio_idx)
-		goto done;
+	/* list changed */
+	if (cb->prev_seq && cb->seq != cb->prev_seq)
+		goto cleanup;
 
-	list_for_each_entry(data, &hwsim_radios, list) {
-		if (data->idx < idx)
-			continue;
+	/* iterator is at head again, finish*/
+	if (data && &data->list == &hwsim_radios)
+		goto cleanup;
 
+	/* data will NULL or valid since we quit, if list changed */
+	data = list_prepare_entry(data, &hwsim_radios, list);
+
+	list_for_each_entry_continue(data, &hwsim_radios, list) {
 		if (!net_eq(wiphy_net(data->hw->wiphy), sock_net(skb->sk)))
 			continue;
 
@@ -3280,13 +3289,11 @@ static int hwsim_dump_radio_nl(struct sk_buff *skb,
 					       NLM_F_MULTI);
 		if (res < 0)
 			break;
-
-		idx = data->idx + 1;
 	}
 
-	cb->args[0] = idx;
+	cb->args[0] = (long)data;
 
-done:
+cleanup:
 	spin_unlock_bh(&hwsim_radio_lock);
 	return skb->len;
 }
@@ -3348,6 +3355,7 @@ static void destroy_radio(struct work_struct *work)
 
 	spin_lock_bh(&hwsim_delete_lock);
 	list_del(&data->list);
+	hwsim_radios_generation++;
 	spin_unlock_bh(&hwsim_delete_lock);
 	mac80211_hwsim_del_radio(data, wiphy_name(data->hw->wiphy), NULL);
 
-- 
2.15.0

Re: [PATCH v2 3/5] mac80211_hwsim: add generation count for netlink dump operation

[Johannes Berg]

On Tue, 2017-11-21 at 13:17 +0100, Benjamin Beichler wrote:
> Make the dump operation aware of changes on radio list and corresponding
> inconsistent dumps. Change the dump iteration to be independent from
> increasing radio indices on radio list.

Looks like this should use nl_dump_check_consistent()?

johannes

Re: [PATCH v2 3/5] mac80211_hwsim: add generation count for netlink dump operation

[Benjamin Beichler]

Am 11.12.2017 um 13:14 schrieb Johannes Berg:
> On Tue, 2017-11-21 at 13:17 +0100, Benjamin Beichler wrote:
>> Make the dump operation aware of changes on radio list and corresponding
>> inconsistent dumps. Change the dump iteration to be independent from
>> increasing radio indices on radio list.
> Looks like this should use nl=5Fdump=5Fcheck=5Fconsistent()=3F
>
> johannes
>
It is called in mac80211=5Fhwsim=5Fget=5Fradio, I didn't changed that.

-- 
M.Sc. Benjamin Beichler

Universit=C3=A4t Rostock, Fakult=C3=A4t f=C3=BCr Informatik und Elektrotechnik
Institut f=C3=BCr Angewandte Mikroelektronik und Datentechnik

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

Richard-Wagner-Stra=C3=9Fe 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 v2 3/5] mac80211_hwsim: add generation count for netlink dump operation

[Johannes Berg]

On Mon, 2017-12-11 at 13:37 +0100, Benjamin Beichler wrote:
> Am 11.12.2017 um 13:14 schrieb Johannes Berg:
> > On Tue, 2017-11-21 at 13:17 +0100, Benjamin Beichler wrote:
> > > Make the dump operation aware of changes on radio list and corresponding
> > > inconsistent dumps. Change the dump iteration to be independent from
> > > increasing radio indices on radio list.
> > 
> > Looks like this should use nl_dump_check_consistent()?
> > 
> > johannes
> > 
> 
> It is called in mac80211_hwsim_get_radio, I didn't changed that.

But you added this:

+       /* list changed */
+       if (cb->prev_seq && cb->seq != cb->prev_seq)
+               goto cleanup;

which is mostly just a copy of the inline.

johannes

Re: [PATCH v2 3/5] mac80211_hwsim: add generation count for netlink dump operation

[Benjamin Beichler]

Am 11.12.2017 um 13:49 schrieb Johannes Berg:
> On Mon, 2017-12-11 at 13:37 +0100, Benjamin Beichler wrote:
>> Am 11.12.2017 um 13:14 schrieb Johannes Berg:
>>> On Tue, 2017-11-21 at 13:17 +0100, Benjamin Beichler wrote:
>>>> Make the dump operation aware of changes on radio list and correspon=
ding
>>>> inconsistent dumps. Change the dump iteration to be independent from=

>>>> increasing radio indices on radio list.
>>> Looks like this should use nl_dump_check_consistent()?
>>>
>>> johannes
>>>
>> It is called in mac80211_hwsim_get_radio, I didn't changed that.
> But you added this:
>
> +       /* list changed */
> +       if (cb->prev_seq && cb->seq !=3D cb->prev_seq)
> +               goto cleanup;
>
> which is mostly just a copy of the inline.
>
> johannes
Year you are right, but for nl_dump_check_consistent() I also need a
header struct to write the flag to it and I thought a ghost header only
to this function is also misleading. But if you think this is better, I
can do that. Or we introduce a function, which really only check
consistency and not also set the flag. I also thought the line is
readable at it's own, because it's simply inconsistent if the sequence
numbers are not equal.

--=20
M.Sc. Benjamin Beichler

Universit=C3=A4t Rostock, Fakult=C3=A4t f=C3=BCr Informatik und Elektrote=
chnik
Institut f=C3=BCr Angewandte Mikroelektronik und Datentechnik

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

Richard-Wagner-Stra=C3=9Fe 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 v2 3/5] mac80211_hwsim: add generation count for netlink dump operation

[Johannes Berg]

On Mon, 2017-12-11 at 14:02 +0100, Benjamin Beichler wrote:
> 
> > But you added this:
> > 
> > +       /* list changed */
> > +       if (cb->prev_seq && cb->seq != cb->prev_seq)
> > +               goto cleanup;
> > 
> > which is mostly just a copy of the inline.
> > 
> > johannes
> 
> Year you are right, but for nl_dump_check_consistent() I also need a
> header struct to write the flag to it and I thought a ghost header only
> to this function is also misleading. But if you think this is better, I
> can do that. Or we introduce a function, which really only check
> consistency and not also set the flag. I also thought the line is
> readable at it's own, because it's simply inconsistent if the sequence
> numbers are not equal.

It's readable, but there should be an indication to userspace in this
case, no?

johannes

Re: [PATCH v2 3/5] mac80211_hwsim: add generation count for netlink dump operation

[Benjamin Beichler]

Am 11.12.2017 um 14:07 schrieb Johannes Berg:
> On Mon, 2017-12-11 at 14:02 +0100, Benjamin Beichler wrote:
>>> But you added this:
>>>
>>> +       /* list changed */
>>> +       if (cb->prev_seq && cb->seq !=3D cb->prev_seq)
>>> +               goto cleanup;
>>>
>>> which is mostly just a copy of the inline.
>>>
>>> johannes
>> Year you are right, but for nl_dump_check_consistent() I also need a
>> header struct to write the flag to it and I thought a ghost header onl=
y
>> to this function is also misleading. But if you think this is better, =
I
>> can do that. Or we introduce a function, which really only check
>> consistency and not also set the flag. I also thought the line is
>> readable at it's own, because it's simply inconsistent if the sequence=

>> numbers are not equal.
> It's readable, but there should be an indication to userspace in this
> case, no?
>
> johannes
>
Mhh, OK you are totally right.

I think we need to send something like an empty message, containing the
flag. Because there exist the corner case, that while a dump is
interrupted the complete list is deleted. Currently this could not
happen because of non-parallel netlink callbacks, but maybe in the
future parallel callbacks are enabled.

Would it break things, if I simply create an header with
HWSIM_CMD_GET_RADIO, but put no other information in it? Or how could it
be done correctly?

--=20
M.Sc. Benjamin Beichler

Universit=C3=A4t Rostock, Fakult=C3=A4t f=C3=BCr Informatik und Elektrote=
chnik
Institut f=C3=BCr Angewandte Mikroelektronik und Datentechnik

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

Richard-Wagner-Stra=C3=9Fe 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 v2 3/5] mac80211_hwsim: add generation count for netlink dump operation

[Johannes Berg]

On Mon, 2017-12-11 at 14:29 +0100, Benjamin Beichler wrote:

> I think we need to send something like an empty message, containing the
> flag. Because there exist the corner case, that while a dump is
> interrupted the complete list is deleted. Currently this could not
> happen because of non-parallel netlink callbacks, but maybe in the
> future parallel callbacks are enabled.

I don't know if we really want parallel, but I guess it's possible
eventually.

> Would it break things, if I simply create an header with
> HWSIM_CMD_GET_RADIO, but put no other information in it? Or how could it
> be done correctly?

I think that's probably OK.

johannes

[PATCH v2 4/5] mac80211_hwsim: add permanent mac address option for new radios

[Benjamin Beichler]

If simulation needs predictable permanent mac addresses of hwsim wireless
phy, this patch add the ability to create a new radio with a user defined
permanent mac address. Allowed mac addresses needs to be locally
administrated mac addresses (as also the former fixed 42:* and 02:* were).

To do not break the operation with legacy software using hwsim, the new
address is set twice. The problem here is, the netlink call backs use
wiphy->addresses[1] as identification of a radio and not the proposed
permanent address (wiphy->addresses[0]). This design decision is not
documented in the kernel repo, therefore this patch simply reproduces this,
but with the same address.

Signed-off-by: Benjamin Beichler <benjamin.beichler@uni-rostock.de>
---
 drivers/net/wireless/mac80211_hwsim.c | 53 +++++++++++++++++++++++++++++------
 drivers/net/wireless/mac80211_hwsim.h | 12 +++++++-
 2 files changed, 55 insertions(+), 10 deletions(-)

diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c
index fc8d9664cbfa..3d2b16822269 100644
--- a/drivers/net/wireless/mac80211_hwsim.c
+++ b/drivers/net/wireless/mac80211_hwsim.c
@@ -2371,6 +2371,7 @@ struct hwsim_new_radio_params {
 	bool destroy_on_close;
 	const char *hwname;
 	bool no_vif;
+	u8 *perm_addr;
 };
 
 static void hwsim_mcast_config_msg(struct sk_buff *mcast_skb,
@@ -2535,15 +2536,25 @@ static int mac80211_hwsim_new_radio(struct genl_info *info,
 	skb_queue_head_init(&data->pending);
 
 	SET_IEEE80211_DEV(hw, data->dev);
-	eth_zero_addr(addr);
-	addr[0] = 0x02;
-	addr[3] = idx >> 8;
-	addr[4] = idx;
-	memcpy(data->addresses[0].addr, addr, ETH_ALEN);
-	memcpy(data->addresses[1].addr, addr, ETH_ALEN);
-	data->addresses[1].addr[0] |= 0x40;
-	hw->wiphy->n_addresses = 2;
-	hw->wiphy->addresses = data->addresses;
+	if (!param->perm_addr) {
+		eth_zero_addr(addr);
+		addr[0] = 0x02;
+		addr[3] = idx >> 8;
+		addr[4] = idx;
+		memcpy(data->addresses[0].addr, addr, ETH_ALEN);
+		/* Why need here second address ? */
+		data->addresses[1].addr[0] |= 0x40;
+		memcpy(data->addresses[1].addr, addr, ETH_ALEN);
+		hw->wiphy->n_addresses = 2;
+		hw->wiphy->addresses = data->addresses;
+		/* possible address clash is checked at hash table insertion */
+	} else {
+		memcpy(data->addresses[0].addr, param->perm_addr, ETH_ALEN);
+		/* compatibility with automatically generated mac addr */
+		memcpy(data->addresses[1].addr, param->perm_addr, ETH_ALEN);
+		hw->wiphy->n_addresses = 2;
+		hw->wiphy->addresses = data->addresses;
+	}
 
 	data->channels = param->channels;
 	data->use_chanctx = param->use_chanctx;
@@ -3167,6 +3178,30 @@ static int hwsim_new_radio_nl(struct sk_buff *msg, struct genl_info *info)
 		param.regd = hwsim_world_regdom_custom[idx];
 	}
 
+	if (info->attrs[HWSIM_ATTR_PERM_ADDR]) {
+		if (nla_len(info->attrs[HWSIM_ATTR_PERM_ADDR]) != ETH_ALEN) {
+			pr_debug("mac80211_hwsim: MAC has wrong size\n");
+			return -EINVAL;
+		}
+		if (!is_valid_ether_addr(
+				nla_data(info->attrs[HWSIM_ATTR_PERM_ADDR]))) {
+			pr_debug("mac80211_hwsim: MAC is no valid source addr\n");
+			return -EINVAL;
+		}
+		if (!is_local_ether_addr(
+				nla_data(info->attrs[HWSIM_ATTR_PERM_ADDR]))) {
+			pr_debug("mac80211_hwsim: MAC is not locally administered\n");
+			return -EINVAL;
+		}
+		if (get_hwsim_data_ref_from_addr(
+				nla_data(info->attrs[HWSIM_ATTR_PERM_ADDR]))) {
+			pr_debug("mac80211_hwsim: perm MAC already in use\n");
+			return -EINVAL;
+		}
+
+		param.perm_addr = nla_data(info->attrs[HWSIM_ATTR_PERM_ADDR]);
+	}
+
 	return mac80211_hwsim_new_radio(info, &param);
 }
 
diff --git a/drivers/net/wireless/mac80211_hwsim.h b/drivers/net/wireless/mac80211_hwsim.h
index 3f5eda591dba..7b2f1e9e66a8 100644
--- a/drivers/net/wireless/mac80211_hwsim.h
+++ b/drivers/net/wireless/mac80211_hwsim.h
@@ -67,7 +67,12 @@ enum hwsim_tx_control_flags {
  *	%HWSIM_ATTR_TX_INFO, %HWSIM_ATTR_SIGNAL, %HWSIM_ATTR_COOKIE
  * @HWSIM_CMD_NEW_RADIO: create a new radio with the given parameters,
  *	returns the radio ID (>= 0) or negative on errors, if successful
- *	then multicast the result
+ *	then multicast the result, uses optional parameter:
+ *	%HWSIM_ATTR_REG_STRICT_REG, %HWSIM_ATTR_SUPPORT_P2P_DEVICE,
+ *	%HWSIM_ATTR_DESTROY_RADIO_ON_CLOSE, %HWSIM_ATTR_CHANNELS,
+ *	%HWSIM_ATTR_NO_VIF, %HWSIM_ATTR_RADIO_NAME, %HWSIM_ATTR_USE_CHANCTX,
+ *	%HWSIM_ATTR_REG_HINT_ALPHA2, %HWSIM_ATTR_REG_CUSTOM_REG,
+ *	%HWSIM_ATTR_PERM_ADDR
  * @HWSIM_CMD_DEL_RADIO: destroy a radio, reply is multicasted
  * @HWSIM_CMD_GET_RADIO: fetch information about existing radios, uses:
  *	%HWSIM_ATTR_RADIO_ID
@@ -123,6 +128,9 @@ enum {
  * @HWSIM_ATTR_RADIO_NAME: Name of radio, e.g. phy666
  * @HWSIM_ATTR_NO_VIF:  Do not create vif (wlanX) when creating radio.
  * @HWSIM_ATTR_FREQ: Frequency at which packet is transmitted or received.
+ * @HWSIM_ATTR_PERM_ADDR: permanent mac address of new radio
+ * @HWSIM_ATTR_TX_INFO_FLAGS: additional flags for corresponding
+ *	rates of %HWSIM_ATTR_TX_INFO
  * @__HWSIM_ATTR_MAX: enum limit
  */
 
@@ -149,6 +157,8 @@ enum {
 	HWSIM_ATTR_NO_VIF,
 	HWSIM_ATTR_FREQ,
 	HWSIM_ATTR_PAD,
+	HWSIM_ATTR_PERM_ADDR,
+	HWSIM_ATTR_TX_INFO_FLAGS,
 	__HWSIM_ATTR_MAX,
 };
 #define HWSIM_ATTR_MAX (__HWSIM_ATTR_MAX - 1)
-- 
2.15.0

Re: [PATCH v2 4/5] mac80211_hwsim: add permanent mac address option for new radios

[Johannes Berg]

On Tue, 2017-11-21 at 13:17 +0100, Benjamin Beichler wrote:
> 
> + * @HWSIM_ATTR_TX_INFO_FLAGS: additional flags for corresponding
> + *	rates of %HWSIM_ATTR_TX_INFO

> +	HWSIM_ATTR_TX_INFO_FLAGS,

This should be in the next patch

johannes

[PATCH v2 5/5] mac80211_hwsim: add hwsim_tx_rate_flags to netlink attributes

[Benjamin Beichler]

For correct interpretation of a tx rate, the corresponding rate flags are
needed (e.g. whether a HT-MCS rate or a legacy rate) and moreover for more
correct simulation the other infos of the flags are important (like
short-GI). Keeping compatibility, the flags are not integrated into the
existing hwsim_tx_rate, but transmitted as an additional netlink attribute.

Signed-off-by: Benjamin Beichler <benjamin.beichler@uni-rostock.de>
---
 drivers/net/wireless/mac80211_hwsim.c | 41 +++++++++++++++++++++-
 drivers/net/wireless/mac80211_hwsim.h | 65 ++++++++++++++++++++++++++++++++++-
 2 files changed, 104 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c
index 3d2b16822269..3689f863da8d 100644
--- a/drivers/net/wireless/mac80211_hwsim.c
+++ b/drivers/net/wireless/mac80211_hwsim.c
@@ -1022,6 +1022,36 @@ static int hwsim_unicast_netgroup(struct mac80211_hwsim_data *data,
 	return res;
 }
 
+static inline u16 trans_tx_rate_flags_ieee2hwsim(struct ieee80211_tx_rate *rate)
+{
+	u16 result = 0;
+
+	if (rate->flags & IEEE80211_TX_RC_USE_RTS_CTS)
+		result |= MAC80211_HWSIM_TX_RC_USE_RTS_CTS;
+	if (rate->flags & IEEE80211_TX_RC_USE_CTS_PROTECT)
+		result |= MAC80211_HWSIM_TX_RC_USE_CTS_PROTECT;
+	if (rate->flags & IEEE80211_TX_RC_USE_SHORT_PREAMBLE)
+		result |= MAC80211_HWSIM_TX_RC_USE_SHORT_PREAMBLE;
+	if (rate->flags & IEEE80211_TX_RC_MCS)
+		result |= MAC80211_HWSIM_TX_RC_MCS;
+	if (rate->flags & IEEE80211_TX_RC_GREEN_FIELD)
+		result |= MAC80211_HWSIM_TX_RC_GREEN_FIELD;
+	if (rate->flags & IEEE80211_TX_RC_40_MHZ_WIDTH)
+		result |= MAC80211_HWSIM_TX_RC_40_MHZ_WIDTH;
+	if (rate->flags & IEEE80211_TX_RC_DUP_DATA)
+		result |= MAC80211_HWSIM_TX_RC_DUP_DATA;
+	if (rate->flags & IEEE80211_TX_RC_SHORT_GI)
+		result |= MAC80211_HWSIM_TX_RC_SHORT_GI;
+	if (rate->flags & IEEE80211_TX_RC_VHT_MCS)
+		result |= MAC80211_HWSIM_TX_RC_VHT_MCS;
+	if (rate->flags & IEEE80211_TX_RC_80_MHZ_WIDTH)
+		result |= MAC80211_HWSIM_TX_RC_80_MHZ_WIDTH;
+	if (rate->flags & IEEE80211_TX_RC_160_MHZ_WIDTH)
+		result |= MAC80211_HWSIM_TX_RC_160_MHZ_WIDTH;
+
+	return result;
+}
+
 static void mac80211_hwsim_tx_frame_nl(struct ieee80211_hw *hw,
 				       struct sk_buff *my_skb,
 				       int dst_portid)
@@ -1034,6 +1064,7 @@ static void mac80211_hwsim_tx_frame_nl(struct ieee80211_hw *hw,
 	unsigned int hwsim_flags = 0;
 	int i;
 	struct hwsim_tx_rate tx_attempts[IEEE80211_TX_MAX_RATES];
+	struct hwsim_tx_rate_flag tx_attempts_flags[IEEE80211_TX_MAX_RATES];
 	uintptr_t cookie;
 
 	if (data->ps != PS_DISABLED)
@@ -1085,7 +1116,11 @@ static void mac80211_hwsim_tx_frame_nl(struct ieee80211_hw *hw,
 
 	for (i = 0; i < IEEE80211_TX_MAX_RATES; i++) {
 		tx_attempts[i].idx = info->status.rates[i].idx;
+		tx_attempts_flags[i].idx = info->status.rates[i].idx;
 		tx_attempts[i].count = info->status.rates[i].count;
+		tx_attempts_flags[i].flags =
+				trans_tx_rate_flags_ieee2hwsim(
+						&info->status.rates[i]);
 	}
 
 	if (nla_put(skb, HWSIM_ATTR_TX_INFO,
@@ -1093,6 +1128,11 @@ static void mac80211_hwsim_tx_frame_nl(struct ieee80211_hw *hw,
 		    tx_attempts))
 		goto nla_put_failure;
 
+	if (nla_put(skb, HWSIM_ATTR_TX_INFO_FLAGS,
+		    sizeof(struct hwsim_tx_rate_flag) * IEEE80211_TX_MAX_RATES,
+		    tx_attempts_flags))
+		goto nla_put_failure;
+
 	/* We create a cookie to identify this skb */
 	data->pending_cookie++;
 	cookie = data->pending_cookie;
@@ -2999,7 +3039,6 @@ static int hwsim_tx_info_frame_received_nl(struct sk_buff *skb_2,
 	for (i = 0; i < IEEE80211_TX_MAX_RATES; i++) {
 		txi->status.rates[i].idx = tx_attempts[i].idx;
 		txi->status.rates[i].count = tx_attempts[i].count;
-		/*txi->status.rates[i].flags = 0;*/
 	}
 
 	txi->status.ack_signal = nla_get_u32(info->attrs[HWSIM_ATTR_SIGNAL]);
diff --git a/drivers/net/wireless/mac80211_hwsim.h b/drivers/net/wireless/mac80211_hwsim.h
index 7b2f1e9e66a8..7ef9aa18fb82 100644
--- a/drivers/net/wireless/mac80211_hwsim.h
+++ b/drivers/net/wireless/mac80211_hwsim.h
@@ -64,7 +64,8 @@ enum hwsim_tx_control_flags {
  * @HWSIM_CMD_TX_INFO_FRAME: Transmission info report from user space to
  *	kernel, uses:
  *	%HWSIM_ATTR_ADDR_TRANSMITTER, %HWSIM_ATTR_FLAGS,
- *	%HWSIM_ATTR_TX_INFO, %HWSIM_ATTR_SIGNAL, %HWSIM_ATTR_COOKIE
+ *	%HWSIM_ATTR_TX_INFO, %WSIM_ATTR_TX_INFO_FLAGS,
+ *	%HWSIM_ATTR_SIGNAL, %HWSIM_ATTR_COOKIE
  * @HWSIM_CMD_NEW_RADIO: create a new radio with the given parameters,
  *	returns the radio ID (>= 0) or negative on errors, if successful
  *	then multicast the result, uses optional parameter:
@@ -181,4 +182,66 @@ struct hwsim_tx_rate {
 	u8 count;
 } __packed;
 
+/**
+ * enum hwsim_tx_rate_flags - per-rate flags set by the rate control algorithm.
+ *	Inspired by structure mac80211_rate_control_flags. New flags may be
+ *	appended, but old flags not deleted, to keep compatibility for
+ *	userspace.
+ *
+ * These flags are set by the Rate control algorithm for each rate during tx,
+ * in the @flags member of struct ieee80211_tx_rate.
+ *
+ * @MAC80211_HWSIM_TX_RC_USE_RTS_CTS: Use RTS/CTS exchange for this rate.
+ * @MAC80211_HWSIM_TX_RC_USE_CTS_PROTECT: CTS-to-self protection is required.
+ *	This is set if the current BSS requires ERP protection.
+ * @MAC80211_HWSIM_TX_RC_USE_SHORT_PREAMBLE: Use short preamble.
+ * @MAC80211_HWSIM_TX_RC_MCS: HT rate.
+ * @MAC80211_HWSIM_TX_RC_VHT_MCS: VHT MCS rate, in this case the idx field is
+ *	split into a higher 4 bits (Nss) and lower 4 bits (MCS number)
+ * @MAC80211_HWSIM_TX_RC_GREEN_FIELD: Indicates whether this rate should be used
+ *	in Greenfield mode.
+ * @MAC80211_HWSIM_TX_RC_40_MHZ_WIDTH: Indicates if the Channel Width should be
+ *	40 MHz.
+ * @MAC80211_HWSIM_TX_RC_80_MHZ_WIDTH: Indicates 80 MHz transmission
+ * @MAC80211_HWSIM_TX_RC_160_MHZ_WIDTH: Indicates 160 MHz transmission
+ *	(80+80 isn't supported yet)
+ * @MAC80211_HWSIM_TX_RC_DUP_DATA: The frame should be transmitted on both of
+ *	the adjacent 20 MHz channels, if the current channel type is
+ *	NL80211_CHAN_HT40MINUS or NL80211_CHAN_HT40PLUS.
+ * @MAC80211_HWSIM_TX_RC_SHORT_GI: Short Guard interval should be used for this
+ *	rate.
+ */
+enum hwsim_tx_rate_flags {
+	MAC80211_HWSIM_TX_RC_USE_RTS_CTS		= BIT(0),
+	MAC80211_HWSIM_TX_RC_USE_CTS_PROTECT		= BIT(1),
+	MAC80211_HWSIM_TX_RC_USE_SHORT_PREAMBLE	= BIT(2),
+
+	/* rate index is an HT/VHT MCS instead of an index */
+	MAC80211_HWSIM_TX_RC_MCS			= BIT(3),
+	MAC80211_HWSIM_TX_RC_GREEN_FIELD		= BIT(4),
+	MAC80211_HWSIM_TX_RC_40_MHZ_WIDTH		= BIT(5),
+	MAC80211_HWSIM_TX_RC_DUP_DATA		= BIT(6),
+	MAC80211_HWSIM_TX_RC_SHORT_GI		= BIT(7),
+	MAC80211_HWSIM_TX_RC_VHT_MCS			= BIT(8),
+	MAC80211_HWSIM_TX_RC_80_MHZ_WIDTH		= BIT(9),
+	MAC80211_HWSIM_TX_RC_160_MHZ_WIDTH		= BIT(10),
+};
+
+/**
+ * struct hwsim_tx_rate - rate selection/status
+ *
+ * @idx: rate index to attempt to send with
+ * @count: number of tries in this rate before going to the next rate
+ *
+ * A value of -1 for @idx indicates an invalid rate and, if used
+ * in an array of retry rates, that no more rates should be tried.
+ *
+ * When used for transmit status reporting, the driver should
+ * always report the rate and number of retries used.
+ *
+ */
+struct hwsim_tx_rate_flag {
+	s8 idx;
+	u16 flags;
+} __packed;
 #endif /* __MAC80211_HWSIM_H */
-- 
2.15.0