netdev - Re: [PATCH 1/2] mac80211: Add rcu read side critical sections

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 19 Sep 2017 15:35:45 +0300
From:   Ville Syrjälä <ville.syrjala@...ux.intel.com>
To:     Johannes Berg <johannes@...solutions.net>
Cc:     linux-wireless@...r.kernel.org,
        "David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org
Subject: Re: [PATCH 1/2] mac80211: Add rcu read side critical sections

On Mon, Sep 18, 2017 at 10:11:17PM +0200, Johannes Berg wrote:
> > I got the following lockdep warning about the rcu_dereference()s in
> > ieee80211_tx_h_select_key(). After tracing all callers of
> > ieee80211_tx_h_select_key() I discovered that
> > ieee80211_get_buffered_bc()
> > and ieee80211_build_data_template() had the rcu_read_lock/unlock()
> > but
> > three other places did not. So I just blindly added them and made the
> > read side critical section extend as far as the lifetime of 'tx'
> > which
> > is where we seem to be stuffing the rcu protected pointers. No real
> > clue whether this is correct or not.
> 
> Heh.
> 
> I think we should do it in ieee80211_tx_dequeue(),

Oh, I guess I didn't trace the call chains far enough. ieee80211_tx()
does indeed look OK. But unless I made another mistake in my analysis
ieee80211_tx_prepare_skb() is still busted.

> if not even in the
> driver (and document that it's required)
> 
> johannes
> 
> > @@ -3411,6 +3430,8 @@ struct sk_buff *ieee80211_tx_dequeue(struct
> > ieee80211_hw *hw,
> >  	ieee80211_tx_result r;
> >  	struct ieee80211_vif *vif;
> >  
> > +	rcu_read_lock();
> > +
> >  	spin_lock_bh(&fq->lock);
> >  
> >  	if (test_bit(IEEE80211_TXQ_STOP, &txqi->flags))
> > @@ -3513,6 +3534,8 @@ struct sk_buff *ieee80211_tx_dequeue(struct
> > ieee80211_hw *hw,
> >  out:
> >  	spin_unlock_bh(&fq->lock);
> >  
> > +	rcu_read_unlock();
> > 
> 
> i.e. this in itself should be sufficient, though you should probably
> reorder and acquire the spinlock first since that might spin, and you
> want to keep the RCU section minimal (it's trivial here, after all)

Good point. I'll respin with that change.

-- 
Ville Syrjälä
Intel OTC