| Message ID | 20190906154303.9303-2-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 |
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). > findings. E.g. the kernel was at fault for the 4096 byte buffer size > limits and not userspace. Nit: I think most of the time when you write "e.g." ("exempli gratia", "for example") you really mean "i.e." ("id est", "which is"). > - 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. > + void *last_good_pos = 0; Use NULL. > + 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. > @@ -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 > [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 { \ BUG_ON(nest_stack_depth > 0); \ nest_stack_depth--; \ BUG_ON(nest_stack[nest_stack_depth] == r); \ nla_nest_end(msg, r); \ } while (0) or something like that (we probably don't want to use nla_nest_start_noflag() for future attributes, etc. but anyway). Then we could unwind any nesting at the end in the common code at the nla_put_failure label very easily, I think? > + * applications that are not legacy, e.g. ones that requested i.e. :) johannes
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 { \ > BUG_ON(nest_stack_depth > 0); \ > nest_stack_depth--; \ > BUG_ON(nest_stack[nest_stack_depth] == r); \ > nla_nest_end(msg, r); \ > } while (0) > > > or something like that (we probably don't want to use > nla_nest_start_noflag() for future attributes, etc. but anyway). > > Then we could unwind any nesting at the end in the common code at the > nla_put_failure label very easily, I think? I see where you're going with this, I think I do anyway... 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... I think the above could be doable, but the code would be even more complex. 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? Regards, -Denis
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
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. > > > [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 { \ > > BUG_ON(nest_stack_depth > 0); \ > > nest_stack_depth--; \ > > BUG_ON(nest_stack[nest_stack_depth] == r); \ > > nla_nest_end(msg, r); \ > > } while (0) > > > > > > or something like that (we probably don't want to use > > nla_nest_start_noflag() for future attributes, etc. but anyway). > > > > Then we could unwind any nesting at the end in the common code at the > > nla_put_failure label very easily, I think? > > I see where you're going with this, I think I do anyway... I'm not sure I am going anywhere with this ;-) > 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. 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? 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? > 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. johannes
Hi, > > 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? I guess I'm saying I don't really care. If we have it now might as well keep it? Actually, don't we already have up to 240 bytes per channel, counting the channel nesting itself and all the flags and WMM data that can be inside? Am I counting this wrong? If this is right, we really do need to be able to split this, because we have ~60 channels in the 6/7 GHz band ... > > > 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. Right. > > 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 ;) No, you didn't, but I thought I probably was missing something :) > 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. See above - unless I'm doing something completely wrong, each channel with all the WMM data and stuff can really be big, no? (Now, why did we put the WMM data into each channel? Maybe we could've done per band? I think it can differ on the UNII subbands though) johannes
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: nl_bands = nla_nest_start_noflag(msg, @@ -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; + + while (state->band_start < NUM_NL80211_BANDS) { struct ieee80211_supported_band *sband; - sband = rdev->wiphy.bands[band]; + sband = rdev->wiphy.bands[state->band_start]; - if (!sband) + if (!sband) { + state->band_start++; continue; + } - nl_band = nla_nest_start_noflag(msg, band); + nl_band = nla_nest_start_noflag(msg, state->band_start); if (!nl_band) - goto nla_put_failure; + goto band_put_failure; - switch (state->chan_start) { - case 0: - if (nl80211_send_band_rateinfo(msg, sband)) - goto nla_put_failure; - state->chan_start++; - if (state->split) - break; - /* fall through */ - default: - /* add frequencies */ - nl_freqs = nla_nest_start_noflag(msg, - NL80211_BAND_ATTR_FREQS); - if (!nl_freqs) - goto nla_put_failure; + if (!state->chan_start && + nl80211_send_band_rateinfo(msg, sband)) + goto band_put_failure; + + nl_freqs = nla_nest_start_noflag(msg, + NL80211_BAND_ATTR_FREQS); + if (!nl_freqs) + goto band_put_failure; - for (i = state->chan_start - 1; - i < sband->n_channels; - i++) { - nl_freq = nla_nest_start_noflag(msg, - i); - if (!nl_freq) - goto nla_put_failure; + while (state->chan_start < sband->n_channels) { + nl_freq = nla_nest_start_noflag(msg, + state->chan_start); + if (!nl_freq) + goto chan_put_failure; - chan = &sband->channels[i]; + chan = &sband->channels[state->chan_start]; - if (nl80211_msg_put_channel( - msg, &rdev->wiphy, chan, - state->split)) - goto nla_put_failure; + if (nl80211_msg_put_channel(msg, &rdev->wiphy, + chan, + !state->legacy)) + goto chan_put_failure; - nla_nest_end(msg, nl_freq); - if (state->split) - break; - } - if (i < sband->n_channels) - state->chan_start = i + 2; - else - state->chan_start = 0; - nla_nest_end(msg, nl_freqs); + nla_nest_end(msg, nl_freq); + state->chan_start++; + last_channel_pos = nlmsg_get_pos(msg); } +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; + + state->band_start = 0; + + last_good_pos = nlmsg_get_pos(msg); + state->split_start++; /* fall through */ case 4: nl_cmds = nla_nest_start_noflag(msg, @@ -2089,7 +2086,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->legacy) { CMD(crit_proto_start, CRIT_PROTOCOL_START); CMD(crit_proto_stop, CRIT_PROTOCOL_STOP); if (rdev->wiphy.flags & WIPHY_FLAG_HAS_CHANNEL_SWITCH) @@ -2105,9 +2102,8 @@ static int nl80211_send_wiphy(struct cfg80211_registered_device *rdev, #undef CMD nla_nest_end(msg, nl_cmds); + last_good_pos = nlmsg_get_pos(msg); state->split_start++; - if (state->split) - break; /* fall through */ case 5: if (rdev->ops->remain_on_channel && @@ -2123,20 +2119,17 @@ static int nl80211_send_wiphy(struct cfg80211_registered_device *rdev, if (nl80211_send_mgmt_stypes(msg, mgmt_stypes)) goto nla_put_failure; + + last_good_pos = nlmsg_get_pos(msg); state->split_start++; - if (state->split) - break; /* fall through */ case 6: #ifdef CONFIG_PM - if (nl80211_send_wowlan(msg, rdev, state->split)) + if (nl80211_send_wowlan(msg, rdev, !state->legacy)) goto nla_put_failure; - state->split_start++; - if (state->split) - break; -#else - state->split_start++; #endif + last_good_pos = nlmsg_get_pos(msg); + state->split_start++; /* fall through */ case 7: if (nl80211_put_iftypes(msg, NL80211_ATTR_SOFTWARE_IFTYPES, @@ -2144,12 +2137,11 @@ 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->legacy)) goto nla_put_failure; + last_good_pos = nlmsg_get_pos(msg); state->split_start++; - if (state->split) - break; /* fall through */ case 8: if ((rdev->wiphy.flags & WIPHY_FLAG_HAVE_AP_SME) && @@ -2160,10 +2152,10 @@ static int nl80211_send_wiphy(struct cfg80211_registered_device *rdev, features = rdev->wiphy.features; /* * We can only add the per-channel limit information if the - * dump is split, otherwise it makes it too big. Therefore - * only advertise it in that case. + * dump is for a non-legacy message, otherwise it makes it too + * big. Therefore only advertise it in that case. */ - if (state->split) + if (!state->legacy) features |= NL80211_FEATURE_ADVERTISE_CHAN_LIMITS; if (nla_put_u32(msg, NL80211_ATTR_FEATURE_FLAGS, features)) goto nla_put_failure; @@ -2182,19 +2174,19 @@ static int nl80211_send_wiphy(struct cfg80211_registered_device *rdev, /* * Any information below this point is only available to - * applications that can deal with it being split. This - * helps ensure that newly added capabilities don't break - * older tools by overrunning their buffers. - * - * 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. + * applications that are not legacy, e.g. ones that requested + * a split-dump or using large buffers. This helps ensure + * that newly added capabilities don't break older tools by + * overrunning their buffers. */ - state->split_start++; - - if (!state->split) + if (state->legacy) { state->split_start = 0; - break; + break; + } + + last_good_pos = nlmsg_get_pos(msg); + state->split_start++; + /* Fall through */ case 9: if (rdev->wiphy.extended_capabilities && (nla_put(msg, NL80211_ATTR_EXT_CAPA, @@ -2235,8 +2227,9 @@ static int nl80211_send_wiphy(struct cfg80211_registered_device *rdev, nla_nest_end(msg, attr); } + last_good_pos = nlmsg_get_pos(msg); state->split_start++; - break; + /* Fall through */ case 10: if (nl80211_send_coalesce(msg, rdev)) goto nla_put_failure; @@ -2251,8 +2244,9 @@ static int nl80211_send_wiphy(struct cfg80211_registered_device *rdev, rdev->wiphy.max_ap_assoc_sta)) goto nla_put_failure; + last_good_pos = nlmsg_get_pos(msg); state->split_start++; - break; + /* Fall through */ case 11: if (rdev->wiphy.n_vendor_commands) { const struct nl80211_vendor_cmd_info *info; @@ -2287,8 +2281,10 @@ static int nl80211_send_wiphy(struct cfg80211_registered_device *rdev, } nla_nest_end(msg, nested); } + + last_good_pos = nlmsg_get_pos(msg); state->split_start++; - break; + /* Fall through */ case 12: if (rdev->wiphy.flags & WIPHY_FLAG_HAS_CHANNEL_SWITCH && nla_put_u8(msg, NL80211_ATTR_MAX_CSA_COUNTERS, @@ -2329,8 +2325,9 @@ static int nl80211_send_wiphy(struct cfg80211_registered_device *rdev, nla_nest_end(msg, nested); } + last_good_pos = nlmsg_get_pos(msg); state->split_start++; - break; + /* Fall through */ case 13: if (rdev->wiphy.num_iftype_ext_capab && rdev->wiphy.iftype_ext_capab) { @@ -2341,8 +2338,7 @@ static int nl80211_send_wiphy(struct cfg80211_registered_device *rdev, if (!nested) goto nla_put_failure; - for (i = state->capa_start; - i < rdev->wiphy.num_iftype_ext_capab; i++) { + for (i = 0; i < rdev->wiphy.num_iftype_ext_capab; i++) { const struct wiphy_iftype_ext_capab *capab; capab = &rdev->wiphy.iftype_ext_capab[i]; @@ -2361,14 +2357,8 @@ static int nl80211_send_wiphy(struct cfg80211_registered_device *rdev, goto nla_put_failure; nla_nest_end(msg, nested_ext_capab); - if (state->split) - break; } nla_nest_end(msg, nested); - if (i < rdev->wiphy.num_iftype_ext_capab) { - state->capa_start = i + 1; - break; - } } if (nla_put_u32(msg, NL80211_ATTR_BANDS, @@ -2397,14 +2387,16 @@ static int nl80211_send_wiphy(struct cfg80211_registered_device *rdev, goto nla_put_failure; } + last_good_pos = nlmsg_get_pos(msg); state->split_start++; - break; + /* Fall through */ case 14: if (nl80211_send_pmsr_capa(rdev, msg)) goto nla_put_failure; + last_good_pos = nlmsg_get_pos(msg); state->split_start++; - break; + /* Fall through */ case 15: if (rdev->wiphy.akm_suites && nla_put(msg, NL80211_ATTR_AKM_SUITES, @@ -2421,8 +2413,14 @@ static int nl80211_send_wiphy(struct cfg80211_registered_device *rdev, return 0; nla_put_failure: - genlmsg_cancel(msg, hdr); - return -EMSGSIZE; + if ((state->legacy && state->split_start < 9) || + !last_good_pos) { + genlmsg_cancel(msg, hdr); + return -EMSGSIZE; + } + + nlmsg_trim(msg, last_good_pos); + goto finish; } static int nl80211_dump_wiphy_parse(struct sk_buff *skb, @@ -2445,7 +2443,7 @@ static int nl80211_dump_wiphy_parse(struct sk_buff *skb, goto out; } - state->split = tb[NL80211_ATTR_SPLIT_WIPHY_DUMP]; + state->legacy = !tb[NL80211_ATTR_SPLIT_WIPHY_DUMP]; if (tb[NL80211_ATTR_WIPHY]) state->filter_wiphy = nla_get_u32(tb[NL80211_ATTR_WIPHY]); if (tb[NL80211_ATTR_WDEV]) @@ -2526,7 +2524,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) && - !skb->len && !state->split && + !skb->len && state->legacy && cb->min_dump_alloc < 4096) { cb->min_dump_alloc = 4096; state->split_start = 0; @@ -2558,6 +2556,8 @@ static int nl80211_get_wiphy(struct sk_buff *skb, struct genl_info *info) struct cfg80211_registered_device *rdev = info->user_ptr[0]; struct nl80211_dump_wiphy_state state = {}; + state.legacy = true; + msg = nlmsg_new(4096, GFP_KERNEL); if (!msg) return -ENOMEM; @@ -14737,6 +14737,8 @@ void nl80211_notify_wiphy(struct cfg80211_registered_device *rdev, WARN_ON(cmd != NL80211_CMD_NEW_WIPHY && cmd != NL80211_CMD_DEL_WIPHY); + state.legacy = true; + msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL); if (!msg) return;
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 <denkenz@gmail.com> --- 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.