| Message ID | 20190906154303.9303-3-denkenz@gmail.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | [RFCv3,1/3] nl80211: Fix broken non-split wiphy dumps | expand |
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). 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? johannes
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
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
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. > > > + 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? johannes
> Ah, now I see what you want. Sure I will take care of this in v4.
I just remembered this because of Martin Willi's patch, and see now that
I actually just did the first patch of this series again while looking
into it ...
Did you ever make a v4 of this?
johannes
Hi Johannes, On 9/28/20 6:14 AM, Johannes Berg wrote: > >> Ah, now I see what you want. Sure I will take care of this in v4. > > I just remembered this because of Martin Willi's patch, and see now that > I actually just did the first patch of this series again while looking > into it ... > > Did you ever make a v4 of this? No. Got distracted by other work and never revisited this. Regards, -Denis
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_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 @@ -1856,6 +1858,7 @@ struct nl80211_dump_wiphy_state { long start; long split_start, band_start, chan_start; bool legacy; + bool large_message; }; static int nl80211_send_wiphy(struct cfg80211_registered_device *rdev, @@ -2414,7 +2417,7 @@ static int nl80211_send_wiphy(struct cfg80211_registered_device *rdev, nla_put_failure: if ((state->legacy && state->split_start < 9) || - !last_good_pos) { + !last_good_pos || state->large_message) { genlmsg_cancel(msg, hdr); return -EMSGSIZE; } @@ -14732,14 +14735,37 @@ 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); + 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 (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: + state.large_message = false; state.legacy = true; - msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL); + alloc_size = NLMSG_DEFAULT_SIZE; + msg = nlmsg_new(alloc_size, GFP_KERNEL); if (!msg) return; @@ -14767,6 +14793,10 @@ void nl80211_notify_iface(struct cfg80211_registered_device *rdev, return; } + /* We will be sending the same message twice, grab an extra ref */ + msg = skb_get(msg); + 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 | 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.