| Message ID | 20190826162637.7535-1-denkenz@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | [1/2] nl80211: Add NL80211_EXT_FEATURE_LIVE_IFTYPE_CHANGE | expand |
On Mon, 2019-08-26 at 11:26 -0500, Denis Kenzior wrote: > > + * Prior to Kernel 5.4, userspace applications should implement the > + * following behavior: I'm not sure mentioning the kernel version here does us any good? I mean, you really need to implement that behaviour regardless of kernel version, if NL80211_EXT_FEATURE_LIVE_IFTYPE_CHANGE isn't set. > + * @NL80211_EXT_FEATURE_LIVE_IFTYPE_CHANGE: This device supports switching > + * the IFTYPE of an interface without having to bring the device DOWN > + * first via RTNL. Exact semantics of this feature is driver > + * implementation dependent. That's not really nice. > For mac80211, the following restrictions > + * apply: > + * - Only devices currently in IFTYPE AP, P2P_GO, P2P_CLIENT, > + * STATION, ADHOC and OCB can be switched. > + * - The target IFTYPE must be one of: AP, P2P_GO, P2P_CLIENT, > + * STATION, ADHOC or OCB. > + * Other drivers are expected to follow similar restrictions. Maybe we should instead have a "bitmask of interface types that can be switched while live" or something like that? johannes
Hi Johannes, On 8/30/19 5:19 AM, Johannes Berg wrote: > On Mon, 2019-08-26 at 11:26 -0500, Denis Kenzior wrote: >> >> + * Prior to Kernel 5.4, userspace applications should implement the >> + * following behavior: > > I'm not sure mentioning the kernel version here does us any good? I > mean, you really need to implement that behaviour regardless of kernel > version, if NL80211_EXT_FEATURE_LIVE_IFTYPE_CHANGE isn't set. > Agreed. I guess I just view nl80211.h as a sort of combination between a uapi file and an actual manpage. And manpages do mention which kernel version a certain feature/flag/whatever was added. Such info can be useful in many ways, e.g. figuring out which kernel version might be required for a certain piece of hardware, etc. Another point where this might be useful is if the kernel starts enforcing certain behavior that it didn't before. E.g. I mentioned this in the purge thread that a lot of mode change failure cases could be caught if the kernel checked this flag prior to doing anything. I really leave this up to you if this is something you think is a good idea or not. >> + * @NL80211_EXT_FEATURE_LIVE_IFTYPE_CHANGE: This device supports switching >> + * the IFTYPE of an interface without having to bring the device DOWN >> + * first via RTNL. Exact semantics of this feature is driver >> + * implementation dependent. > > That's not really nice. Sorry. This came from some doc changes I have pending. I think I wrote this after looking at some fullmac drivers and how they handle mode changes and the wording reflects the exasperation I felt at the time. Do you want to suggest some alternate wording? I think it is worth it to have some fair warning in the docs stating that prior to version so and so the semantics are completely driver dependent. > >> For mac80211, the following restrictions >> + * apply: >> + * - Only devices currently in IFTYPE AP, P2P_GO, P2P_CLIENT, >> + * STATION, ADHOC and OCB can be switched. >> + * - The target IFTYPE must be one of: AP, P2P_GO, P2P_CLIENT, >> + * STATION, ADHOC or OCB. >> + * Other drivers are expected to follow similar restrictions. > > Maybe we should instead have a "bitmask of interface types that can be > switched while live" or something like that? > I'm fine with that, but this would only apply to newer kernels, no? Don't we at least want to attempt to state what the rules are for older ones? An alternative might be to simply state what the restrictions are and just enforce those at the cfg80211 level. Regards, -Denis
Hi, > Agreed. I guess I just view nl80211.h as a sort of combination between > a uapi file and an actual manpage. And manpages do mention which kernel > version a certain feature/flag/whatever was added. Such info can be > useful in many ways, e.g. figuring out which kernel version might be > required for a certain piece of hardware, etc. Yeah, fair point, I just think it's better to document that behaviour as dependent on the flag, not the kernel version; this will continue to be true for drivers that don't set the flag in future kernels after all. IOW, I don't see how it does you any good to know that you're running on kernel version 5.4, when the flag isn't set you still have to follow the 4 steps you outlined. > > > + * @NL80211_EXT_FEATURE_LIVE_IFTYPE_CHANGE: This device supports switching > > > + * the IFTYPE of an interface without having to bring the device DOWN > > > + * first via RTNL. Exact semantics of this feature is driver > > > + * implementation dependent. > > > > That's not really nice. > > Sorry. This came from some doc changes I have pending. I think I wrote > this after looking at some fullmac drivers and how they handle mode > changes and the wording reflects the exasperation I felt at the time. > > Do you want to suggest some alternate wording? I think it is worth it > to have some fair warning in the docs stating that prior to version so > and so the semantics are completely driver dependent. Well, but you're trying to document/advertise the restrictions now. So this feels a bit insufficient, by advertising the feature flag the device is now saying "it's possible I can switch, but don't ask me how or when". (Cont'd below) So I don't really think it's the wording that bothers me so much as the fact that you're basically going only half the way documenting this. We have nothing now, which I can agree isn't a good thing, but adding a flag that says "maybe you can do it on this device" doesn't really change the status quo *much*, since that was already true before. It seems to me you'd rather want a definitive statement. > > > For mac80211, the following restrictions > > > + * apply: > > > + * - Only devices currently in IFTYPE AP, P2P_GO, P2P_CLIENT, > > > + * STATION, ADHOC and OCB can be switched. > > > + * - The target IFTYPE must be one of: AP, P2P_GO, P2P_CLIENT, > > > + * STATION, ADHOC or OCB. > > > + * Other drivers are expected to follow similar restrictions. > > > > Maybe we should instead have a "bitmask of interface types that can be > > switched while live" or something like that? > > > > I'm fine with that, but this would only apply to newer kernels, no? Sure, but all that you're doing here does. > Don't we at least want to attempt to state what the rules are for older > ones? That's what you did above for the NL80211_CMD_SET_INTERFACE documentation update, I don't think it would belong into the documentation for NL80211_EXT_FEATURE_LIVE_IFTYPE_CHANGE? And you wrote that this is what happens when the flag is *set* which by definition cannot happen in older kernels. > An alternative might be to simply state what the restrictions are and > just enforce those at the cfg80211 level. Sounds good to me, we do that for a lot of things. Basically that just takes it one step further - I said above we could advertise the restrictions, but once we do that cfg80211 knows and can also enforce it. johannes
diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h index bf7c4222f512..a9ca2fe67f52 100644 --- a/include/uapi/linux/nl80211.h +++ b/include/uapi/linux/nl80211.h @@ -275,6 +275,29 @@ * single %NL80211_ATTR_IFINDEX is supported. * @NL80211_CMD_SET_INTERFACE: Set type of a virtual interface, requires * %NL80211_ATTR_IFINDEX and %NL80211_ATTR_IFTYPE. + * + * Note that it is driver-dependent whether a SET_INTERFACE will be + * allowed if the underlying netdev is currently UP. Userspace + * can obtain a hint as to whether this is allowed by looking at + * the %NL80211_EXT_FEATURE_LIVE_IFTYPE_CHANGE, but certain restrictions + * will still apply. + * + * Prior to Kernel 5.4, userspace applications should implement the + * following behavior: + * 1. (Optionally) Attempt SET_INTERFACE on a wireless device + * with the underlying netdev in the UP state. If -EBUSY + * is returned proceed to 2. Note that a SET_INTERFACE + * which results in -EBUSY might still result in other + * side-effects, such as Deauthentication, exiting AP mode, + * etc. + * 2. Bring the netdev DOWN via RTNL + * 3. Attempt SET_INTERFACE on the underlying netdev in the DOWN + * state. If successful, proceed to 4. + * 4. Bring the netdev UP via RTNL + * + * Note that bringing down the device might trigger a firmware reset / + * power cycling and/or other effects that are driver dependent. + * * @NL80211_CMD_NEW_INTERFACE: Newly created virtual interface or response * to %NL80211_CMD_GET_INTERFACE. Has %NL80211_ATTR_IFINDEX, * %NL80211_ATTR_WIPHY and %NL80211_ATTR_IFTYPE attributes. Can also @@ -5481,6 +5504,18 @@ enum nl80211_feature_flags { * @NL80211_EXT_FEATURE_SAE_OFFLOAD: Device wants to do SAE authentication in * station mode (SAE password is passed as part of the connect command). * + * @NL80211_EXT_FEATURE_LIVE_IFTYPE_CHANGE: This device supports switching + * the IFTYPE of an interface without having to bring the device DOWN + * first via RTNL. Exact semantics of this feature is driver + * implementation dependent. For mac80211, the following restrictions + * apply: + * - Only devices currently in IFTYPE AP, P2P_GO, P2P_CLIENT, + * STATION, ADHOC and OCB can be switched. + * - The target IFTYPE must be one of: AP, P2P_GO, P2P_CLIENT, + * STATION, ADHOC or OCB. + * Other drivers are expected to follow similar restrictions. + * This flag was introduced in Kernel v5.4 + * * @NUM_NL80211_EXT_FEATURES: number of extended features. * @MAX_NL80211_EXT_FEATURES: highest extended feature index. */ @@ -5526,6 +5561,7 @@ enum nl80211_ext_feature_index { NL80211_EXT_FEATURE_EXT_KEY_ID, NL80211_EXT_FEATURE_STA_TX_PWR, NL80211_EXT_FEATURE_SAE_OFFLOAD, + NL80211_EXT_FEATURE_LIVE_IFTYPE_CHANGE, /* add new features before the definition below */ NUM_NL80211_EXT_FEATURES,
There is some ambiguity in how various drivers support NL80211_CMD_SET_INTERFACE on devices where the underlying netdev is UP. mac80211 for example supports this if the underlying driver provides a change_interface operation. However, most devices do not. For FullMac devices, the situation is even less clear. This patch introduces a new feature flag that lets userspace know whether it can expect a mode change (via SET_INTERFACE) to work while the device is still UP or it should bring down the device first. This commit also updates the documentation for SET_INTERFACE with hints as to how it should be used. Signed-off-by: Denis Kenzior <denkenz@gmail.com> --- include/uapi/linux/nl80211.h | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+)