[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <7719f03304bd525e6b85a8700eb3466acf1cc76b.camel@sipsolutions.net>
Date: Thu, 18 Jan 2024 00:07:47 +0100
From: Johannes Berg <johannes@...solutions.net>
To: Brent Pappas <bpappas@...pasbrent.com>
Cc: Kalle Valo <kvalo@...nel.org>, "David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo
Abeni <pabeni@...hat.com>, linux-wireless@...r.kernel.org,
netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] wifi: mac80211: tx: Add __must_hold() annotation
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
Powered by blists - more mailing lists