| Message ID | 20190619223606.4575-3-denkenz@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | [1/3] nl80211: Update uapi for CMD_FRAME_WAIT_CANCEL | expand |
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.
johannes
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
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. > 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. It doesn't matter if you add it as a separate message or not, you can add it to later messages, i.e. anything in or after "case 9" is fine. johannes
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
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? > 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. > And those > events should have all or nearly all info to not bother the kernel going > forward. That's what you wish for, but ... > 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) johannes
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. 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. johannes
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
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
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,
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 <denkenz@gmail.com> --- net/wireless/nl80211.c | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+)