[PATCH] mac80211: keep BHs disabled while calling drv_tx_wake_queue()

[PATCH] mac80211: keep BHs disabled while calling drv_tx_wake_queue()

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

Drivers typically expect this, as it's the case for almost all cases
where this is called (i.e. from the TX path). Also, the code in mac80211
itself (if the driver calls ieee80211_tx_dequeue()) expects this as it
uses this_cpu_ptr() without additional protection.

This should fix various reports of the problem:
https://bugzilla.kernel.org/show_bug.cgi?id=204127
https://lore.kernel.org/linux-wireless/CAN5HydrWb3o_FE6A1XDnP1E+xS66d5kiEuhHfiGKkLNQokx13Q@mail.gmail.com/
https://lore.kernel.org/lkml/nycvar.YFH.7.76.1909111238470.473@cbobk.fhfr.pm/

Reported-by: Jiri Kosina <jikos@kernel.org>
Reported-by: Aaron Hill <aa1ronham@gmail.com>
Reported-by: Lukas Redlinger <rel+kernel@agilox.net>
Reported-by: Oleksii Shevchuk <alxchk@gmail.com>
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 net/mac80211/util.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/mac80211/util.c b/net/mac80211/util.c
index 051a02ddcb85..ad1e88958da2 100644
--- a/net/mac80211/util.c
+++ b/net/mac80211/util.c
@@ -273,9 +273,9 @@ static void __ieee80211_wake_txqs(struct ieee80211_sub_if_data *sdata, int ac)
 						&txqi->flags))
 				continue;
 
-			spin_unlock_bh(&fq->lock);
+			spin_unlock(&fq->lock);
 			drv_wake_tx_queue(local, txqi);
-			spin_lock_bh(&fq->lock);
+			spin_lock(&fq->lock);
 		}
 	}
 
-- 
2.20.1

Re: [PATCH] mac80211: keep BHs disabled while calling drv_tx_wake_queue()

[Toke Høiland-Jørgensen]

Johannes Berg <johannes@sipsolutions.net> writes:

> From: Johannes Berg <johannes.berg@intel.com>
>
> Drivers typically expect this, as it's the case for almost all cases
> where this is called (i.e. from the TX path). Also, the code in mac80211
> itself (if the driver calls ieee80211_tx_dequeue()) expects this as it
> uses this_cpu_ptr() without additional protection.
>
> This should fix various reports of the problem:
> https://bugzilla.kernel.org/show_bug.cgi?id=204127
> https://lore.kernel.org/linux-wireless/CAN5HydrWb3o_FE6A1XDnP1E+xS66d5kiEuhHfiGKkLNQokx13Q@mail.gmail.com/
> https://lore.kernel.org/lkml/nycvar.YFH.7.76.1909111238470.473@cbobk.fhfr.pm/
>
> Reported-by: Jiri Kosina <jikos@kernel.org>
> Reported-by: Aaron Hill <aa1ronham@gmail.com>
> Reported-by: Lukas Redlinger <rel+kernel@agilox.net>
> Reported-by: Oleksii Shevchuk <alxchk@gmail.com>
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> ---
>  net/mac80211/util.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/net/mac80211/util.c b/net/mac80211/util.c
> index 051a02ddcb85..ad1e88958da2 100644
> --- a/net/mac80211/util.c
> +++ b/net/mac80211/util.c
> @@ -273,9 +273,9 @@ static void __ieee80211_wake_txqs(struct ieee80211_sub_if_data *sdata, int ac)
>  						&txqi->flags))
>  				continue;
>  
> -			spin_unlock_bh(&fq->lock);
> +			spin_unlock(&fq->lock);
>  			drv_wake_tx_queue(local, txqi);
> -			spin_lock_bh(&fq->lock);
> +			spin_lock(&fq->lock);

Okay, so this will mean that the drv_wake_tx_queue() entry point will be
called with bhs disabled. But there are lots of uses of
spin_{,un}lock_bh() in tx.c:

$ git grep lock_bh tx.c
tx.c:   spin_lock_bh(&fq->lock);
tx.c:   spin_unlock_bh(&fq->lock);
tx.c:   spin_lock_bh(&fq->lock);
tx.c:   spin_unlock_bh(&fq->lock);
tx.c:   spin_lock_bh(&fq->lock);
tx.c:   spin_unlock_bh(&fq->lock);
tx.c:   spin_lock_bh(&local->active_txq_lock[txqi->txq.ac]);
tx.c:   spin_unlock_bh(&local->active_txq_lock[txqi->txq.ac]);
tx.c:           spin_lock_bh(&fq->lock);
tx.c:           spin_unlock_bh(&fq->lock);
tx.c:   spin_lock_bh(&fq->lock);
tx.c:   spin_unlock_bh(&fq->lock);
tx.c:   spin_lock_bh(&sta->lock);
tx.c:   spin_unlock_bh(&sta->lock);
tx.c:   spin_lock_bh(&sta->lock);
tx.c:   spin_unlock_bh(&sta->lock);
tx.c:   spin_lock_bh(&fq->lock);
tx.c:   spin_unlock_bh(&fq->lock);
tx.c:   spin_lock_bh(&fq->lock);
tx.c:   spin_unlock_bh(&fq->lock);
tx.c:                   spin_lock_bh(&fq->lock);
tx.c:                   spin_unlock_bh(&fq->lock);
tx.c:   spin_unlock_bh(&fq->lock);
tx.c:   spin_lock_bh(&local->active_txq_lock[ac]);
tx.c:   spin_unlock_bh(&local->active_txq_lock[ac]);
tx.c:   spin_lock_bh(&local->active_txq_lock[txq->ac]);
tx.c:   spin_unlock_bh(&local->active_txq_lock[txq->ac]);
tx.c:   spin_lock_bh(&local->active_txq_lock[ac]);
tx.c:   spin_unlock_bh(&local->active_txq_lock[ac]);
tx.c:   spin_unlock_bh(&local->active_txq_lock[ac]);
tx.c:   spin_lock_bh(&local->active_txq_lock[ac]);
tx.c:   spin_unlock_bh(&local->active_txq_lock[ac]);
tx.c:           spin_lock_bh(&local->tim_lock);
tx.c:           spin_unlock_bh(&local->tim_lock);

so won't that mean that the driver still gets bhs re-enabled after (for
instance) the first call to ieee80211_tx_dequeue()?

I must admit that I'm a bit fuzzy on this whole bh/not-bh thing; I've
mostly been cargo-culting the _bh variant of the locking... :)

-Toke

Re: [PATCH] mac80211: keep BHs disabled while calling drv_tx_wake_queue()

[Johannes Berg]

On Tue, 2019-10-01 at 12:53 +0200, Toke Høiland-Jørgensen wrote:
> 
> > -			spin_unlock_bh(&fq->lock);
> > +			spin_unlock(&fq->lock);
> >  			drv_wake_tx_queue(local, txqi);
> > -			spin_lock_bh(&fq->lock);
> > +			spin_lock(&fq->lock);
> 
> Okay, so this will mean that the drv_wake_tx_queue() entry point will be
> called with bhs disabled.

Right.

>  But there are lots of uses of
> spin_{,un}lock_bh() in tx.c:

[snip]

> so won't that mean that the driver still gets bhs re-enabled after (for
> instance) the first call to ieee80211_tx_dequeue()?

No, local_bh_disable()/local_bh_enable() is re-entrant and nests fine.

johannes

Re: [PATCH] mac80211: keep BHs disabled while calling drv_tx_wake_queue()

[Toke Høiland-Jørgensen]

Johannes Berg <johannes@sipsolutions.net> writes:

> On Tue, 2019-10-01 at 12:53 +0200, Toke Høiland-Jørgensen wrote:
>> 
>> > -			spin_unlock_bh(&fq->lock);
>> > +			spin_unlock(&fq->lock);
>> >  			drv_wake_tx_queue(local, txqi);
>> > -			spin_lock_bh(&fq->lock);
>> > +			spin_lock(&fq->lock);
>> 
>> Okay, so this will mean that the drv_wake_tx_queue() entry point will be
>> called with bhs disabled.
>
> Right.
>
>>  But there are lots of uses of
>> spin_{,un}lock_bh() in tx.c:
>
> [snip]
>
>> so won't that mean that the driver still gets bhs re-enabled after (for
>> instance) the first call to ieee80211_tx_dequeue()?
>
> No, local_bh_disable()/local_bh_enable() is re-entrant and nests fine.

Ah, right, gotcha. Hmm, in that case, would it be more clear to just add
an outer local_bh_disable()/local_bh_enable() to
___ieee80211_wake_txqs(). With this patch we're relying on the
mismatched use of _bh/non-_bh variants of the locking to ensure the bhs
stay off. Isn't that prone to breaking in the future?

Oh, and also, with just this patch, the additional drv_wake_tx_queue()
call for the vif TXQ at the bottom of __ieee80211_wake_txqs() will still
happen without bhs disabled, won't it?

-Toke

Re: [PATCH] mac80211: keep BHs disabled while calling drv_tx_wake_queue()

[Jiri Kosina]

On Tue, 1 Oct 2019, Toke Høiland-Jørgensen wrote:

> > -			spin_unlock_bh(&fq->lock);
> > +			spin_unlock(&fq->lock);
> >  			drv_wake_tx_queue(local, txqi);
> > -			spin_lock_bh(&fq->lock);
> > +			spin_lock(&fq->lock);
> 
> Okay, so this will mean that the drv_wake_tx_queue() entry point will be
> called with bhs disabled. But there are lots of uses of
> spin_{,un}lock_bh() in tx.c:

I am fine with whatever fix for this you guys settle on :) Just for the 
record, I proposed this back then:

	http://lore.kernel.org/r/nycvar.YFH.7.76.1904151300160.9803@cbobk.fhfr.pm

maybe it could be resurected, as I believe it'd fix this one as well?

But again, I have absolutely no preference either way, only that it'd be 
nice to have this fixed :)

Thanks!

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH] mac80211: keep BHs disabled while calling drv_tx_wake_queue()

[Johannes Berg]

On Tue, 2019-10-01 at 12:56 +0200, Jiri Kosina wrote:
> On Tue, 1 Oct 2019, Toke Høiland-Jørgensen wrote:
> 
> > > -			spin_unlock_bh(&fq->lock);
> > > +			spin_unlock(&fq->lock);
> > >  			drv_wake_tx_queue(local, txqi);
> > > -			spin_lock_bh(&fq->lock);
> > > +			spin_lock(&fq->lock);
> > 
> > Okay, so this will mean that the drv_wake_tx_queue() entry point will be
> > called with bhs disabled. But there are lots of uses of
> > spin_{,un}lock_bh() in tx.c:
> 
> I am fine with whatever fix for this you guys settle on :) Just for the 
> record, I proposed this back then:
> 
> 	http://lore.kernel.org/r/nycvar.YFH.7.76.1904151300160.9803@cbobk.fhfr.pm
> 
> maybe it could be resurected, as I believe it'd fix this one as well?

Yes, it would, but it wouldn't address any other driver that likely has
the same issue :)

johannes