Received-SPF: pass (google.com: domain of linux-wireless+bounces-796-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <9d2e0cc7ec292a7a1ea26c43ada305f40bc22829.camel@sipsolutions.net>
Subject: Re: [PATCH] wifi: cfg80211: fix bss rbn double erase issue
From: Johannes Berg <johannes@sipsolutions.net>
To: Jiazi Li <jqqlijiazi@gmail.com>
Cc: "David S. Miller" <davem@davemloft.net>, Eric Dumazet
 <edumazet@google.com>,  Jakub Kicinski <kuba@kernel.org>, Paolo Abeni
 <pabeni@redhat.com>, Jiazi Li <jiazi.li@transsion.com>,
 linux-wireless@vger.kernel.org
Date: Thu, 14 Dec 2023 14:13:38 +0100
In-Reply-To: <20231026013528.GA24122@Jiazi.Li>
References: <20231025103304.22082-1-jiazi.li@transsion.com>
	 <b1358d256500d5bb6b3bbc312673c24b54b3674e.camel@sipsolutions.net>
	 <20231026013528.GA24122@Jiazi.Li>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.50.2 (3.50.2-1.fc39) 
Precedence: bulk
MIME-Version: 1.0

> >=20
> > Ok that's bad - so you hit the WARN_ON there? How that? We should fix
> > that too?
> >=20
> Yes, hit this WARN_ON in the test of direct connection between mobile
> phones and PC. Here is the log:
> [ 2741.982362] -----------[ cut here ]-----------
> [ 2741.982446] WARNING: CPU: 6 PID: 2175 at net/wireless/scan.c:1496 cfg8=
0211_update_assoc_bss_entry+0x350/0x378 [cfg80211]

Right, so you can reproduce that - can you find a fix for it?

> I don't know why this is happening yet.

OK ...

We have some basic kunit infrastructure, maybe you can work out
something there.

> > > this bss->rbn will continue to hold
> > > expired data, such as __rd_parent_color.
> >=20
> > Does that matter in any way?
> >=20
> It caused a null pointer issue in rb_erase:

Well, OK, so the thing isn't about it holding a color or 'expired' data
or whatever, it's about it being still on the rbtree, no?

> > > And this bss still in rdev->bss_list, maybe double erase in
> > > __cfg80211_bss_expire later.
> > > Double erase a rbtree node(with expired parent and color data) maybe
> > > corrupt rbtree, so add a in_rbtree flag to fix this issue.
> >=20
> > This seems overly complex - couldn't we just remove it from the list to=
o
> > or something? It's already a case that "should never happen" so ... not
> > sure we need to do something "good"?
> >=20
> Will remove it from list when re-insert fail cause confusion in it's
> refcount? Which could lead to leakage or use-after-free?
> >=20

It's a warn-on anyway, better we leak it than crash?

johannes