[Sorry, my thunderbird has configuration issue currently] Thanks Johannes for your kindly review. I have aligned the patch to 11BE D2.0 and uploaded a new patch here. https://patchwork.kernel.org/project/linux-wireless/patch/1656991169-25910-1-git-send-email-quic_paulz@xxxxxxxxxxx I replied some info inline following. > > + * Per the description about Neighbor AP Information field about MLD > > + * parameters subfield in section 9.4.2.170.2 of Draft P802.11be_D1.4. > > + * If the reported AP is affiliated with the same MLD of the > > +reporting AP, > > + * the TBTT information is skipped; If the reported AP is affiliated > > +with > > + * the same MLD of the nontransmitted BSSID, the TBTT information is > > +copied > > + * and the MLD ID is changed to 0. > > + * > > + * Return: Length of the element written to @pos */ static size_t > > +cfg80211_handle_rnr_ie_for_mbssid(const struct element *elem, > > + u8 bssid_index, u8 *pos) > > +{ > > + size_t rnr_len; > > + const u8 *rnr, *data, *rnr_end; > > + u8 *rnr_new, *tbtt_info_field; > > + u8 tbtt_type, tbtt_len, tbtt_count; > > + u8 mld_pos, mld_id; > > + u32 i, copy_len; > > + /* The count of TBTT info field whose MLD ID equals to 0 in a > neighbor > > + * AP information field. > > + */ > > + u32 tbtt_info_field_count; > > + /* The total bytes of TBTT info fields whose MLD ID equals to 0 in > > + * current RNR IE. > > + */ > > + u32 tbtt_info_field_len = 0; > > + > > + rnr_new = pos; > > + rnr = (u8 *)elem; > > That's a bit weird, why are you doing manipulations on u8 pointers now? > > Shouldn't the elem struct be more useful? > rnr is the source of memcpy for element ID and length. Also used for count the rnr_end. > > + rnr_len = elem->datalen; > > + rnr_end = rnr + rnr_len + 2; > > + > > + memcpy(pos, rnr, 2); > > + pos += 2; > > That really could be open-coded. And if you have "rnr_new = pos" maybe > use that? > rnr_new is the start position of new rnr element. I have added sanity check in the new patch. > > + data = elem->data; > > + while (data < rnr_end) { > > + tbtt_type = u8_get_bits(data[0], > IEEE80211_TBTT_TYPE_MASK); > > + tbtt_count = u8_get_bits(data[0], > IEEE80211_TBTT_COUNT_MASK); > > + tbtt_len = data[1]; > > You're not checking that any data is present, i.e. that you actually have a > suitable length? > Added sanity check in the new patch for the While condition. > [snip] > it's kind of hard to follow this, to be honest ... maybe that's intrinsic, but > maybe we could do something like > > #define copy(pos, data, len) do { > memcpy(pos, data, len); > pos += len; > data += len; > } while (0) > > or something to simplify? And maybe that should also have a bounds check ... > which I feel are missing quite a bit, not just the one I pointed out above. It is not general. About the following section, it needs to modify the value after copy. I added more comment in the code to explain what it does. Hope the comments could help to understand easier. + /* Copy this TBTT information and change MLD + * to 0 as this reported AP is affiliated with + * the same MLD of the nontransmitted BSSID. + */ + memcpy(pos, data, tbtt_len); + pos[mld_pos] = 0; + data += tbtt_len; + pos += tbtt_len; Thanks, Paul
