Received-SPF: pass (google.com: domain of linux-wireless+bounces-3997-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <abb0c320d284581adc80ecf8328599df9bc2d688.camel@sipsolutions.net>
Subject: Re: What is the lifetime of an instance of struct
 cfg80211_chan_def::chan
From: Johannes Berg <johannes@sipsolutions.net>
To: Jeff Johnson <quic_jjohnson@quicinc.com>, linux-wireless
	 <linux-wireless@vger.kernel.org>
Date: Mon, 26 Feb 2024 09:31:32 +0100
In-Reply-To: <181138f2-77c2-47f5-94d0-28ccd52fb166@quicinc.com>
References: <181138f2-77c2-47f5-94d0-28ccd52fb166@quicinc.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.50.4 (3.50.4-1.fc39) 
Precedence: bulk
MIME-Version: 1.0

On Fri, 2024-02-23 at 14:14 -0800, Jeff Johnson wrote:
> I'm concerned about a potential race condition in the ath12k driver, but
> need to understand the lifetime of struct cfg80211_chan_def::chan to see
> if there is an actual issue.

Almost certainly isn't - the 'chan' pointer in chandef is to a struct
ieee80211_channel, and those are more or less constant and need to be
around for the lifetime of the entire wiphy, at least. Often they're
just in static memory in the driver module.

> This is the target of my concern, which at first glance looks benign:
> static int ath12k_mac_vif_chan(struct ieee80211_vif *vif,
> 			       struct cfg80211_chan_def *def)
> {
> 	struct ieee80211_chanctx_conf *conf;
>=20
> 	rcu_read_lock();
> 	conf =3D rcu_dereference(vif->bss_conf.chanctx_conf);
> 	*def =3D conf->def;
> 	rcu_read_unlock();
>=20
> 	return 0;
> }
> Note: I've omitted some error code that isn't important to this discussio=
n.
>=20
> This code starts a read side critical section, gets the config from the
> BSS configuration, makes a copy of the conf->def and then exits the read
> side critical section. What could go wrong? Well what is this conf->def
> that is being copied?
> struct ieee80211_bss_conf {
> 	struct ieee80211_chanctx_conf __rcu *chanctx_conf;
>=20
> struct ieee80211_chanctx_conf {
> 	struct cfg80211_chan_def def;
>=20
> struct cfg80211_chan_def {
> 	struct ieee80211_channel *chan;
> 	enum nl80211_chan_width width;
> 	u32 center_freq1;
> 	u32 center_freq2;
> 	struct ieee80211_edmg edmg;
> 	u16 freq1_offset;
> };
>=20
> Note well the following:
> 	struct ieee80211_channel *chan;
>=20
> This is a pointer to some memory.=C2=A0

Right.

> During the time we are in the read
> side critical section we are guaranteed that, if this pointer is not
> NULL, the memory backing this pointer is valid.

Actually ... I would say since that pointer _itself_ doesn't even have
__rcu annotation (and doesn't get copied via RCU), the RCU does nothing
for its protection.

> But as soon as we exit
> the read side critical section there is no guarantee, at least not one
> enforced by RCU, that a writer might update, or even free, the memory
> referenced by chan.

There never was though, since you didn't rcu_dereference(chan).

> So I'm trying to determine what else, if anything, protects the lifetime
> of this pointer, and I'm getting lost in the mac80211 code, so any hints
> would be appreciated.

See above. It's always pointing to an entry in the wiphy's supported
band's channels array, which is around for at least the life of the
wiphy. At least should be!

johannes