linux-kernel - Re: [PATCH wireless-next 2/3] wifi: mac80211: Allow scan on a radio while operating on DFS on another radio

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <a91c82f1-df99-4938-8f41-ce90e9e08ad8@quicinc.com>
Date: Thu, 22 May 2025 16:10:29 +0530
From: Raj Kumar Bhagat <quic_rajkbhag@...cinc.com>
To: Johannes Berg <johannes@...solutions.net>
CC: <linux-wireless@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH wireless-next 2/3] wifi: mac80211: Allow scan on a radio
 while operating on DFS on another radio

On 5/16/2025 1:31 PM, Johannes Berg wrote:
> On Wed, 2025-05-14 at 16:58 +0530, Raj Kumar Bhagat wrote:
>> Currently, in multi-radio wiphy cases, if one radio is operating on a DFS
>> channel, -EBUSY is returned even when a scan is requested on a different
>> radio. Because of this, an MLD AP with one radio (link) on a DFS channel
>> and Automatic Channel Selection (ACS) on another radio (link) cannot be
>> brought up.
>>
>> In multi-radio wiphy cases, multiple radios are grouped under a single
>> wiphy. Hence, if a radio is operating on a DFS channel and a scan is
>> requested on a different radio of the same wiphy, the scan can be allowed
>> simultaneously without impacting the DFS operations.
>>
>> Add logic to check the underlying radio used for the requested scan. If the
>> radio on which DFS is already running is not being used, allow the scan
>> operation; otherwise, return -EBUSY.
> 
> So while I agree in principle, I think this needs to be more carefully
> constructed because it relies on an unstated (?) assumption that each
> radio is going to ever be used for scanning on a certain band. That
> seems to make sense, and a radio will certainly only ever be able to
> _operate_ on the frequencies listed for it (due to chanctx etc.), but is
> it really true that it will never be able to operate at all on other
> frequencies?
> 

I'm not sure If I fully understood the this comment.

This patch assumes that multiple radios are grouped under a single wiphy.
Each radio has its own list of frequencies it can scan, and there is no overlap
in frequencies between any two radios within the same wiphy.

If this assumption holds, then if one radio is operating on a DFS channel and a
new scan request does not include any frequencies from that radio's list, the
scan should be allowed—since the DFS radio wouldn't be involved in handling that
scan request.

> I'm not sure I find the notion of e.g. having a 5 and 6 GHz radio that
> are used for operating on those bands, but being able to scan 5 GHz
> channels using the 6 GHz radio completely unimaginable? Maybe it is
> though and I'm just overly paranoid?
> 
> We could also just leave that up to the drivers, of course, but then I
> think we should _state_ this assumption somewhere in the docs/header
> file(s)?
> 
>> +bool ieee80211_is_radio_idx_in_scan_req(struct wiphy *wiphy,
>> +					struct cfg80211_scan_request *scan_req,
>> +					int radio_idx)
>> +{
>> +	struct ieee80211_channel *chan;
>> +	int i, chan_radio_idx;
>> +
>> +	if (!scan_req)
>> +		return false;
> 
> That seems overly paranoid, or maybe it should be WARN_ON()? I mean,
> asking something about a scan request and then not giving one is just
> the wrong thing to do in the first place, no?
> 
> And if you're going to be paranoid then this probably shouldn't be
> called with an invalid/negative radio_idx either :)
> 

sure, this function should not be called with NULL scan_req and invalid
radio_idx. Better will remove the check: if (!scan_req).

> 
>> +	for (i = 0; i < scan_req->n_channels; i++) {
>> +		chan = scan_req->channels[i];
>> +		chan_radio_idx = cfg80211_get_radio_idx_by_chan(wiphy, chan);
>> +		/*
>> +		 * Skip channels with an invalid radio index and continue
>> +		 * checking. If any channel in the scan request matches the
>> +		 * given radio index, return true.
>> +		 */
>> +		if (chan_radio_idx < 0)
>> +			continue;
> 
> This seems ... wrong? If there's a channel in the scan request that
> didn't map to _any_ radio then how are we even scanning there? And the
> comment seems even stranger, why would we _want_ to ignore it (which it
> conveniently doesn't answer)?
> 

It seems, (chan_radio_idx < 0) should never be true because the chan is
taken from the valid scan request. I should remove this check in next version?