| Message ID | 20200222101831.8001-1-madhuparnabhowmik10@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | net: mac80211: rx.c: Use built-in RCU list checking | expand |
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
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 >
> 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
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;