| Message ID | 20190816192703.12445-1-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: > 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")). 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. johannes
On Fri, 2019-08-30 at 11:03 +0200, 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. Actually, going back in time to the code there (e.g. iw and hostap), it seems that it quite possibly never was a userspace issue, just an issue with netlink allocating a 4k SKB by default for dumps. Even then, libnl would've defaulted to a 16k recvmsg() buffer size, and we didn't override that anywhere. So more likely, this was all fixed by kernel 9063e21fb026 ("netlink: autosize skb lengthes") about a year after we ran into the problem. johannes
On Fri, 2019-08-30 at 11:10 +0200, Johannes Berg wrote: > On Fri, 2019-08-30 at 11:03 +0200, 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. > > Actually, going back in time to the code there (e.g. iw and hostap), it > seems that it quite possibly never was a userspace issue, just an issue > with netlink allocating a 4k SKB by default for dumps. > > Even then, libnl would've defaulted to a 16k recvmsg() buffer size, and > we didn't override that anywhere. Ah, also not quite true, at the time it still had a 4k default, until commit 807fddc4cd9e ("nl: Increase receive buffer size to 4 pages") dated May 8, 2013. johannes
On Fri, 2019-08-30 at 11:40 +0200, Johannes Berg wrote: > On Fri, 2019-08-30 at 11:10 +0200, Johannes Berg wrote: > > On Fri, 2019-08-30 at 11:03 +0200, 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. > > > > Actually, going back in time to the code there (e.g. iw and hostap), it > > seems that it quite possibly never was a userspace issue, just an issue > > with netlink allocating a 4k SKB by default for dumps. > > > > Even then, libnl would've defaulted to a 16k recvmsg() buffer size, and > > we didn't override that anywhere. > > Ah, also not quite true, at the time it still had a 4k default, until > commit 807fddc4cd9e ("nl: Increase receive buffer size to 4 pages") > dated May 8, 2013. However, even before that, it would have supported responding to MSG_TRUNC by retrying the recvmsg(), but hostap/iw wouldn't have set nl_socket_enable_msg_peek()... oh well. johannes
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 (0x0000003b) > 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
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 &&
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 (0x00000000) Wiphy Name: phy0 Generation: 1 (0x00000001) <snip> > Result: New Wiphy (0x03) len 68 [multi] Wiphy: 0 (0x00000000) Wiphy Name: phy0 Generation: 1 (0x00000001) 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 (0x00000000) Wiphy Name: phy0 Generation: 1 (0x00000001) > Result: New Wiphy (0x03) len 28 [multi] Wiphy: 0 (0x00000000) Wiphy Name: phy0 Generation: 1 (0x00000001) > Result: New Wiphy (0x03) len 52 [multi] Wiphy: 0 (0x00000000) Wiphy Name: phy0 Generation: 1 (0x00000001) 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 (0x00000000) Wiphy Name: phy0 Generation: 1 (0x00000001) Reserved: len 4 00 00 00 00 .... > Complete: Get Wiphy (0x01) len 4 [multi] Status: 0 Signed-off-by: Denis Kenzior <denkenz@gmail.com> --- net/wireless/nl80211.c | 3 +++ 1 file changed, 3 insertions(+)