Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 23.128.96.34 as permitted sender) client-ip=23.128.96.34;
Message-ID: <08a47f34f85bb64be720e473d5a43193412185a1.camel@sipsolutions.net>
Subject: Re: [PATCH] wifi: mac80211:  work around crash in mlme.c
From:   Johannes Berg <johannes@sipsolutions.net>
To:     greearb@candelatech.com, linux-wireless@vger.kernel.org
Date:   Mon, 23 Oct 2023 20:17:43 +0200
In-Reply-To: <20231023175738.1686631-1-greearb@candelatech.com>
References: <20231023175738.1686631-1-greearb@candelatech.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.48.4 (3.48.4-1.fc38) 
MIME-Version: 1.0
Precedence: bulk

On Mon, 2023-10-23 at 10:57 -0700, greearb@candelatech.com wrote:
> From: Ben Greear <greearb@candelatech.com>
>=20
> Protect from NULL ifmgd->assoc_data in ieee80211_mgd_deauth, crash
> was seen here fairly often in a 32-station test case utilizing
> mtk7922 and be200 radios.  I'm not sure if radio types matters
> though.
>=20
> Signed-off-by: Ben Greear <greearb@candelatech.com>
> ---
>=20
> Patch is for wireless-next tree, bug was likely introduced in
> this release since this crash was not seen in earlier 6.6-rc testing
> nor in 6.5 or earlier.
>=20
> There may be a better way to fix this...

I mean, you're not *actually* suggesting we merge this patch, right?
Right?!

> +++ b/net/mac80211/mlme.c
> @@ -8185,13 +8185,18 @@ int ieee80211_mgd_deauth(struct ieee80211_sub_if_=
data *sdata,

I don't know what you did there, but that file doesn't even have 8k
lines for me.

>  			   "aborting authentication with %pM by local choice (Reason: %u=3D%s=
)\n",
>  			   req->bssid, req->reason_code,
>  			   ieee80211_get_reason_code_string(req->reason_code));

So let me get this straight ... this is the "aborting authentication"
(!) case, but

> -
> -		info.link_id =3D ifmgd->assoc_data->assoc_link_id;

your code is accessing the assoc_data? I'm not surprised that crashes,
but that's in no way what the upstream code looks like?


> +		if (WARN_ON_ONCE((unsigned long)(ifmgd) < 4000 ||
> +				 (unsigned long)(ifmgd->assoc_data) < 4000)) {

You complain that it takes effort to get stuff upstream, but at the same
time this is what you post - you can't have really bad patches and a
fast track into upstream at the same time...

johannes