Re: [PATCH] wifi: mac80211: tx: Add __must_hold() annotation

[Johannes Berg <johannes@sipsolutions.net>]

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