netdev - Re: [PATCH] wifi: mac80211: tx: Add __must

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<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