[PATCH 1/2] nl80211: Add NL80211_EXT_FEATURE_LIVE_IFTYPE_CHANGE

[PATCH 1/2] nl80211: Add NL80211_EXT_FEATURE_LIVE_IFTYPE_CHANGE

[Denis Kenzior]

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(+)

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,
-- 
2.19.2

[PATCH 2/2] mac80211: Set LIVE_IFTYPE_CHANGE if op provided

[Denis Kenzior]

Signed-off-by: Denis Kenzior <denkenz@gmail.com>
---
 net/mac80211/main.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/net/mac80211/main.c b/net/mac80211/main.c
index 29b9d57df1a3..073e5d10a48e 100644
--- a/net/mac80211/main.c
+++ b/net/mac80211/main.c
@@ -597,6 +597,10 @@ struct ieee80211_hw *ieee80211_alloc_hw_nm(size_t priv_data_len,
 
 	wiphy_ext_feature_set(wiphy, NL80211_EXT_FEATURE_RRM);
 
+	if (ops->change_interface)
+		wiphy_ext_feature_set(wiphy,
+				      NL80211_EXT_FEATURE_LIVE_IFTYPE_CHANGE);
+
 	wiphy->bss_priv_size = sizeof(struct ieee80211_bss);
 
 	local = wiphy_priv(wiphy);
-- 
2.19.2

Re: [PATCH 1/2] nl80211: Add NL80211_EXT_FEATURE_LIVE_IFTYPE_CHANGE

[Johannes Berg]

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

Re: [PATCH 1/2] nl80211: Add NL80211_EXT_FEATURE_LIVE_IFTYPE_CHANGE

[Denis Kenzior]

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

Re: [PATCH 1/2] nl80211: Add NL80211_EXT_FEATURE_LIVE_IFTYPE_CHANGE

[Johannes Berg]

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