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
