All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3 1/5] mac80211_hwsim: add workqueue to wait for deferred radio deletion on mod unload
       [not found] <20180110164255.2763-1-benjamin.beichler@uni-rostock.de>
@ 2018-01-10 16:42 ` Benjamin Beichler
  2018-01-15 11:46   ` Johannes Berg
  2018-01-10 16:42 ` [PATCH v3 2/5] mac80211_hwsim: add hashtable with mac address keys for faster lookup Benjamin Beichler
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 20+ messages in thread
From: Benjamin Beichler @ 2018-01-10 16:42 UTC (permalink / raw)
  To: johannes; +Cc: linux-wireless, 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. To wait especially for this deletion work items,
add a work queue, otherwise flush_scheduled_work would be necessary.

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

diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c
index 45cca54c05bf..742a1551bb1c 100644
--- a/drivers/net/wireless/mac80211_hwsim.c
+++ b/drivers/net/wireless/mac80211_hwsim.c
@@ -489,6 +489,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 workqueue_struct *hwsim_wq;
 static int hwsim_radio_idx;
 
 static struct platform_driver mac80211_hwsim_driver = {
@@ -3346,7 +3347,7 @@ static void remove_user_radios(u32 portid)
 		if (entry->destroy_on_close && entry->portid == portid) {
 			list_del(&entry->list);
 			INIT_WORK(&entry->destroy_work, destroy_radio);
-			schedule_work(&entry->destroy_work);
+			queue_work(hwsim_wq, &entry->destroy_work);
 		}
 	}
 	spin_unlock_bh(&hwsim_radio_lock);
@@ -3421,7 +3422,7 @@ static void __net_exit hwsim_exit_net(struct net *net)
 
 		list_del(&data->list);
 		INIT_WORK(&data->destroy_work, destroy_radio);
-		schedule_work(&data->destroy_work);
+		queue_work(hwsim_wq, &data->destroy_work);
 	}
 	spin_unlock_bh(&hwsim_radio_lock);
 }
@@ -3453,6 +3454,10 @@ static int __init init_mac80211_hwsim(void)
 
 	spin_lock_init(&hwsim_radio_lock);
 
+	hwsim_wq = alloc_workqueue("hwsim_wq",WQ_MEM_RECLAIM,0);
+	if (!hwsim_wq)
+		return -ENOMEM;
+
 	err = register_pernet_device(&hwsim_net_ops);
 	if (err)
 		return err;
@@ -3591,8 +3596,11 @@ static void __exit exit_mac80211_hwsim(void)
 	hwsim_exit_netlink();
 
 	mac80211_hwsim_free();
+	flush_workqueue(hwsim_wq);
+
 	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.15.1

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

* [PATCH v3 2/5] mac80211_hwsim: add hashtable with mac address keys for faster lookup
       [not found] <20180110164255.2763-1-benjamin.beichler@uni-rostock.de>
  2018-01-10 16:42 ` [PATCH v3 1/5] mac80211_hwsim: add workqueue to wait for deferred radio deletion on mod unload Benjamin Beichler
@ 2018-01-10 16:42 ` Benjamin Beichler
  2018-01-15 11:46   ` Johannes Berg
  2018-01-10 16:42 ` [PATCH v3 3/5] mac80211_hwsim: add generation count for netlink dump operation Benjamin Beichler
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 20+ messages in thread
From: Benjamin Beichler @ 2018-01-10 16:42 UTC (permalink / raw)
  To: johannes; +Cc: linux-wireless, 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 | 57 +++++++++++++++++++++++++----------
 1 file changed, 41 insertions(+), 16 deletions(-)

diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c
index 742a1551bb1c..55d25e3fbbcc 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
@@ -490,6 +491,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 workqueue_struct *hwsim_wq;
+static struct rhashtable hwsim_radios_rht;
 static int hwsim_radio_idx;
 
 static struct platform_driver mac80211_hwsim_driver = {
@@ -500,6 +502,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];
@@ -574,6 +577,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;
@@ -2731,6 +2748,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);
 
@@ -2739,6 +2765,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:
@@ -2874,22 +2903,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)
@@ -3191,6 +3207,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);
@@ -3346,6 +3364,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);
 			INIT_WORK(&entry->destroy_work, destroy_radio);
 			queue_work(hwsim_wq, &entry->destroy_work);
 		}
@@ -3421,6 +3441,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);
 		INIT_WORK(&data->destroy_work, destroy_radio);
 		queue_work(hwsim_wq, &data->destroy_work);
 	}
@@ -3458,6 +3480,8 @@ static int __init init_mac80211_hwsim(void)
 	if (!hwsim_wq)
 		return -ENOMEM;
 
+	rhashtable_init(&hwsim_radios_rht, &hwsim_rht_params);
+
 	err = register_pernet_device(&hwsim_net_ops);
 	if (err)
 		return err;
@@ -3598,6 +3622,7 @@ static void __exit exit_mac80211_hwsim(void)
 	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);
-- 
2.15.1

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

* [PATCH v3 3/5] mac80211_hwsim: add generation count for netlink dump operation
       [not found] <20180110164255.2763-1-benjamin.beichler@uni-rostock.de>
  2018-01-10 16:42 ` [PATCH v3 1/5] mac80211_hwsim: add workqueue to wait for deferred radio deletion on mod unload Benjamin Beichler
  2018-01-10 16:42 ` [PATCH v3 2/5] mac80211_hwsim: add hashtable with mac address keys for faster lookup Benjamin Beichler
@ 2018-01-10 16:42 ` Benjamin Beichler
  2018-01-15 12:08   ` Johannes Berg
  2018-01-10 16:42 ` [PATCH v3 4/5] mac80211_hwsim: add permanent mac address option for new radios Benjamin Beichler
  2018-01-10 16:42 ` [PATCH v3 5/5] mac80211_hwsim: add hwsim_tx_rate_flags to netlink attributes Benjamin Beichler
  4 siblings, 1 reply; 20+ messages in thread
From: Benjamin Beichler @ 2018-01-10 16:42 UTC (permalink / raw)
  To: johannes; +Cc: linux-wireless, 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 | 43 ++++++++++++++++++++++++-----------
 1 file changed, 30 insertions(+), 13 deletions(-)

diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c
index 55d25e3fbbcc..2d4e97b6dc8f 100644
--- a/drivers/net/wireless/mac80211_hwsim.c
+++ b/drivers/net/wireless/mac80211_hwsim.c
@@ -493,6 +493,7 @@ 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;
 
 static struct platform_driver mac80211_hwsim_driver = {
 	.driver = {
@@ -2758,6 +2759,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)
@@ -3209,6 +3211,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);
@@ -3265,19 +3268,34 @@ 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;
-	int res;
+	struct mac80211_hwsim_data *data =
+			(struct mac80211_hwsim_data *)cb->args[0];
+	int res = 0;
+	void *hdr;
 
 	spin_lock_bh(&hwsim_radio_lock);
+	cb->seq = hwsim_radios_generation;
+
+	/* list changed, send msg with dump interrupted header*/
+	if (cb->prev_seq && cb->seq != cb->prev_seq) {
+		hdr = genlmsg_put(skb, NETLINK_CB(cb->skb).portid,
+				  cb->nlh->nlmsg_seq, &hwsim_genl_family,
+				  NLM_F_MULTI, HWSIM_CMD_GET_RADIO);
+		if (!hdr)
+			res = -EMSGSIZE;
+		genl_dump_check_consistent(cb, hdr);
+		genlmsg_end(skb, hdr);
+		goto cleanup;
+	}
 
-	if (idx == hwsim_radio_idx)
-		goto done;
+	/* iterator is at head again, finish*/
+	if (data && &data->list == &hwsim_radios)
+		goto cleanup;
 
-	list_for_each_entry(data, &hwsim_radios, list) {
-		if (data->idx < idx)
-			continue;
+	/* 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;
 
@@ -3287,15 +3305,13 @@ 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;
+	return (res)? res : skb->len;
 }
 
 /* Generic Netlink operations array */
@@ -3353,6 +3369,7 @@ 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);
 }
 
-- 
2.15.1

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

* [PATCH v3 4/5] mac80211_hwsim: add permanent mac address option for new radios
       [not found] <20180110164255.2763-1-benjamin.beichler@uni-rostock.de>
                   ` (2 preceding siblings ...)
  2018-01-10 16:42 ` [PATCH v3 3/5] mac80211_hwsim: add generation count for netlink dump operation Benjamin Beichler
@ 2018-01-10 16:42 ` Benjamin Beichler
  2018-01-15 12:09   ` Johannes Berg
  2018-01-15 12:14   ` Johannes Berg
  2018-01-10 16:42 ` [PATCH v3 5/5] mac80211_hwsim: add hwsim_tx_rate_flags to netlink attributes Benjamin Beichler
  4 siblings, 2 replies; 20+ messages in thread
From: Benjamin Beichler @ 2018-01-10 16:42 UTC (permalink / raw)
  To: johannes; +Cc: linux-wireless, 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 |  9 +++++-
 2 files changed, 52 insertions(+), 10 deletions(-)

diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c
index 2d4e97b6dc8f..1e0d651c43fc 100644
--- a/drivers/net/wireless/mac80211_hwsim.c
+++ b/drivers/net/wireless/mac80211_hwsim.c
@@ -2375,6 +2375,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,
@@ -2539,15 +2540,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;
@@ -3172,6 +3183,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]);
+	}
+
 	ret = mac80211_hwsim_new_radio(info, &param);
 	kfree(hwname);
 	return ret;
diff --git a/drivers/net/wireless/mac80211_hwsim.h b/drivers/net/wireless/mac80211_hwsim.h
index 3f5eda591dba..e2592b596090 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,7 @@ 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_MAX: enum limit
  */
 
@@ -149,6 +155,7 @@ enum {
 	HWSIM_ATTR_NO_VIF,
 	HWSIM_ATTR_FREQ,
 	HWSIM_ATTR_PAD,
+	HWSIM_ATTR_PERM_ADDR,
 	__HWSIM_ATTR_MAX,
 };
 #define HWSIM_ATTR_MAX (__HWSIM_ATTR_MAX - 1)
-- 
2.15.1

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

* [PATCH v3 5/5] mac80211_hwsim: add hwsim_tx_rate_flags to netlink attributes
       [not found] <20180110164255.2763-1-benjamin.beichler@uni-rostock.de>
                   ` (3 preceding siblings ...)
  2018-01-10 16:42 ` [PATCH v3 4/5] mac80211_hwsim: add permanent mac address option for new radios Benjamin Beichler
@ 2018-01-10 16:42 ` Benjamin Beichler
  2018-01-15 12:19   ` Johannes Berg
  4 siblings, 1 reply; 20+ messages in thread
From: Benjamin Beichler @ 2018-01-10 16:42 UTC (permalink / raw)
  To: johannes; +Cc: linux-wireless, 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 | 68 ++++++++++++++++++++++++++++++++++-
 2 files changed, 107 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c
index 1e0d651c43fc..226fa98f16a6 100644
--- a/drivers/net/wireless/mac80211_hwsim.c
+++ b/drivers/net/wireless/mac80211_hwsim.c
@@ -1026,6 +1026,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)
@@ -1038,6 +1068,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)
@@ -1089,7 +1120,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,
@@ -1097,6 +1132,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;
@@ -3003,7 +3043,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 e2592b596090..b304dfb70036 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:
@@ -76,6 +77,8 @@ enum hwsim_tx_control_flags {
  * @HWSIM_CMD_DEL_RADIO: destroy a radio, reply is multicasted
  * @HWSIM_CMD_GET_RADIO: fetch information about existing radios, uses:
  *	%HWSIM_ATTR_RADIO_ID
+ * @HWSIM_ATTR_TX_INFO_FLAGS: additional flags for corresponding
+ *	rates of %HWSIM_ATTR_TX_INFO
  * @__HWSIM_CMD_MAX: enum limit
  */
 enum {
@@ -156,6 +159,7 @@ enum {
 	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)
@@ -178,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.1

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

* Re: [PATCH v3 1/5] mac80211_hwsim: add workqueue to wait for deferred radio deletion on mod unload
  2018-01-10 16:42 ` [PATCH v3 1/5] mac80211_hwsim: add workqueue to wait for deferred radio deletion on mod unload Benjamin Beichler
@ 2018-01-15 11:46   ` Johannes Berg
  0 siblings, 0 replies; 20+ messages in thread
From: Johannes Berg @ 2018-01-15 11:46 UTC (permalink / raw)
  To: Benjamin Beichler; +Cc: linux-wireless

On Wed, 2018-01-10 at 17:42 +0100, Benjamin Beichler wrote:
> 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. To wait especially for this deletion work items,
> add a work queue, otherwise flush_scheduled_work would be necessary.
> 
Applied this to mac80211, thanks.

johannes

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

* Re: [PATCH v3 2/5] mac80211_hwsim: add hashtable with mac address keys for faster lookup
  2018-01-10 16:42 ` [PATCH v3 2/5] mac80211_hwsim: add hashtable with mac address keys for faster lookup Benjamin Beichler
@ 2018-01-15 11:46   ` Johannes Berg
  0 siblings, 0 replies; 20+ messages in thread
From: Johannes Berg @ 2018-01-15 11:46 UTC (permalink / raw)
  To: Benjamin Beichler; +Cc: linux-wireless

On Wed, 2018-01-10 at 17:42 +0100, Benjamin Beichler wrote:
> 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.

Applied, but

> +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);
> +}

I removed this, because that isn't actually safe - there's only 2-byte
alignment guaranteed... rhashtable will just do jhash() on the whole
thing, which should be fast enough.

johannes

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

* Re: [PATCH v3 3/5] mac80211_hwsim: add generation count for netlink dump operation
  2018-01-10 16:42 ` [PATCH v3 3/5] mac80211_hwsim: add generation count for netlink dump operation Benjamin Beichler
@ 2018-01-15 12:08   ` Johannes Berg
  2018-01-15 14:28     ` Benjamin Beichler
  0 siblings, 1 reply; 20+ messages in thread
From: Johannes Berg @ 2018-01-15 12:08 UTC (permalink / raw)
  To: Benjamin Beichler; +Cc: linux-wireless

On Wed, 2018-01-10 at 17:42 +0100, Benjamin Beichler wrote:
>  Change the dump iteration to be independent from
> increasing radio indices on radio list.

You can't do that, data structures can be deleted while the dump is
ongoing.

johannes

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

* Re: [PATCH v3 4/5] mac80211_hwsim: add permanent mac address option for new radios
  2018-01-10 16:42 ` [PATCH v3 4/5] mac80211_hwsim: add permanent mac address option for new radios Benjamin Beichler
@ 2018-01-15 12:09   ` Johannes Berg
  2018-01-15 14:39     ` Benjamin Beichler
  2018-01-15 12:14   ` Johannes Berg
  1 sibling, 1 reply; 20+ messages in thread
From: Johannes Berg @ 2018-01-15 12:09 UTC (permalink / raw)
  To: Benjamin Beichler; +Cc: linux-wireless

On Wed, 2018-01-10 at 17:42 +0100, Benjamin Beichler wrote:

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

It's weird to have the same address twice - perhaps we can XOR 0x40 or
so?

johannes

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

* Re: [PATCH v3 4/5] mac80211_hwsim: add permanent mac address option for new radios
  2018-01-10 16:42 ` [PATCH v3 4/5] mac80211_hwsim: add permanent mac address option for new radios Benjamin Beichler
  2018-01-15 12:09   ` Johannes Berg
@ 2018-01-15 12:14   ` Johannes Berg
  2018-01-15 14:48     ` Benjamin Beichler
  1 sibling, 1 reply; 20+ messages in thread
From: Johannes Berg @ 2018-01-15 12:14 UTC (permalink / raw)
  To: Benjamin Beichler; +Cc: linux-wireless

On Wed, 2018-01-10 at 17:42 +0100, Benjamin Beichler wrote:
> 
>  
> +	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;
> +		}

Oh, also - don't do this.

1) don't print, use NL_SET_ERR_MSG().

2) use the policy to validate lengths

> +		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;
> +		}

This doesn't really matter - it's purely virtual. I think we can avoid
that.

> +		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;
> +		}

This is racy afaict - remove it and return a clash later when you fail
to insert the new radio.

johannes

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

* Re: [PATCH v3 5/5] mac80211_hwsim: add hwsim_tx_rate_flags to netlink attributes
  2018-01-10 16:42 ` [PATCH v3 5/5] mac80211_hwsim: add hwsim_tx_rate_flags to netlink attributes Benjamin Beichler
@ 2018-01-15 12:19   ` Johannes Berg
  0 siblings, 0 replies; 20+ messages in thread
From: Johannes Berg @ 2018-01-15 12:19 UTC (permalink / raw)
  To: Benjamin Beichler; +Cc: linux-wireless

On Wed, 2018-01-10 at 17:42 +0100, Benjamin Beichler wrote:
> 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.

Also applied.

johannes

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

* Re: [PATCH v3 3/5] mac80211_hwsim: add generation count for netlink dump operation
  2018-01-15 12:08   ` Johannes Berg
@ 2018-01-15 14:28     ` Benjamin Beichler
  2018-01-22 13:24       ` Johannes Berg
  0 siblings, 1 reply; 20+ messages in thread
From: Benjamin Beichler @ 2018-01-15 14:28 UTC (permalink / raw)
  To: Johannes Berg; +Cc: linux-wireless

Am 15.01.2018 um 13:08 schrieb Johannes Berg:
> On Wed, 2018-01-10 at 17:42 +0100, Benjamin Beichler wrote:
>>  Change the dump iteration to be independent from
>> increasing radio indices on radio list.
> You can't do that, data structures can be deleted while the dump is
> ongoing.
That's what I try to address with this patch. In the past there were
corner cases, which might have failed while deletion.

Nonetheless, the radio count is in the current version strictly
monotonic, maybe the description is misleading. :-)
So in the inner loop there is no deletion possible because of the
spinlock, and multiple calls of nl=5Fdump check the generation count and
cancel if it have changed.

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

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

* Re: [PATCH v3 4/5] mac80211_hwsim: add permanent mac address option for new radios
  2018-01-15 12:09   ` Johannes Berg
@ 2018-01-15 14:39     ` Benjamin Beichler
  2018-01-22 13:22       ` Johannes Berg
  0 siblings, 1 reply; 20+ messages in thread
From: Benjamin Beichler @ 2018-01-15 14:39 UTC (permalink / raw)
  To: Johannes Berg; +Cc: linux-wireless

Am 15.01.2018 um 13:09 schrieb Johannes Berg:
> On Wed, 2018-01-10 at 17:42 +0100, Benjamin Beichler wrote:
>
>> To do not break the operation with legacy software using hwsim, the ne=
w
>> 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.
> It's weird to have the same address twice - perhaps we can XOR 0x40 or
> so?
I really don't know, why there were 2 addresses introduced in the first
place, and why the netlink operations for receiving only check against
the second address and not the first address (see
get_hwsim_data_ref_from_addr). My suggestion would be, only allocate one
mac address and check against this one. I don't know, whether really any
user of hwsim rely on the structure, that an adapter needs two mac
addresses. Since this is part of the initial commit, this decision is
not documented in git ...
> johannes
>
>

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

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

* Re: [PATCH v3 4/5] mac80211_hwsim: add permanent mac address option for new radios
  2018-01-15 12:14   ` Johannes Berg
@ 2018-01-15 14:48     ` Benjamin Beichler
  2018-01-22 13:23       ` Johannes Berg
  0 siblings, 1 reply; 20+ messages in thread
From: Benjamin Beichler @ 2018-01-15 14:48 UTC (permalink / raw)
  To: Johannes Berg; +Cc: linux-wireless

Am 15.01.2018 um 13:14 schrieb Johannes Berg:
> On Wed, 2018-01-10 at 17:42 +0100, Benjamin Beichler wrote:
>> =20
>> +	if (info->attrs[HWSIM_ATTR_PERM_ADDR]) {
>> +		if (nla_len(info->attrs[HWSIM_ATTR_PERM_ADDR]) !=3D ETH_ALEN) {
>> +			pr_debug("mac80211_hwsim: MAC has wrong size\n");
>> +			return -EINVAL;
>> +		}
> Oh, also - don't do this.
>
> 1) don't print, use NL_SET_ERR_MSG().
Ok, I only adapted the code already there, but I change this.
>
> 2) use the policy to validate lengths
Yeah, of course I change that, much easier -.-

>
>> +		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;
>> +		}
> This doesn't really matter - it's purely virtual. I think we can avoid
> that.
>
>> +		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;
>> +		}
> This is racy afaict - remove it and return a clash later when you fail
> to insert the new radio.
Ehm yes, actually exactly this test is already in the rhashtable patch.
But maybe I should also change there the error print to a NL_ERR_MSG() ?

>
> johannes
>

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

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

* Re: [PATCH v3 4/5] mac80211_hwsim: add permanent mac address option for new radios
  2018-01-15 14:39     ` Benjamin Beichler
@ 2018-01-22 13:22       ` Johannes Berg
  0 siblings, 0 replies; 20+ messages in thread
From: Johannes Berg @ 2018-01-22 13:22 UTC (permalink / raw)
  To: Benjamin Beichler; +Cc: linux-wireless

On Mon, 2018-01-15 at 15:39 +0100, Benjamin Beichler wrote:
> 
> 
> I really don't know, why there were 2 addresses introduced in the first
> place, and why the netlink operations for receiving only check against
> the second address and not the first address (see
> get_hwsim_data_ref_from_addr). My suggestion would be, only allocate one
> mac address and check against this one. I don't know, whether really any
> user of hwsim rely on the structure, that an adapter needs two mac
> addresses. Since this is part of the initial commit, this decision is
> not documented in git ...

Fair enough, let's leave it then.

johannes

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

* Re: [PATCH v3 4/5] mac80211_hwsim: add permanent mac address option for new radios
  2018-01-15 14:48     ` Benjamin Beichler
@ 2018-01-22 13:23       ` Johannes Berg
  2018-01-22 14:49         ` Benjamin Beichler
  0 siblings, 1 reply; 20+ messages in thread
From: Johannes Berg @ 2018-01-22 13:23 UTC (permalink / raw)
  To: Benjamin Beichler; +Cc: linux-wireless

On Mon, 2018-01-15 at 15:48 +0100, Benjamin Beichler wrote:

> > > +		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;
> > > +		}
> > 
> > This is racy afaict - remove it and return a clash later when you fail
> > to insert the new radio.
> 
> Ehm yes, actually exactly this test is already in the rhashtable patch.
> But maybe I should also change there the error print to a NL_ERR_MSG() ?

Oh, really? I just sent that out, but whatever, we have enough time to
fix it :-)

And yes, using netlink extended ack messages here as well would be
good.

johannes

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

* Re: [PATCH v3 3/5] mac80211_hwsim: add generation count for netlink dump operation
  2018-01-15 14:28     ` Benjamin Beichler
@ 2018-01-22 13:24       ` Johannes Berg
  2018-01-22 14:58         ` Benjamin Beichler
  0 siblings, 1 reply; 20+ messages in thread
From: Johannes Berg @ 2018-01-22 13:24 UTC (permalink / raw)
  To: Benjamin Beichler; +Cc: linux-wireless

On Mon, 2018-01-15 at 15:28 +0100, Benjamin Beichler wrote:
> Am 15.01.2018 um 13:08 schrieb Johannes Berg:
> > On Wed, 2018-01-10 at 17:42 +0100, Benjamin Beichler wrote:
> > >  Change the dump iteration to be independent from
> > > increasing radio indices on radio list.
> > 
> > You can't do that, data structures can be deleted while the dump is
> > ongoing.
> 
> That's what I try to address with this patch. In the past there were
> corner cases, which might have failed while deletion.

Hah, ok.

> Nonetheless, the radio count is in the current version strictly
> monotonic, maybe the description is misleading. :-)
> So in the inner loop there is no deletion possible because of the
> spinlock, and multiple calls of nl_dump check the generation count and
> cancel if it have changed.

Ah, I missed that you cancel when it's changed, we don't usually do
that normally.

I'd still prefer if you could have the code not rely on the pointers,
because if somebody gets it wrong (forgets to update the generation
count) I'm not sure I want that to crash in some corner cases...

johannes

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

* Re: [PATCH v3 4/5] mac80211_hwsim: add permanent mac address option for new radios
  2018-01-22 13:23       ` Johannes Berg
@ 2018-01-22 14:49         ` Benjamin Beichler
  2018-01-22 14:51           ` Johannes Berg
  0 siblings, 1 reply; 20+ messages in thread
From: Benjamin Beichler @ 2018-01-22 14:49 UTC (permalink / raw)
  To: Johannes Berg; +Cc: linux-wireless

Am 22.01.2018 um 14:23 schrieb Johannes Berg:
> On Mon, 2018-01-15 at 15:48 +0100, Benjamin Beichler wrote:
>
>>>> +		if (get=5Fhwsim=5Fdata=5Fref=5Ffrom=5Faddr(
>>>> +				nla=5Fdata(info->attrs[HWSIM=5FATTR=5FPERM=5FADDR]))) {
>>>> +			pr=5Fdebug("mac80211=5Fhwsim: perm MAC already in use\n");
>>>> +			return -EINVAL;
>>>> +		}
>>> This is racy afaict - remove it and return a clash later when you fail
>>> to insert the new radio.
>> Ehm yes, actually exactly this test is already in the rhashtable patch.
>> But maybe I should also change there the error print to a NL=5FERR=5FMSG() =
=3F
> Oh, really=3F I just sent that out, but whatever, we have enough time to
> fix it :-)
Should I sent then the complete patch set again, or should I create a
new one based on your current mac80211next branch, excluding the
accepted patches =3F

> And yes, using netlink extended ack messages here as well would be
> good.
>
> johannes
>
Benjamin

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

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

* Re: [PATCH v3 4/5] mac80211_hwsim: add permanent mac address option for new radios
  2018-01-22 14:49         ` Benjamin Beichler
@ 2018-01-22 14:51           ` Johannes Berg
  0 siblings, 0 replies; 20+ messages in thread
From: Johannes Berg @ 2018-01-22 14:51 UTC (permalink / raw)
  To: Benjamin Beichler; +Cc: linux-wireless

On Mon, 2018-01-22 at 15:49 +0100, Benjamin Beichler wrote:
> 
> Should I sent then the complete patch set again, or should I create a
> new one based on your current mac80211next branch, excluding the
> accepted patches ?

Please only send the ones I don't have yet, if necessary throwing in a
separate fix for them.

johannes

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

* Re: [PATCH v3 3/5] mac80211_hwsim: add generation count for netlink dump operation
  2018-01-22 13:24       ` Johannes Berg
@ 2018-01-22 14:58         ` Benjamin Beichler
  0 siblings, 0 replies; 20+ messages in thread
From: Benjamin Beichler @ 2018-01-22 14:58 UTC (permalink / raw)
  To: Johannes Berg; +Cc: linux-wireless

[-- Attachment #1: Type: text/plain, Size: 1960 bytes --]

Am 22.01.2018 um 14:24 schrieb Johannes Berg:
> On Mon, 2018-01-15 at 15:28 +0100, Benjamin Beichler wrote:
>> Am 15.01.2018 um 13:08 schrieb Johannes Berg:
>>> On Wed, 2018-01-10 at 17:42 +0100, Benjamin Beichler wrote:
>>>>  Change the dump iteration to be independent from
>>>> increasing radio indices on radio list.
>>> You can't do that, data structures can be deleted while the dump is
>>> ongoing.
>> That's what I try to address with this patch. In the past there were
>> corner cases, which might have failed while deletion.
> Hah, ok.
>
>> Nonetheless, the radio count is in the current version strictly
>> monotonic, maybe the description is misleading. :-)
>> So in the inner loop there is no deletion possible because of the
>> spinlock, and multiple calls of nl_dump check the generation count and
>> cancel if it have changed.
> Ah, I missed that you cancel when it's changed, we don't usually do
> that normally.
>
> I'd still prefer if you could have the code not rely on the pointers,
> because if somebody gets it wrong (forgets to update the generation
> count) I'm not sure I want that to crash in some corner cases...
Mkay, I saw the same pattern in other parts of the kernel, that's why I
adopted it. Then I restore the magic with searching the next radio id
bigger then the last one. Since on my machine, It needs several hundreds
of radios for a full message to cause interruptions, I think the code is
really rarely used and should be fail safe :-)


> johannes
>

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



[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 5151 bytes --]

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

end of thread, other threads:[~2018-01-22 14:58 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <20180110164255.2763-1-benjamin.beichler@uni-rostock.de>
2018-01-10 16:42 ` [PATCH v3 1/5] mac80211_hwsim: add workqueue to wait for deferred radio deletion on mod unload Benjamin Beichler
2018-01-15 11:46   ` Johannes Berg
2018-01-10 16:42 ` [PATCH v3 2/5] mac80211_hwsim: add hashtable with mac address keys for faster lookup Benjamin Beichler
2018-01-15 11:46   ` Johannes Berg
2018-01-10 16:42 ` [PATCH v3 3/5] mac80211_hwsim: add generation count for netlink dump operation Benjamin Beichler
2018-01-15 12:08   ` Johannes Berg
2018-01-15 14:28     ` Benjamin Beichler
2018-01-22 13:24       ` Johannes Berg
2018-01-22 14:58         ` Benjamin Beichler
2018-01-10 16:42 ` [PATCH v3 4/5] mac80211_hwsim: add permanent mac address option for new radios Benjamin Beichler
2018-01-15 12:09   ` Johannes Berg
2018-01-15 14:39     ` Benjamin Beichler
2018-01-22 13:22       ` Johannes Berg
2018-01-15 12:14   ` Johannes Berg
2018-01-15 14:48     ` Benjamin Beichler
2018-01-22 13:23       ` Johannes Berg
2018-01-22 14:49         ` Benjamin Beichler
2018-01-22 14:51           ` Johannes Berg
2018-01-10 16:42 ` [PATCH v3 5/5] mac80211_hwsim: add hwsim_tx_rate_flags to netlink attributes Benjamin Beichler
2018-01-15 12:19   ` Johannes Berg

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.