[PATCH 0/5] Improve wireless netdev detection

[PATCH 0/5] Improve wireless netdev detection

[Denis Kenzior]

The current mechanism to detect hot-plug / unplug of wireless devices is
somewhat arcane.  One has to listen to NEW_WIPHY/DEL_WIPHY events over
nl80211 as well as RTM_NEWLINK / RTM_DELLINK events over rtnl, then
somehow find a correlation between these events.  This involves userspace
sending GET_INTERFACE or GET_WIPHY commands to the kernel, which incurs
additional roundtrips.

This patch series proposes that NEW_INTERFACE and DEL_INTERFACE events are
always emitted, regardless of whether a netdev was added/removed by the
driver or explicitly via NEW_INTERFACE/DEL_INTERFACE commands.

One side effect of this approach is that multiple NEW_INTERFACE/DEL_INTERFACE
events might be generated for P2P interfaces.  Once when a wdev is created
or destroyed, and once when the associated p2p netdev is connecte or 
disconnected.  It is likely that only the caller of P2P oriented
NEW_INTERFACE / DEL_INTERFACE commands is interested in the status of these
operations.  E.g. the caller is / should be using SOCKET_OWNER attribute.
Thus one possibility is to not emit NEW_INTERFACE/DEL_INTERFACE events in
such cases.

Denis Kenzior (5):
  nl80211: Add nl80211_notify_iface
  core: Notify of new wireless netdevs
  nl80211: Emit NEW_INTERFACE only in special cases
  core: Notify when wireless netdev is removed
  nl80211: Emit DEL_INTERFACE only in special cases

 net/wireless/core.c    |  4 ++++
 net/wireless/nl80211.c | 63 ++++++++++++++++++++++++++++++++++++++------------
 net/wireless/nl80211.h |  3 +++
 3 files changed, 55 insertions(+), 15 deletions(-)

-- 
2.7.3

[PATCH 1/5] nl80211: Add nl80211_notify_iface

[Denis Kenzior]

This function emits NL80211_CMD_NEW_INTERFACE or
NL80211_CMD_DEL_INTERFACE events.  This is meant to be used by the core
to notify userspace applications such as wpa_supplicant when a netdev
related to a wireless device has been added or removed.
---
 net/wireless/nl80211.c | 28 ++++++++++++++++++++++++++++
 net/wireless/nl80211.h |  3 +++
 2 files changed, 31 insertions(+)

diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index f39fd4d..da03e17 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -11855,6 +11855,34 @@ void nl80211_notify_wiphy(struct cfg80211_registered_device *rdev,
 				NL80211_MCGRP_CONFIG, GFP_KERNEL);
 }
 
+void nl80211_notify_iface(struct cfg80211_registered_device *rdev,
+				struct wireless_dev *wdev,
+				enum nl80211_commands cmd)
+{
+	struct sk_buff *msg;
+	bool removal;
+
+	WARN_ON(cmd != NL80211_CMD_NEW_INTERFACE &&
+		cmd != NL80211_CMD_DEL_INTERFACE);
+
+	if (cmd == NL80211_CMD_DEL_INTERFACE)
+		removal = true;
+	else
+		removal = false;
+
+	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
+	if (!msg)
+		return;
+
+	if (nl80211_send_iface(msg, 0, 0, 0, rdev, wdev, removal) < 0) {
+		nlmsg_free(msg);
+		return;
+	}
+
+	genlmsg_multicast_netns(&nl80211_fam, wiphy_net(&rdev->wiphy), msg, 0,
+				NL80211_MCGRP_CONFIG, GFP_KERNEL);
+}
+
 static int nl80211_add_scan_req(struct sk_buff *msg,
 				struct cfg80211_registered_device *rdev)
 {
diff --git a/net/wireless/nl80211.h b/net/wireless/nl80211.h
index a63f402..6f6b73c 100644
--- a/net/wireless/nl80211.h
+++ b/net/wireless/nl80211.h
@@ -7,6 +7,9 @@ int nl80211_init(void);
 void nl80211_exit(void);
 void nl80211_notify_wiphy(struct cfg80211_registered_device *rdev,
 			  enum nl80211_commands cmd);
+void nl80211_notify_iface(struct cfg80211_registered_device *rdev,
+				struct wireless_dev *wdev,
+				enum nl80211_commands cmd);
 void nl80211_send_scan_start(struct cfg80211_registered_device *rdev,
 			     struct wireless_dev *wdev);
 struct sk_buff *nl80211_build_scan_msg(struct cfg80211_registered_device *rdev,
-- 
2.7.3

[PATCH 2/5] core: Notify of new wireless netdevs

[Denis Kenzior]

---
 net/wireless/core.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/wireless/core.c b/net/wireless/core.c
index 7645e97..7758c0f 100644
--- a/net/wireless/core.c
+++ b/net/wireless/core.c
@@ -1079,6 +1079,8 @@ static int cfg80211_netdev_notifier_call(struct notifier_block *nb,
 		     wdev->iftype == NL80211_IFTYPE_P2P_CLIENT ||
 		     wdev->iftype == NL80211_IFTYPE_ADHOC) && !wdev->use_4addr)
 			dev->priv_flags |= IFF_DONT_BRIDGE;
+
+		nl80211_notify_iface(rdev, wdev, NL80211_CMD_NEW_INTERFACE);
 		break;
 	case NETDEV_GOING_DOWN:
 		cfg80211_leave(rdev, wdev);
-- 
2.7.3

[PATCH 3/5] nl80211: Emit NEW_INTERFACE only in special cases

[Denis Kenzior]

For wireless device objects with an associated netdev, the NEW_INTERFACE
event is generated inside the netdev notifier.  The event is generated
regardless of whether it was due to a NL80211_CMD_NEW_INTERFACE call
from userspace or if the netdev was created implicitly by the driver.

If NL80211_CMD_NEW_INTERFACE handler is called, then a duplicate
NEW_INTERFACE event will be multicast.  This change modifies the logic
inside nl80211_new_interface to avoid this.
---
 net/wireless/nl80211.c | 23 +++++++++--------------
 1 file changed, 9 insertions(+), 14 deletions(-)

diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index da03e17..ec8eb88 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -2787,7 +2787,7 @@ static int nl80211_new_interface(struct sk_buff *skb, struct genl_info *info)
 	struct cfg80211_registered_device *rdev = info->user_ptr[0];
 	struct vif_params params;
 	struct wireless_dev *wdev;
-	struct sk_buff *msg, *event;
+	struct sk_buff *msg;
 	int err;
 	enum nl80211_iftype type = NL80211_IFTYPE_UNSPECIFIED;
 	u32 flags;
@@ -2891,20 +2891,15 @@ static int nl80211_new_interface(struct sk_buff *skb, struct genl_info *info)
 		return -ENOBUFS;
 	}
 
-	event = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
-	if (event) {
-		if (nl80211_send_iface(event, 0, 0, 0,
-				       rdev, wdev, false) < 0) {
-			nlmsg_free(event);
-			goto out;
-		}
-
-		genlmsg_multicast_netns(&nl80211_fam, wiphy_net(&rdev->wiphy),
-					event, 0, NL80211_MCGRP_CONFIG,
-					GFP_KERNEL);
-	}
+	/*
+	 * For wdevs which have no associated netdev object (e.g. of type
+	 * NL80211_IFTYPE_P2P_DEVICE), emit the NEW_INTERFACE event here.
+	 * For all other types, the event will be generated from the
+	 * netdev notifier
+	 */
+	if (!wdev->netdev)
+		nl80211_notify_iface(rdev, wdev, NL80211_CMD_NEW_INTERFACE);
 
-out:
 	return genlmsg_reply(msg, info);
 }
 
-- 
2.7.3

[PATCH 4/5] core: Notify when wireless netdev is removed

[Denis Kenzior]

---
 net/wireless/core.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/wireless/core.c b/net/wireless/core.c
index 7758c0f..d35038b 100644
--- a/net/wireless/core.c
+++ b/net/wireless/core.c
@@ -1159,6 +1159,8 @@ static int cfg80211_netdev_notifier_call(struct notifier_block *nb,
 		 * remove and clean it up.
 		 */
 		if (!list_empty(&wdev->list)) {
+			nl80211_notify_iface(rdev, wdev,
+						NL80211_CMD_DEL_INTERFACE);
 			sysfs_remove_link(&dev->dev.kobj, "phy80211");
 			list_del_rcu(&wdev->list);
 			rdev->devlist_generation++;
-- 
2.7.3

[PATCH 5/5] nl80211: Emit DEL_INTERFACE only in special cases

[Denis Kenzior]

For wireless device objects with an associated netdev, the DEL_INTERFACE
event is generated inside the netdev notifier.  The event is generated
regardless of whether it was due to a NL80211_CMD_DEL_INTERFACE call
from userspace or if the netdev was destroyed by other means.

If NL80211_CMD_DEL_INTERFACE handler is called, then a duplicate
DEL_INTERFACE event will be multicast.  This change modifies the logic
inside nl80211_del_interface to avoid this.
---
 net/wireless/nl80211.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index ec8eb88..a65c271 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -2913,7 +2913,17 @@ static int nl80211_del_interface(struct sk_buff *skb, struct genl_info *info)
 	if (!rdev->ops->del_virtual_intf)
 		return -EOPNOTSUPP;
 
-	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
+	/*
+	 * For wdevs which have no associated netdev object (e.g. of type
+	 * NL80211_IFTYPE_P2P_DEVICE), emit the DEL_INTERFACE event here.
+	 * For all other types, the event will be generated from the
+	 * netdev notifier
+	 */
+	if (!wdev->netdev)
+		msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
+	else
+		msg = NULL;
+
 	if (msg && nl80211_send_iface(msg, 0, 0, 0, rdev, wdev, true) < 0) {
 		nlmsg_free(msg);
 		msg = NULL;
-- 
2.7.3

Re: [PATCH 0/5] Improve wireless netdev detection

[Johannes Berg]

On Thu, 2016-07-07 at 02:08 -0500, Denis Kenzior wrote:
> The current mechanism to detect hot-plug / unplug of wireless devices
> is
> somewhat arcane.  One has to listen to NEW_WIPHY/DEL_WIPHY events
> over
> nl80211 as well as RTM_NEWLINK / RTM_DELLINK events over rtnl, then
> somehow find a correlation between these events.  This involves
> userspace
> sending GET_INTERFACE or GET_WIPHY commands to the kernel, which
> incurs
> additional roundtrips.
> 
> This patch series proposes that NEW_INTERFACE and DEL_INTERFACE
> events are
> always emitted, regardless of whether a netdev was added/removed by
> the
> driver or explicitly via NEW_INTERFACE/DEL_INTERFACE commands.
> 
> One side effect of this approach is that multiple
> NEW_INTERFACE/DEL_INTERFACE
> events might be generated for P2P interfaces.  Once when a wdev is
> created
> or destroyed, and once when the associated p2p netdev is connecte or 
> disconnected.  

I think you got some things mixed up. Are you talking of P2P GO/client
interfaces, which have netdevs, but are really the same as AP/BSS
client and thus the issue here would affect the others? Or are you
talking about the P2P-Device wdev? but that has no netdev.

> It is likely that only the caller of P2P oriented
> NEW_INTERFACE / DEL_INTERFACE commands is interested in the status of
> these
> operations.  E.g. the caller is / should be using SOCKET_OWNER
> attribute.
> Thus one possibility is to not emit NEW_INTERFACE/DEL_INTERFACE
> events in
> such cases.
> 

The breaking up of patches is also confusing. You seem to be
introducing things in the first, then breaking them again, and then
fixing them?

Couldn't the whole thing be done in one or maybe two (new/del)
patch(es)?

(You obviously also need to sign off your patches, see the kernel
Documentation/)

johannes

Re: [PATCH 0/5] Improve wireless netdev detection

[Denis Kenzior]

Hi Johannes,

On 07/08/2016 05:32 AM, Johannes Berg wrote:
> On Thu, 2016-07-07 at 02:08 -0500, Denis Kenzior wrote:
>> The current mechanism to detect hot-plug / unplug of wireless devices
>> is
>> somewhat arcane.  One has to listen to NEW_WIPHY/DEL_WIPHY events
>> over
>> nl80211 as well as RTM_NEWLINK / RTM_DELLINK events over rtnl, then
>> somehow find a correlation between these events.  This involves
>> userspace
>> sending GET_INTERFACE or GET_WIPHY commands to the kernel, which
>> incurs
>> additional roundtrips.
>>
>> This patch series proposes that NEW_INTERFACE and DEL_INTERFACE
>> events are
>> always emitted, regardless of whether a netdev was added/removed by
>> the
>> driver or explicitly via NEW_INTERFACE/DEL_INTERFACE commands.
>>
>> One side effect of this approach is that multiple
>> NEW_INTERFACE/DEL_INTERFACE
>> events might be generated for P2P interfaces.  Once when a wdev is
>> created
>> or destroyed, and once when the associated p2p netdev is connecte or
>> disconnected.
>
> I think you got some things mixed up. Are you talking of P2P GO/client
> interfaces, which have netdevs, but are really the same as AP/BSS
> client and thus the issue here would affect the others? Or are you
> talking about the P2P-Device wdev? but that has no netdev.

Apologies, I've only been looking at the kernel side for several days, 
so my understanding is still incomplete.

I was looking at mac80211/iface.c: ieee80211_if_add() which seems to 
handle NL80211_IFTYPE_P2P_DEVICE specially by not creating/registering a 
net_device object.  For some reason I thought that this object was 
registered somewhere later, but my understanding was incorrect.  So the 
entire 'side effect' paragraph above does not apply.

Are you okay with the general approach?  Are there any locking issues I 
might be overlooking?

>
>> It is likely that only the caller of P2P oriented
>> NEW_INTERFACE / DEL_INTERFACE commands is interested in the status of
>> these
>> operations.  E.g. the caller is / should be using SOCKET_OWNER
>> attribute.
>> Thus one possibility is to not emit NEW_INTERFACE/DEL_INTERFACE
>> events in
>> such cases.
>>
>
> The breaking up of patches is also confusing. You seem to be
> introducing things in the first, then breaking them again, and then
> fixing them?

Sorry, this was meant to be posted as an RFC.

First patch just introduces a notification utility.  The rest of the 
patches were broken up for ease of review.

>
> Couldn't the whole thing be done in one or maybe two (new/del)
> patch(es)?

Sure, I can squash them together however you like.

>
> (You obviously also need to sign off your patches, see the kernel
> Documentation/)
>

Apologies, still working the kinks out of my environment setup.

Regards,
-Denis

Re: [PATCH 0/5] Improve wireless netdev detection

[Johannes Berg]

On Fri, 2016-07-08 at 10:22 -0500, Denis Kenzior wrote:
> 
> Apologies, I've only been looking at the kernel side for several
> days, so my understanding is still incomplete.
> 
> I was looking at mac80211/iface.c: ieee80211_if_add() which seems to 
> handle NL80211_IFTYPE_P2P_DEVICE specially by not
> creating/registering a 
> net_device object.  For some reason I thought that this object was 
> registered somewhere later, but my understanding was incorrect.  So
> the  entire 'side effect' paragraph above does not apply.

Ok, makes sense.

> Are you okay with the general approach? 

I see no issues with sending these events out. I'd like them to
actually be reliable (if present) though, not double as you'd implied -
but I didn't really understand in which cases you were expecting
issues, was it only P2P-Device?

>  Are there any locking issues I 
> might be overlooking?

Not that I'm aware of. All of the netdev/wdev handling should be
protected by RTNL.

johannes

Re: [PATCH 0/5] Improve wireless netdev detection

[Denis Kenzior]

Hi Johannes,

 >> Are you okay with the general approach?
>
> I see no issues with sending these events out. I'd like them to
> actually be reliable (if present) though, not double as you'd implied -
> but I didn't really understand in which cases you were expecting
> issues, was it only P2P-Device?
>

That seems to be the only special case.  At least I didn't find any 
other situations where a NEW_INTERFACE command can be called without a 
corresponding net_device being created.

>>   Are there any locking issues I
>> might be overlooking?
>
> Not that I'm aware of. All of the netdev/wdev handling should be
> protected by RTNL.
>

That was my understanding as well.  Thanks.

Okay, let me spin up a v2 with patches 2+3 and 4+5 squished together. 
Or do you want one big patch?

Regards,
-Denis