Re: [PATCH] iwlwifi: mvm: fix double list_add at iwl_mvm_mac_wake_tx_queue (roaming)

From: Johannes Berg <johannes@sipsolutions.net>
To: Jose Ignacio Tornos Martinez <jtornosm@redhat.com>
Cc: gregory.greenman@intel.com, kvalo@kernel.org,
	linux-wireless@vger.kernel.org, pstourac@redhat.com
Subject: Re: [PATCH] iwlwifi: mvm: fix double list_add at iwl_mvm_mac_wake_tx_queue (roaming)
Date: Tue, 14 Mar 2023 15:56:59 +0100	[thread overview]
Message-ID: <18728c67cffad35f827d9a32af34044592254093.camel@sipsolutions.net> (raw)
In-Reply-To: <20230314145209.401875-1-jtornosm@redhat.com>

On Tue, 2023-03-14 at 15:52 +0100, Jose Ignacio Tornos Martinez wrote:
> 
> We have some Bugzilla (i.e. 
> https://bugzilla.redhat.com/show_bug.cgi?id=2152168).
> You only have to create a user for this tool.

Looks like it's not public, and anyway I was mostly asking for a commit
log record, so that's not useful even if I could get access:

"You are not authorized to access bug #2152168."

> > > This can be reproduced with a single script from the station:
> > >     while true; do
> > >         wpa_cli -i wlp3s0 roam 34:13:E8:B1:DB:9A
> > >         sleep 2
> > >         wpa_cli -i wlp3s0 roam 34:13:E8:3C:FB:DB
> > >         sleep 2
> > >     done
> > > And flooding with tx traffic.
> > Oh, nice to have a reproducer.
> It is not immediate but I can reproduce here like this.

Right.

> > Funny thing is, I was _just_ looking at this exact bug, because we were
> > discussing all this concurrency over in
> So more people is struggling this this, good to get the best solution.

Well that was a different issue in different drivers, but then we looked
at it ...

> > While this might fix the issue as far as you could observe, that is
> > clearly not sufficient, since you don't protect the list on the other
> > side, where the items are removed from it again.

> Ok, I thought about that as well but I was not able to find any problem with
> the other side. Anyway, the better the solution is made the better.

Well it doesn't _look_ like it accesses the list, but the
list_del_init(&mvmtxq->list) on the other side will access the list too.

> > Below are the two patches that I've come up with so far, if anyone wants
> > to try them. Please ignore all the extra metadata, I exported this
> > directly from our internal code base.
> Of course, I can test the soutions here in order to be sure.
> Do you prefer I reply with the result here or in the other thread that you have
> commented me before?

Here is fine, thanks!

johannes