| Message ID | 20200219091102.10709-1-frextrite@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | cfg80211: Pass lockdep expression to RCU lists | expand |
On Wed, 2020-02-19 at 14:41 +0530, Amol Grover wrote: > > - WARN_ON_ONCE(!rcu_read_lock_held() && !lockdep_rtnl_is_held()); > - > - list_for_each_entry_rcu(pos, &rdev->sched_scan_req_list, list) { > + list_for_each_entry_rcu(pos, &rdev->sched_scan_req_list, list, > + lockdep_rtnl_is_held()) { Huh, I didn't even know you _could_ do that :) johannes
On Wed, Feb 19, 2020 at 10:13:36AM +0100, Johannes Berg wrote: > On Wed, 2020-02-19 at 14:41 +0530, Amol Grover wrote: > > > > - WARN_ON_ONCE(!rcu_read_lock_held() && !lockdep_rtnl_is_held()); > > - > > - list_for_each_entry_rcu(pos, &rdev->sched_scan_req_list, list) { > > + list_for_each_entry_rcu(pos, &rdev->sched_scan_req_list, list, > > + lockdep_rtnl_is_held()) { > > Huh, I didn't even know you _could_ do that :) It is a fairly recent addition, courtesy of Joel Fernandes. ;-) Thanx, Paul
On 2/19/2020 10:13 AM, Johannes Berg wrote: > On Wed, 2020-02-19 at 14:41 +0530, Amol Grover wrote: >> >> - WARN_ON_ONCE(!rcu_read_lock_held() && !lockdep_rtnl_is_held()); >> - >> - list_for_each_entry_rcu(pos, &rdev->sched_scan_req_list, list) { >> + list_for_each_entry_rcu(pos, &rdev->sched_scan_req_list, list, >> + lockdep_rtnl_is_held()) { > > Huh, I didn't even know you _could_ do that :) Me neither ;-). Above you are removing the WARN_ON_ONCE() entirely. Would it not be good to keep the WARN_ON_ONCE() with only the !rcu_read_lock_held() check. Regards, Arend
On Wed, 2020-02-19 at 22:27 +0100, Arend Van Spriel wrote: > On 2/19/2020 10:13 AM, Johannes Berg wrote: > > On Wed, 2020-02-19 at 14:41 +0530, Amol Grover wrote: > > > > > > - WARN_ON_ONCE(!rcu_read_lock_held() && !lockdep_rtnl_is_held()); > > > - > > > - list_for_each_entry_rcu(pos, &rdev->sched_scan_req_list, list) { > > > + list_for_each_entry_rcu(pos, &rdev->sched_scan_req_list, list, > > > + lockdep_rtnl_is_held()) { > > > > Huh, I didn't even know you _could_ do that :) > > Me neither ;-). Above you are removing the WARN_ON_ONCE() entirely. > Would it not be good to keep the WARN_ON_ONCE() with only the > !rcu_read_lock_held() check. Not needed, the macro expansion will already contain rcu_read_lock_any_held() just like in all the other cases where you pass a lockdep condition to RCU helpers. johannes
On 2/19/2020 10:29 PM, Johannes Berg wrote: > On Wed, 2020-02-19 at 22:27 +0100, Arend Van Spriel wrote: >> On 2/19/2020 10:13 AM, Johannes Berg wrote: >>> On Wed, 2020-02-19 at 14:41 +0530, Amol Grover wrote: >>>> >>>> - WARN_ON_ONCE(!rcu_read_lock_held() && !lockdep_rtnl_is_held()); >>>> - >>>> - list_for_each_entry_rcu(pos, &rdev->sched_scan_req_list, list) { >>>> + list_for_each_entry_rcu(pos, &rdev->sched_scan_req_list, list, >>>> + lockdep_rtnl_is_held()) { >>> >>> Huh, I didn't even know you _could_ do that :) >> >> Me neither ;-). Above you are removing the WARN_ON_ONCE() entirely. >> Would it not be good to keep the WARN_ON_ONCE() with only the >> !rcu_read_lock_held() check. > > Not needed, the macro expansion will already contain > rcu_read_lock_any_held() just like in all the other cases where you pass > a lockdep condition to RCU helpers. Ah, yes. I see it in __list_check_rcu(). Thanks, Arend
diff --git a/net/wireless/scan.c b/net/wireless/scan.c index aef240fdf8df..7f1af8f347b1 100644 --- a/net/wireless/scan.c +++ b/net/wireless/scan.c @@ -556,9 +556,8 @@ cfg80211_find_sched_scan_req(struct cfg80211_registered_device *rdev, u64 reqid) { struct cfg80211_sched_scan_request *pos; - WARN_ON_ONCE(!rcu_read_lock_held() && !lockdep_rtnl_is_held()); - - list_for_each_entry_rcu(pos, &rdev->sched_scan_req_list, list) { + list_for_each_entry_rcu(pos, &rdev->sched_scan_req_list, list, + lockdep_rtnl_is_held()) { if (pos->reqid == reqid) return pos; }
rdev->sched_scan_req_list maybe traversed using list_for_each_entry_rcu outside an RCU read-side critical section but under the protection of rtnl_mutex. Hence, add corresponding lockdep expression to silence false-positive warnings, and harden RCU lists. Signed-off-by: Amol Grover <frextrite@gmail.com> --- net/wireless/scan.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)