From: Johannes Berg <[email protected]>
This could be used e.g. for temporarily sending quiet
(mode=1 in CSA/ECSA), or updating bandwidth. This is
also useful for testing, since it's something that an
AP may do and the client needs to be prepared. Simply
allow it.
Signed-off-by: Johannes Berg <[email protected]>
---
net/mac80211/cfg.c | 4 ----
1 file changed, 4 deletions(-)
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 8f647e28e354..c92acbf7b002 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -3874,10 +3874,6 @@ __ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev,
if (sdata->wdev.cac_started)
return -EBUSY;
- if (cfg80211_chandef_identical(&chanreq.oper,
- &sdata->vif.bss_conf.chanreq.oper))
- return -EINVAL;
-
if (chanreq.oper.punctured && !sdata->vif.bss_conf.eht_support)
return -EINVAL;
--
2.43.0
On 1/30/24 01:05, Johannes Berg wrote:
> From: Johannes Berg <[email protected]>
>
> This could be used e.g. for temporarily sending quiet
> (mode=1 in CSA/ECSA), or updating bandwidth. This is
I know the intent here (from the other thread), but using the phrase _or
updating bandwidth_ is probably not correct since currently without this
change also, just changing the bandwidth is possible, isn't it?
In the chandef identical check we have -
return (chandef1->chan == chandef2->chan &&
chandef1->width == chandef2->width && ....
``````````````````````````````````
So the width would not match hence false would be returned.
Also, bringing a part of the discussion we had in the other thread -
>>> I'm thinking about removing that identical() check entirely - you
>>> might want to switch to the same channel with quiet=1. At least for
>>> testing that'd be really useful, and I don't think it really serves
>>> any purpose to forbid it.
>
>> Yeah, we can do. But is there any actual use case? Also, what if some
>> notorious user space application simply sends NL command without even
>> quiet=1? There should be some check I guess?
>
> I'm not sure we care much about a broken userspace application running
> with root privileges breaking something here? :-)
>
> And at least for testing it's very useful to be able to do that. Agree
> that identical channel and quiet==0 doesn't make _sense_, but even
> then I'm not sure there's a lot of value in not permitting it. With
> quiet==1 at least it does make some sense still though, and we're
> currently not allowing it, hence my patch (to be able to test
> scenarios like that we saw elsewhere.)
Agreed to your point. So in that case, should we skip the identical
check only when quiet=1?
> also useful for testing, since it's something that an
> AP may do and the client needs to be prepared. Simply
> allow it.
>
> Signed-off-by: Johannes Berg <[email protected]>
> ---
> net/mac80211/cfg.c | 4 ----
> 1 file changed, 4 deletions(-)
>
> diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
> index 8f647e28e354..c92acbf7b002 100644
> --- a/net/mac80211/cfg.c
> +++ b/net/mac80211/cfg.c
> @@ -3874,10 +3874,6 @@ __ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev,
> if (sdata->wdev.cac_started)
> return -EBUSY;
>
> - if (cfg80211_chandef_identical(&chanreq.oper,
> - &sdata->vif.bss_conf.chanreq.oper))
> - return -EINVAL;
> -
> if (chanreq.oper.punctured && !sdata->vif.bss_conf.eht_support)
> return -EINVAL;
>
On Tue, 2024-01-30 at 20:14 +0530, Aditya Kumar Singh wrote:
> On 1/30/24 01:05, Johannes Berg wrote:
> > From: Johannes Berg <[email protected]>
> >
> > This could be used e.g. for temporarily sending quiet
> > (mode=1 in CSA/ECSA), or updating bandwidth. This is
>
> I know the intent here (from the other thread), but using the phrase _or
> updating bandwidth_ is probably not correct since currently without this
> change also, just changing the bandwidth is possible, isn't it?
Err, yes, indeed... bad commit message.
> Also, bringing a part of the discussion we had in the other thread -
>
> >>> I'm thinking about removing that identical() check entirely - you
> >>> might want to switch to the same channel with quiet=1. At least for
> >>> testing that'd be really useful, and I don't think it really serves
> >>> any purpose to forbid it.
> >
> >> Yeah, we can do. But is there any actual use case? Also, what if some
> >> notorious user space application simply sends NL command without even
> >> quiet=1? There should be some check I guess?
> >
> > I'm not sure we care much about a broken userspace application running
> > with root privileges breaking something here? :-)
> >
> > And at least for testing it's very useful to be able to do that. Agree
> > that identical channel and quiet==0 doesn't make _sense_, but even
> > then I'm not sure there's a lot of value in not permitting it. With
> > quiet==1 at least it does make some sense still though, and we're
> > currently not allowing it, hence my patch (to be able to test
> > scenarios like that we saw elsewhere.)
>
> Agreed to your point. So in that case, should we skip the identical
> check only when quiet=1?
We could, though I even now have a test (not posted yet) that's using
this, but I could rewrite it to actually switch bandwidth.
I'm just not sure it's worth the extra complexity? What are we actually
saving ourselves from by doing it? Clearly we have to handle this case,
and whether or not it's quiet shouldn't really matter to the underlying
code?
IOW, yeah, do have that information so we could just add
!params->block_tx &&
to the condition, but I'm not really sure what the value in that would
be, other than some kind of accurate reflecting of how we think today
CSA should be used?
But now that I think about it, Jouni mentioned yesterday that REVme D5.0
is getting language to allow capability changes during CSA, so that'd be
another thing to check for, you might (eventually) want to actually do a
CSA to the same channel to change capabilities, without quiet?
Anyway, even without that, I think the check doesn't really help for
anything. I'm not convinced the kernel needs to enforce a rule that
"userspace doesn't do something useless"? From a kernel POV it doesn't
matter if you can do it or not.
johannes
On 1/30/24 20:23, Johannes Berg wrote:
> On Tue, 2024-01-30 at 20:14 +0530, Aditya Kumar Singh wrote:
>> On 1/30/24 01:05, Johannes Berg wrote:
>>> From: Johannes Berg <[email protected]>
>>>
>>> This could be used e.g. for temporarily sending quiet
>>> (mode=1 in CSA/ECSA), or updating bandwidth. This is
>>
>> I know the intent here (from the other thread), but using the phrase _or
>> updating bandwidth_ is probably not correct since currently without this
>> change also, just changing the bandwidth is possible, isn't it?
>
> Err, yes, indeed... bad commit message.
>
>> Also, bringing a part of the discussion we had in the other thread -
>>
>> >>> I'm thinking about removing that identical() check entirely - you
>> >>> might want to switch to the same channel with quiet=1. At least for
>> >>> testing that'd be really useful, and I don't think it really serves
>> >>> any purpose to forbid it.
>> >
>> >> Yeah, we can do. But is there any actual use case? Also, what if some
>> >> notorious user space application simply sends NL command without even
>> >> quiet=1? There should be some check I guess?
>> >
>> > I'm not sure we care much about a broken userspace application running
>> > with root privileges breaking something here? :-)
>> >
>> > And at least for testing it's very useful to be able to do that. Agree
>> > that identical channel and quiet==0 doesn't make _sense_, but even
>> > then I'm not sure there's a lot of value in not permitting it. With
>> > quiet==1 at least it does make some sense still though, and we're
>> > currently not allowing it, hence my patch (to be able to test
>> > scenarios like that we saw elsewhere.)
>>
>> Agreed to your point. So in that case, should we skip the identical
>> check only when quiet=1?
>
> We could, though I even now have a test (not posted yet) that's using
> this, but I could rewrite it to actually switch bandwidth.
>
> I'm just not sure it's worth the extra complexity? What are we actually
> saving ourselves from by doing it? Clearly we have to handle this case,
> and whether or not it's quiet shouldn't really matter to the underlying
> code?
>
> IOW, yeah, do have that information so we could just add
>
> !params->block_tx &&
>
> to the condition, but I'm not really sure what the value in that would
> be, other than some kind of accurate reflecting of how we think today
> CSA should be used?
>
> But now that I think about it, Jouni mentioned yesterday that REVme D5.0
> is getting language to allow capability changes during CSA, so that'd be
> another thing to check for, you might (eventually) want to actually do a
> CSA to the same channel to change capabilities, without quiet?
>
Oh! In that case yeah it is pointless. We can remove the check from
kernel completely then.
>
> Anyway, even without that, I think the check doesn't really help for
> anything. I'm not convinced the kernel needs to enforce a rule that
> "userspace doesn't do something useless"? From a kernel POV it doesn't
> matter if you can do it or not.
>
Yeah makes sense :) Fine then. Let's remove it.