Hi Brent, On Wed, 2024-01-17 at 15:00 -0500, Brent Pappas wrote: > Thanks for the feedback Johannes. As I mentioned in my original email, I'm still > learning the RCU API, so I appreciate the insight from someone more > knowledgeable. Note this isn't really all that RCU related. > > Much better to put something lockdep_assert_held() or similar into the right > > places. > > I'm not committed to using __must_hold(); would you be willing to accept this > patch if I change it to use lockdep_assert_held() instead? I'm actually not sure what you're trying to check here. The rcu_dereference() inside of it? But that'll already be checked at runtime by lockdep without any further code. So ... right now I don't see that there's any point in adding any further annotations, but I'm also not sure what you're trying to achieve. > > The function ieee80211_set_beacon_cntdwn() is called from a number of places > > in this file, some of which acquire RCU critical section, and some of which > > acquire no locks nor RCU critical section at all. > > Grepping through tx.c, I see ieee80211_set_beacon_cntdwn() is invoked in three > places: > > - Line 5285: Inside the definition of ieee80211_beacon_get_ap(), which is only > invoked in critical sections (both directly and in another nested call). > - Line 5439: Directly inside a critical section. > - Line 5471: Directly inside a critical section (same as previous). Right. > > I tried to fix this in sparse many years ago, some code even got merged (and > > then reverted), and if the experience tells me anything then that it's pretty > > much not fixable. > > I'm sorry to hear that; a solution to this problem sounds very useful. I'm > currently working on making my own static analyzer for performing more checks > than what sparse currently provides. Are you aware of smatch? > Since you've worked on this problem and > have deeper insight into than I do, what sort of checks would you like to see > added to a tool like sparse (besides checking whether specific locks are held)? I haven't really thought about that ... some better taint tracking would be nice but that's _really_ hard ;-) johannes
