NL80211_SCAN_FLAG_RANDOM_ADDR ?
Hi,
I've been poking around at how this flag is used and I noticed this
check in net/wireless/nl80211.c:
nl80211_check_scan_flags()
if (*flags & NL80211_SCAN_FLAG_RANDOM_ADDR) {
int err;
if (!(wiphy->features & randomness_flag) ||
(wdev && wdev->current_bss))
return -EOPNOTSUPP;
The above disallows the use of RANDOM_ADDR for scans while connected.
The nl80211.h uapi header seems to concur:
"@NL80211_FEATURE_SCAN_RANDOM_MAC_ADDR: This device/driver supports
using a random MAC address during scan (if the device is unassociated);"
However, if I create a P2P Device (in addition to the default STA
device), the kernel happily lets me scan on the wdev while the STA
interface is connected.
sudo iw phy0 interface add p2p type __p2pdev
sudo iw wdev 0x2 p2p start
sudo iw wdev 0x2 scan randomize
So the immediate question I have is, should the RANDOM_ADDR flag indeed
be limited to unassociated STA interfaces? It would seem the hardware
is capable randomizing even when connected? Please educate me :)
Regards,
-Denis
Re: NL80211_SCAN_FLAG_RANDOM_ADDR ?
Hi Ben,
On 04/11/2019 06:20 PM, Ben Greear wrote:
On 4/11/19 4:19 PM, Ben Greear wrote:
On 4/11/19 3:30 PM, Denis Kenzior wrote:
Hi,
I've been poking around at how this flag is used and I noticed this
check in net/wireless/nl80211.c:
nl80211_check_scan_flags()
if (*flags & NL80211_SCAN_FLAG_RANDOM_ADDR) {
int err;
if (!(wiphy->features & randomness_flag) ||
(wdev && wdev->current_bss))
return -EOPNOTSUPP;
The above disallows the use of RANDOM_ADDR for scans while connected.
The nl80211.h uapi header seems to concur:
"@NL80211_FEATURE_SCAN_RANDOM_MAC_ADDR: This device/driver supports
using a random MAC address during scan (if the device is unassociated);"
However, if I create a P2P Device (in addition to the default STA
device), the kernel happily lets me scan on the wdev while the STA
interface is connected.
sudo iw phy0 interface add p2p type __p2pdev
sudo iw wdev 0x2 p2p start
sudo iw wdev 0x2 scan randomize
So the immediate question I have is, should the RANDOM_ADDR flag
indeed be limited to unassociated STA interfaces? It would seem the
hardware is capable randomizing even when connected? Please educate
me :)
You can be sure that each driver/hardware has its own bugs and
limitations related to this.
Ath10k wave 1 and wave 2 that I am aware of would ignore and/or not
ACK probe responses
sent back to an MAC address that is not that of the station itself.
And changing the mac of a station
would require complete re-association AFAIK. That is likely just one
of the many issues.
Yes, I understand that some hardware would not support this. But the
question is does this check belong at the nl80211 layer (e.g. no
hardware can do this) vs somewhere at the driver layer + additional
feature bit as needed.
I should add: If you really want to scan in this manner, you could just
create a new station vdev with
random addr and have it do the scanning, then delete it when done? The
original station will continue on
its way unmolested.
So you mean something like:
sudo iw phy0 interface add sta2 type station
sudo iw dev sta2 scan randomize
command failed: Network is down (-100)
sudo ifconfig sta2 up
SIOCSIFFLAGS: Device or resource busy
I guess I'm running into this:
valid interface combinations:
* #{ managed } <= 1, #{ AP, P2P-client, P2P-GO } <= 1, #{ P2P-device
} <= 1,
total <= 3, #channels <= 2
Or did you mean something else?
Regards,
-Denis
Re: NL80211_SCAN_FLAG_RANDOM_ADDR ?
Hi Sergey,
On 04/12/2019 04:26 AM, Sergey Matyukevich wrote:
I've been poking around at how this flag is used and I noticed this
check in net/wireless/nl80211.c:
nl80211_check_scan_flags()
if (*flags & NL80211_SCAN_FLAG_RANDOM_ADDR) {
int err;
if (!(wiphy->features & randomness_flag) ||
(wdev && wdev->current_bss))
return -EOPNOTSUPP;
The above disallows the use of RANDOM_ADDR for scans while connected.
The nl80211.h uapi header seems to concur:
"@NL80211_FEATURE_SCAN_RANDOM_MAC_ADDR: This device/driver supports
using a random MAC address during scan (if the device is unassociated);"
However, if I create a P2P Device (in addition to the default STA
device), the kernel happily lets me scan on the wdev while the STA
interface is connected.
sudo iw phy0 interface add p2p type __p2pdev
sudo iw wdev 0x2 p2p start
sudo iw wdev 0x2 scan randomize
So the immediate question I have is, should the RANDOM_ADDR flag indeed
be limited to unassociated STA interfaces? It would seem the hardware
is capable randomizing even when connected? Please educate me :)
Hello Denis,
IIUC, this feature could be introduced to support Android Compatibility
Definition Document (CDD). Those documents are available at the
following page: https://source.android.com/compatibility/cdd
Thanks for the reference. It looks like a 'At a minimum you should/must
do this' type of document. It doesn't look like it precludes the use of
randomization when connected?
For instance, in the latest CDD randomized scan requirements are described
in the section 7.4.2. It looks like current high level nl80211 API follows
those recommendations. Probably it has been implemented with STA use-case
in mind, that is why you can use that flag for P2P connection. But, as
Ben pointed out, actual application of this flag may depend on
implementation in firwmare and hardware.
Sure, understood. But this is exactly the point of my question. Is the
check at the global level correct? Or should it be relaxed in case
there is hardware out there that can randomize probe requests while
connected? From my test it would seem this is possible?
Or put another way, besides hardware limitations, are there reasons why
you would not want to randomize probe request address when connected?
Regards,
-Denis
Re: FYI: vendor specific nl80211 API upstream
Hi Johannes, On 05/28/2019 06:51 AM, Johannes Berg wrote: Hi all, FYI - at the discussions in Prague we decided to let some vendor specific nl80211 API go upstream, and I've just documented the expected rules here: https://wireless.wiki.kernel.org/en/developers/documentation/nl80211#vendor-specific_api I'm guessing that you guys considered and rejected the idea of pushing these out to a separate, vendor specific genl family instead? Regards, -Denis
brcmfmac & DEL_INTERFACE
Hi Arend, We noticed that brcmfmac doesn't support .del_virtual_intf for non-p2p/ap interface types. Any chance this can be added? We currently remove all wifi interfaces and re-create the needed ones with SOCKET_OWNER set, and it would be nice if we didn't need to treat brcmfmac specially. Regards, -Denis
Re: brcmfmac & DEL_INTERFACE
Hi Arend, On 05/28/2019 03:27 PM, Arend Van Spriel wrote: On 5/28/2019 8:16 PM, Denis Kenzior wrote: Hi Arend, We noticed that brcmfmac doesn't support .del_virtual_intf for non-p2p/ap interface types. Any chance this can be added? We currently remove all wifi interfaces and re-create the needed ones with SOCKET_OWNER set, and it would be nice if we didn't need to treat brcmfmac specially. This came up recently. During probe the driver creates a network interface that we refer to as primary interface. We consider this non-virtual and ownership is with the driver. My guess is that this concept comes from the WEXT era, where we did not have the ieee80211 phy objects to interact with the driver from user-space. I suppose you don't mind the creation of this interface and just want to allow removing it, right? Correct. If we can at least get the DEL_INTERFACE supported, that would solve our immediate use case. I do think that the drivers should not be creating a netdev by default and should wait until userspace asks for it. But that is a separate topic, with backwards compatibility concerns, so I'll leave it for the future :) Regards, -Denis
Re: FYI: vendor specific nl80211 API upstream
Hi Johannes, On 05/29/2019 04:09 AM, Johannes Berg wrote: On Tue, 2019-05-28 at 12:36 -0500, Denis Kenzior wrote: I'm guessing that you guys considered and rejected the idea of pushing these out to a separate, vendor specific genl family instead? We do actually use that internally (though mostly for cases where we don't have a cfg80211 connection like manufacturing support), but vendor commands are there and people do like to use them :-) And herein lies the danger. If you make it too easy to add vendor APIs, there's no incentive for the vendors to do anything else. In the end this all becomes a mess for userspace to deal with. One idea off the top of my head is to introduce a concept of 'experimental' APIs in NL80211, ones that are not guaranteed to be ABI stable going forward. Specifically for dealing with such 'vendor' APIs. The semantic difference might be subtle, but I think the effect will be drastically different. E.g. people will approach this more seriously and you will get more people reviewing the API. The idea with formalizing this is that they actually get more visibility, and I hope that this will lead to more forming of real nl80211 API too. What about ABI guarantees (to tie it in with the discussion above) ? If the vendor wants to change their API, can they? Are NL80211 APIs stable unless they are vendor APIs? Anyhow, speaking from experience with oFono, which has to deal with a bazillion of wwan modem vendors, I suspect that the opposite will actually happen. Any time we let through a vendor API, the vendor lost any interest in generalizing it further. And it becomes a huge pain to implement a proper generic one later. I get that there are cases where something just cannot be generalized. In that case it belongs on a separate genl family (or whatever) altogether. So I would highly encourage you to reconsider this decision and deprecate vendor APIs altogether. If someone really cares, they can implement their own genl family. It is really not that hard. And then they control the API, API stability policy, etc. Regards, -Denis
Re: cellular modem APIs - take 2
Hi Johannes, It just seemed that people do want to have a netdev (if only to see that their driver loaded and use ethtool to dump the firmware version), and then you can reassign it to some actual context later? I can see that this is useful for developers, but really counterproductive in production. You have a bunch of services (systemd, NM, ConnMan, dhcpcd, etc, etc, etc) all observing the newly created devices and trying to 'own' them. Which makes no freaking sense to do until those devices are really usable. Just because it is a netdev, doesn't mean it is ethernet or behaves like it. That also leads to expectations from users that want stuff like udev-consistent-naming to work, even though udev has no idea wtf a given device is, when it is ready or not ready, etc. And the flip side, there may be systems that do not use systemd/udevd, so the daemons responsible for management of such devices cannot sanely assume anything. It is just pure chaos. And then there's hotplug/unplug to worry about ;) So I would like to reiterate Marcel's point: creating unmanaged devices should not be the default behavior. It doesn't really matter much. If you have a control channel and higher- level abstraction (wwan device) then having the netdev is probably more of a nuisance and mostly irrelevant, just might be useful for legacy reasons. Which we should be trying to eradicate, not encourage ;) Should you really need to account for these specially, or would some kind of sysfs linkage like SET_NETDEV_DEV() be more appropriate? Really all you want to do is (a) identify which WWAN device a given control/data channel is for and (b) perhaps tag different control/data channels with attributes like primary/secondary/gps/sim/etc often through USB attributes or hardcoded data on SoCs. Userspace can also choose to do its own multiplexing, so you can't even really assume the above is what you 'want'. Regards, -Denis
Re: cellular modem APIs - take 2
Hi Johannes, After all, I'm not really proposing that we put oFono or something like it into the kernel - far from it! I'm only proposing that we kill the many various ways of creating and managing the necessary netdevs (VLANs, sysfs, rmnet, ...) from a piece of software like oFono (or libmbim or whatever else). I do like the concept of unifying this if possible. The question is, is it actually possible :) I think Dan covered most of the aspects of what userspace has to deal with already. But the basic issue is that there's a heck of a lot of different ways of doing it. Apart from CAIF and phonet, oFono doesn't even try to do this though, afaict, so I guess it relies on the default netdev created, or some out- of-band configuration is still needed? Actually it can. We can drive modems which provide only a single serial port and run multiplexing over that. So we fully control the number of control channels created, the number of netdevs created and even create/destroy them on as needed basis. And these netdevs can be PPP encapsulated or pure IP or whatever else. Regards, -Denis
[PATCH 1/3] nl80211: Update uapi for CMD_FRAME_WAIT_CANCEL
1c38c7f2 added the possibility of NL80211_CMD_FRAME_WAIT_CANCEL being sent whenever the off-channel wait time associated with a CMD_FRAME completes. Document this in the uapi/linux/nl80211.h file. Signed-off-by: Denis Kenzior --- include/uapi/linux/nl80211.h | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h index 8fc3a43cac75..0d9aad98c983 100644 --- a/include/uapi/linux/nl80211.h +++ b/include/uapi/linux/nl80211.h @@ -657,7 +657,9 @@ * is used during CSA period. * @NL80211_CMD_FRAME_WAIT_CANCEL: When an off-channel TX was requested, this * command may be used with the corresponding cookie to cancel the wait - * time if it is known that it is no longer necessary. + * time if it is known that it is no longer necessary. This command is + * also sent as an event whenever the driver has completed the off-channel + * wait time. * @NL80211_CMD_ACTION: Alias for @NL80211_CMD_FRAME for backward compatibility. * @NL80211_CMD_FRAME_TX_STATUS: Report TX status of a management frame * transmitted with %NL80211_CMD_FRAME. %NL80211_ATTR_COOKIE identifies -- 2.21.0
[PATCH 2/3] nl80211: Limit certain commands to interface owner
If the wdev object has been created (via NEW_INTERFACE) with
SOCKET_OWNER attribute set, then limit certain commands only to the
process that created that wdev.
This can be used to make sure no other process on the system interferes
by sending unwanted scans, action frames or any other funny business.
This patch introduces a new internal flag, and checks that flag in the
pre_doit hook.
Signed-off-by: Denis Kenzior
---
net/wireless/nl80211.c | 80 --
1 file changed, 61 insertions(+), 19 deletions(-)
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index ff760ba83449..26bab9560c0f 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -13587,6 +13587,7 @@ static int nl80211_probe_mesh_link(struct sk_buff *skb,
struct genl_info *info)
#define NL80211_FLAG_NEED_WDEV_UP (NL80211_FLAG_NEED_WDEV |\
NL80211_FLAG_CHECK_NETDEV_UP)
#define NL80211_FLAG_CLEAR_SKB 0x20
+#define NL80211_FLAG_OWNER_ONLY0x40
static int nl80211_pre_doit(const struct genl_ops *ops, struct sk_buff *skb,
struct genl_info *info)
@@ -13595,6 +13596,7 @@ static int nl80211_pre_doit(const struct genl_ops *ops,
struct sk_buff *skb,
struct wireless_dev *wdev;
struct net_device *dev;
bool rtnl = ops->internal_flags & NL80211_FLAG_NEED_RTNL;
+ int ret;
if (rtnl)
rtnl_lock();
@@ -13602,10 +13604,10 @@ static int nl80211_pre_doit(const struct genl_ops
*ops, struct sk_buff *skb,
if (ops->internal_flags & NL80211_FLAG_NEED_WIPHY) {
rdev = cfg80211_get_dev_from_info(genl_info_net(info), info);
if (IS_ERR(rdev)) {
- if (rtnl)
- rtnl_unlock();
- return PTR_ERR(rdev);
+ ret = PTR_ERR(rdev);
+ goto done;
}
+
info->user_ptr[0] = rdev;
} else if (ops->internal_flags & NL80211_FLAG_NEED_NETDEV ||
ops->internal_flags & NL80211_FLAG_NEED_WDEV) {
@@ -13614,32 +13616,33 @@ static int nl80211_pre_doit(const struct genl_ops
*ops, struct sk_buff *skb,
wdev = __cfg80211_wdev_from_attrs(genl_info_net(info),
info->attrs);
if (IS_ERR(wdev)) {
- if (rtnl)
- rtnl_unlock();
- return PTR_ERR(wdev);
+ ret = PTR_ERR(wdev);
+ goto done;
}
dev = wdev->netdev;
rdev = wiphy_to_rdev(wdev->wiphy);
+ ret = -EINVAL;
if (ops->internal_flags & NL80211_FLAG_NEED_NETDEV) {
- if (!dev) {
- if (rtnl)
- rtnl_unlock();
- return -EINVAL;
- }
+ if (!dev)
+ goto done;
info->user_ptr[1] = dev;
} else {
info->user_ptr[1] = wdev;
}
+ ret = -ENETDOWN;
if (ops->internal_flags & NL80211_FLAG_CHECK_NETDEV_UP &&
- !wdev_running(wdev)) {
- if (rtnl)
- rtnl_unlock();
- return -ENETDOWN;
- }
+ !wdev_running(wdev))
+ goto done;
+
+ ret = -EPERM;
+ if (ops->internal_flags & NL80211_FLAG_OWNER_ONLY &&
+ wdev->owner_nlportid &&
+ wdev->owner_nlportid != info->snd_portid)
+ goto done;
if (dev)
dev_hold(dev);
@@ -13647,7 +13650,13 @@ static int nl80211_pre_doit(const struct genl_ops
*ops, struct sk_buff *skb,
info->user_ptr[0] = rdev;
}
- return 0;
+ ret = 0;
+
+done:
+ if (rtnl && !ret)
+ rtnl_unlock();
+
+ return ret;
}
static void nl80211_post_doit(const struct genl_ops *ops, struct sk_buff *skb,
@@ -13712,7 +13721,8 @@ static const struct genl_ops nl80211_ops[] = {
.doit = nl80211_set_interface,
.flags = GENL_UNS_ADMIN_PERM,
.internal_flags = NL80211_FLAG_NEED_NETDEV |
- NL80211_FLAG_NEED_RTNL,
+ NL80211_FLAG_NEED_RTNL |
+ NL80211_FLAG_OWNER_ONLY,
},
{
.cmd = NL80211_CMD_NEW_INTERFACE,
@@ -13728,7 +13738,8 @@ static const struct genl_ops nl80211_ops[] = {
[PATCH 3/3] nl80211: Include wiphy address setup in NEW_WIPHY
Include wiphy address setup in wiphy dumps and new wiphy events. The
wiphy permanent address is exposed as ATTR_MAC. If addr_mask is setup,
then it is included as ATTR_MAC_MASK attribute. If multiple addresses
are available, then their are exposed in a nested ATTR_MAC_ADDRS array.
This information is already exposed via sysfs, but it makes sense to
include it in the wiphy dump as well.
Signed-off-by: Denis Kenzior
---
net/wireless/nl80211.c | 25 +
1 file changed, 25 insertions(+)
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 26bab9560c0f..65f3d47d9b63 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -1852,6 +1852,31 @@ static int nl80211_send_wiphy(struct
cfg80211_registered_device *rdev,
switch (state->split_start) {
case 0:
+ if (nla_put(msg, NL80211_ATTR_MAC, ETH_ALEN,
+ rdev->wiphy.perm_addr))
+ goto nla_put_failure;
+
+ if (!is_zero_ether_addr(rdev->wiphy.addr_mask) &&
+ nla_put(msg, NL80211_ATTR_MAC_MASK, ETH_ALEN,
+ rdev->wiphy.addr_mask))
+ goto nla_put_failure;
+
+ if (rdev->wiphy.n_addresses > 1) {
+ void *attr;
+
+ attr = nla_nest_start_noflag(msg,
+NL80211_ATTR_MAC_ADDRS);
+ if (!attr)
+ goto nla_put_failure;
+
+ for (i = 0; i < rdev->wiphy.n_addresses; i++)
+ if (nla_put(msg, i + 1, ETH_ALEN,
+ rdev->wiphy.addresses[i].addr))
+ goto nla_put_failure;
+
+ nla_nest_end(msg, attr);
+ }
+
if (nla_put_u8(msg, NL80211_ATTR_WIPHY_RETRY_SHORT,
rdev->wiphy.retry_short) ||
nla_put_u8(msg, NL80211_ATTR_WIPHY_RETRY_LONG,
--
2.21.0
Re: [PATCH 3/3] nl80211: Include wiphy address setup in NEW_WIPHY
Hi Johannes,
On 06/20/2019 01:58 AM, Johannes Berg wrote:
Didn't really review all of this yet, but
switch (state->split_start) {
case 0:
+ if (nla_put(msg, NL80211_ATTR_MAC, ETH_ALEN,
+ rdev->wiphy.perm_addr))
+ goto nla_put_failure;
We generally can't add anything to any of the cases before the split was
allowed, for compatibility with old userspace.
Can you educate me here? Is it because the non-split dump messages would
grow too large? But then non-dumps aren't split, so I still don't get
how anyone can be broken by this (that isn't already broken in the first
place).
Anyhow, What is the cut off point? It didn't seem worthwhile to send
yet-another-message for ~60 bytes of data, but if you want me to add it
as a separate message, no problem.
Regards,
-Denis
Re: [PATCH 3/3] nl80211: Include wiphy address setup in NEW_WIPHY
Hi Johannes, On 06/20/2019 02:17 PM, Johannes Berg wrote: Hi Denis, We generally can't add anything to any of the cases before the split was allowed, for compatibility with old userspace. Can you educate me here? Is it because the non-split dump messages would grow too large? No. Those messages aren't really relevant, userspace will need to do a larger buffer for it. The problem is that old userspace (like really old) didn't split even dumps. Eventually, we had so much information here that the default dump message size is exceeded, and we simply couldn't dump that particular wiphy anymore. We solved this by splitting the wiphy information into multiple messages, but that needed new userspace, so when userspace doesn't request split dumps, we fall through all the way to "case 8" and then stop - old userspace cannot care about new information anyway. The reason it was split into cases 0-8 that are combined in non-split dumps is that it was safer that way - there were certain configurations where even the original data would go above the message size limit. Ugh. So, if I understand this correctly, NEW_WIPHY events that are generated when a new wiphy is plugged would only send the old 'legacy' info and any info we add in cases 9+ would be 'lost' and the application is forced into re-dumping the phy. This is pretty much counter to what we want. If you want to keep your sanity in userspace, you need proper 'object appeared' / 'object disappeared' events from the kernel. And those events should have all or nearly all info to not bother the kernel going forward. It sounds like nl80211 API has run into the extend-ability wall, no? Any suggestions on how to resolve this? Should NEW_WIPHY events also do the whole split_dump semantic and generate 15+ or whatever messages? Regards, -Denis
Re: [PATCH 3/3] nl80211: Include wiphy address setup in NEW_WIPHY
Hi Johannes, On 06/20/2019 03:09 PM, Johannes Berg wrote: On Thu, 2019-06-20 at 15:05 -0500, Denis Kenzior wrote: Ugh. So, if I understand this correctly, NEW_WIPHY events that are generated when a new wiphy is plugged would only send the old 'legacy' info and any info we add in cases 9+ would be 'lost' and the application is forced into re-dumping the phy. Yes. This is pretty much counter to what we want. Well, you want the info, shouldn't matter how you get it? Well, it kind of does. You're asking userspace to introduce extra complexity, extra round trips, extra stuff to go wrong just because the kernel API has painted itself into a corner. If you want to keep your sanity in userspace, you need proper 'object appeared' / 'object disappeared' events from the kernel. Sure, but you don't really need to know *everything* about the events right there ... you can already filter which ones you care about (perhaps you know you never want to bind hwsim ones for example) and then request data on those that you do need. Sure, but it would be nice to have all the info available if we do not want to filter it... And those events should have all or nearly all info to not bother the kernel going forward. That's what you wish for, but ... Well, it is a pretty basic requirement for any event driven API, no? It sounds like nl80211 API has run into the extend-ability wall, no? I don't really see it that way. Any suggestions on how to resolve this? Should NEW_WIPHY events also do the whole split_dump semantic and generate 15+ or whatever messages? No, that'd be awful, and anyway you'd have to send a new command because otherwise old applications might be completely confused (not that I know of any other than "iw event" that would event listen to this, but who knows) Well, given that we're the only ones that seem to care about this right now, I don't see sending a new command as much of a big deal. I welcome other ideas, but having the kernel send us an event, then us turning around and requesting the *same* info is just silly. Regards, -Denis
[PATCH v2 1/3] nl80211: Update uapi for CMD_FRAME_WAIT_CANCEL
Commit 1c38c7f22068 ("nl80211: send event when CMD_FRAME duration
expires") added the possibility of NL80211_CMD_FRAME_WAIT_CANCEL
being sent whenever the off-channel wait time associated with a
CMD_FRAME completes. Document this in the uapi/linux/nl80211.h file.
Signed-off-by: Denis Kenzior
---
include/uapi/linux/nl80211.h | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
Changes in v2:
- update commit formatting
diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index 8fc3a43cac75..0d9aad98c983 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -657,7 +657,9 @@
* is used during CSA period.
* @NL80211_CMD_FRAME_WAIT_CANCEL: When an off-channel TX was requested, this
* command may be used with the corresponding cookie to cancel the wait
- * time if it is known that it is no longer necessary.
+ * time if it is known that it is no longer necessary. This command is
+ * also sent as an event whenever the driver has completed the off-channel
+ * wait time.
* @NL80211_CMD_ACTION: Alias for @NL80211_CMD_FRAME for backward
compatibility.
* @NL80211_CMD_FRAME_TX_STATUS: Report TX status of a management frame
* transmitted with %NL80211_CMD_FRAME. %NL80211_ATTR_COOKIE identifies
--
2.21.0
[PATCH v2 2/3] nl80211: Limit certain commands to interface owner
If the wdev object has been created (via NEW_INTERFACE) with
SOCKET_OWNER attribute set, then limit certain commands only to the
process that created that wdev.
This can be used to make sure no other process on the system interferes
by sending unwanted scans, action frames or any other funny business.
This patch introduces a new internal flag, and checks that flag in the
pre_doit hook.
Signed-off-by: Denis Kenzior
---
net/wireless/nl80211.c | 80 --
1 file changed, 61 insertions(+), 19 deletions(-)
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index ff760ba83449..26bab9560c0f 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -13587,6 +13587,7 @@ static int nl80211_probe_mesh_link(struct sk_buff *skb,
struct genl_info *info)
#define NL80211_FLAG_NEED_WDEV_UP (NL80211_FLAG_NEED_WDEV |\
NL80211_FLAG_CHECK_NETDEV_UP)
#define NL80211_FLAG_CLEAR_SKB 0x20
+#define NL80211_FLAG_OWNER_ONLY0x40
static int nl80211_pre_doit(const struct genl_ops *ops, struct sk_buff *skb,
struct genl_info *info)
@@ -13595,6 +13596,7 @@ static int nl80211_pre_doit(const struct genl_ops *ops,
struct sk_buff *skb,
struct wireless_dev *wdev;
struct net_device *dev;
bool rtnl = ops->internal_flags & NL80211_FLAG_NEED_RTNL;
+ int ret;
if (rtnl)
rtnl_lock();
@@ -13602,10 +13604,10 @@ static int nl80211_pre_doit(const struct genl_ops
*ops, struct sk_buff *skb,
if (ops->internal_flags & NL80211_FLAG_NEED_WIPHY) {
rdev = cfg80211_get_dev_from_info(genl_info_net(info), info);
if (IS_ERR(rdev)) {
- if (rtnl)
- rtnl_unlock();
- return PTR_ERR(rdev);
+ ret = PTR_ERR(rdev);
+ goto done;
}
+
info->user_ptr[0] = rdev;
} else if (ops->internal_flags & NL80211_FLAG_NEED_NETDEV ||
ops->internal_flags & NL80211_FLAG_NEED_WDEV) {
@@ -13614,32 +13616,33 @@ static int nl80211_pre_doit(const struct genl_ops
*ops, struct sk_buff *skb,
wdev = __cfg80211_wdev_from_attrs(genl_info_net(info),
info->attrs);
if (IS_ERR(wdev)) {
- if (rtnl)
- rtnl_unlock();
- return PTR_ERR(wdev);
+ ret = PTR_ERR(wdev);
+ goto done;
}
dev = wdev->netdev;
rdev = wiphy_to_rdev(wdev->wiphy);
+ ret = -EINVAL;
if (ops->internal_flags & NL80211_FLAG_NEED_NETDEV) {
- if (!dev) {
- if (rtnl)
- rtnl_unlock();
- return -EINVAL;
- }
+ if (!dev)
+ goto done;
info->user_ptr[1] = dev;
} else {
info->user_ptr[1] = wdev;
}
+ ret = -ENETDOWN;
if (ops->internal_flags & NL80211_FLAG_CHECK_NETDEV_UP &&
- !wdev_running(wdev)) {
- if (rtnl)
- rtnl_unlock();
- return -ENETDOWN;
- }
+ !wdev_running(wdev))
+ goto done;
+
+ ret = -EPERM;
+ if (ops->internal_flags & NL80211_FLAG_OWNER_ONLY &&
+ wdev->owner_nlportid &&
+ wdev->owner_nlportid != info->snd_portid)
+ goto done;
if (dev)
dev_hold(dev);
@@ -13647,7 +13650,13 @@ static int nl80211_pre_doit(const struct genl_ops
*ops, struct sk_buff *skb,
info->user_ptr[0] = rdev;
}
- return 0;
+ ret = 0;
+
+done:
+ if (rtnl && !ret)
+ rtnl_unlock();
+
+ return ret;
}
static void nl80211_post_doit(const struct genl_ops *ops, struct sk_buff *skb,
@@ -13712,7 +13721,8 @@ static const struct genl_ops nl80211_ops[] = {
.doit = nl80211_set_interface,
.flags = GENL_UNS_ADMIN_PERM,
.internal_flags = NL80211_FLAG_NEED_NETDEV |
- NL80211_FLAG_NEED_RTNL,
+ NL80211_FLAG_NEED_RTNL |
+ NL80211_FLAG_OWNER_ONLY,
},
{
.cmd = NL80211_CMD_NEW_INTERFACE,
@@ -13728,7 +13738,8 @@ static const struct genl_ops nl80211_ops[] = {
[PATCH v2 3/3] nl80211: Include wiphy address setup in NEW_WIPHY
Include wiphy address setup in wiphy dumps and new wiphy events. The
wiphy permanent address is exposed as ATTR_MAC. If addr_mask is setup,
then it is included as ATTR_MAC_MASK attribute. If multiple addresses
are available, then their are exposed in a nested ATTR_MAC_ADDRS array.
This information is already exposed via sysfs, but it makes sense to
include it in the wiphy dump as well.
Signed-off-by: Denis Kenzior
---
net/wireless/nl80211.c | 25 +
1 file changed, 25 insertions(+)
Changes in v2:
- Move from case 0 to 9
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 26bab9560c0f..f4b3e6f1dfbf 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -2166,6 +2166,31 @@ static int nl80211_send_wiphy(struct
cfg80211_registered_device *rdev,
rdev->wiphy.vht_capa_mod_mask))
goto nla_put_failure;
+ if (nla_put(msg, NL80211_ATTR_MAC, ETH_ALEN,
+ rdev->wiphy.perm_addr))
+ goto nla_put_failure;
+
+ if (!is_zero_ether_addr(rdev->wiphy.addr_mask) &&
+ nla_put(msg, NL80211_ATTR_MAC_MASK, ETH_ALEN,
+ rdev->wiphy.addr_mask))
+ goto nla_put_failure;
+
+ if (rdev->wiphy.n_addresses > 1) {
+ void *attr;
+
+ attr = nla_nest_start_noflag(msg,
+NL80211_ATTR_MAC_ADDRS);
+ if (!attr)
+ goto nla_put_failure;
+
+ for (i = 0; i < rdev->wiphy.n_addresses; i++)
+ if (nla_put(msg, i + 1, ETH_ALEN,
+ rdev->wiphy.addresses[i].addr))
+ goto nla_put_failure;
+
+ nla_nest_end(msg, attr);
+ }
+
state->split_start++;
break;
case 10:
--
2.21.0
Re: [PATCH 3/3] nl80211: Include wiphy address setup in NEW_WIPHY
Hi Johannes, On 06/20/2019 03:21 PM, Johannes Berg wrote: On Thu, 2019-06-20 at 22:09 +0200, Johannes Berg wrote: Sure, but you don't really need to know *everything* about the events right there ... you can already filter which ones you care about (perhaps you know you never want to bind hwsim ones for example) and then request data on those that you do need. Btw, you can send a filter down when you do request the data, so you only get the data for the new wiphy you actually just discovered. Yes, I know that. I did help fix this ~3 years ago in commit b7fb44dacae04. Nobody was using that prior, which really leads me to wonder what other userspace tools are doing for hotplug and how broken they are... So realistically, vs. your suggestion of sending all of the data in multiple events, that just adds 2 messages (the request and the data you already had), which isn't nearly as bad as you paint it. I never 'painted' the message overhead as 'bad'. The performance overhead of this ping-pong is probably irrelevant in the grand scheme of things. But I find the approach inelegant. But really I'm more worried about race conditions that userspace has to deal with. We already have the weird case of ATTR_GENERATION (which nobody actually uses btw). And then we also need to dump both the wiphys and the interfaces separately, cross-reference them while dealing with the possibility of a wiphy or interface going away or being added at any point. Then there's the fact that some drivers always add a default netdev, others that (possibly) don't and the possibility that the system was left in a weird state. So from that standpoint it is far better to have the kernel generate atomic change events with all the info present than having userspace re-poll it when stuff might have changed in the meantime. Going back to your 2 message point. What about sending the 'NEW_WIPHY' event with the info in labels 0-8. And then another event type with the 'rest' of the info. And perhaps another feature bit to tell userspace to expect multiple events. That would still end up being 2 messages and still be more efficient than the ping-pong you suggest. Regards, -Denis
Re: [PATCH v2 2/3] nl80211: Limit certain commands to interface owner
Hi Arend, On 06/21/2019 03:09 AM, Arend Van Spriel wrote: On 6/21/2019 12:07 AM, Denis Kenzior wrote: If the wdev object has been created (via NEW_INTERFACE) with SOCKET_OWNER attribute set, then limit certain commands only to the process that created that wdev. This can be used to make sure no other process on the system interferes by sending unwanted scans, action frames or any other funny business. The flag is a good addition opposed to having handlers deal with it. However, earlier motivation for SOCKET_OWNER use was about netlink multicast being unreliable, which I can agree to. However, avoiding ??? I can't agree to that as I have no idea what you're talking about :) Explain? SOCKET_OWNER was introduced mainly to bring down links / scans / whatever in case the initiating process died. As a side effect it also helped in the beginning when users ran iwd + wpa_s simultaneously (by accident) and all sorts of fun ensued. We then re-used SOCKET_OWNER for running EAPoL over NL80211. But 'multicast unreliability' was never an issue that I recall? "funny business" is a different thing. Our testing infrastructure is doing all kind of funny business. Guess we will need to refrain from So you're going behind the managing daemon's back and messing with the kernel state... I guess the question is why? But really, if wpa_s wants to tolerate that, that is their problem :) iwd doesn't want to, nor do we want to deal with the various race conditions and corner cases associated with that. Life is hard as it is ;) using any user-space wireless tools that use the SOCKET_OWNER attribute, but how do we know? Somehow I suspect iwd is one to avoid ;-) I have yet I guess you will be avoiding wpa_s since that one uses SOCKET_OWNER too ;) to give iwd a spin, but this SOCKET_OWNER strategy kept me from it. Maybe iwd could have a developer option which disables the use of the SOCKET_OWNER attribute. Okay? Not sure what you're trying to say here? I'd interpret this as "You guys suck. I'm taking my ball and going home?" but I hope this isn't what you're saying? Regards, -Denis
Re: iwlwifi/brcmfmac public action frames crash (RESENDING)
Ping, is anyone looking into these crashes? On 06/13/2019 11:45 AM, James Prestwood wrote: Sorry if this comes in twice, I sent it ~12 hours ago but never saw it hit the list, nor in the archives so I am resending it. Hi, Both iwlwifi/brcmfmac seem to be unable to send public action frames to an unassociated AP. I am attempting to do a GAS ANQP request with a public action frame (via CMD_FRAME). Immediately after CMD_FRAME any of the following happens depending on the card: Intel 7260 (iwlwifi) - System lockup freeze (must hard reboot) Intel 3160 (iwlwifi) - CMD_FRAME returns -EINVAL BCM43602 (brcmfmac) - Kernel crash (below) AR9462 (ath9k) - works Random USB adapter (rt2800usb) - works iwlwifi (on 7260) completely locks the system, where the only way to recover is hard reboot. I have reproduced this on two separate systems, both with a 7260. I *have* seen it not lock the system once although lately it seems to happen every time. The 3160 did not cause a hang with my limited testing, though it did not accept CMD_FRAME which is likely why it never hung. Not sure how I can get any more info about the iwlwifi problem as the system is completely hung, but if there is a way I'll be happy to do that. Here is the brcmfmac crash: [19735.643941] BUG: unable to handle kernel NULL pointer dereference at [19735.643965] PGD 8001874aa067 P4D 8001874aa067 PUD 2735fe067 PMD 0 [19735.643984] Oops: [#1] SMP PTI [19735.643993] CPU: 7 PID: 5051 Comm: iwd Tainted: GW I 4.19.0-rc2-custom #27 [19735.644002] Hardware name: System manufacturer System Product Name/SABERTOOTH X58, BIOS 140208/09/2012 [19735.644027] RIP: 0010:brcmf_p2p_send_action_frame+0x23a/0x850 [brcmfmac] [19735.644037] Code: 41 c7 86 e0 00 00 00 00 00 00 00 f0 41 80 66 20 bf f0 41 80 66 20 7f 49 8b 46 48 b9 24 07 00 00 48 89 da 48 c7 c6 3d 00 8f c0 <48> 8b 38 e8 3e d7 ff ff 85 c0 41 89 c5 0f 85 c4 00 00 00 8b 03 49 [19735.644051] RSP: 0018:a879c8477a00 EFLAGS: 00010246 [19735.644059] RAX: RBX: 954a2e059000 RCX: 0724 [19735.644067] RDX: 954a2e059000 RSI: c08f003d RDI: 0002 [19735.644075] RBP: a879c8477a50 R08: 001c R09: 0999 [19735.644083] R10: 954b157a2f00 R11: c072 R12: 954c32f26021 [19735.644091] R13: 954a2e059000 R14: 954c32f26000 R15: [19735.644099] FS: 7f8d5aa30740() GS:954c369c() knlGS: [19735.644108] CS: 0010 DS: ES: CR0: 80050033 [19735.644115] CR2: CR3: 0001845c8000 CR4: 06e0 [19735.644123] Call Trace: [19735.644133] ? _cond_resched+0x19/0x40 [19735.644153] brcmf_cfg80211_mgmt_tx+0x170/0x2f0 [brcmfmac] [19735.644192] cfg80211_mlme_mgmt_tx+0x115/0x2f0 [cfg80211] [19735.644219] nl80211_tx_mgmt+0x24d/0x3d0 [cfg80211] [19735.644228] genl_family_rcv_msg+0x1fe/0x3f0 [19735.644237] ? nlmon_xmit+0x2c/0x30 [19735.644246] ? dev_hard_start_xmit+0xa8/0x210 [19735.644254] genl_rcv_msg+0x4c/0x90 [19735.644261] ? genl_family_rcv_msg+0x3f0/0x3f0 [19735.644268] netlink_rcv_skb+0x54/0x130 [19735.644275] genl_rcv+0x28/0x40 [19735.644281] netlink_unicast+0x1ab/0x250 [19735.644288] netlink_sendmsg+0x2d1/0x3d0 [19735.644297] sock_sendmsg+0x3e/0x50 [19735.644304] __sys_sendto+0x13f/0x180 [19735.644313] ? do_epoll_wait+0xb0/0xc0 [19735.644321] __x64_sys_sendto+0x28/0x30 [19735.644329] do_syscall_64+0x5a/0x120 [19735.644336] entry_SYSCALL_64_after_hwframe+0x44/0xa9 [19735.644344] RIP: 0033:0x7f8d5a352c4d [19735.644350] Code: ff ff ff ff eb b6 0f 1f 80 00 00 00 00 48 8d 05 c1 dc 2c 00 41 89 ca 8b 00 85 c0 75 20 45 31 c9 45 31 c0 b8 2c 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 6b f3 c3 66 0f 1f 84 00 00 00 00 00 41 56 41 [19735.644365] RSP: 002b:7ffc9a618048 EFLAGS: 0246 ORIG_RAX: 002c [19735.644374] RAX: ffda RBX: 007077d0 RCX: 7f8d5a352c4d [19735.644382] RDX: 0068 RSI: 0072bc40 RDI: 0004 [19735.644390] RBP: 00733510 R08: R09: [19735.644397] R10: R11: 0246 R12: 7ffc9a618094 [19735.644405] R13: 7ffc9a61809c R14: R15: [19735.644414] Modules linked in: ccm algif_aead snd_hda_codec_realtek snd_hda_codec_generic snd_hda_codec_hdmi binfmt_misc arc4 nouveau gpio_ich ath9k mxm_wmi ath9k_common video rt2800usb intel_powerclamp snd_hda_intel ath9k_hw rt2x00usb iwlmvm rt2800lib snd_hda_codec rt2x00lib ath snd_seq_midi snd_seq_midi_event coretemp ttm mac80211 snd_hda_core brcmfmac snd_hwdep snd_rawmidi iwlwifi intel_cstate drm_kms_helper brcmutil snd_seq drm snd_pcm input_leds serio_raw lpc_ich cfg80211 snd_seq_device i2c_algo_bit snd_timer fb_sys_fops syscopyarea sysfillrect snd sysimgblt i5500_temp wmi asus_atk0110 soundcore mac_hid i7core_edac sch_fq_codel kvm_intel kvm vfio_pci vfio_virqfd irqbypass vfio_iommu_
Re: [PATCH v2 2/3] nl80211: Limit certain commands to interface owner
Hi Arend, "funny business" is a different thing. Our testing infrastructure is doing all kind of funny business. Guess we will need to refrain from So you're going behind the managing daemon's back and messing with the kernel state... I guess the question is why? But really, if wpa_s wants to tolerate that, that is their problem :) iwd doesn't want to, nor do we want to deal with the various race conditions and corner cases associated with that. Life is hard as it is ;) That's just it, right. This is what Marcel calls the real environment, but is it. The nl80211 is a kernel API and should that mean that there must be a managing daemon locking down APIs for other user-space tools to use. If I want a user-space app to show a radar screen with surrounding APs using scanning and FTM nl80211 commands it seems now it has to create a new interface and hope the resources are there for it to succeed. Where is my freedom in that? If I am using such an app don't you think I don't accept it could impact the managing daemon. I get it. But on the flip side, should the managing daemon accept you messing with it? I mean there is a definite associated cost here, whether it is stuff crashing, having to account for extra corner cases and race conditions, giving out erroneous results, etc. As Marcel pointed out, the proper solution is to do this via some diagnostic interface on the managing daemon, so it can properly manage such requests to not interfere with whatever else is going on. By the way, the above would be generally useful to many people, so if you have some code lying around... ;) to give iwd a spin, but this SOCKET_OWNER strategy kept me from it. Maybe iwd could have a developer option which disables the use of the SOCKET_OWNER attribute. Okay? Not sure what you're trying to say here? I'd interpret this as "You guys suck. I'm taking my ball and going home?" but I hope this isn't what you're saying? Not saying that. Just saying that the "real environment" is in the eye of the beholder and it would be nice if there was a way to opt out, but Marcel seems strongly opposed to it. So there seems no point in scratching that itch and come up with a patch. I guess the question is, what do you want this for? If you want this for pure manual testing and accept the consequence of the managing daemon crashing, giving erroneous results or being otherwise confused? If you're fine with the above, I don't see a problem with such a patch. Regards, -Denis
Re: [PATCH v2 2/3] nl80211: Limit certain commands to interface owner
Hi Arend, On 06/24/2019 03:39 AM, Arend Van Spriel wrote: On 6/22/2019 3:44 PM, Marcel Holtmann wrote: Hi Arend, If the wdev object has been created (via NEW_INTERFACE) with SOCKET_OWNER attribute set, then limit certain commands only to the process that created that wdev. This can be used to make sure no other process on the system interferes by sending unwanted scans, action frames or any other funny business. The flag is a good addition opposed to having handlers deal with it. However, earlier motivation for SOCKET_OWNER use was about netlink multicast being unreliable, which I can agree to. However, avoiding ??? I can't agree to that as I have no idea what you're talking about :) Explain? SOCKET_OWNER was introduced mainly to bring down links / scans / whatever in case the initiating process died. As a side effect it also helped in the beginning when users ran iwd + wpa_s simultaneously (by accident) and all sorts of fun ensued. We then re-used SOCKET_OWNER for running EAPoL over NL80211. But 'multicast unreliability' was never an issue that I recall? hmm. I tried searching in memory... of my email client but to no avail. I somehow recalled that netlink multicast was not guaranteed to be delivered/seen by all listeners. "funny business" is a different thing. Our testing infrastructure is doing all kind of funny business. Guess we will need to refrain from So you're going behind the managing daemon's back and messing with the kernel state... I guess the question is why? But really, if wpa_s wants to tolerate that, that is their problem :) iwd doesn't want to, nor do we want to deal with the various race conditions and corner cases associated with that. Life is hard as it is ;) That's just it, right. This is what Marcel calls the real environment, but is it. The nl80211 is a kernel API and should that mean that there must be a managing daemon locking down APIs for other user-space tools to use. If I want a user-space app to show a radar screen with surrounding APs using scanning and FTM nl80211 commands it seems now it has to create a new interface and hope the resources are there for it to succeed. Where is my freedom in that? If I am using such an app don't you think I don't accept it could impact the managing daemon. if you are operating on a shared radio resource you have to have some way of ensuring that nobody steals resources from you. Having an external application that will also use scanning and other off-channel operation will result in a bad experience. Especially if it involves scanning. Currently we still have 3 or more parties triggering scanning on nl80211. Essentially they are now fighting for radio time. You have wpa_supplicant scanning, you have NetworkManager scanning and you have the UI scanning. Now adding just another application that just scans at its decided time location / direction finding is not helping the situation. My app was just a hypothetical example. I understand your conundrum, but my point was that you can not know how a system is configured. Now for the SOCKET_OWNER I should say it does not provide you any guarantees. At best it improves your chances. With the nl80211 API being as it is, you can not rule out multiple application controlling the same device. The virtual interfaces can be guarded with SOCKET_OWNER, but in the end +1, SOCKET_OWNER is a band-aid. But in the end this is a fairly simple change. So we can have the managing daemon destroy any existing wdevs and re-create them with SOCKET_OWNER set (can we get brcmfmac to support this please?). This at least gives us a chance that nothing will inadvertently cause interference. It won't stop other processes from creating other wdevs or other funny business, but that is currently much more rare anyway. In case it really becomes a problem, we can at least say "Don't hold it that way." there is still one physical device and only if you are lucky you may come across a device with two physical radios, but most of them just have one. If you really want to be in control we should allow only one socket or at least only one "control" socket. +1. I've been saying this for a while, there should only be a single controlling socket/process, or at least an option for something like that. But the current nl80211 design makes this quite hard. Hopefully the recognition that this is needed will gain traction, until then we're stuck with band-aids. If our kernel cfg80211 / nl80211 would be smart enough to handle these concurrent tasks, I would have little objection to let all clients do whatever they want, but we don’t have that. I do not want an external application messing with my planned radio time. And frankly if I am in the middle of roaming, I don’t want to be delayed because some fancy radar looking UI decides to start a full spectrum scan or blocks us via an action frame that times out. The have been some effort
[PATCH v3 2/3] nl80211: Limit certain commands to interface owner
If the wdev object has been created (via NEW_INTERFACE) with
SOCKET_OWNER attribute set, then limit certain commands only to the
process that created that wdev.
This can be used to make sure no other process on the system interferes
by sending unwanted scans, action frames or any other funny business.
This patch introduces a new internal flag, and checks that flag in the
pre_doit hook.
Signed-off-by: Denis Kenzior
---
net/wireless/nl80211.c | 80 --
1 file changed, 61 insertions(+), 19 deletions(-)
Changes in v3:
- Fix minor locking mistake reported by kernel test robot
Changes in v2:
- None
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index ff760ba83449..ebf5eab1f9b2 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -13587,6 +13587,7 @@ static int nl80211_probe_mesh_link(struct sk_buff *skb,
struct genl_info *info)
#define NL80211_FLAG_NEED_WDEV_UP (NL80211_FLAG_NEED_WDEV |\
NL80211_FLAG_CHECK_NETDEV_UP)
#define NL80211_FLAG_CLEAR_SKB 0x20
+#define NL80211_FLAG_OWNER_ONLY0x40
static int nl80211_pre_doit(const struct genl_ops *ops, struct sk_buff *skb,
struct genl_info *info)
@@ -13595,6 +13596,7 @@ static int nl80211_pre_doit(const struct genl_ops *ops,
struct sk_buff *skb,
struct wireless_dev *wdev;
struct net_device *dev;
bool rtnl = ops->internal_flags & NL80211_FLAG_NEED_RTNL;
+ int ret;
if (rtnl)
rtnl_lock();
@@ -13602,10 +13604,10 @@ static int nl80211_pre_doit(const struct genl_ops
*ops, struct sk_buff *skb,
if (ops->internal_flags & NL80211_FLAG_NEED_WIPHY) {
rdev = cfg80211_get_dev_from_info(genl_info_net(info), info);
if (IS_ERR(rdev)) {
- if (rtnl)
- rtnl_unlock();
- return PTR_ERR(rdev);
+ ret = PTR_ERR(rdev);
+ goto done;
}
+
info->user_ptr[0] = rdev;
} else if (ops->internal_flags & NL80211_FLAG_NEED_NETDEV ||
ops->internal_flags & NL80211_FLAG_NEED_WDEV) {
@@ -13614,32 +13616,33 @@ static int nl80211_pre_doit(const struct genl_ops
*ops, struct sk_buff *skb,
wdev = __cfg80211_wdev_from_attrs(genl_info_net(info),
info->attrs);
if (IS_ERR(wdev)) {
- if (rtnl)
- rtnl_unlock();
- return PTR_ERR(wdev);
+ ret = PTR_ERR(wdev);
+ goto done;
}
dev = wdev->netdev;
rdev = wiphy_to_rdev(wdev->wiphy);
+ ret = -EINVAL;
if (ops->internal_flags & NL80211_FLAG_NEED_NETDEV) {
- if (!dev) {
- if (rtnl)
- rtnl_unlock();
- return -EINVAL;
- }
+ if (!dev)
+ goto done;
info->user_ptr[1] = dev;
} else {
info->user_ptr[1] = wdev;
}
+ ret = -ENETDOWN;
if (ops->internal_flags & NL80211_FLAG_CHECK_NETDEV_UP &&
- !wdev_running(wdev)) {
- if (rtnl)
- rtnl_unlock();
- return -ENETDOWN;
- }
+ !wdev_running(wdev))
+ goto done;
+
+ ret = -EPERM;
+ if (ops->internal_flags & NL80211_FLAG_OWNER_ONLY &&
+ wdev->owner_nlportid &&
+ wdev->owner_nlportid != info->snd_portid)
+ goto done;
if (dev)
dev_hold(dev);
@@ -13647,7 +13650,13 @@ static int nl80211_pre_doit(const struct genl_ops
*ops, struct sk_buff *skb,
info->user_ptr[0] = rdev;
}
- return 0;
+ ret = 0;
+
+done:
+ if (rtnl && ret < 0)
+ rtnl_unlock();
+
+ return ret;
}
static void nl80211_post_doit(const struct genl_ops *ops, struct sk_buff *skb,
@@ -13712,7 +13721,8 @@ static const struct genl_ops nl80211_ops[] = {
.doit = nl80211_set_interface,
.flags = GENL_UNS_ADMIN_PERM,
.internal_flags = NL80211_FLAG_NEED_NETDEV |
- NL80211_FLAG_NEED_RTNL,
+ NL80211_FLAG_NEED_RTNL |
+ NL80211_FLAG_OWNER_ONLY,
},
{
.cmd = NL802
[PATCH v3 1/3] nl80211: Update uapi for CMD_FRAME_WAIT_CANCEL
Commit 1c38c7f22068 ("nl80211: send event when CMD_FRAME duration
expires") added the possibility of NL80211_CMD_FRAME_WAIT_CANCEL
being sent whenever the off-channel wait time associated with a
CMD_FRAME completes. Document this in the uapi/linux/nl80211.h file.
Signed-off-by: Denis Kenzior
---
include/uapi/linux/nl80211.h | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
Changes in v3:
- None
Changes in v2:
- update commit formatting
diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index 8fc3a43cac75..0d9aad98c983 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -657,7 +657,9 @@
* is used during CSA period.
* @NL80211_CMD_FRAME_WAIT_CANCEL: When an off-channel TX was requested, this
* command may be used with the corresponding cookie to cancel the wait
- * time if it is known that it is no longer necessary.
+ * time if it is known that it is no longer necessary. This command is
+ * also sent as an event whenever the driver has completed the off-channel
+ * wait time.
* @NL80211_CMD_ACTION: Alias for @NL80211_CMD_FRAME for backward
compatibility.
* @NL80211_CMD_FRAME_TX_STATUS: Report TX status of a management frame
* transmitted with %NL80211_CMD_FRAME. %NL80211_ATTR_COOKIE identifies
--
2.21.0
[PATCH v3 3/3] nl80211: Include wiphy address setup in NEW_WIPHY
Include wiphy address setup in wiphy dumps and new wiphy events. The
wiphy permanent address is exposed as ATTR_MAC. If addr_mask is setup,
then it is included as ATTR_MAC_MASK attribute. If multiple addresses
are available, then their are exposed in a nested ATTR_MAC_ADDRS array.
This information is already exposed via sysfs, but it makes sense to
include it in the wiphy dump as well.
Signed-off-by: Denis Kenzior
---
net/wireless/nl80211.c | 25 +
1 file changed, 25 insertions(+)
Changes in v3:
- None
Changes in v2:
- Move from case 0 to 9
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index ebf5eab1f9b2..fb09c2301ed8 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -2166,6 +2166,31 @@ static int nl80211_send_wiphy(struct
cfg80211_registered_device *rdev,
rdev->wiphy.vht_capa_mod_mask))
goto nla_put_failure;
+ if (nla_put(msg, NL80211_ATTR_MAC, ETH_ALEN,
+ rdev->wiphy.perm_addr))
+ goto nla_put_failure;
+
+ if (!is_zero_ether_addr(rdev->wiphy.addr_mask) &&
+ nla_put(msg, NL80211_ATTR_MAC_MASK, ETH_ALEN,
+ rdev->wiphy.addr_mask))
+ goto nla_put_failure;
+
+ if (rdev->wiphy.n_addresses > 1) {
+ void *attr;
+
+ attr = nla_nest_start_noflag(msg,
+NL80211_ATTR_MAC_ADDRS);
+ if (!attr)
+ goto nla_put_failure;
+
+ for (i = 0; i < rdev->wiphy.n_addresses; i++)
+ if (nla_put(msg, i + 1, ETH_ALEN,
+ rdev->wiphy.addresses[i].addr))
+ goto nla_put_failure;
+
+ nla_nest_end(msg, attr);
+ }
+
state->split_start++;
break;
case 10:
--
2.21.0
Re: [PATCH v3 1/3] nl80211: Update uapi for CMD_FRAME_WAIT_CANCEL
ping? Regards, -Denis
Re: [PATCH v3 2/3] nl80211: Limit certain commands to interface owner
Hi Arend, On 7/18/19 3:24 AM, Arend Van Spriel wrote: On 7/1/2019 5:33 PM, Denis Kenzior wrote: If the wdev object has been created (via NEW_INTERFACE) with SOCKET_OWNER attribute set, then limit certain commands only to the process that created that wdev. This can be used to make sure no other process on the system interferes by sending unwanted scans, action frames or any other funny business. This patch introduces a new internal flag, and checks that flag in the pre_doit hook. Signed-off-by: Denis Kenzior --- net/wireless/nl80211.c | 80 -- 1 file changed, 61 insertions(+), 19 deletions(-) Changes in v3: - Fix minor locking mistake reported by kernel test robot Changes in v2: - None diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c index ff760ba83449..ebf5eab1f9b2 100644 --- a/net/wireless/nl80211.c +++ b/net/wireless/nl80211.c [snip] - return 0; + ret = 0; I suggest to keep the return 0 here for success path and only do the below for failure case (and obviously dropping '&& ret < 0'). Maybe rename label 'done' to 'fail' as well. Sure, makes sense. I've made the suggested changes for v4. +done: + if (rtnl && ret < 0) + rtnl_unlock(); + + return ret; } Regards, Arend Regards, -Denis
[PATCH v4 3/3] nl80211: Include wiphy address setup in NEW_WIPHY
Include wiphy address setup in wiphy dumps and new wiphy events. The
wiphy permanent address is exposed as ATTR_MAC. If addr_mask is setup,
then it is included as ATTR_MAC_MASK attribute. If multiple addresses
are available, then their are exposed in a nested ATTR_MAC_ADDRS array.
This information is already exposed via sysfs, but it makes sense to
include it in the wiphy dump as well.
Signed-off-by: Denis Kenzior
---
net/wireless/nl80211.c | 25 +
1 file changed, 25 insertions(+)
Changes in v4:
- None
Changes in v3:
- None
Changes in v2:
- Move from case 0 to 9
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index a075d86a52f6..3fc4a9006155 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -2172,6 +2172,31 @@ static int nl80211_send_wiphy(struct
cfg80211_registered_device *rdev,
rdev->wiphy.vht_capa_mod_mask))
goto nla_put_failure;
+ if (nla_put(msg, NL80211_ATTR_MAC, ETH_ALEN,
+ rdev->wiphy.perm_addr))
+ goto nla_put_failure;
+
+ if (!is_zero_ether_addr(rdev->wiphy.addr_mask) &&
+ nla_put(msg, NL80211_ATTR_MAC_MASK, ETH_ALEN,
+ rdev->wiphy.addr_mask))
+ goto nla_put_failure;
+
+ if (rdev->wiphy.n_addresses > 1) {
+ void *attr;
+
+ attr = nla_nest_start_noflag(msg,
+NL80211_ATTR_MAC_ADDRS);
+ if (!attr)
+ goto nla_put_failure;
+
+ for (i = 0; i < rdev->wiphy.n_addresses; i++)
+ if (nla_put(msg, i + 1, ETH_ALEN,
+ rdev->wiphy.addresses[i].addr))
+ goto nla_put_failure;
+
+ nla_nest_end(msg, attr);
+ }
+
state->split_start++;
break;
case 10:
--
2.21.0
[PATCH v4 2/3] nl80211: Limit certain commands to interface owner
If the wdev object has been created (via NEW_INTERFACE) with
SOCKET_OWNER attribute set, then limit certain commands only to the
process that created that wdev.
This can be used to make sure no other process on the system interferes
by sending unwanted scans, action frames or any other funny business.
This patch introduces a new internal flag, and checks that flag in the
pre_doit hook.
Signed-off-by: Denis Kenzior
---
net/wireless/nl80211.c | 78 --
1 file changed, 60 insertions(+), 18 deletions(-)
Changes in v4:
- Minor restructuring suggested by Arend
Changes in v3:
- Fix minor locking mistake reported by kernel test robot
Changes in v2:
- None
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index fc83dd179c1a..a075d86a52f6 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -13602,6 +13602,7 @@ static int nl80211_probe_mesh_link(struct sk_buff *skb,
struct genl_info *info)
#define NL80211_FLAG_NEED_WDEV_UP (NL80211_FLAG_NEED_WDEV |\
NL80211_FLAG_CHECK_NETDEV_UP)
#define NL80211_FLAG_CLEAR_SKB 0x20
+#define NL80211_FLAG_OWNER_ONLY0x40
static int nl80211_pre_doit(const struct genl_ops *ops, struct sk_buff *skb,
struct genl_info *info)
@@ -13610,6 +13611,7 @@ static int nl80211_pre_doit(const struct genl_ops *ops,
struct sk_buff *skb,
struct wireless_dev *wdev;
struct net_device *dev;
bool rtnl = ops->internal_flags & NL80211_FLAG_NEED_RTNL;
+ int ret;
if (rtnl)
rtnl_lock();
@@ -13617,10 +13619,10 @@ static int nl80211_pre_doit(const struct genl_ops
*ops, struct sk_buff *skb,
if (ops->internal_flags & NL80211_FLAG_NEED_WIPHY) {
rdev = cfg80211_get_dev_from_info(genl_info_net(info), info);
if (IS_ERR(rdev)) {
- if (rtnl)
- rtnl_unlock();
- return PTR_ERR(rdev);
+ ret = PTR_ERR(rdev);
+ goto fail;
}
+
info->user_ptr[0] = rdev;
} else if (ops->internal_flags & NL80211_FLAG_NEED_NETDEV ||
ops->internal_flags & NL80211_FLAG_NEED_WDEV) {
@@ -13629,32 +13631,33 @@ static int nl80211_pre_doit(const struct genl_ops
*ops, struct sk_buff *skb,
wdev = __cfg80211_wdev_from_attrs(genl_info_net(info),
info->attrs);
if (IS_ERR(wdev)) {
- if (rtnl)
- rtnl_unlock();
- return PTR_ERR(wdev);
+ ret = PTR_ERR(wdev);
+ goto fail;
}
dev = wdev->netdev;
rdev = wiphy_to_rdev(wdev->wiphy);
+ ret = -EINVAL;
if (ops->internal_flags & NL80211_FLAG_NEED_NETDEV) {
- if (!dev) {
- if (rtnl)
- rtnl_unlock();
- return -EINVAL;
- }
+ if (!dev)
+ goto fail;
info->user_ptr[1] = dev;
} else {
info->user_ptr[1] = wdev;
}
+ ret = -ENETDOWN;
if (ops->internal_flags & NL80211_FLAG_CHECK_NETDEV_UP &&
- !wdev_running(wdev)) {
- if (rtnl)
- rtnl_unlock();
- return -ENETDOWN;
- }
+ !wdev_running(wdev))
+ goto fail;
+
+ ret = -EPERM;
+ if (ops->internal_flags & NL80211_FLAG_OWNER_ONLY &&
+ wdev->owner_nlportid &&
+ wdev->owner_nlportid != info->snd_portid)
+ goto fail;
if (dev)
dev_hold(dev);
@@ -13663,6 +13666,12 @@ static int nl80211_pre_doit(const struct genl_ops
*ops, struct sk_buff *skb,
}
return 0;
+
+fail:
+ if (rtnl)
+ rtnl_unlock();
+
+ return ret;
}
static void nl80211_post_doit(const struct genl_ops *ops, struct sk_buff *skb,
@@ -13727,7 +13736,8 @@ static const struct genl_ops nl80211_ops[] = {
.doit = nl80211_set_interface,
.flags = GENL_UNS_ADMIN_PERM,
.internal_flags = NL80211_FLAG_NEED_NETDEV |
- NL80211_FLAG_NEED_RTNL,
+ NL80211_FLAG_NEED_RTNL |
+ NL80211_FLAG_OWNER_ONLY,
},
{
.cmd = NL80211_CMD_NEW_INT
[PATCH v4 1/3] nl80211: Update uapi for CMD_FRAME_WAIT_CANCEL
Commit 1c38c7f22068 ("nl80211: send event when CMD_FRAME duration
expires") added the possibility of NL80211_CMD_FRAME_WAIT_CANCEL
being sent whenever the off-channel wait time associated with a
CMD_FRAME completes. Document this in the uapi/linux/nl80211.h file.
Signed-off-by: Denis Kenzior
---
include/uapi/linux/nl80211.h | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
Changes in v4:
- None
Changes in v3:
- None
Changes in v2:
- update commit formatting
diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index 8fc3a43cac75..0d9aad98c983 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -657,7 +657,9 @@
* is used during CSA period.
* @NL80211_CMD_FRAME_WAIT_CANCEL: When an off-channel TX was requested, this
* command may be used with the corresponding cookie to cancel the wait
- * time if it is known that it is no longer necessary.
+ * time if it is known that it is no longer necessary. This command is
+ * also sent as an event whenever the driver has completed the off-channel
+ * wait time.
* @NL80211_CMD_ACTION: Alias for @NL80211_CMD_FRAME for backward
compatibility.
* @NL80211_CMD_FRAME_TX_STATUS: Report TX status of a management frame
* transmitted with %NL80211_CMD_FRAME. %NL80211_ATTR_COOKIE identifies
--
2.21.0
Re: [PATCH] cfg80211: use parallel_ops for genl
Hi Johannes,
On 7/26/19 2:16 PM, Johannes Berg wrote:
From: Johannes Berg
Over time, we really need to get rid of all of our global locking.
One of the things needed is to use parallel_ops. This isn't really
the most important (RTNL is much more important) but OTOH we just
keep adding uses of genl_family_attrbuf() now. Use .parallel_ops to
disallow this.
Signed-off-by: Johannes Berg
---
net/wireless/nl80211.c | 112 +
1 file changed, 81 insertions(+), 31 deletions(-)
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 10b57aa10227..59aefcd7ccb6 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -749,19 +749,29 @@ int nl80211_prepare_wdev_dump(struct netlink_callback *cb,
int err;
if (!cb->args[0]) {
+ struct nlattr **attrbuf;
+
+ attrbuf = kcalloc(NUM_NL80211_ATTR, sizeof(*attrbuf),
+ GFP_KERNEL);
+ if (!attrbuf)
+ return -ENOMEM;
+
err = nlmsg_parse_deprecated(cb->nlh,
GENL_HDRLEN + nl80211_fam.hdrsize,
-genl_family_attrbuf(&nl80211_fam),
-nl80211_fam.maxattr,
+attrbuf, nl80211_fam.maxattr,
nl80211_policy, NULL);
- if (err)
+ if (err) {
+ kfree(attrbuf);
return err;
+ }
- *wdev = __cfg80211_wdev_from_attrs(
- sock_net(cb->skb->sk),
- genl_family_attrbuf(&nl80211_fam));
- if (IS_ERR(*wdev))
+ *wdev = __cfg80211_wdev_from_attrs(sock_net(cb->skb->sk),
+ attrbuf);
+ kfree(attrbuf);
+ if (IS_ERR(*wdev)) {
+ kfree(attrbuf);
Hmm, you just freed attrbuf above?
return PTR_ERR(*wdev);
+ }
*rdev = wiphy_to_rdev((*wdev)->wiphy);
/* 0 is the first index - add 1 to parse only once */
cb->args[0] = (*rdev)->wiphy_idx + 1;
@@ -12846,24 +12880,32 @@ static int nl80211_prepare_vendor_dump(struct sk_buff
*skb,
return 0;
}
+ attrbuf = kcalloc(NUM_NL80211_ATTR, sizeof(*attrbuf), GFP_KERNEL);
+ if (!attrbuf)
+ return -ENOMEM;
+
err = nlmsg_parse_deprecated(cb->nlh,
GENL_HDRLEN + nl80211_fam.hdrsize,
attrbuf, nl80211_fam.maxattr,
nl80211_policy, NULL);
if (err)
- return err;
+ goto out;
if (!attrbuf[NL80211_ATTR_VENDOR_ID] ||
- !attrbuf[NL80211_ATTR_VENDOR_SUBCMD])
- return -EINVAL;
+ !attrbuf[NL80211_ATTR_VENDOR_SUBCMD]) {
+ err = -EINVAL;
+ goto out;
+ }
Might be nicer to just set err = -EINVAL before the if instead of using
{} here
*wdev = __cfg80211_wdev_from_attrs(sock_net(skb->sk), attrbuf);
if (IS_ERR(*wdev))
*wdev = NULL;
*rdev = __cfg80211_rdev_from_attrs(sock_net(skb->sk), attrbuf);
- if (IS_ERR(*rdev))
- return PTR_ERR(*rdev);
+ if (IS_ERR(*rdev)) {
+ err = PTR_ERR(*rdev);
+ goto out;
+ }
vid = nla_get_u32(attrbuf[NL80211_ATTR_VENDOR_ID]);
subcmd = nla_get_u32(attrbuf[NL80211_ATTR_VENDOR_SUBCMD]);
@@ -12876,15 +12918,19 @@ static int nl80211_prepare_vendor_dump(struct sk_buff
*skb,
if (vcmd->info.vendor_id != vid || vcmd->info.subcmd != subcmd)
continue;
- if (!vcmd->dumpit)
- return -EOPNOTSUPP;
+ if (!vcmd->dumpit) {
+ err = -EOPNOTSUPP;
+ goto out;
+ }
Same thing here, setting err = -EOPNOTSUPP before the for...
vcmd_idx = i;
break;
}
- if (vcmd_idx < 0)
- return -EOPNOTSUPP;
+ if (vcmd_idx < 0) {
+ err = -EOPNOTSUPP;
+ goto out;
+ }
if (attrbuf[NL80211_ATTR_VENDOR_DATA]) {
data = nla_data(attrbuf[NL80211_ATTR_VENDOR_DATA]);
Otherwise LGTM.
Feel free to add: Reviewed-by: Denis Kenzior
Regards,
-Denis
Re: [PATCH v2] cfg80211: use parallel_ops for genl
On 7/29/19 9:31 AM, Johannes Berg wrote: From: Johannes Berg Over time, we really need to get rid of all of our global locking. One of the things needed is to use parallel_ops. This isn't really the most important (RTNL is much more important) but OTOH we just keep adding uses of genl_family_attrbuf() now. Use .parallel_ops to disallow this. Signed-off-by: Johannes Berg Link: https://lore.kernel.org/r/20190726191621.5031-1-johan...@sipsolutions.net Signed-off-by: Johannes Berg --- net/wireless/nl80211.c | 108 + 1 file changed, 78 insertions(+), 30 deletions(-) Reviewed-By: Denis Kenzior Regards, -Denis
Re: [PATCH v4 2/3] nl80211: Limit certain commands to interface owner
Hi Johannes, On 7/22/19 6:33 AM, Denis Kenzior wrote: If the wdev object has been created (via NEW_INTERFACE) with SOCKET_OWNER attribute set, then limit certain commands only to the process that created that wdev. This can be used to make sure no other process on the system interferes by sending unwanted scans, action frames or any other funny business. This patch introduces a new internal flag, and checks that flag in the pre_doit hook. Signed-off-by: Denis Kenzior --- net/wireless/nl80211.c | 78 -- 1 file changed, 60 insertions(+), 18 deletions(-) Changes in v4: - Minor restructuring suggested by Arend Changes in v3: - Fix minor locking mistake reported by kernel test robot Changes in v2: - None I noticed that the other patches in this series got applied. Was this one left out on purpose? Regards, -Denis
Re: [PATCH v3 2/3] nl80211: Limit certain commands to interface owner
Hi Johannnes, On 7/31/19 4:51 AM, Johannes Berg wrote: On Mon, 2019-07-01 at 10:33 -0500, Denis Kenzior wrote: If the wdev object has been created (via NEW_INTERFACE) with SOCKET_OWNER attribute set, then limit certain commands only to the process that created that wdev. This can be used to make sure no other process on the system interferes by sending unwanted scans, action frames or any other funny business. This patch introduces a new internal flag, and checks that flag in the pre_doit hook. So, looking at this ... I can't say I'm convinced. You're tagging 35 out of about 106 commands, and even if a handful of those are new and were added after your patch, this doesn't really make sense. NL80211_CMD_LEAVE_IBSS is tagged, but not NL80211_CMD_LEAVE_MESH? NL80211_CMD_NEW_STATION is tagged, but not NL80211_CMD_NEW_MPATH? NL80211_CMD_SET_KEY is tagged, but not NL80211_CMD_SET_PMK or NL80211_CMD_SET_PMKSA? NL80211_CMD_UPDATE_CONNECT_PARAMS is tagged, but not NL80211_CMD_UPDATE_OWE_INFO (though this could be patch crossing?) NL80211_CMD_CONTROL_PORT_FRAME isn't tagged? So for some of these I was planning to submit a patch to check that the request comes from the SOCKET_OWNER for the connection itself. E.g. it makes no sense to accept CONTROL_PORT_FRAME from a process that didn't issue the CMD_CONNECT. Same applies for some of the offload commands. But I can include these in this list as well if you prefer. For others, it was pure ignorance as to what the commands do. E.g. we haven't looked into Mesh at all. So if you want to suggest which commands should be included, I'm happy to add these. NL80211_CMD_SET_QOS_MAP isn't tagged? It almost feels like you just did a "git grep NL80211_CMD_" on your code, and then dropped the flag on everything you were using. And honestly, I think you need a better justification than just "unwanted scans, action frames or any other funny business". We have a limited resource that we are managing in userspace. We can't just have any random process coming in and messing with that resource. So either the userspace daemon should do it or the kernel. Right now it is just pure chaos... And really, in the end, how is this different from SOCKET_OWNER for CMD_CONNECT? It is optional in the end, so if you don't want to use it, don't? Also, how's this not just a workaround for some very specific setup issue you were seeing, where people trying out iwd didn't remove wpa_s properly (*)? I'm really not convinced that this buys us anything except in very limited development scenarios - and those are typically the exact scenarios where you _want_ to be able to do things like that (and honestly, I'd be pretty pissed off if I couldn't do an "iw wlan0 scan" just because some tool decided it wanted to have control over things). I understand where you're coming from, but you're just one user who can disable this behavior anyway if you really cared. For 99.9% of the users this is never going to be a problem. And I'd rather cater to the 99%... (*) also, that would just happen to work for you now with iwd winning because you claim ownership and wpa_s doesn't, you'd still get the same complaints "iwd doesn't work" if/when wpa_s *does* start to claim ownership and you get locked out with a patch like this, so I don't feel you'd actually win much even in this case. This is in no way the motivation for this. wpa_s or iwd winning is a distro/user configuration problem. I don't care about that now. Besides, this was mostly taken care of by the SOCKET_OWNER set on CMD_CONNECT... I'm trying to come up with places where we do something similar, defend one application running as root against another ... but can't really? Think about VPN - we don't stop anying from removing or adding IP addresses that the VPN application didn't intend to use, yet that can obviously break your connection. You could even run dhcp on it, even if for (most) VPN protocols that's rather useless. File locking would be one example. Systemd can and does all kinds of fun stuff (e.g. locking a process out from twiddling rfkill). LSM modules can do just about everything. I think there are plenty of examples. But really, a lot of this works just because various processes play 'nice' and stay out of each other's way. Also, as you point out, most things aren't done because they don't make sense. But with nl80211 this isn't the case. Many processes would be tempted to start an operation or get some info out of nl80211 directly. So this patch tries to narrow down what they can use, e.g. informational-only commands are fine. Anything that can affect state is not fine. Overall, I'm not really convinced. The design is rather uncle
[RFCv1 1/2] nl80211: Support >4096 byte NEW_WIPHY event nlmsg
For historical reasons, NEW_WIPHY messages generated by dumps or
GET_WIPHY commands were limited to 4096 bytes due to userspace tools
using limited buffers. Once the sizes NEW_WIPHY messages exceeded these
sizes, split dumps were introduced. All any non-legacy data was added
only to messages using split-dumps (including filtered dumps).
When unsolicited NEW_WIPHY events were introduced they inherited the
4096 byte limitation. These messages thus do not contain any non-legacy
wiphy dump data. This means that userspace still needs to re-dump the
information from the kernel after receiving such NEW_WIPHY event since
some of the information is missing. Thus it is desirable to relax such
restrictions for these messages and include the non-legacy data in these
events.
It should be safe to assume that any users of these new unsolicited
NEW_WIPHY events are non-legacy clients, which can use a
larger receive buffer for netlink messages. Since older, legacy clients
did not utilize NEW_WIPHY events (they did not exist), it is assumed
that even if the client receives such a message (even if truncated), no
harm would result and backwards-compatibility would be kept.
---
net/wireless/nl80211.c | 49 ++
1 file changed, 40 insertions(+), 9 deletions(-)
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 1a107f29016b..6774072e836f 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -1839,6 +1839,7 @@ struct nl80211_dump_wiphy_state {
long start;
long split_start, band_start, chan_start, capa_start;
bool split;
+ bool large_message;
};
static int nl80211_send_wiphy(struct cfg80211_registered_device *rdev,
@@ -2168,12 +2169,23 @@ static int nl80211_send_wiphy(struct
cfg80211_registered_device *rdev,
* helps ensure that newly added capabilities don't break
* older tools by overrunning their buffers.
*
+* For unsolicited NEW_WIPHY notifications, it is assumed
+* that the client can handle larger messages. Unsolicited
+* NEW_WIPHY notifications were added relatively recently
+* and it is not expected that older tools with limited
+* buffers would utilize these messages anyway. E.g. even
+* if the message is truncated, it would not have been
+* used regardless.
+*
* We still increment split_start so that in the split
* case we'll continue with more data in the next round,
-* but break unconditionally so unsplit data stops here.
+* but break unless large_messages are requested, so
+* legacy unsplit data stops here.
*/
state->split_start++;
- break;
+ if (state->split || !state->large_message)
+ break;
+ /* Fall through */
case 9:
if (rdev->wiphy.extended_capabilities &&
(nla_put(msg, NL80211_ATTR_EXT_CAPA,
@@ -2215,7 +2227,9 @@ static int nl80211_send_wiphy(struct
cfg80211_registered_device *rdev,
}
state->split_start++;
- break;
+ if (state->split)
+ break;
+ /* Fall through */
case 10:
if (nl80211_send_coalesce(msg, rdev))
goto nla_put_failure;
@@ -2231,7 +2245,9 @@ static int nl80211_send_wiphy(struct
cfg80211_registered_device *rdev,
goto nla_put_failure;
state->split_start++;
- break;
+ if (state->split)
+ break;
+ /* Fall through */
case 11:
if (rdev->wiphy.n_vendor_commands) {
const struct nl80211_vendor_cmd_info *info;
@@ -2267,7 +2283,9 @@ static int nl80211_send_wiphy(struct
cfg80211_registered_device *rdev,
nla_nest_end(msg, nested);
}
state->split_start++;
- break;
+ if (state->split)
+ break;
+ /* Fall through */
case 12:
if (rdev->wiphy.flags & WIPHY_FLAG_HAS_CHANNEL_SWITCH &&
nla_put_u8(msg, NL80211_ATTR_MAX_CSA_COUNTERS,
@@ -2309,7 +2327,9 @@ static int nl80211_send_wiphy(struct
cfg80211_registered_device *rdev,
}
state->split_start++;
- break;
+ if (state->split)
+ break;
+ /* Fall through */
case 13:
if (rdev->wiphy.num_iftype_ext_capab &&
rdev->wiphy.iftype_ext_capab) {
@@ -2377,13 +2397,17 @@ static int nl80211_send_wiphy(struct
cfg80211_registered_device *rdev,
}
sta
[RFCv1 2/2] nl80211: Don't split-dump for clients with large buffers
---
net/wireless/nl80211.c | 17 +
1 file changed, 17 insertions(+)
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 6774072e836f..e1707cfd9076 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -2496,6 +2496,22 @@ static int nl80211_dump_wiphy(struct sk_buff *skb,
struct netlink_callback *cb)
rtnl_unlock();
return ret;
}
+
+ /*
+* auto-detect support for large buffer sizes: af_netlink
+* will allocate skbufs larger than 4096 in cases where
+* it detects that the client receive buffer (given to
+* recvmsg) is bigger. In such cases we can assume that
+* performing split dumps is wasteful since the client
+* can likely safely consume the entire un-split wiphy
+* message in one go without the extra message header
+* overhead.
+*/
+ if (skb_tailroom(skb) > 4096) {
+ state->large_message = true;
+ state->split = false;
+ }
+
cb->args[0] = (long)state;
}
@@ -2529,6 +2545,7 @@ static int nl80211_dump_wiphy(struct sk_buff *skb, struct
netlink_callback *cb)
* We can then retry with the larger buffer.
*/
if ((ret == -ENOBUFS || ret == -EMSGSIZE) &&
+ !state->large_message &&
!skb->len && !state->split &&
cb->min_dump_alloc < 4096) {
cb->min_dump_alloc = 4096;
--
2.21.0
Re: [RFCv1 2/2] nl80211: Don't split-dump for clients with large buffers
Hi Johannes,
On 8/1/19 4:13 AM, Johannes Berg wrote:
On Thu, 2019-08-01 at 02:14 -0500, Denis Kenzior wrote:
+ /*
+* auto-detect support for large buffer sizes: af_netlink
+* will allocate skbufs larger than 4096 in cases where
+* it detects that the client receive buffer (given to
+* recvmsg) is bigger. In such cases we can assume that
+* performing split dumps is wasteful since the client
+* can likely safely consume the entire un-split wiphy
+* message in one go without the extra message header
+* overhead.
+*/
+ if (skb_tailroom(skb) > 4096) {
+ state->large_message = true;
+ state->split = false;
+ }
Hmm. That's kinda a neat idea, but I don't think it's a good idea. Have
you checked how long the message is now?
I only have hwsim and a ath9k to test with. The hwsim comes out to be
4448 bytes with an updated version of my patch. ath9k is smaller. V1
did not address some extra gotcha code which didn't include some
attributes in the unsplit case. As far as I can tell all the attributes
are now identical with v2.
Note that the overhead for split dumps is actually quite big. The same
info using split-dumps is 7724 bytes. So there would definitely be an
advantage in not using such fragmentation if not needed...
Since we *did* in fact hit the previous limit, and have added a *lot* of
things since then (this was years ago, after all), I wouldn't be
surprised if we're reasonably close to the new limit you propose even
now already.
It seems not?
Also, keep in mind that there are some devices that just have an
*enormous* amount of channels, and that's only going to increase (right
now with 6/7 GHz, etc.)
The 2.4 & 5 Ghz bands account for about 2k. So even if we add another
band, we're likely still within an 8k buffer. And really the kernel
recommends a 16k buffer to be used as a minimum...
Also, the way nl80211 encodes channel information is really quite
wasteful. Not sure if anything can be done about it now, but the flags
really, really, really add up. So there is significant savings to be
had here...
So in general, given all the variable things we have here, all this
buffer size estimation doesn't seem very robust to me. You could have
any number of variable things in a message:
* channel list - which we alleviated somewhat by having a separate
channel dump, so not all data is included here (which I guess you'll
complain about next :P)
Not sure I follow? I don't see a separate channel dump? Can you point
me in the right direction?
* nl80211_send_mgmt_stypes() things are also a bit variable, and we
keep adding interface types etc., and some devices may support lots
of frames (there's an upper bound, but it's not that small)
* interface combinations - only getting more complex with more complex
devices and more concurrency use cases
* vendor commands have no real limit
* I'm sure measurement use cases will only increases
* and generally of course we keep adding to everything
Also, I don't really buy the *need* for this since you're just removing
a few kernel/user roundtrips here when new devices are discovered, a
rare event. The parsing isn't really any more complicated for the
userspace side.
roundtrips to the kernel introduce races. The less potential for a
race, the less code we have to write and the less buggy it is. Pretty
simple...
Regarding the other patch, I think most of the above also applies there.
I can sort of see how you think it's *nice* to have all the data right
there, but I really don't see why you're so hung up about having to
request the full information ... And I really don't want to see this hit
the wall again in the future, in some weird scenarios with devices that
have lots of .
See above...
It should be safe to assume that any users of these new unsolicited
NEW_WIPHY events are non-legacy clients, which can use a
larger receive buffer for netlink messages. Since older, legacy clients
did not utilize NEW_WIPHY events (they did not exist), it is assumed
that even if the client receives such a message (even if truncated), no
harm would result and backwards-compatibility would be kept.
Interesting idea, but no, in general you cannot assume that. Older
clients might have added support for NEW_WIPHY without fixing the split
dumps first ...
The two commits are over a year apart, but okay, fair enough. Then
again, you sort of hinted that nobody used this anyhow.
But regardless, if this mythical legacy/broken client is truly a
concern, we can introduce a NEW_WIPHY_BIG or something.
Also, you mention in the code that messages are tr
BUG: atk9k/mac80211 CONTROL_PORT_FRAME processing + MFP
Hi, We're getting reports of weird behavior on ath9k/mac80211 devices when CONTROL_PORT_FRAME support is used. iwlwifi, works fine under the same circumstances. If CONTROL_PORT_FRAME is disabled and the old legacy PAE transport is used, everything works fine. The short version: - We Connect with MFP enabled - Handshake packet 1 & 2 is exchanged - Handshake packet 3 is received & reply sent - Keys are set - AP never receives our packet 4, or the card drops it locally - Subsequent retransmissions are sent un-encrypted by the AP and are probably sent encrypted (via MFP) by mac80211. Since the driver never sets NO_ENCRYPT flag, userspace has no knowledge to try and send the reply un-encrypted. Here's a log (some parts are removed for brevity, but if someone wants the full log, I can provide this as well): < Request: Connect (0x2e) len 160 [ack] 1565892849.337243 Interface Index: 13 (0x000d) Wiphy Frequency: 5805 (0x16ad) MAC Address 10:C3:7B:54:74:D4 SSID: len 9 Auth Type: 0 (0x) Privacy: true Interface Socket Owner: true Cipher Suites Pairwise: CCMP (00:0f:ac) suite 04 Cipher Suite Group: CCMP (00:0f:ac) suite 04 Use MFP: 1 (0x0001) AKM Suites: PSK; RSNA PSK (00:0f:ac) suite 02 WPA Versions: 2 (0x0002) Control Port: true Control Port over NL80211: true Use RRM: true Information Elements: len 41 RSN: Group Data Cipher Suite: len 4 CCMP (00:0f:ac) suite 04 Pairwise Cipher Suite: len 4 CCMP (00:0f:ac) suite 04 AKM Suite: len 4 PSK; RSNA PSK (00:0f:ac) suite 02 RSN capabilities: bits 2 - 3: 1 replay counter per PTKSA RSN capabilities: bits 4 - 5: 1 replay counter per GTKSA RSN capabilities: bit 7: Management Frame Protection Capable 01 00 00 0f ac 04 01 00 00 0f ac 04 01 00 00 0f ac 02 80 00 RM Enabled Capabilities: len 5 Operating Channel Max Measurement Duration: 0 Non-Operating Channel Max Measurement Duration: 0 Measurement Pilot Capability: 0 00 00 00 00 00 . Extended Capabilities: len 10 Capability: bit 62: Opmode Notification 00 00 00 00 00 00 00 40 00 00...@.. > Event: New Station (0x13) len 32 1565892851.673886 Interface Index: 13 (0x000d) MAC Address 10:C3:7B:54:74:D4 Generation: 5 (0x0005) Station Info: len 0 > Response: Connect (0x2e) len 4 [0x100] 1565892851.673971 Status: Success (0) > Event: Authenticate (0x25) len 64 1565892851.676737 Wiphy: 1 (0x0001) Interface Index: 13 (0x000d) Frame: len 41 Here comes the Handshake packet 1 from the AP. Why the hell is it here prior to Authenticate ? But whatever ;) > Event: Control Port Frame (0x81) len 176 1565892851.686055 Wiphy: 1 (0x0001) Interface Index: 13 (0x000d) Wireless Device: 4294967299 (0x00010003) MAC Address 10:C3:7B:54:74:D4 Control Port Ethertype: 34958 (0x888e) Frame: len 121 Protocol Version: 2 (802.1X-2004) Type: 3 (Key) Length: 117 Descriptor Type: 2 Key MIC: false Secure: false Error: false Request: false Encrypted Key Data: false SMK Message: false Key Descriptor Version: 2 (02) Key Type: true Install: false Key ACK: true Key Length: 16 Key Replay Counter: 0 Key NONCE e4 5b 1d 98 fd db 54 ae 54 98 fc 39 69 f5 bc 0b .[T.T..9i... f6 a3 aa c5 50 43 9a 6d 27 e5 a4 bb e9 c5 6d de PC.m'.m. Key IV 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 Key RSC 00 00 00 00 00 00 00 00 Key MIC Data 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 Key Data: len 22 Vendor specific: len 20 IEEE 802.11 (00:0f:ac) type: 04 PMKID KDE 00 0f ac 04 d9 81 f4 29 57 31 7e ad 33 57 b8 af ...)W1~.3W.. c7 a7 40 8f ..@. 02 03 00 75 02 00 8a 00 10 00 00 00 00 00 00 00 ...u 00 e4 5b 1d 98 fd db 54 ae 54 98 fc 39 69 f5 bc ..[T.T..9i.. 0b f6 a3 aa c5 50 43 9a 6d 27 e5 a4 bb e9 c5 6d .PC.m'.m de 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 16 dd 14 00 0f ac 04 d9 81 f4 29 57 31 7e )W1~ ad
[RFCv2 2/4] nl80211: Support >4096 byte NEW_WIPHY event nlmsg
For historical reasons, NEW_WIPHY messages generated by dumps or
GET_WIPHY commands were limited to 4096 bytes due to userspace tools
using limited buffers. Once the sizes NEW_WIPHY messages exceeded these
sizes, split dumps were introduced. All any non-legacy data was added
only to messages using split-dumps (including filtered dumps).
However, split-dumping has quite a significant overhead. On cards
tested, split dumps generated message sizes 1.7-1.8x compared to
non-split dumps, while still comfortably fitting into an 8k buffer. The
kernel now expects userspace to provide 16k buffers by default, and 32k
buffers are possible.
Introduce a concept of a large message, so that if the kernel detects
that userspace has provided a buffer of sufficient size, a non-split
message could be generated.
Signed-off-by: Denis Kenzior
---
net/wireless/nl80211.c | 51 ++
1 file changed, 37 insertions(+), 14 deletions(-)
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index b9b0199b5ec6..682a095415eb 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -1839,6 +1839,7 @@ struct nl80211_dump_wiphy_state {
long start;
long split_start, band_start, chan_start, capa_start;
bool split;
+ bool large_message;
};
static int nl80211_send_wiphy(struct cfg80211_registered_device *rdev,
@@ -2027,7 +2028,8 @@ static int nl80211_send_wiphy(struct
cfg80211_registered_device *rdev,
if (nl80211_msg_put_channel(
msg, &rdev->wiphy, chan,
- state->split))
+ state->split ||
+ state->large_message))
goto nla_put_failure;
nla_nest_end(msg, nl_freq);
@@ -2072,7 +2074,7 @@ static int nl80211_send_wiphy(struct
cfg80211_registered_device *rdev,
i = nl80211_add_commands_unsplit(rdev, msg);
if (i < 0)
goto nla_put_failure;
- if (state->split) {
+ if (state->split || state->large_message) {
CMD(crit_proto_start, CRIT_PROTOCOL_START);
CMD(crit_proto_stop, CRIT_PROTOCOL_STOP);
if (rdev->wiphy.flags & WIPHY_FLAG_HAS_CHANNEL_SWITCH)
@@ -2111,7 +2113,8 @@ static int nl80211_send_wiphy(struct
cfg80211_registered_device *rdev,
/* fall through */
case 6:
#ifdef CONFIG_PM
- if (nl80211_send_wowlan(msg, rdev, state->split))
+ if (nl80211_send_wowlan(msg, rdev,
+ state->split || state->large_message))
goto nla_put_failure;
state->split_start++;
if (state->split)
@@ -2126,7 +2129,8 @@ static int nl80211_send_wiphy(struct
cfg80211_registered_device *rdev,
goto nla_put_failure;
if (nl80211_put_iface_combinations(&rdev->wiphy, msg,
- state->split))
+ state->split ||
+ state->large_message))
goto nla_put_failure;
state->split_start++;
@@ -2145,7 +2149,7 @@ static int nl80211_send_wiphy(struct
cfg80211_registered_device *rdev,
* dump is split, otherwise it makes it too big. Therefore
* only advertise it in that case.
*/
- if (state->split)
+ if (state->split || state->large_message)
features |= NL80211_FEATURE_ADVERTISE_CHAN_LIMITS;
if (nla_put_u32(msg, NL80211_ATTR_FEATURE_FLAGS, features))
goto nla_put_failure;
@@ -2170,13 +2174,20 @@ static int nl80211_send_wiphy(struct
cfg80211_registered_device *rdev,
*
* We still increment split_start so that in the split
* case we'll continue with more data in the next round,
-* but break unconditionally so unsplit data stops here.
+* but break unless large_messages are requested, so
+* legacy unsplit data stops here.
*/
state->split_start++;
- if (!state->split)
+ if (state->split)
+ break;
+
+ if (!state->large_message) {
state->split_start = 0;
- break;
+ break;
+ }
+
+ /* Fall through */
case 9:
[RFCv2 1/4] nl80211: Fix broken non-split wiphy dumps
If a (legacy) client requested a wiphy dump but did not provide the
NL80211_ATTR_SPLIT_WIPHY_DUMP attribute, the dump was supposed to be
composed of purely non-split NEW_WIPHY messages, with 1 wiphy per
message. At least this was the intent after commit:
3713b4e364ef ("nl80211: allow splitting wiphy information in dumps")
However, in reality the non-split dumps were broken very shortly after.
Perhaps around commit:
fe1abafd942f ("nl80211: re-add channel width and extended capa advertising")
The reason for the bug is a missing setting of split_start to 0 in the
case of a non-split dump.
Here is a sample non-split dump performed on kernel 4.19, some parts
were cut for brevity:
< Request: Get Wiphy (0x01) len 0 [ack,0x300]
> Result: New Wiphy (0x03) len 3496 [multi]
Wiphy: 0 (0x)
Wiphy Name: phy0
Generation: 1 (0x0001)
> Result: New Wiphy (0x03) len 68 [multi]
Wiphy: 0 (0x)
Wiphy Name: phy0
Generation: 1 (0x0001)
Extended Capabilities: len 8
Capability: bit 2: Extended channel switching
Capability: bit 62: Opmode Notification
Extended Capabilities Mask: len 8
04 00 00 00 00 00 00 40 ...@
VHT Capability Mask: len 12
f0 1f 80 33 ff ff 00 00 ff ff 00 00 ...3
> Result: New Wiphy (0x03) len 28 [multi]
Wiphy: 0 (0x)
Wiphy Name: phy0
Generation: 1 (0x0001)
> Result: New Wiphy (0x03) len 28 [multi]
Wiphy: 0 (0x)
Wiphy Name: phy0
Generation: 1 (0x0001)
> Result: New Wiphy (0x03) len 52 [multi]
Wiphy: 0 (0x)
Wiphy Name: phy0
Generation: 1 (0x0001)
Max CSA Counters: len 1
02 .
Scheduled Scan Maximum Requests: len 4
01 00 00 00
Extended Features: len 4
02 02 00 04
> Result: New Wiphy (0x03) len 36 [multi]
Wiphy: 0 (0x)
Wiphy Name: phy0
Generation: 1 (0x0001)
Reserved: len 4
00 00 00 00
> Complete: Get Wiphy (0x01) len 4 [multi]
Status: 0
Signed-off-by: Denis Kenzior
---
net/wireless/nl80211.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 1a107f29016b..b9b0199b5ec6 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -2173,6 +2173,9 @@ static int nl80211_send_wiphy(struct
cfg80211_registered_device *rdev,
* but break unconditionally so unsplit data stops here.
*/
state->split_start++;
+
+ if (!state->split)
+ state->split_start = 0;
break;
case 9:
if (rdev->wiphy.extended_capabilities &&
--
2.21.0
[RFCv2 3/4] nl80211: Don't split-dump for clients with large buffers
Signed-off-by: Denis Kenzior
---
net/wireless/nl80211.c | 17 +
1 file changed, 17 insertions(+)
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 682a095415eb..24b67de99f3a 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -2498,6 +2498,22 @@ static int nl80211_dump_wiphy(struct sk_buff *skb,
struct netlink_callback *cb)
rtnl_unlock();
return ret;
}
+
+ /*
+* auto-detect support for large buffer sizes: af_netlink
+* will allocate skbufs larger than 4096 in cases where
+* it detects that the client receive buffer (given to
+* recvmsg) is bigger. In such cases we can assume that
+* performing split dumps is wasteful since the client
+* can likely safely consume the entire un-split wiphy
+* message in one go without the extra message header
+* overhead.
+*/
+ if (skb_tailroom(skb) > 4096) {
+ state->large_message = true;
+ state->split = false;
+ }
+
cb->args[0] = (long)state;
}
@@ -2531,6 +2547,7 @@ static int nl80211_dump_wiphy(struct sk_buff *skb, struct
netlink_callback *cb)
* We can then retry with the larger buffer.
*/
if ((ret == -ENOBUFS || ret == -EMSGSIZE) &&
+ !state->large_message &&
!skb->len && !state->split &&
cb->min_dump_alloc < 4096) {
cb->min_dump_alloc = 4096;
--
2.21.0
[RFCv2 4/4] nl80211: Send large new_wiphy events
Send large NEW_WIPHY events on a new multicast group so that clients
that can accept larger messages do not need to round-trip to the kernel
and perform extra filtered wiphy dumps.
A new multicast group is introduced and the large message is sent before
the legacy message. This way clients that listen on both multicast
groups can ignore duplicate legacy messages if needed.
Signed-off-by: Denis Kenzior
---
include/uapi/linux/nl80211.h | 1 +
net/wireless/nl80211.c | 28 +++-
2 files changed, 28 insertions(+), 1 deletion(-)
diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index 822851d369ab..b9c1cf29cf09 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -50,6 +50,7 @@
#define NL80211_MULTICAST_GROUP_MLME "mlme"
#define NL80211_MULTICAST_GROUP_VENDOR "vendor"
#define NL80211_MULTICAST_GROUP_NAN"nan"
+#define NL80211_MULTICAST_GROUP_CONFIG2"config2"
#define NL80211_MULTICAST_GROUP_TESTMODE "testmode"
/**
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 24b67de99f3a..9ba9e1938d6b 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -46,6 +46,7 @@ enum nl80211_multicast_groups {
NL80211_MCGRP_MLME,
NL80211_MCGRP_VENDOR,
NL80211_MCGRP_NAN,
+ NL80211_MCGRP_CONFIG2,
NL80211_MCGRP_TESTMODE /* keep last - ifdef! */
};
@@ -56,6 +57,7 @@ static const struct genl_multicast_group nl80211_mcgrps[] = {
[NL80211_MCGRP_MLME] = { .name = NL80211_MULTICAST_GROUP_MLME },
[NL80211_MCGRP_VENDOR] = { .name = NL80211_MULTICAST_GROUP_VENDOR },
[NL80211_MCGRP_NAN] = { .name = NL80211_MULTICAST_GROUP_NAN },
+ [NL80211_MCGRP_CONFIG2] = { .name = NL80211_MULTICAST_GROUP_CONFIG2 },
#ifdef CONFIG_NL80211_TESTMODE
[NL80211_MCGRP_TESTMODE] = { .name = NL80211_MULTICAST_GROUP_TESTMODE }
#endif
@@ -14730,12 +14732,34 @@ void nl80211_notify_wiphy(struct
cfg80211_registered_device *rdev,
enum nl80211_commands cmd)
{
struct sk_buff *msg;
+ size_t alloc_size;
struct nl80211_dump_wiphy_state state = {};
WARN_ON(cmd != NL80211_CMD_NEW_WIPHY &&
cmd != NL80211_CMD_DEL_WIPHY);
- msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
+ if (cmd == NL80211_CMD_NEW_WIPHY) {
+ state.large_message = true;
+ alloc_size = 8192UL;
+ } else
+ alloc_size = NLMSG_DEFAULT_SIZE;
+
+ msg = nlmsg_new(alloc_size, GFP_KERNEL);
+ if (!msg)
+ goto legacy;
+
+ if (nl80211_send_wiphy(rdev, cmd, msg, 0, 0, 0, &state) < 0) {
+ nlmsg_free(msg);
+ goto legacy;
+ }
+
+ genlmsg_multicast_netns(&nl80211_fam, wiphy_net(&rdev->wiphy), msg, 0,
+ NL80211_MCGRP_CONFIG2, GFP_KERNEL);
+
+legacy:
+ state.large_message = false;
+ alloc_size = NLMSG_DEFAULT_SIZE;
+ msg = nlmsg_new(alloc_size, GFP_KERNEL);
if (!msg)
return;
@@ -14763,6 +14787,8 @@ void nl80211_notify_iface(struct
cfg80211_registered_device *rdev,
return;
}
+ genlmsg_multicast_netns(&nl80211_fam, wiphy_net(&rdev->wiphy), msg, 0,
+ NL80211_MCGRP_CONFIG2, GFP_KERNEL);
genlmsg_multicast_netns(&nl80211_fam, wiphy_net(&rdev->wiphy), msg, 0,
NL80211_MCGRP_CONFIG, GFP_KERNEL);
}
--
2.21.0
Re: [RFC 0/1] Allow MAC change on up interface
Hi Johannes, TBH, I'm not really sure I see any point in this to start with, many devices will give the address to the firmware when the interface is brought up (even iwlwifi does - I'm not sure we'd want to take your patch for iwlwifi even if that works today, nothing says the firmware might not change its mind on that), and so it's quite likely only going to be supported in few devices. Hmm... I sense a pattern of you not seeing a point in doing many things... Do you actually use the stuff you maintain? You've also not really explained what exactly is troubling you with changing the MAC address, you just mentioned some sort of "race condition"? Well, one possible use case might be, oh something like this: https://source.android.com/devices/tech/connect/wifi-mac-randomization Now, one thing I can imagine would be that you'd want to optimize * ifdown - remove iface from fw/hw - stop fw/hw * change MAC * ifup - start fw/hw - add iface to fw/hw to just * ifdown - remove iface from fw/hw * change MAC * ifup - add iface to fw/hw That would be a part of it... i.e. not restart the firmware (which takes time) for all this, but that seems much easier to solve by e.g. having a combined operation for all of this that gets handled in mac80211, or more generally by having a "please keep the firmware running" token that you can hold while you do the operation? And also maybe a bunch of other optimizations like not flushing scan results? Your changes are also a bit strange - you modified the "connect" path and iwlwifi, but the connect path is not usually (other than with iw or even iwconfig) taken for iwlwifi? And if you modify auth/assoc paths, you get into even weirder problems - what if you use different addresses for auth and assoc? What if the assoc (or even connect) really was a *re*assoc, and thus must have the same MAC address? To me, the whole thing seems like more of a problem than a solution. Okay, so there are some obstacles. But can you get over the whole "Don't hold it like this" part and actually offer up something constructive? Regards, -Denis
Re: [RFC 0/1] Allow MAC change on up interface
Hi Johannes, Stop. Your tone, and in particular the constant snide comments and attacks on me are, quite frankly, getting extremely tiring. Look, I'm sorry I hit a nerve, but from where I am sitting, it had to be said... Regardless. Peace, I'm not trying to start drama here. We just want to improve things and it feels like you're shutting down every idea without truly understanding the issues we're facing. It almost seems like you're just trying to bully me into taking your patches by constantly trying to make me feel that I cannot know better anyway. This is not how you should be treating anyone. Before lecturing me on my tone, can you go back and re-read your responses to many of the contributors here? I mean really read them? Your tone is quite dismissive, whether intentional or not. When one of my guys comes to me and says: "Johannes' response was completely useless, it feels like he didn't even read my cover letter" I will come out and call you on it. So if you don't mean to come off that way, great. We'll just chalk it up to a mis-understanding. Look, I did say I don't see a point in this, but you're taking that out of context. I also stated that I didn't understand the whole thing about "race conditions" and all, because nobody actually explained the reasoning behind the changes here. Fine. I get that. But how about asking what the use case is? Or say you don't quite understand why something is needed? We'll happily explain. When the first reaction to an RFC is: "I don't see the point" or "You're doing it wrong" from a maintainer who's job (by definition) is to encourage new contributors and improve the subsystem he maintains...? Well that's kind of a downer, don't you agree? You're the maintainer and you should be held to a higher standard... I maintain 3 projects, I know the job isn't great, but you still should be (more) civil to people... James, unlike you, managed to reply on point and explain why it was needed. If all you can do is accuse me of not using the software and therefore not knowing how it should be used, even implying that I'm not smart enough to understand the use cases, then I don't know why you bother replying at all. Good on James. I council all my guys to keep cool when dealing with upstream. But that doesn't mean you should be dismissing things out of hand on the very first interaction you have with a new contributor. I can understand your frustration to some extent, and I want to give you the benefit of doubt and want to believe this behaviour was borne out of it, since I've been reviewing your changes relatively critically. Good. I want you to do that. The changes are in very tricky areas and you know the code best. However, I also want to believe that I've been (trying to) keep the discussion on a technical level, telling you why I believe some things shouldn't be done the way you did them, rather than telling you that you're not smart enough to be working on this. If you feel otherwise, please do let me know and I'll go back to understand, in order to improve my behaviour in the future. If you interpreted my rants as an assault to your intelligence, then I'm sorry. They really were not meant this way. But sometimes we really had to wonder if you were using the same API we were? So the question I asked above was purely logical consequence of what I was seeing. You yourself admitted that you have never implemented an event driven stack. So how about listening to the guys that are? We are using your APIs in different ways. So instead of questioning why or attacking those ways, how about asking whether improvements can be made? We are facing serious pain points with the API. So instead of dismissing things out of hand, can we work together to improve things? We are trying to make things fast. The API is frequently not setup for that. The MAC randomization is just one example. Bringing down the interface (and shutting down the firmware, toggling power state, cleaning up resources/state) prior to every connection is just not acceptable. Look at the link I sent. The Android guys state 3 seconds is the typical 'hit'. This is literally wasting everyone's time. Please help keep the discussion technical, without demeaning undertones. Point taken. And I apologize again. But please consider what I said above. Regards, -Denis
Re: [RFC 0/1] Allow MAC change on up interface
Hi Dan, On 8/20/19 12:53 PM, Dan Williams wrote: On Tue, 2019-08-20 at 10:40 -0500, Denis Kenzior wrote: Hi Johannes, Stop. Your tone, and in particular the constant snide comments and attacks on me are, quite frankly, getting extremely tiring. Look, I'm sorry I hit a nerve, but from where I am sitting, it had to be said... But did it really? And in that way? There were certainly better ways to go about that response. The issue is that this isn't the first such occurrence. There is a pattern here and it needs to change. So +1 on handling this better. I don't recall seeing a NAK anywhere his email chain (which you'd get with some other kernel maintainers) but instead (a) an explanation of why the proposed solution had some problems, (b) some alternative possibilities and (c) requests for more information so the discussion could continue. So the cover letter states: "Set IFF_LIVE_ADDR_CHANGE on net_device. Note: I know setting this where I did is likely not the right way to do it, but for this proof-of-concept it works. With guidance I can move this around to a proper place." and I'll leave it up to you to read the first response from the maintainer. It does the requested changes no good to take that kind of tone. Let's Neither is: "don't do that then" or "I'm not really sure I see any point in this to start with" or "To me, the whole thing seems like more of a problem than a solution." move on from here and keep things constructive to solve the problem at hand, which is: "Changing the MAC address of a WiFi interface takes longer than I'd like and clears some state that I'd like it to keep." That is a technical problem we can solve, so let's keep it at that level. I'm all for moving on and having the people that know this stuff well giving actual guidance, as was requested originally. Regards, -Denis
Re: [RFC 0/1] Allow MAC change on up interface
Hi Johannes, So keeping things purely technical now :) I thought so, but I had another thought later. It might be possible to set LIVE_ADDR_CHANGE, but then block it in mac80211 when the interface is already connected (or beaconing, or whatever, using the MAC address in some way - even while scanning, remain-on-channel is active, etc.) Here's what we would like to see: - The ability for userspace to add a 'Local Mac Address to use' attribute on CMD_CONNECT and CMD_AUTHENTICATE. - It doesn't seem useful to add this to CMD_ASSOCIATE at the moment, as for new connections you'd always go either through CMD_AUTHENTICATE, CMD_ASSOCIATE sequence or use CMD_CONNECT. This should take care of some (most) of your objections about specifying different addresses for authenticate/associate. Feel free to correct me here. - For Fast Transitions, Re-associations, roaming, etc userspace would simply omit the 'Local Mac Address to use' attribute and the currently set address would be used. - Optionally (and I'm thinking of tools like NM here), add the ability to set the mac address via RTNL while the device is UP but has no carrier, and things like scanning, offchannel, etc are not in progress. Though really I don't see how NM could guarantee this without bringing the device down first, so I'll let NM guys chime in to see if this is a good idea. - We definitely do not want to to mess with the device state otherwise. E.g. no firmware downloading, powering the adapter down, etc. That just takes too much time. The goal here is to keep things as fast as possible. The randomization feature is basically standard on every modern OS. So users expect it to be used on every connection. Slowing down the connection time by up to 3 seconds just isn't acceptable. So tell us what you would like to see. A new IFF_NO_CARRIER_ADDRESS_CHANGE or whether it is possible to use IFF_LIVE_ADDR_CHANGE with some additional restrictions. I still think you'd have to bake it into the mac80211<->driver API somehow, because we normally "add_interface()" with the MAC address, and nothing says that the driver cannot ignore the MAC address from that point on. The fact that iwlwifi just copies it into every new MAC_CTXT command and the firmware actually accepts the update seems rather accidental and therefore fragile to rely on. Since you seem to have a clear(er?) idea here, can you elaborate or possibly provide the driver interface changes you want to see? Regards, -Denis
Re: [RFC 0/1] Allow MAC change on up interface
Hi Johannes, On 8/20/19 2:32 PM, Johannes Berg wrote: Hi Denis, Rather than replying to all the separate items here, just two things: 1) I'll reiterate - please keep things civil. You're taking things out of context a *lot* here, in what seems like an attempt to draw a parallel between my and your utterances. 2) I'll take your point that I've been somewhat dismissive in tone, and will try to change that. I'll apologize for the methods I used, but you were not getting to 2) above via our earlier discussions. Anyway, peace. I do want to reply to two things specifically though: Fine. I get that. But how about asking what the use case is? Or say you don't quite understand why something is needed? Really, I should *not* have to ask that. You should consider it your obligation to provide enough information for me to review patches without having to go back and ask what it is you actually want to achieve. So let me ask you this. What do you _want_ to see when a contributor submits something as an RFC, which that contributor states is not ready for 'true' review and is really there to generate feedback? Do you want to have that contributor use a different prefix? Every maintainer is differrent. I get that. So if we want to start an actual brainstorming session with you, how do we accomplish that? Compared to some other subsystems and maintainers I've dealt with, I think I've actually been rather patient in trying to extract the purpose of your changes, rather than *really* just dismissing them (which I've felt like many times.) I'll admit you're not the worst I've dealt with, but you can always improve, right? :) a maintainer who's job (by definition) is to encourage new contributors and improve the subsystem he maintains...? This is what maybe you see as the maintainer's role, but I disagree, at least to some extent. I see the role more of a supervisor, somebody who's ensuring that all the changes coming in will work together. Yes, I have my own incentive to improve things, but if you have a particular incentive to improve a particular use case, the onus really is on you to convince me of that, not on me to try to ferret the reasoning out of you and make those improvements my own. So this goes back to my earlier point. How do you want us to start a discussion with you such that you don't become a 'supervisor' and instead try to understand the pain points first? And really, we are not expecting you to do the improvements on your own. But you know the subsystem best. So you really should consider giving actual guidance on how to accomplish something in the best way. Also, look at it from the PoV of any new contributor. So while I can definitely relate to what you're saying here, I think you should put yourself in your peer's shoes and try to be more understanding of their perspective. At least make the effort to hear people out... So please - come with some actual reasoning. This particular thread only offered "would elminate a few potential race conditions", in the cover letter, not even the patch itself, so it wasn't very useful. I was perhaps less than courteous trying to extract the reasoning, but I shouldn't have to in the first place. Okay, so we'll work on that. But this is a two way street too. And sometimes it seems like you're not actually reading the cover letters ;) Regards, -Denis
Re: [RFC 0/1] Allow MAC change on up interface
Hi Johannes, On 8/20/19 2:43 PM, Johannes Berg wrote: On Tue, 2019-08-20 at 14:22 -0500, Denis Kenzior wrote: Hi Johannes, So keeping things purely technical now :) I thought so, but I had another thought later. It might be possible to set LIVE_ADDR_CHANGE, but then block it in mac80211 when the interface is already connected (or beaconing, or whatever, using the MAC address in some way - even while scanning, remain-on-channel is active, etc.) Here's what we would like to see: - The ability for userspace to add a 'Local Mac Address to use' attribute on CMD_CONNECT and CMD_AUTHENTICATE. Why here though? I don't really like this as it's extra complexity there, and dev_set_mac_address() is really easy to call from userspace. Yes, that's sort of a round-trip more (you wouldn't even really have to wait for it I guess), but we have to make trade-offs like that (vs. kernel complexity) at some point. But what actual complexity are we talking about here? If the kernel can do this while the CONNECT is pending, why not? It makes things simpler and faster for userspace. I don't see the downside unless you can somehow objectively explain 'complexity'. - It doesn't seem useful to add this to CMD_ASSOCIATE at the moment, as for new connections you'd always go either through CMD_AUTHENTICATE, CMD_ASSOCIATE sequence or use CMD_CONNECT. This should take care of some (most) of your objections about specifying different addresses for authenticate/associate. Feel free to correct me here. That wasn't really my objection, I was more wondering why James implemented it *only* for CMD_CONNECT, when I had been under the (apparently mistaken) impression that one would generally prefer CMD_ASSOCIATE over CMD_CONNECT, if available. This was an RFC. There isn't much point for us to cross all the 't's and dot all the 'i's if you hate the idea in the first place. - Optionally (and I'm thinking of tools like NM here), add the ability to set the mac address via RTNL while the device is UP but has no carrier, and things like scanning, offchannel, etc are not in progress. Though really I don't see how NM could guarantee this without bringing the device down first, so I'll let NM guys chime in to see if this is a good idea. I'm thinking along the lines of letting you do this *instead* of the scheme you described above. That way, we don't even need to have a discussion over whether it makes sense at CONNECT, AUTH, ASSOC, or whatever because you can essentially do it before/with any of these commands. Why would this not be sufficient? It would get the job done. But it is still a waste of time and still slowing us down. Look at it this way, even if we save 10ms here. Take that and multiply by 3-4 billion devices and then by the number of connections one does each day. This adds up to some serious time wasted. So why not do this elegantly and faster in the first place? - We definitely do not want to to mess with the device state otherwise. E.g. no firmware downloading, powering the adapter down, etc. That just takes too much time. I can understand this part. So tell us what you would like to see. A new IFF_NO_CARRIER_ADDRESS_CHANGE or whether it is possible to use IFF_LIVE_ADDR_CHANGE with some additional restrictions. I don't know. This is not something I'm intimately familiar with. I could imagine (as you quoted above) that we just set IFF_LIVE_ADDR_CHANGE and manage the exclusions local to e.g. mac80211. Okay, so lets operate under the assumption we can hi-jack IFF_LIVE_ADDR_CHANGE for this I still think you'd have to bake it into the mac80211<->driver API somehow, because we normally "add_interface()" with the MAC address, and nothing says that the driver cannot ignore the MAC address from that point on. The fact that iwlwifi just copies it into every new MAC_CTXT command and the firmware actually accepts the update seems rather accidental and therefore fragile to rely on. Since you seem to have a clear(er?) idea here, can you elaborate or possibly provide the driver interface changes you want to see? I can see a few ways of doing this, for example having an optional "change_vif_addr" method in the mac80211 ops, so that drivers can do the necessary work. I suppose iwlwifi would actually want to send a new MAC_CONTEXT command at this time, for example, because the firmware is notoriously finicky when it comes to command sequences. Alternatively, and this would work with all drivers, you could just pretend to remove/add the interface, ie. in mac80211 call drv_remove_interface(sdata) // set new mac addr drv_add_interface(sdata) This has the advantage that it'll be guaranteed to work with all drivers, at the expense of perhaps a few more firmware commands (depending on the driver). You can probably come
Re: [RFC 0/1] Allow MAC change on up interface
Hi Johannes, On 8/20/19 3:15 PM, Johannes Berg wrote: On Tue, 2019-08-20 at 14:58 -0500, Denis Kenzior wrote: But what actual complexity are we talking about here? If the kernel can do this while the CONNECT is pending, why not? It makes things simpler and faster for userspace. I don't see the downside unless you can somehow objectively explain 'complexity'. It's just extra code that we have to worry about. Right now you want it for CMD_CONNECT and CMD_AUTH. Somebody will come up with a reason to do it in CMD_ASSOC next, perhaps, who knows. Somebody else will say "oh, this is how it's done, so let's add it to CMD_JOIN_IBSS", because of course that's what they care about. OCB? Mesh? AP mode for tethering? Etc. I don't buy the extra code argument. If you want to do something useful you need to write 'extra code'. The rest, I'm not sure why you are worried about them now? For station there's a very clear & present use case. If such a clear and present use case is presented for AP or Mesh, then deal with it then. I don't see how this will not keep proliferating, and each new thing will come with its own dozen lines of code, a new feature flag, etc. Such is life? :) Relaxing and defining once and for all in which situations while the interface is up you can actually allow changing the address, and then having userspace do it that way is IMHO a better way to design the system, since it forgoes entirely all those questions of when and how and which new use cases will come up etc. That would be great in theory, but practically never works at least in my experience. So maybe keep and open mind? There is a clear need to make this path as fast as possible for STA. There is no such need (yet) for the other cases you mentioned. This was an RFC. There isn't much point for us to cross all the 't's and dot all the 'i's if you hate the idea in the first place. Sure, but I cannot distinguish between "we only want it on CMD_CONNECT" and "we'll extend this once we agree" unless you actually say so. It'd help to communicate which t's and i's you didn't cross or dot. Okay, I'll admit the RFC description could have been better. But in the end you're human last I checked (at least I recall meeting you several times? ;) How about a simple "Why do you think you need this?" first? Regards, -Denis
Re: [RFC 0/1] Allow MAC change on up interface
Hi Dan, On 8/20/19 4:18 PM, Dan Williams wrote: Code will be written, but I'd rather it be written once rather than 3+ times for STA/AP/Mesh/etc. I'm not sure you can state that definitively just yet? So the real question is whether saving the extra round-trip to the kernel is worth the in-kernel complexity. Given that interleaved system calls are _always_ a problem, I argue that it is worth it for at least the Station case (and it will keep connection times even faster to boot). Isn't minimizing the latency of connections the end goal here? I get that there are trade offs and people have other opinions on what a good trade off is. But don't misunderstand, either solution is better than what we have today. My argument is: "why close the door on a particular solution until the costs are known?" The rest, I'm not sure why you are worried about them now? For station there's a very clear & present use case. If such a clear and present use case is presented for AP or Mesh, then deal with it then. Why would you not want to pass a random MAC for AP or Mesh modes? The same reasons for MAC randomization apply for all those too, I'd think. Umm, I was not arguing against doing that at all? All I said was that no such use case was yet presented. For AP it isn't typically needed to rapidly switch between MAC addresses while keeping the device UP. If you think there's such a need, I'm happy to learn something new? Same goes for Mesh really? I don't see how this will not keep proliferating, and each new thing will come with its own dozen lines of code, a new feature flag, etc. Such is life? :) Not really. It's the job of maintainers to balance all these things, to step back and think of the bigger picture and the future rather than just solving one particular use-case today. > Your tone leaves the impression you want a particular solution pushed through without the normal planning/architecture discussions that accompany API changes. And that's not how the process typically works. So who's attacking who now? We're trying to solve a long standing issue that nobody has bothered to fix for years in a clean way. Something that one of your projects would benefit from, btw. I have a technical opinion about how it should look like. Johannes might have a different opinion. In the end it is up to him and I can go pound sand. So yes, I know how the process works ;) Regards, -Denis
[PATCH 1/2] nl80211: Add NL80211_EXT_FEATURE_LIVE_IFTYPE_CHANGE
There is some ambiguity in how various drivers support
NL80211_CMD_SET_INTERFACE on devices where the underlying netdev is UP.
mac80211 for example supports this if the underlying driver provides a
change_interface operation. However, most devices do not. For FullMac
devices, the situation is even less clear.
This patch introduces a new feature flag that lets userspace know
whether it can expect a mode change (via SET_INTERFACE) to work while
the device is still UP or it should bring down the device first.
This commit also updates the documentation for SET_INTERFACE with hints
as to how it should be used.
Signed-off-by: Denis Kenzior
---
include/uapi/linux/nl80211.h | 36
1 file changed, 36 insertions(+)
diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index bf7c4222f512..a9ca2fe67f52 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -275,6 +275,29 @@
* single %NL80211_ATTR_IFINDEX is supported.
* @NL80211_CMD_SET_INTERFACE: Set type of a virtual interface, requires
* %NL80211_ATTR_IFINDEX and %NL80211_ATTR_IFTYPE.
+ *
+ * Note that it is driver-dependent whether a SET_INTERFACE will be
+ * allowed if the underlying netdev is currently UP. Userspace
+ * can obtain a hint as to whether this is allowed by looking at
+ * the %NL80211_EXT_FEATURE_LIVE_IFTYPE_CHANGE, but certain restrictions
+ * will still apply.
+ *
+ * Prior to Kernel 5.4, userspace applications should implement the
+ * following behavior:
+ * 1. (Optionally) Attempt SET_INTERFACE on a wireless device
+ *with the underlying netdev in the UP state. If -EBUSY
+ *is returned proceed to 2. Note that a SET_INTERFACE
+ *which results in -EBUSY might still result in other
+ *side-effects, such as Deauthentication, exiting AP mode,
+ *etc.
+ * 2. Bring the netdev DOWN via RTNL
+ * 3. Attempt SET_INTERFACE on the underlying netdev in the DOWN
+ *state. If successful, proceed to 4.
+ * 4. Bring the netdev UP via RTNL
+ *
+ * Note that bringing down the device might trigger a firmware reset /
+ * power cycling and/or other effects that are driver dependent.
+ *
* @NL80211_CMD_NEW_INTERFACE: Newly created virtual interface or response
* to %NL80211_CMD_GET_INTERFACE. Has %NL80211_ATTR_IFINDEX,
* %NL80211_ATTR_WIPHY and %NL80211_ATTR_IFTYPE attributes. Can also
@@ -5481,6 +5504,18 @@ enum nl80211_feature_flags {
* @NL80211_EXT_FEATURE_SAE_OFFLOAD: Device wants to do SAE authentication in
* station mode (SAE password is passed as part of the connect command).
*
+ * @NL80211_EXT_FEATURE_LIVE_IFTYPE_CHANGE: This device supports switching
+ * the IFTYPE of an interface without having to bring the device DOWN
+ * first via RTNL. Exact semantics of this feature is driver
+ * implementation dependent. For mac80211, the following restrictions
+ * apply:
+ * - Only devices currently in IFTYPE AP, P2P_GO, P2P_CLIENT,
+ * STATION, ADHOC and OCB can be switched.
+ * - The target IFTYPE must be one of: AP, P2P_GO, P2P_CLIENT,
+ * STATION, ADHOC or OCB.
+ * Other drivers are expected to follow similar restrictions.
+ * This flag was introduced in Kernel v5.4
+ *
* @NUM_NL80211_EXT_FEATURES: number of extended features.
* @MAX_NL80211_EXT_FEATURES: highest extended feature index.
*/
@@ -5526,6 +5561,7 @@ enum nl80211_ext_feature_index {
NL80211_EXT_FEATURE_EXT_KEY_ID,
NL80211_EXT_FEATURE_STA_TX_PWR,
NL80211_EXT_FEATURE_SAE_OFFLOAD,
+ NL80211_EXT_FEATURE_LIVE_IFTYPE_CHANGE,
/* add new features before the definition below */
NUM_NL80211_EXT_FEATURES,
--
2.19.2
[PATCH 2/2] mac80211: Set LIVE_IFTYPE_CHANGE if op provided
Signed-off-by: Denis Kenzior --- net/mac80211/main.c | 4 1 file changed, 4 insertions(+) diff --git a/net/mac80211/main.c b/net/mac80211/main.c index 29b9d57df1a3..073e5d10a48e 100644 --- a/net/mac80211/main.c +++ b/net/mac80211/main.c @@ -597,6 +597,10 @@ struct ieee80211_hw *ieee80211_alloc_hw_nm(size_t priv_data_len, wiphy_ext_feature_set(wiphy, NL80211_EXT_FEATURE_RRM); + if (ops->change_interface) + wiphy_ext_feature_set(wiphy, + NL80211_EXT_FEATURE_LIVE_IFTYPE_CHANGE); + wiphy->bss_priv_size = sizeof(struct ieee80211_bss); local = wiphy_priv(wiphy); -- 2.19.2
[PATCH 1/2] mac80211: Don't memset RXCB prior to PAE intercept
In ieee80211_deliver_skb_to_local_stack intercepts EAPoL frames if
mac80211 is configured to do so and forwards the contents over nl80211.
During this process some additional data is also forwarded, including
whether the frame was received encrypted or not. Unfortunately just
prior to the call to ieee80211_deliver_skb_to_local_stack, skb->cb is
cleared, resulting in incorrect data being exposed over nl80211.
Fixes: 018f6fbf540d ("mac80211: Send control port frames over nl80211")
Cc: sta...@vger.kernel.org
Signed-off-by: Denis Kenzior
---
net/mac80211/rx.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index 3c1ab870fefe..7c4aeac006fb 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -2452,6 +2452,8 @@ static void ieee80211_deliver_skb_to_local_stack(struct
sk_buff *skb,
cfg80211_rx_control_port(dev, skb, noencrypt);
dev_kfree_skb(skb);
} else {
+ memset(skb->cb, 0, sizeof(skb->cb));
+
/* deliver to local stack */
if (rx->napi)
napi_gro_receive(rx->napi, skb);
@@ -2546,8 +2548,6 @@ ieee80211_deliver_skb(struct ieee80211_rx_data *rx)
if (skb) {
skb->protocol = eth_type_trans(skb, dev);
- memset(skb->cb, 0, sizeof(skb->cb));
-
ieee80211_deliver_skb_to_local_stack(skb, rx);
}
--
2.19.2
[PATCH 2/2] mac80211: Correctly set noencrypt for PAE frames
The noencrypt flag was intended to be set if the "frame was received
unencrypted" according to include/uapi/linux/nl80211.h. However, the
current behavior is opposite of this.
Cc: sta...@vger.kernel.org
Fixes: 018f6fbf540d ("mac80211: Send control port frames over nl80211")
Signed-off-by: Denis Kenzior
---
net/mac80211/rx.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index 7c4aeac006fb..8514c1f4ca90 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -2447,7 +2447,7 @@ static void ieee80211_deliver_skb_to_local_stack(struct
sk_buff *skb,
skb->protocol == cpu_to_be16(ETH_P_PREAUTH)) &&
sdata->control_port_over_nl80211)) {
struct ieee80211_rx_status *status = IEEE80211_SKB_RXCB(skb);
- bool noencrypt = status->flag & RX_FLAG_DECRYPTED;
+ bool noencrypt = (status->flag & RX_FLAG_DECRYPTED) == 0;
cfg80211_rx_control_port(dev, skb, noencrypt);
dev_kfree_skb(skb);
--
2.19.2
[PATCH 0/2] mac80211: Control Port over nl80211 fixes
Couple of small fixes for Control Port handling in mac80211. The original commit was working by some crazy luck in all testing, but manifested itself on certain hardware that managed to drop PAE frames with uncanny consistency. Denis Kenzior (2): mac80211: Don't memset RXCB prior to PAE intercept mac80211: Correctly set noencrypt for PAE frames net/mac80211/rx.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) -- 2.19.2
[PATCH] cfg80211: Purge frame registrations on iftype change
Currently frame registrations are not purged, even when changing the interface type. This can lead to potentially weird / dangerous situations where frames possibly not relevant to a given interface type remain registered and mgmt_frame_register is not called for the no-longer-relevant frame types. The kernel currently relies on userspace apps to actually purge the registrations themselves, e.g. by closing the nl80211 socket associated with those frames. However, this requires multiple nl80211 sockets to be open by the userspace app, and for userspace to be aware of all state changes. This is not something that the kernel should rely on. This commit adds a call to cfg80211_mlme_purge_registrations() to forcefully remove any registrations left over prior to switching the iftype. Cc: sta...@vger.kernel.org Signed-off-by: Denis Kenzior --- net/wireless/util.c | 1 + 1 file changed, 1 insertion(+) diff --git a/net/wireless/util.c b/net/wireless/util.c index c99939067bb0..3fa092b78e62 100644 --- a/net/wireless/util.c +++ b/net/wireless/util.c @@ -964,6 +964,7 @@ int cfg80211_change_iface(struct cfg80211_registered_device *rdev, } cfg80211_process_rdev_events(rdev); + cfg80211_mlme_purge_registrations(dev->ieee80211_ptr); } err = rdev_change_virtual_intf(rdev, dev, ntype, params); -- 2.19.2
Re: [PATCH] cfg80211: Purge frame registrations on iftype change
Hi Johannes, On 8/30/19 3:53 AM, Johannes Berg wrote: On Wed, 2019-08-28 at 16:11 -0500, Denis Kenzior wrote: Currently frame registrations are not purged, even when changing the interface type. This can lead to potentially weird / dangerous situations where frames possibly not relevant to a given interface type remain registered and mgmt_frame_register is not called for the no-longer-relevant frame types. I'd argue really just "weird and non-working", hardly dangerous. Even in the mac80211 design where we want to not let you intercept e.g. AUTH frames in client mode - if you did, then you'd just end up with a non- working interface. Not sure I see any "dangerous situation". Not really an all that important distinction though. Fair enough, I'm happy to drop / reword this language. It seemed fishy to me since the unregistration operation was not called at all, and the driver does go to some lengths to set up the valid frame registration types. Depending on the design, it may also just be that those registrations are *ignored*, because e.g. firmware intercepts the AUTH frame already, which would just (maybe) confuse userspace - but that seems unlikely since it switched interface type and has no real need for those frames then. There might be corner cases where userspace gets confused and doesn't update the frame registrations properly. For example, wpa_s/hostap does not listen to SET_INTERFACE events that I can tell. So if some external app sets the mode (particularly on a 'live' interface) then all kinds of unexpected things might happen. This is one of the motivations for restricting certain NL80211 commands to interface SOCKET_OWNER. So really this patch is intended more as a hot-fix / backport to stable to make sure the older kernels can deal with some of these situations. The kernel currently relies on userspace apps to actually purge the registrations themselves, e.g. by closing the nl80211 socket associated with those frames. However, this requires multiple nl80211 sockets to be open by the userspace app, and for userspace to be aware of all state changes. This is not something that the kernel should rely on. I tend to agree with that the kernel shouldn't rely on it. This commit adds a call to cfg80211_mlme_purge_registrations() to forcefully remove any registrations left over prior to switching the iftype. However, I do wonder if we should make this more transactional, and hang on to them if the type switching fails. We're not notifying userspace that the registrations have disappeared, so if type switching fails and it continues to work with the old type rather than throwing its hands up and quitting or something, it'd make a possibly bigger mess to just silently have removed them already. I do like that idea, not sure how to go about implementing it though? The failure case is a bit hard to deal with. Something like NL80211_EXT_FEATURE_LIVE_IFTYPE_CHANGE would help, particularly if nl80211/cfg80211 actually checked it prior to doing anything (e.g. disconnecting, etc). That would then take care of the majority of the 'typical' failure paths. I didn't add such checking in the other patch set since I felt you might find it overly intrusive on userspace. But maybe we really should do this? So playing devil's advocate, another argument might be that by the time we got here, we've already tore down a bunch of state. E.g. disconnected the station, stopped AP, etc. So we've already side-effected state in a bunch of ways, what's one more? I *think* it should be safe to just move this after the switching succeeds, since the switching can pretty much only be done at a point where nothing is happening on the interface anyway, though that might confuse the driver when the remove happens. I would concur as that is what happens today. But should it? Also, perhaps it'd be better to actually hang on to those registrations that *are* still possible afterwards? But to not confuse the driver I guess that might require unregister/re-register to happen, all of which requires hanging on to the list and going through it after the type switch completed? Yes, I had those exact thoughts as well. It isn't currently clear to me if there are any guarantees on the driver operation call sequence that cfg80211 provides. E.g. can the driver expect rdev_change_virtual_intf to be called only once all the old registrations are purged and the new registrations are performed after the fact? Or should it expect things to just happen in any order? What do you think? A big part of me thinks that just wiping the slate clean and having userspace set it up from scratch isn't that much to ask and it might want to do that anyway. It might (a big maybe?) also make the driver's life easier if it can rely on certain guarantees from cf
Re: [RFCv2 4/4] nl80211: Send large new_wiphy events
Hi Johannes,
On 8/30/19 5:14 AM, Johannes Berg wrote:
On Fri, 2019-08-16 at 14:27 -0500, Denis Kenzior wrote:
Send large NEW_WIPHY events on a new multicast group so that clients
that can accept larger messages do not need to round-trip to the kernel
and perform extra filtered wiphy dumps.
A new multicast group is introduced and the large message is sent before
the legacy message. This way clients that listen on both multicast
groups can ignore duplicate legacy messages if needed.
Since I just did the digging, it seems that this would affect (old)
applications with libnl up to 3.2.22, unless they changed the default
recvmsg() buffer size.
Sorry, I'm not sure I understand. Are you saying new clients would try
to use old libnl and subscribe to this new multicast group for large
messages? Legacy clients shouldn't even see messages on this multicast
group since they would never subscribe to it.
I think this is a pretty decent approach, but I'm slightly worried about
hitting the new limits (16k) eventually. It seems far off now, but who
knows what kind of data we'll add. HE is (and likely will be) adding
quite a bit since it has everything for each interface type - something
drivers have for the most part not implemented yet. That trend will only
continue, as complexity in the spec doesn't seem to be going down.
Right, but the kernel will go up to 32k buffers if userspace read buffer
is that large. So I think we have quite some room to grow. On the
other hand, we probably should be vigilant that any new stuff added
tries to minimize message sizes whenever possible.
And I don't really want to see "config3" a couple of years down the
road...
Agreed.
So can we at least mandate (document) that "config2" basically has no
message limit, and you will use MSG_PEEK/handle MSG_TRUNC with it?
Yes, I will take care of that in v3.
That way, we can later bump the 8192 even beyond 16k if needed, and not
run into problems.
+ if (cmd == NL80211_CMD_NEW_WIPHY) {
+ state.large_message = true;
+ alloc_size = 8192UL;
+ } else
+ alloc_size = NLMSG_DEFAULT_SIZE;
+
nit: there should be braces on both branches
will fix
+ if (nl80211_send_wiphy(rdev, cmd, msg, 0, 0, 0, &state) < 0) {
+ nlmsg_free(msg);
+ goto legacy;
+ }
I think that'd be worth a WARN_ON(), it should never happen that you
actually run out of space, it means that the above wasn't big enough.
Yep
Now, on the previous patches I actually thought that you could set
"state->split" (and you should) and not need "state->large_message" in
order to indicate that the sub-functions are allowed to create larger
data - just keep filling the SKBs as much as possible for the dump.
Here, it seems like we do need it. It might be possible to get away
without it (by setting split here, and then having some special code to
handle the case of it not getting to the end), but that doesn't seem
worth it.
@@ -14763,6 +14787,8 @@ void nl80211_notify_iface(struct
cfg80211_registered_device *rdev,
return;
}
+ genlmsg_multicast_netns(&nl80211_fam, wiphy_net(&rdev->wiphy), msg, 0,
+ NL80211_MCGRP_CONFIG2, GFP_KERNEL);
Hmm. That seems only needed if you don't want to listen on "config" at
all, but in the patch description you explicitly said that you send it
on "config2" *before* "config" for compatibility reasons (which makes
sense) - so what is it?
Well it can be both, depending on whether large messages can fail or
not. So one use case might be that a client detects whether the config2
multicast group exists. If so, then it only subscribes to it and that's it.
Another use case might be (if userspace is worried about losing large
messages) to subscribe to both groups. If it receives the large
message, it can ignore the one that comes on the legacy multicast group.
I'm having a hard time seeing anyone get away with only listening on
config2 since that'd basically require very recent (as of now future)
kernel. Are you planning this for a world where you can ditch support
for kernel<5.4 (or so)?
No, but there's nothing stopping the client in making the choice at
runtime depending on the genl family info it gets. E.g. by peeking into
CTRL_ATTR_MCAST_GROUPS.
Regards,
-Denis
Re: [RFCv2 1/4] nl80211: Fix broken non-split wiphy dumps
Hi Johannes,
On 8/30/19 4:03 AM, Johannes Berg wrote:
On Fri, 2019-08-16 at 14:27 -0500, Denis Kenzior wrote:
If a (legacy) client requested a wiphy dump but did not provide the
NL80211_ATTR_SPLIT_WIPHY_DUMP attribute, the dump was supposed to be
composed of purely non-split NEW_WIPHY messages, with 1 wiphy per
message. At least this was the intent after commit:
3713b4e364ef ("nl80211: allow splitting wiphy information in dumps")
However, in reality the non-split dumps were broken very shortly after.
Perhaps around commit:
fe1abafd942f ("nl80211: re-add channel width and extended capa advertising")
Fun. I guess we updated all userspace quickly enough to not actually
have any issues there. As far as I remember, nobody ever complained, so
I guess people just updated their userspace.
Given that it's been 6+ years, maybe we're better off just removing the
whole non-split thing then, instead of fixing it. Seems even less likely
now that somebody would run a 6+yo supplicant (from before its commit
c30a4ab045ce ("nl80211: Fix mode settings with split wiphy dump")).
That would be my vote, given that we're probably one of a handful of
people in this world that understand that code path.
But... How would we handle non-dump versions of GET_WIPHY? To this day
I have dhcpcd issuing fun stuff like:
< Request: Get Wiphy (0x01) len 8 [ack]
0.374832
Interface Index: 59 (0x003b)
OTOH, this is a simple fix, would removing the non-split mode result in
any appreciable cleanups? Perhaps not, and we'd have to insert something
instead to reject non-split and log a warning, or whatnot.
Getting rid of the legacy non-split case would simplify things. We
could also be a-lot smarter about how we split up the messages in order
to utilize buffer space more efficiently. I think you cover this in
your other replies, but I haven't processed those yet.
Regards,
-Denis
Re: [PATCH 1/2] nl80211: Add NL80211_EXT_FEATURE_LIVE_IFTYPE_CHANGE
Hi Johannes, On 8/30/19 5:19 AM, Johannes Berg wrote: On Mon, 2019-08-26 at 11:26 -0500, Denis Kenzior wrote: + * Prior to Kernel 5.4, userspace applications should implement the + * following behavior: I'm not sure mentioning the kernel version here does us any good? I mean, you really need to implement that behaviour regardless of kernel version, if NL80211_EXT_FEATURE_LIVE_IFTYPE_CHANGE isn't set. Agreed. I guess I just view nl80211.h as a sort of combination between a uapi file and an actual manpage. And manpages do mention which kernel version a certain feature/flag/whatever was added. Such info can be useful in many ways, e.g. figuring out which kernel version might be required for a certain piece of hardware, etc. Another point where this might be useful is if the kernel starts enforcing certain behavior that it didn't before. E.g. I mentioned this in the purge thread that a lot of mode change failure cases could be caught if the kernel checked this flag prior to doing anything. I really leave this up to you if this is something you think is a good idea or not. + * @NL80211_EXT_FEATURE_LIVE_IFTYPE_CHANGE: This device supports switching + * the IFTYPE of an interface without having to bring the device DOWN + * first via RTNL. Exact semantics of this feature is driver + * implementation dependent. That's not really nice. Sorry. This came from some doc changes I have pending. I think I wrote this after looking at some fullmac drivers and how they handle mode changes and the wording reflects the exasperation I felt at the time. Do you want to suggest some alternate wording? I think it is worth it to have some fair warning in the docs stating that prior to version so and so the semantics are completely driver dependent. For mac80211, the following restrictions + * apply: + * - Only devices currently in IFTYPE AP, P2P_GO, P2P_CLIENT, + * STATION, ADHOC and OCB can be switched. + * - The target IFTYPE must be one of: AP, P2P_GO, P2P_CLIENT, + * STATION, ADHOC or OCB. + * Other drivers are expected to follow similar restrictions. Maybe we should instead have a "bitmask of interface types that can be switched while live" or something like that? I'm fine with that, but this would only apply to newer kernels, no? Don't we at least want to attempt to state what the rules are for older ones? An alternative might be to simply state what the restrictions are and just enforce those at the cfg80211 level. Regards, -Denis
Re: [RFCv2 2/4] nl80211: Support >4096 byte NEW_WIPHY event nlmsg
Hi Johannes,
On 8/30/19 4:36 AM, Johannes Berg wrote:
On Fri, 2019-08-16 at 14:27 -0500, Denis Kenzior wrote:
For historical reasons, NEW_WIPHY messages generated by dumps or
GET_WIPHY commands were limited to 4096 bytes due to userspace tools
using limited buffers.
I think now that I've figured out why, it'd be good to note that it
wasn't due to userspace tools, but rather due to the default netlink
dump skb allocation at the time, prior to commit 9063e21fb026
("netlink: autosize skb lengthes").
Sure, will take care of it.
Once the sizes NEW_WIPHY messages exceeded these
sizes, split dumps were introduced. All any non-legacy data was added
only to messages using split-dumps (including filtered dumps).
However, split-dumping has quite a significant overhead. On cards
tested, split dumps generated message sizes 1.7-1.8x compared to
non-split dumps, while still comfortably fitting into an 8k buffer. The
kernel now expects userspace to provide 16k buffers by default, and 32k
buffers are possible.
Introduce a concept of a large message, so that if the kernel detects
that userspace has provided a buffer of sufficient size, a non-split
message could be generated.
So, there's still a wrinkle with this. Larger SKB allocation can fail,
and instead of returning an error to userspace, the kernel will allocate
a smaller SKB instead.
With genetlink, we currently don't even have a way of controlling the
minimum allocation that's always required.
Since we already have basically all of the mechanics, I'd say perhaps a
better concept would be to "split when necessary", aborting if split
isn't supported.
IOW, do something like
... nl80211_send_wiphy(...)
{
[...]
switch (state->split_start) {
[...]
case :
[...] // put stuff
state->split_start++;
state->skb_end = nlmsg_get_pos(skb);
/* fall through */
case :
[...]
}
finish:
genlmsg_end(msg, hdr);
return 0;
nla_put_failure:
if (state->split_start < 9) {
genlmsg_cancel(msg, hdr);
return -EMSGSIZE;
}
nlmsg_trim(msg, state->skb_end);
goto finish;
}
That way, we fill each SKB as much as possible, up to 32k if userspace
provided big enough buffers *and* we could allocate the SKB.
Your userspace would still set the split flag, and thus be compatible
with all kinds of options:
* really old kernel not supporting split
* older kernel sending many messages
* kernel after this change packing more into one message
* even if allocating big SKBs failed
What I was thinking was to attempt to build a large message first and if
that fails to fail over to the old logic. But I think what you propose
is even better. I'll incorporate this feedback into the next version.
Regards,
-Denis
[PATCH] nl80211: Support mgmt frame unregistrations
Currently nl80211 supports purging management frame registrations
only under the following circumstances:
- If the underlying wireless device is destroyed
- If userspace closes the socket associated with that frame
registration
Thus userspace applications that want to gracefully support changing
interface modes (and thus the set of management frames they're
interested in seeing) must resort to opening multiple genl sockets and
manage these sockets appropriately. To state another way, it is
currently not possible to write a userspace application that utilizes a
single nl80211 genl socket, instead it must open multiple genl sockets
for multiple wdevs on multiple phys.
This commit introduces two new NL80211 commands:
NL80211_CMD_REGISTER_FRAME2 and NL80211_CMD_UNREGISTER_FRAME. The
former acts very much like NL80211_CMD_REGISTER_FRAME, except it returns
an NL80211_ATTR_COOKIE with a unique id identifying the management frame
registration. This cookie can then be used with
NL80211_CMD_UNREGISTER_FRAME to delete a previous registration.
NL80211_CMD_UNREGISTER_FRAME can also be used to remove all frame
registrations currently associated with the calling socket. This is
done by omitting the NL80211_ATTR_COOKIE attribute. Only frame
registrations owned by the calling socket can be removed.
NL80211_CMD_REGISTER_FRAME2 was added to keep backwards compatibility
with older clients which rely on NL80211_CMD_REGISTER_FRAME and might
not be able to deal with introduction of a new attribute as a part of
the return value.
Signed-off-by: Denis Kenzior
---
include/uapi/linux/nl80211.h | 17
net/wireless/core.h | 4 +-
net/wireless/mlme.c | 37 +++-
net/wireless/nl80211.c | 83 +++-
4 files changed, 137 insertions(+), 4 deletions(-)
diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index beee59c831a7..cef7e6920a6d 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -1101,6 +1101,20 @@
* peer MAC address and %NL80211_ATTR_FRAME is used to specify the frame
* content. The frame is ethernet data.
*
+ * @NL80211_CMD_REGISTER_FRAME2: Same as %NL80211_CMD_REGISTER_FRAME,
+ * except returns an ATTR_COOKIE so that the frame can be unregistered
+ * Unlike %NL80211_CMD_REGISTER_FRAME, this command requires a frame
+ * type attribute. Registration can be dropped
+ * using %NL80211_CMD_UNREGISTER_FRAME
+ *
+ * @NL80211_CMD_UNREGISTER_FRAME: Unregisters a previously registered frame
+ * that was registered with %NL80211_CMD_REGISTER_FRAME2. If
+ * %NL80211_ATTR_COOKIE is provided, then a single frame registration
+ * matching that cookie is unregistered. Otherwise, all frames
+ * associated with the current socket are unregistered. Note that this
+ * command can only affect registrations for a single wdev. So
+ * %NL80211_ATTR_IFINDEX or %NL80211_ATTR_WDEV must be provided.
+ *
* @NL80211_CMD_MAX: highest used command number
* @__NL80211_CMD_AFTER_LAST: internal use
*/
@@ -1325,6 +1339,9 @@ enum nl80211_commands {
NL80211_CMD_PROBE_MESH_LINK,
+ NL80211_CMD_REGISTER_FRAME2,
+ NL80211_CMD_UNREGISTER_FRAME,
+
/* add new commands above here */
/* used to define NL80211_CMD_MAX below */
diff --git a/net/wireless/core.h b/net/wireless/core.h
index 77556c58d9ac..c81a03fa8d39 100644
--- a/net/wireless/core.h
+++ b/net/wireless/core.h
@@ -385,9 +385,11 @@ void cfg80211_mlme_down(struct cfg80211_registered_device
*rdev,
struct net_device *dev);
int cfg80211_mlme_register_mgmt(struct wireless_dev *wdev, u32 snd_pid,
u16 frame_type, const u8 *match_data,
- int match_len);
+ int match_len, u64 cookie);
void cfg80211_mlme_unreg_wk(struct work_struct *wk);
void cfg80211_mlme_unregister_socket(struct wireless_dev *wdev, u32 nlpid);
+bool cfg80211_mlme_remove_registrations(struct wireless_dev *wdev,
+ u32 nlportid, u64 cookie);
void cfg80211_mlme_purge_registrations(struct wireless_dev *wdev);
int cfg80211_mlme_mgmt_tx(struct cfg80211_registered_device *rdev,
struct wireless_dev *wdev,
diff --git a/net/wireless/mlme.c b/net/wireless/mlme.c
index f9462010575f..3cf189742dfb 100644
--- a/net/wireless/mlme.c
+++ b/net/wireless/mlme.c
@@ -425,6 +425,8 @@ struct cfg80211_mgmt_registration {
__le16 frame_type;
+ u64 cookie;
+
u8 match[];
};
@@ -470,7 +472,7 @@ void cfg80211_mlme_unreg_wk(struct work_struct *wk)
int cfg80211_mlme_register_mgmt(struct wireless_dev *wdev, u32 snd_portid,
u16 frame_type, const u8 *match_data,
- int match_len)
+ int match_len, u64 cookie)
{
s
[RFCv3 2/3] nl80211: Support >4096 byte NEW_WIPHY event nlmsg
For historical reasons, NEW_WIPHY messages generated by dumps or
GET_WIPHY commands were limited to 4096 bytes. This was due to the
fact that the kernel only allocated 4k buffers prior to commit
9063e21fb026 ("netlink: autosize skb lengthes"). Once the sizes of
NEW_WIPHY messages exceeded these sizes, split dumps were introduced.
Any new, non-legacy data was added only to messages using split-dumps
(including filtered dumps).
However, split-dumping has quite a significant overhead. On cards
tested, split dumps generated message sizes 1.7-1.8x compared to
non-split dumps, while still comfortably fitting into an 8k buffer. The
kernel now expects userspace to provide 16k buffers by default, and 32k
buffers are possible.
The kernel netlink layer is now much smarter and utilizes certain
heuristics for figuring out what buffer sizes userspace provides, so it
can allocate optimally sized buffers for netlink messages accordingly.
This commit changes the split logic so that messages are packed more
compactly into the (potentially) larger buffers provided by userspace.
If large-enough buffers are provided, then split dumps will generate a
single netlink NEW_WIPHY message for each wiphy being dumped, removing
any overhead.
Signed-off-by: Denis Kenzior
---
net/wireless/nl80211.c | 222 +
1 file changed, 112 insertions(+), 110 deletions(-)
Changes since last version:
- Patch completely rewritten based on Johannes' feedback. We now try to
pack as many attributes as can fit into the current message. If the
application uses large enough buffers (typically 8k is sufficient as
of the time of this writing), then no splitting is even required.
- Rewrote the commit description based on Johannes' git history
findings. E.g. the kernel was at fault for the 4096 byte buffer size
limits and not userspace.
- Patch 3 was dropped as it was no longer required
Other thoughts:
- I tested the split dump with 3k, 4k and 8k userspace buffers and
things seem to work as expected.
- The code in case '3' is quite complex, but it does try to support a
message running out of room in the middle of a channel dump and
restarting from where it left off in the next split message. Perhaps
this can be simplified, but it seems this capability is useful.
Please take extra care when reviewing this.
- I dropped the split and restart logic in case 13 as no current driver
besides iwlwifi seems to support the attributes here, and the
attributes appear to be quite small anyway.
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index ff6200fcd492..03421f66eea3 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -1854,8 +1854,8 @@ static int nl80211_send_pmsr_capa(struct
cfg80211_registered_device *rdev,
struct nl80211_dump_wiphy_state {
s64 filter_wiphy;
long start;
- long split_start, band_start, chan_start, capa_start;
- bool split;
+ long split_start, band_start, chan_start;
+ bool legacy;
};
static int nl80211_send_wiphy(struct cfg80211_registered_device *rdev,
@@ -1867,8 +1867,9 @@ static int nl80211_send_wiphy(struct
cfg80211_registered_device *rdev,
struct nlattr *nl_bands, *nl_band;
struct nlattr *nl_freqs, *nl_freq;
struct nlattr *nl_cmds;
- enum nl80211_band band;
struct ieee80211_channel *chan;
+ void *last_good_pos = 0;
+ void *last_channel_pos;
int i;
const struct ieee80211_txrx_stypes *mgmt_stypes =
rdev->wiphy.mgmt_stypes;
@@ -1939,9 +1940,9 @@ static int nl80211_send_wiphy(struct
cfg80211_registered_device *rdev,
if ((rdev->wiphy.flags & WIPHY_FLAG_TDLS_EXTERNAL_SETUP) &&
nla_put_flag(msg, NL80211_ATTR_TDLS_EXTERNAL_SETUP))
goto nla_put_failure;
+
+ last_good_pos = nlmsg_get_pos(msg);
state->split_start++;
- if (state->split)
- break;
/* fall through */
case 1:
if (nla_put(msg, NL80211_ATTR_CIPHER_SUITES,
@@ -1986,17 +1987,16 @@ static int nl80211_send_wiphy(struct
cfg80211_registered_device *rdev,
}
}
+ last_good_pos = nlmsg_get_pos(msg);
state->split_start++;
- if (state->split)
- break;
/* fall through */
case 2:
if (nl80211_put_iftypes(msg, NL80211_ATTR_SUPPORTED_IFTYPES,
rdev->wiphy.interface_modes))
goto nla_put_failure;
+
+ last_good_pos = nlmsg_get_pos(msg);
state->split_start++;
- if (state->split)
- break;
/* fall through */
case 3:
[RFCv3 3/3] nl80211: Send large new_wiphy events
Send large NEW_WIPHY events on a new multicast group so that clients
that can accept larger messages do not need to round-trip to the kernel
and perform extra filtered wiphy dumps.
A new multicast group is introduced and the large message is sent before
the legacy message. This way clients that listen on both multicast
groups can ignore duplicate legacy messages if needed.
Signed-off-by: Denis Kenzior
---
include/uapi/linux/nl80211.h | 31 +++
net/wireless/nl80211.c | 34 --
2 files changed, 63 insertions(+), 2 deletions(-)
Changes in this version:
- Updated the docs based on Johannes' feedback
- Added WARN_ON in case the large message building fails (e.g. our
buffer size needs to be increased)
- Minor style fixes based on Johannes' feedback
- Added a missing skb_get to take an extra reference when sending
NEW/DEL INTERFACE events.
diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index beee59c831a7..7a125cb4d9d9 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -50,6 +50,7 @@
#define NL80211_MULTICAST_GROUP_MLME "mlme"
#define NL80211_MULTICAST_GROUP_VENDOR "vendor"
#define NL80211_MULTICAST_GROUP_NAN"nan"
+#define NL80211_MULTICAST_GROUP_CONFIG2"config2"
#define NL80211_MULTICAST_GROUP_TESTMODE "testmode"
#define NL80211_EDMG_BW_CONFIG_MIN 4
@@ -267,8 +268,30 @@
* @NL80211_CMD_NEW_WIPHY: Newly created wiphy, response to get request
* or rename notification. Has attributes %NL80211_ATTR_WIPHY and
* %NL80211_ATTR_WIPHY_NAME.
+ *
+ * Note that when %NL80211_CMD_NEW_WIPHY is being sent as an event, it
+ * will be multicast on two groups: "config" and "config2". The messages
+ * on the two multicast groups will be different. On "config" multicast
+ * group, %NL80211_CMD_NEW_WIPHY messages will be 'reduced' size and will
+ * only contain legacy information. This is due to historical kernel
+ * behavior that limited such messages to 4096 bytes. The "config2"
+ * multicast group was introduced to support applications that can
+ * allocate larger buffers and can thus accept new wiphy events with
+ * the full set of information included. Messages on the "config2"
+ * multicast group are sent before the "config" multicast group.
+ *
+ * There are no limits (outside of netlink protocol limits) on
+ * message sizes that can be sent over the "config2" multicast group. It
+ * is assumed that applications utilizing "config2" multicast group
+ * utilize buffers that are inherently large enough or can utilize
+ * MSG_PEEK/MSG_TRUNC in the netlink transport in order to allocate big
+ * enough buffers.
* @NL80211_CMD_DEL_WIPHY: Wiphy deleted. Has attributes
* %NL80211_ATTR_WIPHY and %NL80211_ATTR_WIPHY_NAME.
+ * Note that when %NL80211_CMD_DEL_WIPHY is being sent as an event, it
+ * will be multicast on two groups: "config" and "config2". Messages on
+ * the "config2" multicast group are sent before the "config" multicast
+ * group.
*
* @NL80211_CMD_GET_INTERFACE: Request an interface's configuration;
* either a dump request for all interfaces or a specific get with a
@@ -281,10 +304,18 @@
* be sent from userspace to request creation of a new virtual interface,
* then requires attributes %NL80211_ATTR_WIPHY, %NL80211_ATTR_IFTYPE and
* %NL80211_ATTR_IFNAME.
+ * Note that when %NL80211_CMD_NEW_INTERFACE is being sent as an event, it
+ * will be multicast on two groups: "config" and "config2". Messages on
+ * the "config2" multicast group are sent before the "config" multicast
+ * group.
* @NL80211_CMD_DEL_INTERFACE: Virtual interface was deleted, has attributes
* %NL80211_ATTR_IFINDEX and %NL80211_ATTR_WIPHY. Can also be sent from
* userspace to request deletion of a virtual interface, then requires
* attribute %NL80211_ATTR_IFINDEX.
+ * Note that when %NL80211_CMD_DEL_INTERFACE is being sent as an event, it
+ * will be multicast on two groups: "config" and "config2". Messages on
+ * the "config2" multicast group are sent before the "config" multicast
+ * group.
*
* @NL80211_CMD_GET_KEY: Get sequence counter information for a key specified
* by %NL80211_ATTR_KEY_IDX and/or %NL80211_ATTR_MAC.
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 03421f66eea3..68f496c0c0a4 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -46,6 +46,7 @@ enum nl80211_multicast_groups {
NL80211_MCGRP_MLME,
NL80211_MCGRP_VE
[RFCv3 1/3] nl80211: Fix broken non-split wiphy dumps
If a (legacy) client requested a wiphy dump but did not provide the
NL80211_ATTR_SPLIT_WIPHY_DUMP attribute, the dump was supposed to be
composed of purely non-split NEW_WIPHY messages, with 1 wiphy per
message. At least this was the intent after commit:
3713b4e364ef ("nl80211: allow splitting wiphy information in dumps")
However, in reality the non-split dumps were broken very shortly after.
Perhaps around commit:
fe1abafd942f ("nl80211: re-add channel width and extended capa advertising")
The reason for the bug is a missing setting of split_start to 0 in the
case of a non-split dump.
Here is a sample non-split dump performed on kernel 4.19, some parts
were cut for brevity:
< Request: Get Wiphy (0x01) len 0 [ack,0x300]
> Result: New Wiphy (0x03) len 3496 [multi]
Wiphy: 0 (0x)
Wiphy Name: phy0
Generation: 1 (0x0001)
> Result: New Wiphy (0x03) len 68 [multi]
Wiphy: 0 (0x)
Wiphy Name: phy0
Generation: 1 (0x0001)
Extended Capabilities: len 8
Capability: bit 2: Extended channel switching
Capability: bit 62: Opmode Notification
Extended Capabilities Mask: len 8
04 00 00 00 00 00 00 40 ...@
VHT Capability Mask: len 12
f0 1f 80 33 ff ff 00 00 ff ff 00 00 ...3
> Result: New Wiphy (0x03) len 28 [multi]
Wiphy: 0 (0x)
Wiphy Name: phy0
Generation: 1 (0x0001)
> Result: New Wiphy (0x03) len 28 [multi]
Wiphy: 0 (0x)
Wiphy Name: phy0
Generation: 1 (0x0001)
> Result: New Wiphy (0x03) len 52 [multi]
Wiphy: 0 (0x)
Wiphy Name: phy0
Generation: 1 (0x0001)
Max CSA Counters: len 1
02 .
Scheduled Scan Maximum Requests: len 4
01 00 00 00
Extended Features: len 4
02 02 00 04
> Result: New Wiphy (0x03) len 36 [multi]
Wiphy: 0 (0x)
Wiphy Name: phy0
Generation: 1 (0x0001)
Reserved: len 4
00 00 00 00
> Complete: Get Wiphy (0x01) len 4 [multi]
Status: 0
Signed-off-by: Denis Kenzior
---
net/wireless/nl80211.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 3e30e18d1d89..ff6200fcd492 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -2191,6 +2191,9 @@ static int nl80211_send_wiphy(struct
cfg80211_registered_device *rdev,
* but break unconditionally so unsplit data stops here.
*/
state->split_start++;
+
+ if (!state->split)
+ state->split_start = 0;
break;
case 9:
if (rdev->wiphy.extended_capabilities &&
--
2.19.2
Re: [PATCH] cfg80211: Purge frame registrations on iftype change
Hi Johannes, 'typical' failure paths. I didn't add such checking in the other patch set since I felt you might find it overly intrusive on userspace. But maybe we really should do this? As I just said on the other patch, I think we probably should do that there, if just to be able to advertise a correct set of interface types that you can switch between there. I don't see how it'd be more intrusive to userspace than failing later? :-) What I was worried about was that all the fullmac drivers would have had to be updated to set the feature bit, and it would have caused wpa_s/hostapd to no longer be able to do the whole set_iftype -> ebusy -> ifdown & set_iftype retry logic until all were updated. I would concur as that is what happens today. But should it? Well, dunno, what should happen? If you ask drivers they might want to remove & re-register after, for those registrations that are still possible. I would think you'd want to define a clear order of operations that cfg80211 / mac80211 would enforce :) Let's not then. I've applied this patch now. Great, thanks. Regards, -Denis
Re: [RFCv3 3/3] nl80211: Send large new_wiphy events
hi Johannes,
On 9/11/19 4:47 AM, Johannes Berg wrote:
On Fri, 2019-09-06 at 10:43 -0500, Denis Kenzior wrote:
+ * There are no limits (outside of netlink protocol limits) on
+ * message sizes that can be sent over the "config2" multicast group. It
+ * is assumed that applications utilizing "config2" multicast group
+ * utilize buffers that are inherently large enough or can utilize
+ * MSG_PEEK/MSG_TRUNC in the netlink transport in order to allocate big
+ * enough buffers.
I'm not sure I see how the applications could do buffers that are
"inherently" large enough, there's no practical message size limit, is
there (32-bits for the size).
The kernel caps this to 32k right now if I read the code correctly. But
fair point.
I'd argue this should just say that applications should use large
buffers and still use MSG_PEEK/handle MSG_TRUNC, but I can also edit it
later.
+ msg = nlmsg_new(alloc_size, GFP_KERNEL);
+ if (!msg)
+ goto legacy;
+
+ if (WARN_ON(nl80211_send_wiphy(rdev, cmd, msg, 0, 0, 0, &state) < 0)) {
+ nlmsg_free(msg);
+ goto legacy;
+ }
+
+ genlmsg_multicast_netns(&nl80211_fam, wiphy_net(&rdev->wiphy), msg, 0,
+ NL80211_MCGRP_CONFIG2, GFP_KERNEL);
+
+legacy:
nit: just use "else" instead of the goto?
I'm not sure I understand? We want to send both messages here...
Regards,
-Denis
Re: [RFCv3 2/3] nl80211: Support >4096 byte NEW_WIPHY event nlmsg
Hi Johannes,
On 9/11/19 4:44 AM, Johannes Berg wrote:
Hi,
The first patch looks good, couple of nits/comments on this one below.
On Fri, 2019-09-06 at 10:43 -0500, Denis Kenzior wrote:
For historical reasons, NEW_WIPHY messages generated by dumps or
GET_WIPHY commands were limited to 4096 bytes. This was due to the
fact that the kernel only allocated 4k buffers prior to commit
9063e21fb026 ("netlink: autosize skb lengthes"). Once the sizes of
NEW_WIPHY messages exceeded these sizes, split dumps were introduced.
Actually, userspace prior to around the same time *also* only used 4k
buffers (old libnl), and so even with that kernel we still could
possibly have to deal with userspace that had 4k messages only ... but
we could have solved that part trivially instead of adding code to split
it, just the kernel part was still in the way then.
Anyway, I can reword this per my understanding (but will have to reread
all my messages I guess).
Sure
- The code in case '3' is quite complex, but it does try to support a
message running out of room in the middle of a channel dump and
restarting from where it left off in the next split message. Perhaps
this can be simplified, but it seems this capability is useful.
Please take extra care when reviewing this.
Is it useful? You say it basically all fits today, and that means the
channels will either fit into a single message or not ... Then again, if
we add a lot of channels or a lot more data to each channel. Hmm. OK, I
guess better if we do have it.
For my dual band cards the channel dump all fits into a ~1k attribute if
I recall correctly. So there may not really be a need for this. Or at
the very least we could keep things simple(r) and only split at the band
level, not at the individual channel level.
All the cards I tried would split well after case 9 with 4096 byte
buffers anyway. The channel dump is quite early in the message and it
would really need to become bloated for this code path to be triggered...
+ void *last_good_pos = 0;
Use NULL.
Will fix
+ last_good_pos = nlmsg_get_pos(msg);
state->split_start++;
Maybe we're better off having a local macro for these two lines? That
way, we don't risk updating one without the other, which would be
confusing.
Yep, will do that.
@@ -2004,81 +2004,78 @@ static int nl80211_send_wiphy(struct
cfg80211_registered_device *rdev,
if (!nl_bands)
goto nla_put_failure;
- for (band = state->band_start;
-band < NUM_NL80211_BANDS; band++) {
+ /* Position in the buffer if we added a set of channel info */
+ last_channel_pos = 0;
NULL
Will fix
[snip]
+chan_put_failure:
+ if (!last_channel_pos)
+ goto nla_put_failure;
+
+ nlmsg_trim(msg, last_channel_pos);
+ nla_nest_end(msg, nl_freqs);
nla_nest_end(msg, nl_band);
- if (state->split) {
- /* start again here */
- if (state->chan_start)
- band--;
+ if (state->chan_start < sband->n_channels)
break;
- }
+
+ state->chan_start = 0;
+ state->band_start++;
}
- nla_nest_end(msg, nl_bands);
- if (band < NUM_NL80211_BANDS)
- state->band_start = band + 1;
- else
- state->band_start = 0;
+band_put_failure:
+ if (!last_channel_pos)
+ goto nla_put_failure;
+
+ nla_nest_end(msg, nl_bands);
- /* if bands & channels are done, continue outside */
- if (state->band_start == 0 && state->chan_start == 0)
- state->split_start++;
- if (state->split)
+ if (state->band_start < NUM_NL80211_BANDS)
break;
Thinking out loud, maybe we could simplify this by just having a small
"stack" of nested attributes to end?
I mean, essentially, you have here similar code to the nla_put_failure
label, in that it finishes and sends out the message, except here you
have to end a bunch of nested attributes.
What if we did something like
#define dump_nest_start(msg, attr) ({ \
struct nlattr r = nla_nest_start_noflag(msg, attr); \
BUG_ON(nest_stack_depth >= ARRAY_SIZE(nest_stack); \
nest_stack[nest_stack_depth++] = r; \
r; \
})
#define dump_nest_end(msg, r) do {
Re: [RFCv3 3/3] nl80211: Send large new_wiphy events
Hi Johannes,
On 9/11/19 10:12 AM, Johannes Berg wrote:
On Wed, 2019-09-11 at 07:20 -0500, Denis Kenzior wrote:
I'm not sure I see how the applications could do buffers that are
"inherently" large enough, there's no practical message size limit, is
there (32-bits for the size).
The kernel caps this to 32k right now if I read the code correctly. But
fair point.
The kernel caps this for dumps only, no? We can allocate here ourselves
for multicasting a message as large as we like I think.
Right, but it is set for only 8k at the moment. Anyway, I will take
care of this.
+ if (WARN_ON(nl80211_send_wiphy(rdev, cmd, msg, 0, 0, 0, &state) < 0)) {
+ nlmsg_free(msg);
+ goto legacy;
+ }
+
+ genlmsg_multicast_netns(&nl80211_fam, wiphy_net(&rdev->wiphy), msg, 0,
+ NL80211_MCGRP_CONFIG2, GFP_KERNEL);
+
+legacy:
nit: just use "else" instead of the goto?
I'm not sure I understand? We want to send both messages here...
It's equivalent to:
-
if (WARN_ON(nl80211_send_wiphy(...) < 0)
nlmsg_free(msg);
else
genlmsg_multicast_netns(...);
... code for legacy ...
-
no?
Ah, now I see what you want. Sure I will take care of this in v4.
Regards,
-Denis
Re: [PATCH] nl80211: Support mgmt frame unregistrations
Hi Johannes, On 9/11/19 3:53 AM, Johannes Berg wrote: On Wed, 2019-09-04 at 11:22 -0500, Denis Kenzior wrote: To state another way, it is currently not possible to write a userspace application that utilizes a single nl80211 genl socket, instead it must open multiple genl sockets for multiple wdevs on multiple phys. I don't see how this is too onerous for the application, every application is basically going to have an event loop anyway. What does having an event loop have to do with this? :) Thus, I don't really see any reason for us to add a bunch of code just to make an application track fewer file descriptors - we need to have the cleanup on close already anyway, so why not actually exercise those code paths? I just find this super wasteful. We have instances where we need to register to a single management frame temporarily. So opening and closing a socket just for that is just bloat. I do note that with the "unregister on iftype change" patch you could switch to an unsupported type and reach this, but I don't think you'd want to rely on that :-) Not sure I understand? Possibly I could imagine a reason for this if you needed a single socket for functional reason, but you're not really giving any such reason. I could imagine that there might be races, but I'm having a hard time coming up with a scenario where they actually matter ... if you really really get a race between e.g. RX-AUTH and INTERFACE-DEL you'll try to do some operations that will just fail, but so what? - Waste on the userspace side. Typically userspace uses some sort of abstraction for tracking genl sockets. So it has to allocate buffers, etc. Can we get around that? Sure, but you're not winning any arguments that the nl80211 is 'nice to use' that way. - Waste on the kernel side. Each socket costs something for the kernel, makes things harder to audit, etc, etc. And we now have people trying to stuff 15+ cards into a single system. Each card might have multiple netdevs. Each netdev might need multiple file descriptors open. So we're ending up needing 30-60, or whatever file descriptors when we could just as easily use 1. Extreme case? Sure, but I like to remove bloat whenever / wherever I can. Regards, -Denis
Re: [RFCv3 2/3] nl80211: Support >4096 byte NEW_WIPHY event nlmsg
Hi Johannes, On 9/11/19 10:28 AM, Johannes Berg wrote: On Wed, 2019-09-11 at 07:41 -0500, Denis Kenzior wrote: For my dual band cards the channel dump all fits into a ~1k attribute if I recall correctly. So there may not really be a need for this. Or at the very least we could keep things simple(r) and only split at the band level, not at the individual channel level. Yeah, that does seem reasonable, especially if we're moving to bigger messages anyway. If we do add something huge to each channel, we can recover that code I suppose. So do you want me to drop the channel splitting logic and only allow this for bands? Or just keep this since it is already done? The current logic uses last_channel_pos for some of the trickery in addition to last_good_pos. So nla_put_failure would have to be made aware of that. Perhaps we can store last_good_pos in the stack, but the split mechanism only allows the splits to be done at certain points... Hmm, not sure I understand. last_channel_pos and last_good_pos are basically equivalent, no? In fact, I'm not sure why you need last_channel_pos vs. just using last_good_pos instead? Maybe I'm missing something. Sort of. The way I did it, last_channel_pos keeps track of whether any channel info was added or not. So if NULL, we simply backtrack to last_good_pos in nla_put_failure. You can probably use last_good_pos for everything and an extra variable for the channel info tracking. To me, conceptually, the "state->band_start" and "state->chan_start" is basically a sub-state of "state->split", so this is underneath state- split == 3 (I think?), you basically get 3.0.0, 3.0.1, 3.0.2, ..., 3.1.0, 3.1.1 ... for the state? Which you have to unwind in terms of message formatting, but the last_good_pos should be sufficient? Right. And as I mentioned above, this could be done, but you probably need another state variable.. IOW, the only difference I see between the normal split states 1, 2, ... and the band/channel split states 3.0.0, 3.0.1, ... is the fact that we have to also fix up the nested attributes after we trim to last_good_pos on failures. Where am I wrong? Didn't say that you were ;) Right now only the channel dump uses this (and I'm still not fully convinced we should go to all the trouble), so one argument would be not to introduce something this generic until another user of it manifests itself? I was thinking it'd actually be less complex, but I guess I have to try to write it to see for myself. To be clear, I think it is a good approach and can be made to work. My main hesitation is whether doing it now is worth it given the discussion at the very top. But I can see what I can come up with if you want. Regards, -Denis
Re: [PATCH 04/11] wil6210: fix PTK re-key race
Hi Alexander, I don't know anything about the driver here but in mac80211 the idea to avoid the race is to simply flush the queues prior deleting the outgoing key. Maybe a silly question, but what does flushing the queue mean in this context? Is it waiting for all the packets to be sent or dropping them on the floor? Now wpa_supplicant is not yet bypassing qdisks, but adding the socket parameter PACKET_QDISC_BYPASS is basically a one-liner in wpa_supplicant and should allow a generic way for drivers to avoid the race with a simple queue flush... Can you expand on this actually? What would the sequence of events be? Also, how would this be made to work with CONTROL_PORT over NL80211 ? Regards, -Denis
Re: [PATCH 04/11] wil6210: fix PTK re-key race
Hi Arend, Alexander, Basically, we now have two bypass methods dealing with the same/similar issue: 1) bypass the QDISC. 2) bypass network stack entirely with CONTROL_PORT. It also raises the question in my mind as to why we have two ways of doing the same thing? From the discussion so far it also sounds like each requires somewhat different / special handling in the driver. Wouldn't it make sense to deprecate one and encourage drivers to implement the other? CONTROL_PORT was added specifically to take care of the re-keying races and can be extended with additional attributes over time, as needed (perhaps for extended key id, etc). Also note that in our testing CONTROL_PORT is _way_ faster than PAE socket... Regards, -Denis
Re: [PATCH 04/11] wil6210: fix PTK re-key race
Hi Alexander, And that the intend of it is to replace the "old" path. Correct. So the best way forward here would be to 1) implement the patch here to work around the problem without control_port or the theoretical QDISC bypass 2) start implementing control port for the future. correct? I don't know what the right answer is, but it seems strange to me that we developed a 'better way', upstreamed it several years ago, but are still trying to kludge around adding special flags to what is now considered a legacy approach. Also disconcerting that not a single fullmac driver has added support for this 'better way' yet. CONTROL_PORT was added specifically to take care of the re-keying races and can be extended with additional attributes over time, as needed (perhaps for extended key id, etc). Also note that in our testing CONTROL_PORT is _way_ faster than PAE socket... Extended Key ID is pretty robust when rekeying and the driver/card only has to take care to not mix KeyIDs within one A-MPDU. It's no problem encrypting eapol#4 with the new key. You can even encrypt it at the initial connect and it will work. Basically all races the "classical" rekey has to work around go away. For "normal" rekeys it's working pretty well with ath9k and iwlwifi even without control_port and just learned some weeks ago that QDISC could still cause issues... Okay, if control port doesn't need to handle extended keys then even better. By the way, thanks for your earlier explanation (upthread). Regards, -Denis
Re: [RFC 0/4] Allow live MAC address change
Hi Kalle, For user experience scanning and DHCP are also important, what kind of > numbers you get when those are included? No need to have anything> precise, I would like just to get an understanding where we are> nowadays. Scanning heavily depends on the RF environment and the hardware. In our testing ath9k takes stupid long to scan for example. But in a sort of best case scenario, using limited scan and no mac change, iwd connects in ~300ms. People have reported that they have not finished opening their laptop screen and they're connected, so at that level of latency, every millisecond is important and totally worth fighting for. Randomizing the MAC would penalize our connection times by 2X (300 ms at least). And Android folks have reported the penalty to be as high as 3 seconds. So this needs to be fixed. And do note that this is a feature every modern OS implements by default. As you only provided one number it's clear that you are only working with one driver. But for us it's not that simple, we have to support a Please don't jump to conclusions like you seem to be doing here. James gave you one number that is pretty typical. If you want us to provide numbers for other drivers under given conditions, just ask. We have a framework for timing these. myriad of different types of hardware and there can be complications and additions later on, even for simple features. Like the dynamic power save support I submitted to mac80211 over 10 years which was supposed to be simple, and still we talk almost every year how do we get it out of mac80211 as it makes maintenance difficult. I'm not sure what point you're trying to make here? Regards, -Denis
Re: [PATCH 2/2] mac80211: Support LIVE_ADDRESS_CHANGE feature
Hi Johannes, And, I'm confused, but isn't the polarity of the scanning check wrong? Ah yeah, after you pointed that out I realized 'scanning' is a bit field. I should be doing: test_bit(SCAN_HW_SCANNING, &sdata->local->scanning) I think checking for all the bits is fine (and necessary, just HW scan is unlikely to be enough, changing the MAC address would also disrupt a software scan) - just need to invert the polarity? I concur that scanning should be checked as if (sdata->local->scanning). So Johannes you're right that the polarity is reversed. However, __ieee80211_start_scan seems to check for local->scan_req instead to take deferred scans into account. So I wonder if that is a better approach. Regards, -Denis
Re: [PATCH 2/2] mac80211: Support LIVE_ADDRESS_CHANGE feature
Hi Johannes, On 10/7/19 4:16 PM, Johannes Berg wrote: Hi, If you do care about this being more granular then you should check *which* interface is scanning, and then you can still switch the MAC address for *other* interfaces - but I'd still argue it should be independent of interface type. So yes these can scan, but this should be covered by the netif_carrier_ok check which is done first. Not sure what you mean by that. You could have two interfaces, one which is scanning right now, right? And then theoretically you don't care about the other one - it *should* be OK to remove/re-add (with new MAC address) the one that *isn't* scanning, right? Actually, I don't think you can? Unless I'm missing something? All the scan state is stored on struct ieee80211_local, so if that struct is allocated per phy as you point out below, then what you suggest is currently not possible? But we don't have that granularity here for anything - you're just checking "sdata->local->something", and by going from sdata to local you've now checked the whole NIC, not just a single interface on that NIC. Right. But that seems to be a limitation of mac80211 actually. We can't run two scans concurrently on different interfaces. This is rather unintuitive given that scan requests require an ifindex/wdev. Can this be changed / fixed in mac80211 actually? I would expect that if a card supports p2p and station simultaneously, then it can scan / go offchannel on two interfaces simultaneously? Or not? What can iwlwifi do for example? Which is fine, no complaint from me, just in that case you end up failing when really there isn't much need to fail. In fact, in a case like this, actually clearing IFF_UP, changing address and setting IFF_UP would work, concurrently with another interface scanning. We can just remove the switch entirely, but the roc_list/scanning check only matters for station/p2p_client so checking for the other interface types is kinda pointless and redundant. But it's also completely confusing to do it this way because you go from "sdata" to "local", and at that point the data that you're working on is no longer specific to that one interface, it's actually for the whole NIC. I agree its confusing, but that seems to be how mac80211 works? Basically what I'm saying is this: it's confusing and makes no sense to do something like if (this_is_a_certain_netdev_type) check_some_global_data(); Also I am not sure what you mean by *which* interface. This function is called on a single interface, so checking what other interfaces are doing seems strange... My point exactly - but that's what you're doing here in the code. Now I think perhaps without even realizing? Given the above, I'm not sure I see anything wrong? The switch/case can probably be gotten rid of, but it actually makes things clear that only station/p2p_device and adhoc are handled specially. Regards, -Denis
Re: [PATCH 2/2] mac80211: Support LIVE_ADDRESS_CHANGE feature
Hi Johannes, On 10/8/19 10:52 AM, Johannes Berg wrote: Hi, You could have two interfaces, one which is scanning right now, right? And then theoretically you don't care about the other one - it *should* be OK to remove/re-add (with new MAC address) the one that *isn't* scanning, right? Actually, I don't think you can? Unless I'm missing something? All the scan state is stored on struct ieee80211_local, so if that struct is allocated per phy as you point out below, then what you suggest is currently not possible? ? The scan_req struct contains a reference to which interface is scanning, so it should very well be possible to have phy0: wlan0: IFF_UP & scanning wlan1: IFF_UP & change MAC address all the time just like it's possible to change the MAC address when wlan1 *isn't* IFF_UP even if wlan0 is scanning, right? Indeed. But that is not what you were suggesting earlier with just checking local->scanning. So if scan_req contains a wdev, then yes it should be possible to compare the scan_req->wdev to the interface being changed. But we don't have that granularity here for anything - you're just checking "sdata->local->something", and by going from sdata to local you've now checked the whole NIC, not just a single interface on that NIC. Right. But that seems to be a limitation of mac80211 actually. We can't run two scans concurrently on different interfaces. This is rather unintuitive given that scan requests require an ifindex/wdev. Can this be changed / fixed in mac80211 actually? I would expect that if a card supports p2p and station simultaneously, then it can scan / go offchannel on two interfaces simultaneously? Or not? What can iwlwifi do for example? No, this typically cannot be fixed, and it doesn't really make sense. The NIC cannot possibly do two scans at a time since it has only a single radio resource :-) So why is the scan request not per phy then? And should mac address even affect the ongoing scan? Can we simply change it with the scan ongoing? Given the above, I'm not sure I see anything wrong? The switch/case can probably be gotten rid of, but it actually makes things clear that only station/p2p_device and adhoc are handled specially. I just don't think they *should* be handled specially. Fair enough. Given your code now, you can have phy0: wlan0: STATION, IFF_UP & something is doing remain-on-channel wlan1: STATION, IFF_UP --> cannot change wlan1 MAC address phy0: wlan0: STATION, IFF_UP & something is doing remain-on-channel wlan1: AP, IFF_UP --> *can* change wlan1 MAC address This doesn't really make much sense? Agreed. Regards, -Denis
Re: [PATCH 2/2] mac80211: Support LIVE_ADDRESS_CHANGE feature
Hi Johannes, Indeed. But that is not what you were suggesting earlier with just checking local->scanning. So if scan_req contains a wdev, then yes it should be possible to compare the scan_req->wdev to the interface being changed. Well, yes, but only because I was incrementally going from James's patch, which was checking that only. Well, something to improve. Sometimes it is pretty hard to figure out what you mean. Similar with the other local-> things being checked here, btw, though in some cases it might be harder to actually determine which wdev is doing something and which isn't. Right No, this typically cannot be fixed, and it doesn't really make sense. The NIC cannot possibly do two scans at a time since it has only a single radio resource :-) So why is the scan request not per phy then? And should mac address even affect the ongoing scan? Can we simply change it with the scan ongoing? There are things that affect the scan from the interface, e.g. capability overrides, (extended) capabilities, the MAC address is used unless randomization is requested, etc. But they shouldn't change due a mac address change? I wonder if we can further relax the requirements to allow mac change if NL80211_SCAN_FLAG_RANDOM_ADDR was used? Regards, -Denis
[PATCH] mac80211: More strictly validate .abort_scan
nl80211 requires NL80211_CMD_ABORT_SCAN to have a wdev or netdev
attribute present and checks that if netdev is provided it is UP.
However, mac80211 does not check that an ongoing scan actually belongs
to the netdev/wdev provided by the user. In other words, it is possible
for an application to cancel scans on an interface it doesn't manage.
Signed-off-by: Denis Kenzior
Cc: sta...@vger.kernel.org
---
net/mac80211/cfg.c | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 70739e746c13..ece344f9e9ca 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -2333,7 +2333,13 @@ static int ieee80211_scan(struct wiphy *wiphy,
static void ieee80211_abort_scan(struct wiphy *wiphy, struct wireless_dev
*wdev)
{
- ieee80211_scan_cancel(wiphy_priv(wiphy));
+ struct ieee80211_local *local = wiphy_priv(wiphy);
+ struct ieee80211_sub_if_data *sdata =
+ IEEE80211_WDEV_TO_SUB_IF(wdev);
+ bool cancel_scan = rcu_access_pointer(local->scan_sdata) == sdata;
+
+ if (cancel_scan)
+ ieee80211_scan_cancel(local);
}
static int
--
2.21.0
[PATCH] nl80211: trivial: Remove redundant loop
cfg80211_assign_cookie already checks & prevents a 0 from being
returned, so the explicit loop is unnecessary.
Signed-off-by: Denis Kenzior
---
net/wireless/nl80211.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index d21b1581a665..57bade7ea41c 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -8227,10 +8227,8 @@ static int nl80211_start_sched_scan(struct sk_buff *skb,
/* leave request id zero for legacy request
* or if driver does not support multi-scheduled scan
*/
- if (want_multi && rdev->wiphy.max_sched_scan_reqs > 1) {
- while (!sched_scan_req->reqid)
- sched_scan_req->reqid = cfg80211_assign_cookie(rdev);
- }
+ if (want_multi && rdev->wiphy.max_sched_scan_reqs > 1)
+ sched_scan_req->reqid = cfg80211_assign_cookie(rdev);
err = rdev_sched_scan_start(rdev, dev, sched_scan_req);
if (err)
--
2.21.0
Re: [PATCH 2/2] mac80211: Support LIVE_ADDRESS_CHANGE feature
Hi Johannes, But they shouldn't change due a mac address change? I wonder if we can further relax the requirements to allow mac change if NL80211_SCAN_FLAG_RANDOM_ADDR was used? No, at least with HW scan that would completely confuse the driver - since from the driver's POV we'd remove the interface it's currently managing the scan for. So help me understand this better. Just by virtue of copying the new mac into sdata->vif.addr we'd be confusing the driver such that it can't associate the ongoing scan request to the wdev it was started on? Even when it has all this info? I mean you have a single scan request on a phy, how hard can this be? :) Note that some apps perform poor-man's scan address randomization by varying the mac (I assume prior to each nth scan). So being able to change the mac while scanning might be a boon to them. I personally don't think changing mac via rtnl to accomplish this is a great idea, but just tossing it out for you. Regards, -Denis
Re: [PATCH 2/2] mac80211: Support LIVE_ADDRESS_CHANGE feature
Hi Johannes, On 10/8/19 3:16 PM, Johannes Berg wrote: On Tue, 2019-10-08 at 13:50 -0500, Denis Kenzior wrote: Hi Johannes, But they shouldn't change due a mac address change? I wonder if we can further relax the requirements to allow mac change if NL80211_SCAN_FLAG_RANDOM_ADDR was used? No, at least with HW scan that would completely confuse the driver - since from the driver's POV we'd remove the interface it's currently managing the scan for. So help me understand this better. include/net/mac80211.h: int (*hw_scan)(struct ieee80211_hw *hw, struct ieee80211_vif *vif, struct ieee80211_scan_request *req); How is it difficult to understand that with an API like that, it might not be a good idea to remove the vif from the driver while it's scanning? Right, so you're talking in the context of this implementation which performs a remove/add interface. You're right about that. But I was asking more in general terms. If all we're doing is scanning, can we just change the mac? Anyway, not important. Maybe I bring this up once this set is accepted. Regards, -Denis
[RFC] nl80211: Allow GET_INTERFACE dumps to be filtered
This patch allows GET_INTERFACE dumps to be filtered based on
NL80211_ATTR_WIPHY or NL80211_ATTR_WDEV. The documentation for
GET_INTERFACE mentions that this is possible:
"Request an interface's configuration; either a dump request on
a %NL80211_ATTR_WIPHY or ..."
However, this behavior has not been implemented until now.
---
net/wireless/nl80211.c | 36
1 file changed, 36 insertions(+)
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 244d552..24cb4d9 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -2515,15 +2515,47 @@ static int nl80211_send_iface(struct sk_buff *msg, u32
portid, u32 seq, int flag
return -EMSGSIZE;
}
+static int nl80211_dump_interface_parse(struct sk_buff *skb,
+ struct netlink_callback *cb,
+ int *filter_wiphy)
+{
+ struct nlattr **tb = nl80211_fam.attrbuf;
+ int ret = nlmsg_parse(cb->nlh, GENL_HDRLEN + nl80211_fam.hdrsize,
+ tb, nl80211_fam.maxattr, nl80211_policy);
+ /* ignore parse errors for backward compatibility */
+ if (ret)
+ return 0;
+
+ if (tb[NL80211_ATTR_WIPHY])
+ *filter_wiphy = nla_get_u32(tb[NL80211_ATTR_WIPHY]);
+ if (tb[NL80211_ATTR_WDEV])
+ *filter_wiphy = nla_get_u64(tb[NL80211_ATTR_WDEV]) >> 32;
+
+ return 0;
+}
+
static int nl80211_dump_interface(struct sk_buff *skb, struct netlink_callback
*cb)
{
int wp_idx = 0;
int if_idx = 0;
int wp_start = cb->args[0];
int if_start = cb->args[1];
+ int filter_wiphy = cb->args[2];
struct cfg80211_registered_device *rdev;
struct wireless_dev *wdev;
+ if (!wp_start && !if_start && !filter_wiphy) {
+ int ret;
+
+ filter_wiphy = -1;
+
+ ret = nl80211_dump_interface_parse(skb, cb, &filter_wiphy);
+ if (ret)
+ return ret;
+
+ cb->args[2] = filter_wiphy;
+ }
+
rtnl_lock();
list_for_each_entry(rdev, &cfg80211_rdev_list, list) {
if (!net_eq(wiphy_net(&rdev->wiphy), sock_net(skb->sk)))
@@ -2532,6 +2564,10 @@ static int nl80211_dump_interface(struct sk_buff *skb,
struct netlink_callback *
wp_idx++;
continue;
}
+
+ if (filter_wiphy != -1 && filter_wiphy != rdev->wiphy_idx)
+ continue;
+
if_idx = 0;
list_for_each_entry(wdev, &rdev->wiphy.wdev_list, list) {
--
2.7.3
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] nl80211: Allow GET_INTERFACE dumps to be filtered
This patch allows GET_INTERFACE dumps to be filtered based on
NL80211_ATTR_WIPHY or NL80211_ATTR_WDEV. The documentation for
GET_INTERFACE mentions that this is possible:
"Request an interface's configuration; either a dump request on
a %NL80211_ATTR_WIPHY or ..."
However, this behavior has not been implemented until now.
Signed-off-by: Denis Kenzior
---
net/wireless/nl80211.c | 36
1 file changed, 36 insertions(+)
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 5782f71..f39fd4d 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -2519,15 +2519,47 @@ static int nl80211_send_iface(struct sk_buff *msg, u32
portid, u32 seq, int flag
return -EMSGSIZE;
}
+static int nl80211_dump_interface_parse(struct sk_buff *skb,
+ struct netlink_callback *cb,
+ int *filter_wiphy)
+{
+ struct nlattr **tb = nl80211_fam.attrbuf;
+ int ret = nlmsg_parse(cb->nlh, GENL_HDRLEN + nl80211_fam.hdrsize,
+ tb, nl80211_fam.maxattr, nl80211_policy);
+ /* ignore parse errors for backward compatibility */
+ if (ret)
+ return 0;
+
+ if (tb[NL80211_ATTR_WIPHY])
+ *filter_wiphy = nla_get_u32(tb[NL80211_ATTR_WIPHY]);
+ if (tb[NL80211_ATTR_WDEV])
+ *filter_wiphy = nla_get_u64(tb[NL80211_ATTR_WDEV]) >> 32;
+
+ return 0;
+}
+
static int nl80211_dump_interface(struct sk_buff *skb, struct netlink_callback
*cb)
{
int wp_idx = 0;
int if_idx = 0;
int wp_start = cb->args[0];
int if_start = cb->args[1];
+ int filter_wiphy = cb->args[2];
struct cfg80211_registered_device *rdev;
struct wireless_dev *wdev;
+ if (!wp_start && !if_start && !filter_wiphy) {
+ int ret;
+
+ filter_wiphy = -1;
+
+ ret = nl80211_dump_interface_parse(skb, cb, &filter_wiphy);
+ if (ret)
+ return ret;
+
+ cb->args[2] = filter_wiphy;
+ }
+
rtnl_lock();
list_for_each_entry(rdev, &cfg80211_rdev_list, list) {
if (!net_eq(wiphy_net(&rdev->wiphy), sock_net(skb->sk)))
@@ -2536,6 +2568,10 @@ static int nl80211_dump_interface(struct sk_buff *skb,
struct netlink_callback *
wp_idx++;
continue;
}
+
+ if (filter_wiphy != -1 && filter_wiphy != rdev->wiphy_idx)
+ continue;
+
if_idx = 0;
list_for_each_entry(wdev, &rdev->wiphy.wdev_list, list) {
--
2.7.3
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] nl80211: Allow GET_INTERFACE dumps to be filtered
Hi Johannes, > Looks fine. Thanks for the review. I rebased & resent this patch as a non-RFC. Regards, -Denis -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/5] Improve wireless netdev detection
The current mechanism to detect hot-plug / unplug of wireless devices is somewhat arcane. One has to listen to NEW_WIPHY/DEL_WIPHY events over nl80211 as well as RTM_NEWLINK / RTM_DELLINK events over rtnl, then somehow find a correlation between these events. This involves userspace sending GET_INTERFACE or GET_WIPHY commands to the kernel, which incurs additional roundtrips. This patch series proposes that NEW_INTERFACE and DEL_INTERFACE events are always emitted, regardless of whether a netdev was added/removed by the driver or explicitly via NEW_INTERFACE/DEL_INTERFACE commands. One side effect of this approach is that multiple NEW_INTERFACE/DEL_INTERFACE events might be generated for P2P interfaces. Once when a wdev is created or destroyed, and once when the associated p2p netdev is connecte or disconnected. It is likely that only the caller of P2P oriented NEW_INTERFACE / DEL_INTERFACE commands is interested in the status of these operations. E.g. the caller is / should be using SOCKET_OWNER attribute. Thus one possibility is to not emit NEW_INTERFACE/DEL_INTERFACE events in such cases. Denis Kenzior (5): nl80211: Add nl80211_notify_iface core: Notify of new wireless netdevs nl80211: Emit NEW_INTERFACE only in special cases core: Notify when wireless netdev is removed nl80211: Emit DEL_INTERFACE only in special cases net/wireless/core.c| 4 net/wireless/nl80211.c | 63 ++ net/wireless/nl80211.h | 3 +++ 3 files changed, 55 insertions(+), 15 deletions(-) -- 2.7.3 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/5] nl80211: Emit NEW_INTERFACE only in special cases
For wireless device objects with an associated netdev, the NEW_INTERFACE
event is generated inside the netdev notifier. The event is generated
regardless of whether it was due to a NL80211_CMD_NEW_INTERFACE call
from userspace or if the netdev was created implicitly by the driver.
If NL80211_CMD_NEW_INTERFACE handler is called, then a duplicate
NEW_INTERFACE event will be multicast. This change modifies the logic
inside nl80211_new_interface to avoid this.
---
net/wireless/nl80211.c | 23 +--
1 file changed, 9 insertions(+), 14 deletions(-)
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index da03e17..ec8eb88 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -2787,7 +2787,7 @@ static int nl80211_new_interface(struct sk_buff *skb,
struct genl_info *info)
struct cfg80211_registered_device *rdev = info->user_ptr[0];
struct vif_params params;
struct wireless_dev *wdev;
- struct sk_buff *msg, *event;
+ struct sk_buff *msg;
int err;
enum nl80211_iftype type = NL80211_IFTYPE_UNSPECIFIED;
u32 flags;
@@ -2891,20 +2891,15 @@ static int nl80211_new_interface(struct sk_buff *skb,
struct genl_info *info)
return -ENOBUFS;
}
- event = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
- if (event) {
- if (nl80211_send_iface(event, 0, 0, 0,
- rdev, wdev, false) < 0) {
- nlmsg_free(event);
- goto out;
- }
-
- genlmsg_multicast_netns(&nl80211_fam, wiphy_net(&rdev->wiphy),
- event, 0, NL80211_MCGRP_CONFIG,
- GFP_KERNEL);
- }
+ /*
+* For wdevs which have no associated netdev object (e.g. of type
+* NL80211_IFTYPE_P2P_DEVICE), emit the NEW_INTERFACE event here.
+* For all other types, the event will be generated from the
+* netdev notifier
+*/
+ if (!wdev->netdev)
+ nl80211_notify_iface(rdev, wdev, NL80211_CMD_NEW_INTERFACE);
-out:
return genlmsg_reply(msg, info);
}
--
2.7.3
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 4/5] core: Notify when wireless netdev is removed
---
net/wireless/core.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/net/wireless/core.c b/net/wireless/core.c
index 7758c0f..d35038b 100644
--- a/net/wireless/core.c
+++ b/net/wireless/core.c
@@ -1159,6 +1159,8 @@ static int cfg80211_netdev_notifier_call(struct
notifier_block *nb,
* remove and clean it up.
*/
if (!list_empty(&wdev->list)) {
+ nl80211_notify_iface(rdev, wdev,
+ NL80211_CMD_DEL_INTERFACE);
sysfs_remove_link(&dev->dev.kobj, "phy80211");
list_del_rcu(&wdev->list);
rdev->devlist_generation++;
--
2.7.3
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 5/5] nl80211: Emit DEL_INTERFACE only in special cases
For wireless device objects with an associated netdev, the DEL_INTERFACE
event is generated inside the netdev notifier. The event is generated
regardless of whether it was due to a NL80211_CMD_DEL_INTERFACE call
from userspace or if the netdev was destroyed by other means.
If NL80211_CMD_DEL_INTERFACE handler is called, then a duplicate
DEL_INTERFACE event will be multicast. This change modifies the logic
inside nl80211_del_interface to avoid this.
---
net/wireless/nl80211.c | 12 +++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index ec8eb88..a65c271 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -2913,7 +2913,17 @@ static int nl80211_del_interface(struct sk_buff *skb,
struct genl_info *info)
if (!rdev->ops->del_virtual_intf)
return -EOPNOTSUPP;
- msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
+ /*
+* For wdevs which have no associated netdev object (e.g. of type
+* NL80211_IFTYPE_P2P_DEVICE), emit the DEL_INTERFACE event here.
+* For all other types, the event will be generated from the
+* netdev notifier
+*/
+ if (!wdev->netdev)
+ msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
+ else
+ msg = NULL;
+
if (msg && nl80211_send_iface(msg, 0, 0, 0, rdev, wdev, true) < 0) {
nlmsg_free(msg);
msg = NULL;
--
2.7.3
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/5] nl80211: Add nl80211_notify_iface
This function emits NL80211_CMD_NEW_INTERFACE or
NL80211_CMD_DEL_INTERFACE events. This is meant to be used by the core
to notify userspace applications such as wpa_supplicant when a netdev
related to a wireless device has been added or removed.
---
net/wireless/nl80211.c | 28
net/wireless/nl80211.h | 3 +++
2 files changed, 31 insertions(+)
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index f39fd4d..da03e17 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -11855,6 +11855,34 @@ void nl80211_notify_wiphy(struct
cfg80211_registered_device *rdev,
NL80211_MCGRP_CONFIG, GFP_KERNEL);
}
+void nl80211_notify_iface(struct cfg80211_registered_device *rdev,
+ struct wireless_dev *wdev,
+ enum nl80211_commands cmd)
+{
+ struct sk_buff *msg;
+ bool removal;
+
+ WARN_ON(cmd != NL80211_CMD_NEW_INTERFACE &&
+ cmd != NL80211_CMD_DEL_INTERFACE);
+
+ if (cmd == NL80211_CMD_DEL_INTERFACE)
+ removal = true;
+ else
+ removal = false;
+
+ msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
+ if (!msg)
+ return;
+
+ if (nl80211_send_iface(msg, 0, 0, 0, rdev, wdev, removal) < 0) {
+ nlmsg_free(msg);
+ return;
+ }
+
+ genlmsg_multicast_netns(&nl80211_fam, wiphy_net(&rdev->wiphy), msg, 0,
+ NL80211_MCGRP_CONFIG, GFP_KERNEL);
+}
+
static int nl80211_add_scan_req(struct sk_buff *msg,
struct cfg80211_registered_device *rdev)
{
diff --git a/net/wireless/nl80211.h b/net/wireless/nl80211.h
index a63f402..6f6b73c 100644
--- a/net/wireless/nl80211.h
+++ b/net/wireless/nl80211.h
@@ -7,6 +7,9 @@ int nl80211_init(void);
void nl80211_exit(void);
void nl80211_notify_wiphy(struct cfg80211_registered_device *rdev,
enum nl80211_commands cmd);
+void nl80211_notify_iface(struct cfg80211_registered_device *rdev,
+ struct wireless_dev *wdev,
+ enum nl80211_commands cmd);
void nl80211_send_scan_start(struct cfg80211_registered_device *rdev,
struct wireless_dev *wdev);
struct sk_buff *nl80211_build_scan_msg(struct cfg80211_registered_device *rdev,
--
2.7.3
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/5] core: Notify of new wireless netdevs
--- net/wireless/core.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/net/wireless/core.c b/net/wireless/core.c index 7645e97..7758c0f 100644 --- a/net/wireless/core.c +++ b/net/wireless/core.c @@ -1079,6 +1079,8 @@ static int cfg80211_netdev_notifier_call(struct notifier_block *nb, wdev->iftype == NL80211_IFTYPE_P2P_CLIENT || wdev->iftype == NL80211_IFTYPE_ADHOC) && !wdev->use_4addr) dev->priv_flags |= IFF_DONT_BRIDGE; + + nl80211_notify_iface(rdev, wdev, NL80211_CMD_NEW_INTERFACE); break; case NETDEV_GOING_DOWN: cfg80211_leave(rdev, wdev); -- 2.7.3 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/5] Improve wireless netdev detection
Hi Johannes, On 07/08/2016 05:32 AM, Johannes Berg wrote: On Thu, 2016-07-07 at 02:08 -0500, Denis Kenzior wrote: The current mechanism to detect hot-plug / unplug of wireless devices is somewhat arcane. One has to listen to NEW_WIPHY/DEL_WIPHY events over nl80211 as well as RTM_NEWLINK / RTM_DELLINK events over rtnl, then somehow find a correlation between these events. This involves userspace sending GET_INTERFACE or GET_WIPHY commands to the kernel, which incurs additional roundtrips. This patch series proposes that NEW_INTERFACE and DEL_INTERFACE events are always emitted, regardless of whether a netdev was added/removed by the driver or explicitly via NEW_INTERFACE/DEL_INTERFACE commands. One side effect of this approach is that multiple NEW_INTERFACE/DEL_INTERFACE events might be generated for P2P interfaces. Once when a wdev is created or destroyed, and once when the associated p2p netdev is connecte or disconnected. I think you got some things mixed up. Are you talking of P2P GO/client interfaces, which have netdevs, but are really the same as AP/BSS client and thus the issue here would affect the others? Or are you talking about the P2P-Device wdev? but that has no netdev. Apologies, I've only been looking at the kernel side for several days, so my understanding is still incomplete. I was looking at mac80211/iface.c: ieee80211_if_add() which seems to handle NL80211_IFTYPE_P2P_DEVICE specially by not creating/registering a net_device object. For some reason I thought that this object was registered somewhere later, but my understanding was incorrect. So the entire 'side effect' paragraph above does not apply. Are you okay with the general approach? Are there any locking issues I might be overlooking? It is likely that only the caller of P2P oriented NEW_INTERFACE / DEL_INTERFACE commands is interested in the status of these operations. E.g. the caller is / should be using SOCKET_OWNER attribute. Thus one possibility is to not emit NEW_INTERFACE/DEL_INTERFACE events in such cases. The breaking up of patches is also confusing. You seem to be introducing things in the first, then breaking them again, and then fixing them? Sorry, this was meant to be posted as an RFC. First patch just introduces a notification utility. The rest of the patches were broken up for ease of review. Couldn't the whole thing be done in one or maybe two (new/del) patch(es)? Sure, I can squash them together however you like. (You obviously also need to sign off your patches, see the kernel Documentation/) Apologies, still working the kinks out of my environment setup. Regards, -Denis -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/5] Improve wireless netdev detection
Hi Johannes, >> Are you okay with the general approach? I see no issues with sending these events out. I'd like them to actually be reliable (if present) though, not double as you'd implied - but I didn't really understand in which cases you were expecting issues, was it only P2P-Device? That seems to be the only special case. At least I didn't find any other situations where a NEW_INTERFACE command can be called without a corresponding net_device being created. Are there any locking issues I might be overlooking? Not that I'm aware of. All of the netdev/wdev handling should be protected by RTNL. That was my understanding as well. Thanks. Okay, let me spin up a v2 with patches 2+3 and 4+5 squished together. Or do you want one big patch? Regards, -Denis -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC v2 0/3] Improve wireless netdev detection
The current mechanism to detect hot-plug / unplug of wireless devices is somewhat arcane. One has to listen to NEW_WIPHY/DEL_WIPHY events over nl80211 as well as RTM_NEWLINK / RTM_DELLINK events over rtnl, then somehow find a correlation between these events. This involves userspace sending GET_INTERFACE or GET_WIPHY commands to the kernel, which incurs additional roundtrips. This patch series proposes that NEW_INTERFACE and DEL_INTERFACE events are always emitted, regardless of whether a netdev was added/removed by the driver or explicitly via NEW_INTERFACE/DEL_INTERFACE commands. v2: Squished patches 2+3, 4+5. DEL_INTERFACE event notification is now sent inside cfg80211_unregister_wdev instead of nl80211_del_interface. Denis Kenzior (3): nl80211: Add nl80211_notify_iface core: Always notify of new wireless netdevs core: Always notify when wireless netdev is removed net/wireless/core.c| 6 + net/wireless/nl80211.c | 69 +++--- net/wireless/nl80211.h | 3 +++ 3 files changed, 47 insertions(+), 31 deletions(-) -- 2.7.3 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC v2 1/3] nl80211: Add nl80211_notify_iface
This function emits NL80211_CMD_NEW_INTERFACE or
NL80211_CMD_DEL_INTERFACE events. This is meant to be used by the core
to notify userspace applications such as wpa_supplicant when a netdev
related to a wireless device has been added or removed.
Signed-off-by: Denis Kenzior
---
net/wireless/nl80211.c | 28
net/wireless/nl80211.h | 3 +++
2 files changed, 31 insertions(+)
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index f39fd4d..da03e17 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -11855,6 +11855,34 @@ void nl80211_notify_wiphy(struct
cfg80211_registered_device *rdev,
NL80211_MCGRP_CONFIG, GFP_KERNEL);
}
+void nl80211_notify_iface(struct cfg80211_registered_device *rdev,
+ struct wireless_dev *wdev,
+ enum nl80211_commands cmd)
+{
+ struct sk_buff *msg;
+ bool removal;
+
+ WARN_ON(cmd != NL80211_CMD_NEW_INTERFACE &&
+ cmd != NL80211_CMD_DEL_INTERFACE);
+
+ if (cmd == NL80211_CMD_DEL_INTERFACE)
+ removal = true;
+ else
+ removal = false;
+
+ msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
+ if (!msg)
+ return;
+
+ if (nl80211_send_iface(msg, 0, 0, 0, rdev, wdev, removal) < 0) {
+ nlmsg_free(msg);
+ return;
+ }
+
+ genlmsg_multicast_netns(&nl80211_fam, wiphy_net(&rdev->wiphy), msg, 0,
+ NL80211_MCGRP_CONFIG, GFP_KERNEL);
+}
+
static int nl80211_add_scan_req(struct sk_buff *msg,
struct cfg80211_registered_device *rdev)
{
diff --git a/net/wireless/nl80211.h b/net/wireless/nl80211.h
index a63f402..6f6b73c 100644
--- a/net/wireless/nl80211.h
+++ b/net/wireless/nl80211.h
@@ -7,6 +7,9 @@ int nl80211_init(void);
void nl80211_exit(void);
void nl80211_notify_wiphy(struct cfg80211_registered_device *rdev,
enum nl80211_commands cmd);
+void nl80211_notify_iface(struct cfg80211_registered_device *rdev,
+ struct wireless_dev *wdev,
+ enum nl80211_commands cmd);
void nl80211_send_scan_start(struct cfg80211_registered_device *rdev,
struct wireless_dev *wdev);
struct sk_buff *nl80211_build_scan_msg(struct cfg80211_registered_device *rdev,
--
2.7.3
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC v2 3/3] core: Always notify when wireless netdev is removed
This change alters the semantics of NL80211_CMD_DEL_INTERFACE events
by always sending this event whenever a net_device object associated
with a wdev is destroyed. Prior to this change, this event was only
emitted as a result of NL80211_CMD_DEL_INTERFACE command sent from
userspace. This allows userspace to reliably detect when wireless
interfaces have been removed, e.g. due to USB removal events, etc.
For wireless device objects without an associated net_device (e.g.
NL80211_IFTYPE_P2P_DEVICE), the NL80211_CMD_DEL_INTERFACE event is
now generated inside cfg80211_unregister_wdev.
Signed-off-by: Denis Kenzior
---
net/wireless/core.c| 4
net/wireless/nl80211.c | 18 +-
2 files changed, 5 insertions(+), 17 deletions(-)
diff --git a/net/wireless/core.c b/net/wireless/core.c
index 7758c0f..fccead7 100644
--- a/net/wireless/core.c
+++ b/net/wireless/core.c
@@ -906,6 +906,8 @@ void cfg80211_unregister_wdev(struct wireless_dev *wdev)
if (WARN_ON(wdev->netdev))
return;
+ nl80211_notify_iface(rdev, wdev, NL80211_CMD_DEL_INTERFACE);
+
list_del_rcu(&wdev->list);
rdev->devlist_generation++;
@@ -1159,6 +1161,8 @@ static int cfg80211_netdev_notifier_call(struct
notifier_block *nb,
* remove and clean it up.
*/
if (!list_empty(&wdev->list)) {
+ nl80211_notify_iface(rdev, wdev,
+ NL80211_CMD_DEL_INTERFACE);
sysfs_remove_link(&dev->dev.kobj, "phy80211");
list_del_rcu(&wdev->list);
rdev->devlist_generation++;
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index ec8eb88..bc45d8a 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -2907,18 +2907,10 @@ static int nl80211_del_interface(struct sk_buff *skb,
struct genl_info *info)
{
struct cfg80211_registered_device *rdev = info->user_ptr[0];
struct wireless_dev *wdev = info->user_ptr[1];
- struct sk_buff *msg;
- int status;
if (!rdev->ops->del_virtual_intf)
return -EOPNOTSUPP;
- msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
- if (msg && nl80211_send_iface(msg, 0, 0, 0, rdev, wdev, true) < 0) {
- nlmsg_free(msg);
- msg = NULL;
- }
-
/*
* If we remove a wireless device without a netdev then clear
* user_ptr[1] so that nl80211_post_doit won't dereference it
@@ -2929,15 +2921,7 @@ static int nl80211_del_interface(struct sk_buff *skb,
struct genl_info *info)
if (!wdev->netdev)
info->user_ptr[1] = NULL;
- status = rdev_del_virtual_intf(rdev, wdev);
- if (status >= 0 && msg)
- genlmsg_multicast_netns(&nl80211_fam, wiphy_net(&rdev->wiphy),
- msg, 0, NL80211_MCGRP_CONFIG,
- GFP_KERNEL);
- else
- nlmsg_free(msg);
-
- return status;
+ return rdev_del_virtual_intf(rdev, wdev);
}
static int nl80211_set_noack_map(struct sk_buff *skb, struct genl_info *info)
--
2.7.3
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC v2 2/3] core: Always notify of new wireless netdevs
This change alters the semantics of NL80211_CMD_NEW_INTERFACE events
by always sending this event whenever a new net_device object
associated with a wdev is registered. Prior to this change, this event
was only sent as a result of NL80211_CMD_NEW_INTERFACE command sent
from userspace. This allows userspace to reliably detect new wireless
interfaces (e.g. due to hardware hot-plug events, etc).
For wdevs created without an associated net_device object (e.g.
NL80211_IFTYPE_P2P_DEVICE), the NL80211_CMD_NEW_INTERFACE event is
still generated inside the relevant nl80211 command handler.
Signed-off-by: Denis Kenzior
---
net/wireless/core.c| 2 ++
net/wireless/nl80211.c | 23 +--
2 files changed, 11 insertions(+), 14 deletions(-)
diff --git a/net/wireless/core.c b/net/wireless/core.c
index 7645e97..7758c0f 100644
--- a/net/wireless/core.c
+++ b/net/wireless/core.c
@@ -1079,6 +1079,8 @@ static int cfg80211_netdev_notifier_call(struct
notifier_block *nb,
wdev->iftype == NL80211_IFTYPE_P2P_CLIENT ||
wdev->iftype == NL80211_IFTYPE_ADHOC) && !wdev->use_4addr)
dev->priv_flags |= IFF_DONT_BRIDGE;
+
+ nl80211_notify_iface(rdev, wdev, NL80211_CMD_NEW_INTERFACE);
break;
case NETDEV_GOING_DOWN:
cfg80211_leave(rdev, wdev);
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index da03e17..ec8eb88 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -2787,7 +2787,7 @@ static int nl80211_new_interface(struct sk_buff *skb,
struct genl_info *info)
struct cfg80211_registered_device *rdev = info->user_ptr[0];
struct vif_params params;
struct wireless_dev *wdev;
- struct sk_buff *msg, *event;
+ struct sk_buff *msg;
int err;
enum nl80211_iftype type = NL80211_IFTYPE_UNSPECIFIED;
u32 flags;
@@ -2891,20 +2891,15 @@ static int nl80211_new_interface(struct sk_buff *skb,
struct genl_info *info)
return -ENOBUFS;
}
- event = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
- if (event) {
- if (nl80211_send_iface(event, 0, 0, 0,
- rdev, wdev, false) < 0) {
- nlmsg_free(event);
- goto out;
- }
-
- genlmsg_multicast_netns(&nl80211_fam, wiphy_net(&rdev->wiphy),
- event, 0, NL80211_MCGRP_CONFIG,
- GFP_KERNEL);
- }
+ /*
+* For wdevs which have no associated netdev object (e.g. of type
+* NL80211_IFTYPE_P2P_DEVICE), emit the NEW_INTERFACE event here.
+* For all other types, the event will be generated from the
+* netdev notifier
+*/
+ if (!wdev->netdev)
+ nl80211_notify_iface(rdev, wdev, NL80211_CMD_NEW_INTERFACE);
-out:
return genlmsg_reply(msg, info);
}
--
2.7.3
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 1/3] nl80211: Add nl80211_notify_iface
This function emits NL80211_CMD_NEW_INTERFACE or
NL80211_CMD_DEL_INTERFACE events. This is meant to be used by the core
to notify userspace applications such as wpa_supplicant when a netdev
related to a wireless device has been added or removed.
Signed-off-by: Denis Kenzior
---
net/wireless/nl80211.c | 28
net/wireless/nl80211.h | 3 +++
2 files changed, 31 insertions(+)
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index ac19eb8..c174770 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -11855,6 +11855,34 @@ void nl80211_notify_wiphy(struct
cfg80211_registered_device *rdev,
NL80211_MCGRP_CONFIG, GFP_KERNEL);
}
+void nl80211_notify_iface(struct cfg80211_registered_device *rdev,
+ struct wireless_dev *wdev,
+ enum nl80211_commands cmd)
+{
+ struct sk_buff *msg;
+ bool removal;
+
+ WARN_ON(cmd != NL80211_CMD_NEW_INTERFACE &&
+ cmd != NL80211_CMD_DEL_INTERFACE);
+
+ if (cmd == NL80211_CMD_DEL_INTERFACE)
+ removal = true;
+ else
+ removal = false;
+
+ msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
+ if (!msg)
+ return;
+
+ if (nl80211_send_iface(msg, 0, 0, 0, rdev, wdev, removal) < 0) {
+ nlmsg_free(msg);
+ return;
+ }
+
+ genlmsg_multicast_netns(&nl80211_fam, wiphy_net(&rdev->wiphy), msg, 0,
+ NL80211_MCGRP_CONFIG, GFP_KERNEL);
+}
+
static int nl80211_add_scan_req(struct sk_buff *msg,
struct cfg80211_registered_device *rdev)
{
diff --git a/net/wireless/nl80211.h b/net/wireless/nl80211.h
index a63f402..6f6b73c 100644
--- a/net/wireless/nl80211.h
+++ b/net/wireless/nl80211.h
@@ -7,6 +7,9 @@ int nl80211_init(void);
void nl80211_exit(void);
void nl80211_notify_wiphy(struct cfg80211_registered_device *rdev,
enum nl80211_commands cmd);
+void nl80211_notify_iface(struct cfg80211_registered_device *rdev,
+ struct wireless_dev *wdev,
+ enum nl80211_commands cmd);
void nl80211_send_scan_start(struct cfg80211_registered_device *rdev,
struct wireless_dev *wdev);
struct sk_buff *nl80211_build_scan_msg(struct cfg80211_registered_device *rdev,
--
2.7.3
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
