This patch series (based on mac80211next branch) includes the rest of our proposed
changes:
- add argument to set permanent MAC address of a hwsim interface while creation
- fixing bad behavior of radio dump and module unload with many radios
Changes since v3:
* applied all comments of jberg
* only included non-merged commits to avoid conflicts
Changes since v2:
* added a separate workqueue for the deferred deletion tasks (and therefore
reduce the introduced code massively)
* fixed missing signaling of interrupted nl-dump operation
* fixed small formatting errors
Changes since v1:
* syntactical/formal issues
* the hashtable is now formed by rhashtable
* instead of the radio id, the radio mac-address can be configured by new
radio command
* parallel ops patch is removed
* removed the reverse synchronization of the tx-flags from driver to mac80211,
since I believe the tx-rate flags are not used after the transmission
is done
Benjamin Beichler (3):
mac80211_hwsim: add permanent mac address option for new radios
mac80211_hwsim: add nl_err_msg in hwsim_new_radio in netlink case
mac80211_hwsim: add generation count for netlink dump operation
drivers/net/wireless/mac80211_hwsim.c | 83 +++++++++++++++++++++++++++--------
drivers/net/wireless/mac80211_hwsim.h | 9 +++-
2 files changed, 73 insertions(+), 19 deletions(-)
--
2.15.1
Am 30.01.2018 um 08:56 schrieb Johannes Berg:
> On Mon, 2018-01-22 at 18:04 +0100, Benjamin Beichler wrote:
>> + if(!info)
>> + pr_debug("mac80211_hwsim: radio index %d already present\n",
>> + idx);
> Can this really trigger? I think I'll edit it out - we do pass
> info==NULL in the case of module init, but in that case nothing else
> can interfere and we can't even really fail.
It should not be triggered because of already existing macs, I added it
for somehow completeness, but you can leave it out.
After having again a look on it, there could be an -ENOMEM on
rhashtable_insert. Maybe it's better to explicitly catch only -EEXIST
and print for the rest an generic error. Do you agree ?
> 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: [email protected]
www: http://www.imd.uni-rostock.de/
Am 31.01.2018 um 13:05 schrieb Johannes Berg:
> On Tue, 2018-01-30 at 10:47 +0100, Benjamin Beichler wrote:
>
>> It should not be triggered because of already existing macs, I added it
>> for somehow completeness, but you can leave it out.
> Sure.
>
>> After having again a look on it, there could be an -ENOMEM on
>> rhashtable_insert. Maybe it's better to explicitly catch only -EEXIST
>> and print for the rest an generic error. Do you agree ?
> Nah, that's unlikely enough ... and you get -ENOMEM back in userspace
> too.
>
> That said, good thing I looked at this - you forgot a set of braces :-)
Damn ... but fortunately you saw it. As you already accepted it, I don't
need to send new patches :-D
> johannes
>
thanks for your efforts!
Benjamin
--
M.Sc. Benjamin Beichler
Universität Rostock, Fakultät für Informatik und Elektrotechnik
Institut für Angewandte Mikroelektronik und Datentechnik
University of Rostock, Department of CS and EE
Institute of Applied Microelectronics and CE
Richard-Wagner-Straße 31
18119 Rostock
Deutschland/Germany
phone: +49 (0) 381 498 - 7278
email: [email protected]
www: http://www.imd.uni-rostock.de/
On Mon, 2018-01-22 at 18:04 +0100, Benjamin Beichler wrote:
>
> + if(!info)
> + pr_debug("mac80211_hwsim: radio index %d already present\n",
> + idx);
Can this really trigger? I think I'll edit it out - we do pass
info==NULL in the case of module init, but in that case nothing else
can interfere and we can't even really fail.
johannes
Add a NL_ERR_MSG in case of creating a radio by a netlink message to give
clear output to the creating process instead of creating only a debug
message in kernel log. The same function is used for the creation while
module load, so keep the old message, although it should never be thrown
while load, because the module controls all mac addresses.
Signed-off-by: Benjamin Beichler <[email protected]>
---
drivers/net/wireless/mac80211_hwsim.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c
index e9c92f70b97d..8baf61dea961 100644
--- a/drivers/net/wireless/mac80211_hwsim.c
+++ b/drivers/net/wireless/mac80211_hwsim.c
@@ -2796,8 +2796,13 @@ static int mac80211_hwsim_new_radio(struct genl_info *info,
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);
+ if(!info)
+ pr_debug("mac80211_hwsim: radio index %d already present\n",
+ idx);
+ else
+ GENL_SET_ERR_MSG(info,"perm addr already present");
+ NL_SET_BAD_ATTR(info->extack,
+ info->attrs[HWSIM_ATTR_PERM_ADDR]);
spin_unlock_bh(&hwsim_radio_lock);
goto failed_final_insert;
}
--
2.15.1
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 <[email protected]>
---
drivers/net/wireless/mac80211_hwsim.c | 43 +++++++++++++++++++++++++++--------
drivers/net/wireless/mac80211_hwsim.h | 9 +++++++-
2 files changed, 42 insertions(+), 10 deletions(-)
diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c
index 66c2ac0397da..e9c92f70b97d 100644
--- a/drivers/net/wireless/mac80211_hwsim.c
+++ b/drivers/net/wireless/mac80211_hwsim.c
@@ -636,6 +636,7 @@ static const struct nla_policy hwsim_genl_policy[HWSIM_ATTR_MAX + 1] = {
[HWSIM_ATTR_RADIO_NAME] = { .type = NLA_STRING },
[HWSIM_ATTR_NO_VIF] = { .type = NLA_FLAG },
[HWSIM_ATTR_FREQ] = { .type = NLA_U32 },
+ [HWSIM_ATTR_PERM_ADDR] = { .type = NLA_UNSPEC, .len = ETH_ALEN },
};
static void mac80211_hwsim_tx_frame(struct ieee80211_hw *hw,
@@ -2407,6 +2408,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,
@@ -2571,15 +2573,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;
@@ -3204,6 +3216,19 @@ 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 (!is_valid_ether_addr(
+ nla_data(info->attrs[HWSIM_ATTR_PERM_ADDR]))) {
+ GENL_SET_ERR_MSG(info,"MAC is no valid source addr");
+ NL_SET_BAD_ATTR(info->extack,
+ info->attrs[HWSIM_ATTR_PERM_ADDR]);
+ return -EINVAL;
+ }
+
+
+ param.perm_addr = nla_data(info->attrs[HWSIM_ATTR_PERM_ADDR]);
+ }
+
ret = mac80211_hwsim_new_radio(info, ¶m);
kfree(hwname);
return ret;
diff --git a/drivers/net/wireless/mac80211_hwsim.h b/drivers/net/wireless/mac80211_hwsim.h
index a96a79c1eff5..0fe3199f8c72 100644
--- a/drivers/net/wireless/mac80211_hwsim.h
+++ b/drivers/net/wireless/mac80211_hwsim.h
@@ -68,7 +68,12 @@ enum hwsim_tx_control_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
+ * 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
@@ -126,6 +131,7 @@ enum {
* @HWSIM_ATTR_FREQ: Frequency at which packet is transmitted or received.
* @HWSIM_ATTR_TX_INFO_FLAGS: additional flags for corresponding
* rates of %HWSIM_ATTR_TX_INFO
+ * @HWSIM_ATTR_PERM_ADDR: permanent mac address of new radio
* @__HWSIM_ATTR_MAX: enum limit
*/
@@ -153,6 +159,7 @@ enum {
HWSIM_ATTR_FREQ,
HWSIM_ATTR_PAD,
HWSIM_ATTR_TX_INFO_FLAGS,
+ HWSIM_ATTR_PERM_ADDR,
__HWSIM_ATTR_MAX,
};
#define HWSIM_ATTR_MAX (__HWSIM_ATTR_MAX - 1)
--
2.15.1
On Tue, 2018-01-30 at 10:47 +0100, Benjamin Beichler wrote:
> It should not be triggered because of already existing macs, I added it
> for somehow completeness, but you can leave it out.
Sure.
> After having again a look on it, there could be an -ENOMEM on
> rhashtable_insert. Maybe it's better to explicitly catch only -EEXIST
> and print for the rest an generic error. Do you agree ?
Nah, that's unlikely enough ... and you get -ENOMEM back in userspace
too.
That said, good thing I looked at this - you forgot a set of braces :-)
johannes
Make the dump operation aware of changes on radio list and corresponding
inconsistent dumps. Changed variable name for better understanding.
Signed-off-by: Benjamin Beichler <[email protected]>
---
drivers/net/wireless/mac80211_hwsim.c | 31 ++++++++++++++++++++++++-------
1 file changed, 24 insertions(+), 7 deletions(-)
diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c
index 8baf61dea961..7cff948ef438 100644
--- a/drivers/net/wireless/mac80211_hwsim.c
+++ b/drivers/net/wireless/mac80211_hwsim.c
@@ -492,6 +492,7 @@ static spinlock_t hwsim_radio_lock;
static LIST_HEAD(hwsim_radios);
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 = {
@@ -2808,6 +2809,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)
@@ -3273,6 +3275,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);
@@ -3329,17 +3332,19 @@ 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];
+ int last_idx = cb->args[0];
struct mac80211_hwsim_data *data = NULL;
- int res;
+ int res = 0;
+ void *hdr;
spin_lock_bh(&hwsim_radio_lock);
+ cb->seq = hwsim_radios_generation;
- if (idx == hwsim_radio_idx)
+ if (last_idx >= hwsim_radio_idx-1)
goto done;
list_for_each_entry(data, &hwsim_radios, list) {
- if (data->idx < idx)
+ if (data->idx <= last_idx)
continue;
if (!net_eq(wiphy_net(data->hw->wiphy), sock_net(skb->sk)))
@@ -3352,14 +3357,25 @@ static int hwsim_dump_radio_nl(struct sk_buff *skb,
if (res < 0)
break;
- idx = data->idx + 1;
+ last_idx = data->idx;
}
- cb->args[0] = idx;
+ cb->args[0] = last_idx;
+
+ /* list changed, but no new element sent, set interrupted flag */
+ if (skb->len == 0 && 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);
+ }
done:
spin_unlock_bh(&hwsim_radio_lock);
- return skb->len;
+ return (res)? res : skb->len;
}
/* Generic Netlink operations array */
@@ -3417,6 +3433,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
On Mon, Jan 22, 2018 at 06:04:35PM +0100, Benjamin Beichler wrote:
> 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.
But this patch does break operation with legacy software.. Namely, some
mac80211_hwsim test cases in hostap.git. However, that seems to be due
to a strange swapping of operations here:
> diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c
> @@ -2571,15 +2573,25 @@ static int mac80211_hwsim_new_radio(struct genl_info *info,
> - 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;
Why did that ORing of 0x40 got moved to be before the memcpy()?
Effectively, that removes the OR operation completely since the
following memcpy() overrides the first octets of
data->addresses[1].addr. Restoring the order of those two lines seems to
fix the behavior.
--
Jouni Malinen PGP id EFC895FA