| Message ID | 20240129203544.ef7258d5790d.Idafe22e41621757458d4960659b9621853f7104d@changeid (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | wifi: mac80211: allow CSA to same channel | expand |
On 1/30/24 01:05, Johannes Berg wrote: > From: Johannes Berg <johannes.berg@intel.com> > > 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 <johannes.berg@intel.com> > --- > 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 <johannes.berg@intel.com> > > > > 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 <johannes.berg@intel.com> >>> >>> 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.
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;