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.
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. 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. 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.
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.
/jeff
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!
johannes
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