Received-SPF: pass (google.com: domain of linux-wireless+bounces-2128-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <7719f03304bd525e6b85a8700eb3466acf1cc76b.camel@sipsolutions.net>
Subject: Re: [PATCH] wifi: mac80211: tx: Add __must_hold() annotation
From: Johannes Berg <johannes@sipsolutions.net>
To: Brent Pappas <bpappas@pappasbrent.com>
Cc: Kalle Valo <kvalo@kernel.org>, "David S. Miller" <davem@davemloft.net>, 
 Eric Dumazet <edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>, Paolo
 Abeni <pabeni@redhat.com>,  linux-wireless@vger.kernel.org,
 netdev@vger.kernel.org,  linux-kernel@vger.kernel.org
Date: Thu, 18 Jan 2024 00:07:47 +0100
In-Reply-To: <Zagxwx40ja4JRUQ2@pappasbrent.com>
References: <20240113011145.10888-2-bpappas@pappasbrent.com>
	 <87sf31hhfp.fsf@kernel.org>
	 <26d364547d3bbb04800877e899cfebe0e1ec4dc0.camel@sipsolutions.net>
	 <Zagxwx40ja4JRUQ2@pappasbrent.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.50.3 (3.50.3-1.fc39) 
Precedence: bulk
MIME-Version: 1.0

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.
>=20
> 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 p=
laces
> > in this file, some of which acquire RCU critical section, and some of w=
hich
> > acquire no locks nor RCU critical section at all.
>=20
> Grepping through tx.c, I see ieee80211_set_beacon_cntdwn() is invoked in =
three
> places:
>=20
> - 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.
>=20
> 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 ch=
ecks
> than what sparse currently provides.=C2=A0

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