netdev - Re: [PATCH] net: mac80211: rx.c: Use built-in RCU list checking

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] [day] [month] [year] [list]

Message-ID: <229913c0b0481c3572032b2f64ce0202f5c66c23.camel@sipsolutions.net>
Date:   Sat, 22 Feb 2020 14:54:47 +0100
From:   Johannes Berg <johannes@...solutions.net>
To:     Madhuparna Bhowmik <madhuparnabhowmik10@...il.com>
Cc:     davem@...emloft.net, linux-wireless@...r.kernel.org,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        joel@...lfernandes.org, frextrite@...il.com,
        linux-kernel-mentees@...ts.linuxfoundation.org, paulmck@...nel.org
Subject: Re: [PATCH] net: mac80211: rx.c: Use built-in RCU list checking


> If list_for_each_entry_rcu() is called from non rcu protection
> i.e without holding rcu_read_lock, but under the protection of
> a different lock then we can pass that as the condition for lockdep checking
> because otherwise lockdep will complain if list_for_each_entry_rcu()
> is used without rcu protection. So, if we do not pass this argument
> (cond) it may lead to false lockdep warnings.

Sure. But what's the specific warning you see?

> > > -	list_for_each_entry_rcu(sdata, &local->interfaces, list) {
> > > +	list_for_each_entry_rcu(sdata, &local->interfaces, list,
> > > +				lockdep_is_held(&rx->local->rx_path_lock)) {
> > >  		if (!ieee80211_sdata_running(sdata))
> > >  			continue;
> > 
> > This is not related at all.
> 
> I analysed the following traces:
> ieee80211_rx_handlers() -> ieee80211_rx_handlers_result() -> ieee80211_rx_cooked_monitor()
> 
> here ieee80211_rx_handlers() is holding the rx->local->rx_path_lock and
> therefore I used this for the cond argument.
> 
>  If this is not right, can you help me in figuring out that which other
>  lock is held?

It's _clearly_ not right, that's the RX spinlock, it has nothing to do
with the interface list.

But I'd have to see the warning. Perhaps the driver you're using is
wrongly calling something in the stack.

> > >  	lockdep_assert_held(&local->sta_mtx);
> > >  
> > > -	list_for_each_entry_rcu(sta, &local->sta_list, list) {
> > > +	list_for_each_entry_rcu(sta, &local->sta_list, list,
> > > +				lockdep_is_held(&local->sta_mtx)) {
> > 
> > And this isn't even a real RCU iteration, since we _must_ hold the mutex
> > here.
> > 
> Yeah exactly, dropping _rcu (use list_for_each_entry()) would be a good option in this case.
> Let me know if that is alright and I will send a new patch with all the
> changes required.

Seems fine, also better to split the patches anyway.

johannes