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
<[email protected]> 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
> <[email protected]> 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
<[email protected]> wrote:
>
> On Wed, 2023-07-12 at 11:05 +0200, Koen Vandeputte wrote:
> > On Wed, Jul 12, 2023 at 10:44 AM Johannes Berg
> > <[email protected]> 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