| Message ID | 20230712083841.222607-1-koen.vandeputte@citymesh.com (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | wifi: nl80211: fix mbssid nesting | expand |
On Wed, 2023-07-12 at 10:38 +0200, Koen Vandeputte wrote: > Executing command NL80211_CMD_GET_WIPHY and parsing it's output > natively without libnl shows following attributes as part of > the nl80211 generated netlink message (part 16): > > GetWiphy: Type: 1 > GetWiphy: Type: 2 > GetWiphy: Type: 46 > GetWiphy: Type: 33074 <-- wrong enum value, above MAX also .. That's not wrong, that's just NLA_F_NESTED | NL80211_ATTR_MBSSID_CONFIG, since it *is* in fact a nested attribute. > Switching to nla_nest_start_noflag() which ommits the NLA_F_NESTED > flag (like most other similar functions do) fixes this: > > GetWiphy: Type: 1 > GetWiphy: Type: 2 > GetWiphy: Type: 46 > GetWiphy: Type: 306 <-- correct enum value > GetWiphy: Type: 316 Let's say it _changes_ it, but it doesn't _fix_ it, since it's not broken. Using nla_nest_start_noflag() is a legacy thing, it shouldn't be done any more. You need to update your userspace, I'm not applying this patch. johannes
On Wed, Jul 12, 2023 at 10:44 AM Johannes Berg <johannes@sipsolutions.net> wrote: > > On Wed, 2023-07-12 at 10:38 +0200, Koen Vandeputte wrote: > > Executing command NL80211_CMD_GET_WIPHY and parsing it's output > > natively without libnl shows following attributes as part of > > the nl80211 generated netlink message (part 16): > > > > GetWiphy: Type: 1 > > GetWiphy: Type: 2 > > GetWiphy: Type: 46 > > GetWiphy: Type: 33074 <-- wrong enum value, above MAX also .. > > That's not wrong, that's just NLA_F_NESTED | NL80211_ATTR_MBSSID_CONFIG, > since it *is* in fact a nested attribute. ahha! so one should check on each received attribute if this flag is set? > > > Switching to nla_nest_start_noflag() which ommits the NLA_F_NESTED > > flag (like most other similar functions do) fixes this: > > > > GetWiphy: Type: 1 > > GetWiphy: Type: 2 > > GetWiphy: Type: 46 > > GetWiphy: Type: 306 <-- correct enum value > > GetWiphy: Type: 316 > > Let's say it _changes_ it, but it doesn't _fix_ it, since it's not > broken. noted > > Using nla_nest_start_noflag() is a legacy thing, it shouldn't be done > any more. You need to update your userspace, I'm not applying this > patch. Sure, no problem. I guess all the ones with the noflag so far are using it to avoid breaking legacy stuff? > > johannes > Thanks for the quick review. Highly appreciated!
On Wed, 2023-07-12 at 11:05 +0200, Koen Vandeputte wrote: > On Wed, Jul 12, 2023 at 10:44 AM Johannes Berg > <johannes@sipsolutions.net> wrote: > > > > On Wed, 2023-07-12 at 10:38 +0200, Koen Vandeputte wrote: > > > Executing command NL80211_CMD_GET_WIPHY and parsing it's output > > > natively without libnl shows following attributes as part of > > > the nl80211 generated netlink message (part 16): > > > > > > GetWiphy: Type: 1 > > > GetWiphy: Type: 2 > > > GetWiphy: Type: 46 > > > GetWiphy: Type: 33074 <-- wrong enum value, above MAX also .. > > > > That's not wrong, that's just NLA_F_NESTED | NL80211_ATTR_MBSSID_CONFIG, > > since it *is* in fact a nested attribute. > > ahha! so one should check on each received attribute if this flag is set? Yeah, that indicates that it's nested, without having the out-of-band information that it should be nested. There's also NLA_F_NET_BYTEORDER, so we really only have 14 bits for the type, so you should probably use NLA_TYPE_MASK. > I guess all the ones with the noflag so far are using it to avoid > breaking legacy stuff? Right. When we realized that nla_put_nested() didn't actually put the nested flag it was kind of too late - although a lot of userspace probably would have handled it correctly (if it uses libnl, etc.) But still, we converted it all to nla_put_nested_noflag(), but we really shouldn't be adding new code that calls nla_put_nested_noflag() since by definition new code is new attributes, and then userspace can be updated. Unless you're saying this somehow breaks old userspace that doesn't even understand NL80211_ATTR_MBSSID_CONFIG yet? But that seems unlikely, you have to be prepared to see attributes bigger than what you expected and skip them? johannes
On Wed, Jul 12, 2023 at 12:34 PM Johannes Berg <johannes@sipsolutions.net> wrote: > > On Wed, 2023-07-12 at 11:05 +0200, Koen Vandeputte wrote: > > On Wed, Jul 12, 2023 at 10:44 AM Johannes Berg > > <johannes@sipsolutions.net> wrote: > > > > > > On Wed, 2023-07-12 at 10:38 +0200, Koen Vandeputte wrote: > > > > Executing command NL80211_CMD_GET_WIPHY and parsing it's output > > > > natively without libnl shows following attributes as part of > > > > the nl80211 generated netlink message (part 16): > > > > > > > > GetWiphy: Type: 1 > > > > GetWiphy: Type: 2 > > > > GetWiphy: Type: 46 > > > > GetWiphy: Type: 33074 <-- wrong enum value, above MAX also .. > > > > > > That's not wrong, that's just NLA_F_NESTED | NL80211_ATTR_MBSSID_CONFIG, > > > since it *is* in fact a nested attribute. > > > > ahha! so one should check on each received attribute if this flag is set? > > Yeah, that indicates that it's nested, without having the out-of-band > information that it should be nested. > > > There's also NLA_F_NET_BYTEORDER, so we really only have 14 bits for the > type, so you should probably use NLA_TYPE_MASK. > > > I guess all the ones with the noflag so far are using it to avoid > > breaking legacy stuff? > > Right. When we realized that nla_put_nested() didn't actually put the > nested flag it was kind of too late - although a lot of userspace > probably would have handled it correctly (if it uses libnl, etc.) But > still, we converted it all to nla_put_nested_noflag(), but we really > shouldn't be adding new code that calls nla_put_nested_noflag() since by > definition new code is new attributes, and then userspace can be > updated. > > Unless you're saying this somehow breaks old userspace that doesn't even > understand NL80211_ATTR_MBSSID_CONFIG yet? But that seems unlikely, you > have to be prepared to see attributes bigger than what you expected and > skip them? > Yeah, I've already updated our internal code to filter netlink type using NLA_TYPE_MASK. Works like a charm now. Our codebase was initially made during kernel 3.x time when that filtering was not needed.. and within OpenWRT, the wireless stack got updated to 6.1.x bringing in mbssid netlink from 5.16. So this popped up suddenly :-) Thanks for the patience and sorry for the noise! > johannes
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c index 0da2e6a2a7ea..4d4860f9428d 100644 --- a/net/wireless/nl80211.c +++ b/net/wireless/nl80211.c @@ -2359,7 +2359,7 @@ static int nl80211_put_mbssid_support(struct wiphy *wiphy, struct sk_buff *msg) if (!wiphy->mbssid_max_interfaces) return 0; - config = nla_nest_start(msg, NL80211_ATTR_MBSSID_CONFIG); + config = nla_nest_start_noflag(msg, NL80211_ATTR_MBSSID_CONFIG); if (!config) return -ENOBUFS;
Executing command NL80211_CMD_GET_WIPHY and parsing it's output natively without libnl shows following attributes as part of the nl80211 generated netlink message (part 16): GetWiphy: Type: 1 GetWiphy: Type: 2 GetWiphy: Type: 46 GetWiphy: Type: 33074 <-- wrong enum value, above MAX also .. GetWiphy: Type: 316 Switching to nla_nest_start_noflag() which ommits the NLA_F_NESTED flag (like most other similar functions do) fixes this: GetWiphy: Type: 1 GetWiphy: Type: 2 GetWiphy: Type: 46 GetWiphy: Type: 306 <-- correct enum value GetWiphy: Type: 316 Fixes: dc1e3cb8da8b ("nl80211: MBSSID and EMA support in AP mode") Signed-off-by: Koen Vandeputte <koen.vandeputte@citymesh.com> Cc: Johannes Berg <johannes@sipsolutions.net> Cc: John Crispin <john@phrozen.org> Cc: Aloka Dixit <alokad@codeaurora.org> Cc: stable@vger.kernel.org # 5.16 --- net/wireless/nl80211.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)