| Message ID | 20231208063820.25983-1-allen.ye@mediatek.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] wifi: mac80211: Update bssid indicator with real BSS numbers | expand |
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 :)
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]; } if (csa) {