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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ