Re: [PATCH 14/16] wifi: mac80211: Modify type of "changed" variable.

[Jeff Johnson <quic_jjohnson@quicinc.com>]

On 6/4/2023 2:11 AM, gregory.greenman@intel.com wrote:
> From: Anjaneyulu <pagadala.yesu.anjaneyulu@intel.com>
> 
> Currently, enum ieee80211_bss_change has more than 32 flags.
> Change the type of the corresponding variables from u32 to u64.
> 
> Signed-off-by: Anjaneyulu <pagadala.yesu.anjaneyulu@intel.com>
> Signed-off-by: Gregory Greenman <gregory.greenman@intel.com>
> ---
>  net/mac80211/cfg.c         | 79 ++++++++++++++++++--------------------
>  net/mac80211/chan.c        |  4 +-
>  net/mac80211/ibss.c        | 16 ++++----
>  net/mac80211/ieee80211_i.h | 14 ++++---
>  net/mac80211/iface.c       |  4 +-
>  net/mac80211/main.c        |  4 +-
>  net/mac80211/mesh.c        | 30 ++++++++-------
>  net/mac80211/mesh.h        | 19 ++++-----
>  net/mac80211/mesh_plink.c  | 37 +++++++++---------
>  net/mac80211/mesh_ps.c     |  7 ++--
>  net/mac80211/mlme.c        | 12 +++---
>  net/mac80211/ocb.c         |  4 +-
>  12 files changed, 119 insertions(+), 111 deletions(-)

We are finally at the point where we are testing mesh and this patch is
breaking mesh on 32-bit systems. There seems to be a fundamental issue
with both the original code and the revised code where bitop operations
are being used on bitmaps which have not been defined and sized with
DECLARE_BITMAP.

Note the bitops all use unsigned long * for the bitmap pointer, and
hence it seems important that all the bitmaps being used with these
operations use that same underlying type.

The specific code that is causing us issues is in
ieee80211_mbss_info_change_notify():
	for_each_set_bit(bit, &bits, sizeof(changed) * BITS_PER_BYTE)
		set_bit(bit, ifmsh->mbss_changed);

Here in the current code changed is u64 and ifmsh->mbss_changed is
unsigned long and hence processing a bit > 32 writes outside
ifmsh->mbss_changed on a 32-bit system.

At a minimum struct ieee80211_if_mesh::mbss_changed needs to be properly
sized.

We have validated what I consider to be a quick and dirty change which
modifies struct ieee80211_if_mesh::mbss_changed from unsigned long to u64.

Do you want that change?

Or do you want to propose an encompassing solution that correctly uses
unsigned long * and DECLARE_BOTMAP


/jeff