Received-SPF: pass (google.com: domain of linux-wireless+bounces-337-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <f31decec-f6a1-472b-8e35-001bbf18b668@quicinc.com>
Date: Sun, 3 Dec 2023 08:48:23 -0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 14/16] wifi: mac80211: Modify type of "changed" variable.
Content-Language: en-US
From: Jeff Johnson <quic_jjohnson@quicinc.com>
To: <gregory.greenman@intel.com>, <johannes@sipsolutions.net>
CC: <linux-wireless@vger.kernel.org>,
        Anjaneyulu
	<pagadala.yesu.anjaneyulu@intel.com>
References: <20230604091128.609335-1-gregory.greenman@intel.com>
 <20230604120651.10354a05eaf1.If19359262fe2728dd523ea6d7c3aa7dc50940411@changeid>
 <4baf4dcd-26e5-47d6-bb17-4e23ccc8c12d@quicinc.com>
In-Reply-To: <4baf4dcd-26e5-47d6-bb17-4e23ccc8c12d@quicinc.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit

On 12/2/2023 9:56 AM, Jeff Johnson wrote:
> 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
> 
> 

Apologies for reporting an issue that has already been (somewhat) fixed
by 6e48ebffc2db ("wifi: mac80211: fix mesh id corruption on 32 bit
systems"). Issue was found internally in a backported kernel and that
fix was not present.

But note that fix did not use DECLARE_BITMAP which I still think is the
right thing to do everywhere we are using bitops.

/jeff