| Message ID | 20190816192703.12445-4-denkenz@gmail.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | [RFCv2,1/4] nl80211: Fix broken non-split wiphy dumps | expand |
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. 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. And I don't really want to see "config3" a couple of years down the road... 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? 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 > + 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. 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? 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)? johannes
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
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); }
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 <denkenz@gmail.com> --- include/uapi/linux/nl80211.h | 1 + net/wireless/nl80211.c | 28 +++++++++++++++++++++++++++- 2 files changed, 28 insertions(+), 1 deletion(-)