Re: [PATCH 2/2] mac80211: Fixing Races for skipping tailroom reservation

[Yogesh Ashok Powar <yogeshp@marvell.com>]

On Thu, Jun 16, 2011 at 08:36:24AM -0700, Johannes Berg wrote:
> On Thu, 16 Jun 2011 15:57:08 +0530, Yogesh Ashok Powar wrote:
> > Following warning was observed after the commit
> > aac6af5534fade2b18682a0b9efad1a6c04c34c6
> >
> >>WARNING: at net/mac80211/wpa.c:397 ccmp_encrypt_skb+0xc4/0x1f0
> >
> > Consider a scenario where reserving skb tailroom is skipped
> > because software encryption is not enabled. SW encryption
> > can be disabled because of a) All the keys are hardware
> > planted b) No key has been created.  But, before actual
> > transmit if hardware encryption is disabled or software
> > encryption is started, there will not be enough tailroom
> > space to accommodate the sw crypto's MMIC or IV and
> > WARN_ON will be hit.
> >
> > This race between updations of hw keys and skipping & using
> > tailroom, is fixed by protecting critical regions (code
> > accessing crypto_tx_tailroom_needed_cnt and code from
> > tailroom skipping to skb_put for IV or MMIC) with the
> > spinlock.


> Haha, good joke. You've got to be kidding. NACK. No inserting spinlocks 
> into the TX/RX paths.
Using spinlocks in TX/RX path is not allowed because spinlocks will make
TX/RX path to block and its illegal to block while in an RCU read-side
critical section. I think, I got the joke here :(.


In my opinion, following should be the fix for the races explained above

a) The call synchronize_rcu() should be present before enabling sw
encryption :
	This will make pre-existing RCU readers in RCU readside critical
	section to complete. Or, SKBs which have already skipped
	tailroom to xmit before the end of synchronize_rcu's grace
	period.

b) Avoid tailroom skip check for RCU readside critical sections that begin
inside the grace period :
	This will make new entrants to mandatory allocating tailroom
	space irrespective of sw encryption (or hw encryption with
	generate IV/MMIC flags set) and hence avoid the WARN_ON.


Implementing first part is straight forward.
For later part, RCU readers must be aware of on going grace period. I am
currently not sure of any way to implement this. May be iff atomic read
is allowed inside RCU read-side critical sections, then before entering
the grace period, updater can set a atomic flag and unset it once done;
and RCU reader can check this atomic flag to avoid the
__tailroom skip check__.


Please suggest your opinion.

Thanks
Yogesh