Re: [PATCH v2] cfg80211: modify MLD ID params about RNR element

[Johannes Berg <johannes@sipsolutions.net>]

On Tue, 2022-07-05 at 11:19 +0800, Paul Zhang wrote:
> 
> Signed-off-by: Paul Zhang <quic_paulz@quicinc.com>
> ---
>  include/linux/ieee80211.h |   9 +++
>  net/wireless/scan.c       | 159 ++++++++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 162 insertions(+), 6 deletions(-)
> 
> diff --git a/include/linux/ieee80211.h b/include/linux/ieee80211.h
> index 559b6c6..0e547b5 100644
> --- a/include/linux/ieee80211.h
> +++ b/include/linux/ieee80211.h
> @@ -3996,6 +3996,15 @@ static inline bool for_each_element_completed(const struct element *element,
>  #define IEEE80211_TBTT_INFO_OFFSET_BSSID_BSS_PARAM		9
>  #define IEEE80211_TBTT_INFO_OFFSET_BSSID_SSSID_BSS_PARAM	13
>  
> +/*
> + * TBTT Information field, based on Draft P802.11be_D2.0
> + * section 9.4.2.170.2
> + */
> +#define IEEE80211_TBTT_INFO_BSSID_SSID_BSS_PARAM_PSD		13
> +#define IEEE80211_TBTT_INFO_BSSID_SSID_BSS_PARAM_PSD_MLD_PARAM	16
> +/* TBTT information header(2) + Operating class(1) + Channel number(1) */

might be easier to have a struct for this, and then

> +#define IEEE80211_NBR_AP_INFO_LEN	4

becomes a sizeof()?

This code all makes me nervous again because it's over-the-air parsing
code I don't understand immediately :-)

> +	memcpy(pos, rnr, 2);
> +	pos += 2;
> +	data = elem->data;
> +	while (data + IEEE80211_NBR_AP_INFO_LEN <= rnr_end) {
> +		tbtt_type = u8_get_bits(data[0],
> +					IEEE80211_AP_INFO_TBTT_HDR_TYPE);
> +		tbtt_count = u8_get_bits(data[0],
> +					 IEEE80211_AP_INFO_TBTT_HDR_COUNT);
> +		tbtt_len = data[1];
> +
> +		copy_len = tbtt_len * (tbtt_count + 1) +
> +			   IEEE80211_NBR_AP_INFO_LEN;
> +		if (data + copy_len > rnr_end)
> +			break;
> +
> +		if (tbtt_len >=
> +		    IEEE80211_TBTT_INFO_BSSID_SSID_BSS_PARAM_PSD_MLD_PARAM)
> +			mld_pos =
> +			    IEEE80211_TBTT_INFO_BSSID_SSID_BSS_PARAM_PSD;
> +		else
> +			mld_pos = 0;
> +		/* If MLD params do not exist, copy this neighbor AP
> +		 * information field.
> +		 * Draft P802.11be_D2.0, tbtt_type value 1, 2 and 3
> +		 * are reserved.
> +		 */
> +		if (mld_pos == 0 || tbtt_type != 0) {
> +			memcpy(pos, data, copy_len);
> +			pos += copy_len;
> +			data += copy_len;
> +			continue;
> +		}
> +		/* If MLD params exists, copy the 4 bytes fixed field.
> +		 * tbtt_info_field is used to modify the tbtt_count field later.
> +		 */
> +		memcpy(pos, data, IEEE80211_NBR_AP_INFO_LEN);
> +		tbtt_info_field = pos;
> +		pos += IEEE80211_NBR_AP_INFO_LEN;
> +		data += IEEE80211_NBR_AP_INFO_LEN;
> +
> +		tbtt_info_field_count = 0;
> +		for (i = 0; i < tbtt_count + 1; i++) {
> +			mld_id = data[mld_pos];
> +			/* Refer to Draft P802.11be_D2.0
> +			 * 9.4.2.170.2 Neighbor AP Information field about
> +			 * MLD parameters subfield
> +			 */
> +			if (mld_id == 0) {
> +				/* Skip this TBTT information since this
> +				 * reported AP is affiliated with the same MLD
> +				 * of the reporting AP who sending the frame
> +				 * carrying this element.
> +				 */
> +				tbtt_info_field_len += tbtt_len;
> +				data += tbtt_len;
> +				tbtt_info_field_count++;
> +			} else if (mld_id == bssid_index) {
> +				/* 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;
> +			} else {
> +				/* Just copy this TBTT information */
> +				memcpy(pos, data, tbtt_len);
> +				data += tbtt_len;
> +				pos += tbtt_len;
> +			}
> +		}
> +		if (tbtt_info_field_count == (tbtt_count + 1)) {
> +			/* If all the TBTT informations are skipped, then also
> +			 * revert the 4 bytes fixed field which has been copied.
> +			 */
> +			pos -= IEEE80211_NBR_AP_INFO_LEN;
> +			tbtt_info_field_len += IEEE80211_NBR_AP_INFO_LEN;
> +		} else {
> +			/* Modify the tbtt_count field if some TBTT informations
> +			 * are skipped.
> +			 */
> +			u8p_replace_bits(&tbtt_info_field[0],
> +					 tbtt_count - tbtt_info_field_count,
> +					 IEEE80211_AP_INFO_TBTT_HDR_COUNT);
> +		}
> +	}
> +
> +	/* Sanity check if any parsing issue happens */
> +	if (data != rnr_end)
> +		return 0;

That can only happen in the case of the 'break' above, so maybe just
return 0 immediately there?

johannes