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

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

[madhuparnabhowmik10]

From: Madhuparna Bhowmik <madhuparnabhowmik10@gmail.com>

list_for_each_entry_rcu() has built-in RCU and lock checking.

Pass cond argument to list_for_each_entry_rcu() to silence
false lockdep warning when CONFIG_PROVE_RCU_LIST is enabled
by default.

Signed-off-by: Madhuparna Bhowmik <madhuparnabhowmik10@gmail.com>
---
 net/mac80211/rx.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index 0e05ff037672..0967bdc75938 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -3547,7 +3547,8 @@ static void ieee80211_rx_cooked_monitor(struct ieee80211_rx_data *rx,
 	skb->pkt_type = PACKET_OTHERHOST;
 	skb->protocol = htons(ETH_P_802_2);
 
-	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;
 
@@ -4114,7 +4115,8 @@ void __ieee80211_check_fast_rx_iface(struct ieee80211_sub_if_data *sdata)
 
 	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)) {
 		if (sdata != sta->sdata &&
 		    (!sta->sdata->bss || sta->sdata->bss != sdata->bss))
 			continue;
-- 
2.17.1

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

[Johannes Berg]

On Sat, 2020-02-22 at 15:48 +0530, madhuparnabhowmik10@gmail.com wrote:
> From: Madhuparna Bhowmik <madhuparnabhowmik10@gmail.com>
> 
> list_for_each_entry_rcu() has built-in RCU and lock checking.
> 
> Pass cond argument to list_for_each_entry_rcu() to silence
> false lockdep warning when CONFIG_PROVE_RCU_LIST is enabled
> by default.

Umm. What warning?

> +++ b/net/mac80211/rx.c
> @@ -3547,7 +3547,8 @@ static void ieee80211_rx_cooked_monitor(struct ieee80211_rx_data *rx,
>  	skb->pkt_type = PACKET_OTHERHOST;
>  	skb->protocol = htons(ETH_P_802_2);
>  
> -	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.
 
> @@ -4114,7 +4115,8 @@ void __ieee80211_check_fast_rx_iface(struct ieee80211_sub_if_data *sdata)
>  
>  	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.

johannes

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

[Madhuparna Bhowmik]

On Sat, Feb 22, 2020 at 01:53:25PM +0100, Johannes Berg wrote:
> On Sat, 2020-02-22 at 15:48 +0530, madhuparnabhowmik10@gmail.com wrote:
> > From: Madhuparna Bhowmik <madhuparnabhowmik10@gmail.com>
> > 
> > list_for_each_entry_rcu() has built-in RCU and lock checking.
> > 
> > Pass cond argument to list_for_each_entry_rcu() to silence
> > false lockdep warning when CONFIG_PROVE_RCU_LIST is enabled
> > by default.
> 
> Umm. What warning?
>
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.

> > +++ b/net/mac80211/rx.c
> > @@ -3547,7 +3547,8 @@ static void ieee80211_rx_cooked_monitor(struct ieee80211_rx_data *rx,
> >  	skb->pkt_type = PACKET_OTHERHOST;
> >  	skb->protocol = htons(ETH_P_802_2);
> >  
> > -	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?

and 
__ieee80211_rx_handle_packet() -> ieee80211_prepare_and_rx_handle() -> ieee80211_invoke_rx_handlers() -> 
ieee80211_rx_handlers_result() -> ieee80211_rx_cooked_monitor()

Here __ieee80211_rx_handle_packet() should be called under
rcu_read_lock protection.
So this trace seems okay and no need to pass any cond.

I may have missed something, please correct me in that case.

> > @@ -4114,7 +4115,8 @@ void __ieee80211_check_fast_rx_iface(struct ieee80211_sub_if_data *sdata)
> >  
> >  	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.

Thank you,
Madhuparna

> johannes
> 

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

[Johannes Berg]

> 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