Received-SPF: pass (google.com: domain of linux-wireless+bounces-4018-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <e3039fbe-9c8d-4dc5-8196-f94104f1062e@quicinc.com>
Date: Mon, 26 Feb 2024 08:43:29 -0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: What is the lifetime of an instance of struct
 cfg80211_chan_def::chan
Content-Language: en-US
To: Johannes Berg <johannes@sipsolutions.net>,
        linux-wireless
	<linux-wireless@vger.kernel.org>
References: <181138f2-77c2-47f5-94d0-28ccd52fb166@quicinc.com>
 <abb0c320d284581adc80ecf8328599df9bc2d688.camel@sipsolutions.net>
From: Jeff Johnson <quic_jjohnson@quicinc.com>
In-Reply-To: <abb0c320d284581adc80ecf8328599df9bc2d688.camel@sipsolutions.net>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8bit

On 2/26/2024 12:31 AM, Johannes Berg wrote:
> 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;
>>
>> 	rcu_read_lock();
>> 	conf = rcu_dereference(vif->bss_conf.chanctx_conf);
>> 	*def = conf->def;
>> 	rcu_read_unlock();
>>
>> 	return 0;
>> }
>> Note: I've omitted some error code that isn't important to this discussion.
>>
>> 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;
>>
>> struct ieee80211_chanctx_conf {
>> 	struct cfg80211_chan_def def;
>>
>> 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;
>> };
>>
>> Note well the following:
>> 	struct ieee80211_channel *chan;
>>
>> This is a pointer to some memory. 
> 
> 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!

Thanks for that explanation!

/jeff