[v2] wifi: mac80211: Update bssid indicator with real BSS numbers

The cnt member in mbssid is the count of total number of MBSSID elements
instead of BSSID. Therefore, we fix this by reading the MaxBSSID Indicator
field directly.

Co-developed-by: Evelyn Tsai <evelyn.tsai@mediatek.com>
Signed-off-by: Evelyn Tsai <evelyn.tsai@mediatek.com>
Co-developed-by: Money Wang <money.wang@mediatek.com>
Signed-off-by: Money Wang <money.wang@mediatek.com>
Signed-off-by: Allen Ye <allen.ye@mediatek.com>
---
v2:
  - Fix From and s-o-b format error
---
 net/mac80211/cfg.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Hi,

We should have Lorenzo here, he wrote the original code.

On Fri, 2023-12-08 at 14:38 +0800, Allen Ye wrote:
> The cnt member in mbssid is the count of total number of MBSSID elements
> instead of BSSID. Therefore, we fix this by reading the MaxBSSID Indicator
> field directly.

I'll say I don't understand this much ...

Are you trying to have BSSIDs that are hidden from the kernel? Or not
contiguous in the MBSSID set? Not sure how the two can be not
equivalent?

> Co-developed-by: Evelyn Tsai <evelyn.tsai@mediatek.com>
> Signed-off-by: Evelyn Tsai <evelyn.tsai@mediatek.com>
> Co-developed-by: Money Wang <money.wang@mediatek.com>
> Signed-off-by: Money Wang <money.wang@mediatek.com>
> Signed-off-by: Allen Ye <allen.ye@mediatek.com>

I have to admit that I chuckled a bit about this for a 5 line patch :-)

> diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
> index 606b1b2e4123..f90bcd59f85a 100644
> --- a/net/mac80211/cfg.c
> +++ b/net/mac80211/cfg.c
> @@ -1164,9 +1164,11 @@ ieee80211_assign_beacon(struct ieee80211_sub_if_data *sdata,
>  	/* copy in optional mbssid_ies */
>  	if (mbssid) {
>  		u8 *pos = new->tail + new->tail_len;
> +		const struct element *mbssid_elem;
>  
>  		new->mbssid_ies = (void *)pos;
>  		pos += struct_size(new->mbssid_ies, elem, mbssid->cnt);
> +		mbssid_elem = (const struct element *)pos;
>  		pos += ieee80211_copy_mbssid_beacon(pos, new->mbssid_ies,
>  						    mbssid);
>  		if (rnr) {
> @@ -1175,8 +1177,7 @@ ieee80211_assign_beacon(struct ieee80211_sub_if_data *sdata,
>  			ieee80211_copy_rnr_beacon(pos, new->rnr_ies, rnr);
>  		}
>  		/* update bssid_indicator */
> -		link_conf->bssid_indicator =
> -			ilog2(__roundup_pow_of_two(mbssid->cnt + 1));
> +		link_conf->bssid_indicator = mbssid_elem->data[0];

But this seems fishy to me, if you look into the element itself, you're
going to have to do some validation on it? And what about fragmentation?

johannes

On Thu, 2023-12-14 at 13:29 +0100, Johannes Berg wrote:
>  
>  Hi,
> 
> We should have Lorenzo here, he wrote the original code.
> 
> On Fri, 2023-12-08 at 14:38 +0800, Allen Ye wrote:
> > The cnt member in mbssid is the count of total number of MBSSID
> elements
> > instead of BSSID. Therefore, we fix this by reading the MaxBSSID
> Indicator
> > field directly.
> 
> I'll say I don't understand this much ...
> 
> Are you trying to have BSSIDs that are hidden from the kernel? Or not
> contiguous in the MBSSID set? Not sure how the two can be not
> equivalent?
> 
Hi Johannes,

The number of BSS is not equal to MBSSID element count plus 1 because
there might be multiple nontransmitted BSSID profile subelements in one
MBSSID element. Also, one nontransmitted BSSID profile subelement might
be fragmented across two MBSSID elements if the length of the
subelement exceeds 255 octets.

> > Co-developed-by: Evelyn Tsai <evelyn.tsai@mediatek.com>
> > Signed-off-by: Evelyn Tsai <evelyn.tsai@mediatek.com>
> > Co-developed-by: Money Wang <money.wang@mediatek.com>
> > Signed-off-by: Money Wang <money.wang@mediatek.com>
> > Signed-off-by: Allen Ye <allen.ye@mediatek.com>
> 
> I have to admit that I chuckled a bit about this for a 5 line patch
> :-)
> 
> > diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
> > index 606b1b2e4123..f90bcd59f85a 100644
> > --- a/net/mac80211/cfg.c
> > +++ b/net/mac80211/cfg.c
> > @@ -1164,9 +1164,11 @@ ieee80211_assign_beacon(struct
> ieee80211_sub_if_data *sdata,
> >  /* copy in optional mbssid_ies */
> >  if (mbssid) {
> >  u8 *pos = new->tail + new->tail_len;
> > +const struct element *mbssid_elem;
> >  
> >  new->mbssid_ies = (void *)pos;
> >  pos += struct_size(new->mbssid_ies, elem, mbssid->cnt);
> > +mbssid_elem = (const struct element *)pos;
> >  pos += ieee80211_copy_mbssid_beacon(pos, new->mbssid_ies,
> >      mbssid);
> >  if (rnr) {
> > @@ -1175,8 +1177,7 @@ ieee80211_assign_beacon(struct
> ieee80211_sub_if_data *sdata,
> >  ieee80211_copy_rnr_beacon(pos, new->rnr_ies, rnr);
> >  }
> >  /* update bssid_indicator */
> > -link_conf->bssid_indicator =
> > -ilog2(__roundup_pow_of_two(mbssid->cnt + 1));
> > +link_conf->bssid_indicator = mbssid_elem->data[0];
> 
> But this seems fishy to me, if you look into the element itself,
> you're
> going to have to do some validation on it? And what about
> fragmentation?
> 
> johannes

Whether the subelement is aggregated or fragmented, the MaxBSSID
Indicator field would be the same for the multiple BSSID set.
Therefore, we directly retrieve this field from the element.
For example, there are five BSSes in one multiple BSSID set, one
transmitted and four non-transmitted BSSes. We might use just two
MBSSID elements to store all the non-transmitted BSS information. Here
the MaxBSSID Indicator is 3 in both MBSSID elements. However, the
element cnt is 2 and if we use the original method to calculate the
BSSID Indicator we will get 2 which is wrong.

Thanks,
Allen

On Wed, 2023-12-27 at 09:38 +0000, Allen Ye (葉芷勳) wrote:
> > 
> > We should have Lorenzo here, he wrote the original code.

Actually John/Aloka at least also did ... so whatever.


All this only _really_ matters I think when you have EMA, right?


> The number of BSS is not equal to MBSSID element count plus 1 because
> there might be multiple nontransmitted BSSID profile subelements in one
> MBSSID element.

Yes.

Actually it's even more confusing:

> Also, one nontransmitted BSSID profile subelement might
> be fragmented across two MBSSID elements if the length of the
> subelement exceeds 255 octets.

True; however, in this case, a single entry in the
NL80211_ATTR_MBSSID_ELEMS array (and thus in mbssid_ies) must contain
both MBSSID elements together, so it's only counted once still. More
importantly, otherwise we would split them across two frames with EMA,
which is wrong.

So really mbssid_ies / NL80211_ATTR_MBSSID_ELEMS is *not* the list of
MBSSID elements as they should appear in the frame, individually, but an
array of *sets* of MBSSID elements.

For example:

mbssid_ies[0] = mbssid_elem(profile1, profile2), mbssid_elem(profile3)
mbssid_ies[1] = mbssid_elem(profile4_part1), mbssid_elem(profile4_part2)

when you have EMA with periodicity 2, and 4 non-transmitted BSSes where
the first two are small and fit into one element, number 3 is bigger and
needs its own, and number 4 needs to be split ...


> > But this seems fishy to me, if you look into the element itself,
> > you're going to have to do some validation on it?

I stand by this though.

> > And what about fragmentation?

But not this :)

> Whether the subelement is aggregated or fragmented, the MaxBSSID
> Indicator field would be the same for the multiple BSSID set.

Right.

> Therefore, we directly retrieve this field from the element.

Yeah, makes sense.

> For example, there are five BSSes in one multiple BSSID set, one
> transmitted and four non-transmitted BSSes. We might use just two
> MBSSID elements to store all the non-transmitted BSS information. Here
> the MaxBSSID Indicator is 3 in both MBSSID elements. However, the
> element cnt is 2 and if we use the original method to calculate the
> BSSID Indicator we will get 2 which is wrong.

Right.

Anyway, I think I agree, but can you please add some validation of this
to cfg80211 as a first patch, and also document all this better in the
commit message? Optional, but some additional nl80211 documention would
be very nice.

Thanks,
johannes

On Tue, 2024-04-23 at 13:21 +0200, Johannes Berg wrote:
> 
> Anyway, I think I agree, but can you please add some validation of this
> to cfg80211 as a first patch

I guess I should say what kind of validation? I think it'd make sense to
ensure that the elements even exist/are long enough (currently there's
no validation in nl80211_parse_mbssid_elems at all!!), perhaps call
validate_ie_attr() there as well.

Feels like something should also ensure that not only is

	config->index < wiphy->mbssid_max_interfaces

but also actually < 2^max_bssid_indicator?

johannes

On 12/7/2023 10:38 PM, Allen Ye wrote:
> The cnt member in mbssid is the count of total number of MBSSID elements
> instead of BSSID. Therefore, we fix this by reading the MaxBSSID Indicator
> field directly.

Commit text is much more readable if you somewhat follow the guidance:
Describe what the current code does.
Describe the problem with the current code.
Describe how to modify the code to fix the problem.

And write this at a high enough level that a manager can understand :)

On 4/23/2024 4:28 AM, Johannes Berg wrote:
> On Tue, 2024-04-23 at 13:21 +0200, Johannes Berg wrote:
>>
>> Anyway, I think I agree, but can you please add some validation of this
>> to cfg80211 as a first patch
> 
> I guess I should say what kind of validation? I think it'd make sense to
> ensure that the elements even exist/are long enough (currently there's
> no validation in nl80211_parse_mbssid_elems at all!!), perhaps call
> validate_ie_attr() there as well.
> 
> Feels like something should also ensure that not only is
> 
> 	config->index < wiphy->mbssid_max_interfaces
> 
> but also actually < 2^max_bssid_indicator?
> 
> johannes
> 

I agree with the validation concerns.
But the actual logic in this patch is valid, although considering we 
have had this code for so many years now, feels like no driver/target 
actually needs this field yet even though it is used :-)