[PATCH v2 1/3] nl80211: Update uapi for CMD_FRAME_WAIT_CANCEL

[PATCH v2 1/3] nl80211: Update uapi for CMD_FRAME_WAIT_CANCEL

[Denis Kenzior]

Commit 1c38c7f22068 ("nl80211: send event when CMD_FRAME duration
expires") added the possibility of NL80211_CMD_FRAME_WAIT_CANCEL
being sent whenever the off-channel wait time associated with a
CMD_FRAME completes.  Document this in the uapi/linux/nl80211.h file.

Signed-off-by: Denis Kenzior <denkenz@gmail.com>
---
 include/uapi/linux/nl80211.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

 Changes in v2:
  - update commit formatting

diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index 8fc3a43cac75..0d9aad98c983 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -657,7 +657,9 @@
  *	is used during CSA period.
  * @NL80211_CMD_FRAME_WAIT_CANCEL: When an off-channel TX was requested, this
  *	command may be used with the corresponding cookie to cancel the wait
- *	time if it is known that it is no longer necessary.
+ *	time if it is known that it is no longer necessary.  This command is
+ *	also sent as an event whenever the driver has completed the off-channel
+ *	wait time.
  * @NL80211_CMD_ACTION: Alias for @NL80211_CMD_FRAME for backward compatibility.
  * @NL80211_CMD_FRAME_TX_STATUS: Report TX status of a management frame
  *	transmitted with %NL80211_CMD_FRAME. %NL80211_ATTR_COOKIE identifies
-- 
2.21.0

[PATCH v2 2/3] nl80211: Limit certain commands to interface owner

[Denis Kenzior]

If the wdev object has been created (via NEW_INTERFACE) with
SOCKET_OWNER attribute set, then limit certain commands only to the
process that created that wdev.

This can be used to make sure no other process on the system interferes
by sending unwanted scans, action frames or any other funny business.

This patch introduces a new internal flag, and checks that flag in the
pre_doit hook.

Signed-off-by: Denis Kenzior <denkenz@gmail.com>
---
 net/wireless/nl80211.c | 80 ++++++++++++++++++++++++++++++++----------
 1 file changed, 61 insertions(+), 19 deletions(-)

diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index ff760ba83449..26bab9560c0f 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -13587,6 +13587,7 @@ static int nl80211_probe_mesh_link(struct sk_buff *skb, struct genl_info *info)
 #define NL80211_FLAG_NEED_WDEV_UP	(NL80211_FLAG_NEED_WDEV |\
 					 NL80211_FLAG_CHECK_NETDEV_UP)
 #define NL80211_FLAG_CLEAR_SKB		0x20
+#define NL80211_FLAG_OWNER_ONLY		0x40
 
 static int nl80211_pre_doit(const struct genl_ops *ops, struct sk_buff *skb,
 			    struct genl_info *info)
@@ -13595,6 +13596,7 @@ static int nl80211_pre_doit(const struct genl_ops *ops, struct sk_buff *skb,
 	struct wireless_dev *wdev;
 	struct net_device *dev;
 	bool rtnl = ops->internal_flags & NL80211_FLAG_NEED_RTNL;
+	int ret;
 
 	if (rtnl)
 		rtnl_lock();
@@ -13602,10 +13604,10 @@ static int nl80211_pre_doit(const struct genl_ops *ops, struct sk_buff *skb,
 	if (ops->internal_flags & NL80211_FLAG_NEED_WIPHY) {
 		rdev = cfg80211_get_dev_from_info(genl_info_net(info), info);
 		if (IS_ERR(rdev)) {
-			if (rtnl)
-				rtnl_unlock();
-			return PTR_ERR(rdev);
+			ret = PTR_ERR(rdev);
+			goto done;
 		}
+
 		info->user_ptr[0] = rdev;
 	} else if (ops->internal_flags & NL80211_FLAG_NEED_NETDEV ||
 		   ops->internal_flags & NL80211_FLAG_NEED_WDEV) {
@@ -13614,32 +13616,33 @@ static int nl80211_pre_doit(const struct genl_ops *ops, struct sk_buff *skb,
 		wdev = __cfg80211_wdev_from_attrs(genl_info_net(info),
 						  info->attrs);
 		if (IS_ERR(wdev)) {
-			if (rtnl)
-				rtnl_unlock();
-			return PTR_ERR(wdev);
+			ret = PTR_ERR(wdev);
+			goto done;
 		}
 
 		dev = wdev->netdev;
 		rdev = wiphy_to_rdev(wdev->wiphy);
 
+		ret = -EINVAL;
 		if (ops->internal_flags & NL80211_FLAG_NEED_NETDEV) {
-			if (!dev) {
-				if (rtnl)
-					rtnl_unlock();
-				return -EINVAL;
-			}
+			if (!dev)
+				goto done;
 
 			info->user_ptr[1] = dev;
 		} else {
 			info->user_ptr[1] = wdev;
 		}
 
+		ret = -ENETDOWN;
 		if (ops->internal_flags & NL80211_FLAG_CHECK_NETDEV_UP &&
-		    !wdev_running(wdev)) {
-			if (rtnl)
-				rtnl_unlock();
-			return -ENETDOWN;
-		}
+		    !wdev_running(wdev))
+			goto done;
+
+		ret = -EPERM;
+		if (ops->internal_flags & NL80211_FLAG_OWNER_ONLY &&
+		    wdev->owner_nlportid &&
+		    wdev->owner_nlportid != info->snd_portid)
+			goto done;
 
 		if (dev)
 			dev_hold(dev);
@@ -13647,7 +13650,13 @@ static int nl80211_pre_doit(const struct genl_ops *ops, struct sk_buff *skb,
 		info->user_ptr[0] = rdev;
 	}
 
-	return 0;
+	ret = 0;
+
+done:
+	if (rtnl && !ret)
+		rtnl_unlock();
+
+	return ret;
 }
 
 static void nl80211_post_doit(const struct genl_ops *ops, struct sk_buff *skb,
@@ -13712,7 +13721,8 @@ static const struct genl_ops nl80211_ops[] = {
 		.doit = nl80211_set_interface,
 		.flags = GENL_UNS_ADMIN_PERM,
 		.internal_flags = NL80211_FLAG_NEED_NETDEV |
-				  NL80211_FLAG_NEED_RTNL,
+				  NL80211_FLAG_NEED_RTNL |
+				  NL80211_FLAG_OWNER_ONLY,
 	},
 	{
 		.cmd = NL80211_CMD_NEW_INTERFACE,
@@ -13728,7 +13738,8 @@ static const struct genl_ops nl80211_ops[] = {
 		.doit = nl80211_del_interface,
 		.flags = GENL_UNS_ADMIN_PERM,
 		.internal_flags = NL80211_FLAG_NEED_WDEV |
-				  NL80211_FLAG_NEED_RTNL,
+				  NL80211_FLAG_NEED_RTNL |
+				  NL80211_FLAG_OWNER_ONLY,
 	},
 	{
 		.cmd = NL80211_CMD_GET_KEY,
@@ -13745,6 +13756,7 @@ static const struct genl_ops nl80211_ops[] = {
 		.flags = GENL_UNS_ADMIN_PERM,
 		.internal_flags = NL80211_FLAG_NEED_NETDEV_UP |
 				  NL80211_FLAG_NEED_RTNL |
+				  NL80211_FLAG_OWNER_ONLY |
 				  NL80211_FLAG_CLEAR_SKB,
 	},
 	{
@@ -13754,6 +13766,7 @@ static const struct genl_ops nl80211_ops[] = {
 		.flags = GENL_UNS_ADMIN_PERM,
 		.internal_flags = NL80211_FLAG_NEED_NETDEV_UP |
 				  NL80211_FLAG_NEED_RTNL |
+				  NL80211_FLAG_OWNER_ONLY |
 				  NL80211_FLAG_CLEAR_SKB,
 	},
 	{
@@ -13762,6 +13775,7 @@ static const struct genl_ops nl80211_ops[] = {
 		.doit = nl80211_del_key,
 		.flags = GENL_UNS_ADMIN_PERM,
 		.internal_flags = NL80211_FLAG_NEED_NETDEV_UP |
+				  NL80211_FLAG_OWNER_ONLY |
 				  NL80211_FLAG_NEED_RTNL,
 	},
 	{
@@ -13778,6 +13792,7 @@ static const struct genl_ops nl80211_ops[] = {
 		.flags = GENL_UNS_ADMIN_PERM,
 		.doit = nl80211_start_ap,
 		.internal_flags = NL80211_FLAG_NEED_NETDEV_UP |
+				  NL80211_FLAG_OWNER_ONLY |
 				  NL80211_FLAG_NEED_RTNL,
 	},
 	{
@@ -13786,6 +13801,7 @@ static const struct genl_ops nl80211_ops[] = {
 		.flags = GENL_UNS_ADMIN_PERM,
 		.doit = nl80211_stop_ap,
 		.internal_flags = NL80211_FLAG_NEED_NETDEV_UP |
+				  NL80211_FLAG_OWNER_ONLY |
 				  NL80211_FLAG_NEED_RTNL,
 	},
 	{
@@ -13802,6 +13818,7 @@ static const struct genl_ops nl80211_ops[] = {
 		.doit = nl80211_set_station,
 		.flags = GENL_UNS_ADMIN_PERM,
 		.internal_flags = NL80211_FLAG_NEED_NETDEV_UP |
+				  NL80211_FLAG_OWNER_ONLY |
 				  NL80211_FLAG_NEED_RTNL,
 	},
 	{
@@ -13810,6 +13827,7 @@ static const struct genl_ops nl80211_ops[] = {
 		.doit = nl80211_new_station,
 		.flags = GENL_UNS_ADMIN_PERM,
 		.internal_flags = NL80211_FLAG_NEED_NETDEV_UP |
+				  NL80211_FLAG_OWNER_ONLY |
 				  NL80211_FLAG_NEED_RTNL,
 	},
 	{
@@ -13818,6 +13836,7 @@ static const struct genl_ops nl80211_ops[] = {
 		.doit = nl80211_del_station,
 		.flags = GENL_UNS_ADMIN_PERM,
 		.internal_flags = NL80211_FLAG_NEED_NETDEV_UP |
+				  NL80211_FLAG_OWNER_ONLY |
 				  NL80211_FLAG_NEED_RTNL,
 	},
 	{
@@ -13921,6 +13940,7 @@ static const struct genl_ops nl80211_ops[] = {
 		.doit = nl80211_trigger_scan,
 		.flags = GENL_UNS_ADMIN_PERM,
 		.internal_flags = NL80211_FLAG_NEED_WDEV_UP |
+				  NL80211_FLAG_OWNER_ONLY |
 				  NL80211_FLAG_NEED_RTNL,
 	},
 	{
@@ -13929,6 +13949,7 @@ static const struct genl_ops nl80211_ops[] = {
 		.doit = nl80211_abort_scan,
 		.flags = GENL_UNS_ADMIN_PERM,
 		.internal_flags = NL80211_FLAG_NEED_WDEV_UP |
+				  NL80211_FLAG_OWNER_ONLY |
 				  NL80211_FLAG_NEED_RTNL,
 	},
 	{
@@ -13942,6 +13963,7 @@ static const struct genl_ops nl80211_ops[] = {
 		.doit = nl80211_start_sched_scan,
 		.flags = GENL_UNS_ADMIN_PERM,
 		.internal_flags = NL80211_FLAG_NEED_NETDEV_UP |
+				  NL80211_FLAG_OWNER_ONLY |
 				  NL80211_FLAG_NEED_RTNL,
 	},
 	{
@@ -13950,6 +13972,7 @@ static const struct genl_ops nl80211_ops[] = {
 		.doit = nl80211_stop_sched_scan,
 		.flags = GENL_UNS_ADMIN_PERM,
 		.internal_flags = NL80211_FLAG_NEED_NETDEV_UP |
+				  NL80211_FLAG_OWNER_ONLY |
 				  NL80211_FLAG_NEED_RTNL,
 	},
 	{
@@ -13958,6 +13981,7 @@ static const struct genl_ops nl80211_ops[] = {
 		.doit = nl80211_authenticate,
 		.flags = GENL_UNS_ADMIN_PERM,
 		.internal_flags = NL80211_FLAG_NEED_NETDEV_UP |
+				  NL80211_FLAG_OWNER_ONLY |
 				  NL80211_FLAG_NEED_RTNL |
 				  NL80211_FLAG_CLEAR_SKB,
 	},
@@ -13967,6 +13991,7 @@ static const struct genl_ops nl80211_ops[] = {
 		.doit = nl80211_associate,
 		.flags = GENL_UNS_ADMIN_PERM,
 		.internal_flags = NL80211_FLAG_NEED_NETDEV_UP |
+				  NL80211_FLAG_OWNER_ONLY |
 				  NL80211_FLAG_NEED_RTNL |
 				  NL80211_FLAG_CLEAR_SKB,
 	},
@@ -13976,6 +14001,7 @@ static const struct genl_ops nl80211_ops[] = {
 		.doit = nl80211_deauthenticate,
 		.flags = GENL_UNS_ADMIN_PERM,
 		.internal_flags = NL80211_FLAG_NEED_NETDEV_UP |
+				  NL80211_FLAG_OWNER_ONLY |
 				  NL80211_FLAG_NEED_RTNL,
 	},
 	{
@@ -13984,6 +14010,7 @@ static const struct genl_ops nl80211_ops[] = {
 		.doit = nl80211_disassociate,
 		.flags = GENL_UNS_ADMIN_PERM,
 		.internal_flags = NL80211_FLAG_NEED_NETDEV_UP |
+				  NL80211_FLAG_OWNER_ONLY |
 				  NL80211_FLAG_NEED_RTNL,
 	},
 	{
@@ -13992,6 +14019,7 @@ static const struct genl_ops nl80211_ops[] = {
 		.doit = nl80211_join_ibss,
 		.flags = GENL_UNS_ADMIN_PERM,
 		.internal_flags = NL80211_FLAG_NEED_NETDEV_UP |
+				  NL80211_FLAG_OWNER_ONLY |
 				  NL80211_FLAG_NEED_RTNL,
 	},
 	{
@@ -14000,6 +14028,7 @@ static const struct genl_ops nl80211_ops[] = {
 		.doit = nl80211_leave_ibss,
 		.flags = GENL_UNS_ADMIN_PERM,
 		.internal_flags = NL80211_FLAG_NEED_NETDEV_UP |
+				  NL80211_FLAG_OWNER_ONLY |
 				  NL80211_FLAG_NEED_RTNL,
 	},
 #ifdef CONFIG_NL80211_TESTMODE
@@ -14019,6 +14048,7 @@ static const struct genl_ops nl80211_ops[] = {
 		.doit = nl80211_connect,
 		.flags = GENL_UNS_ADMIN_PERM,
 		.internal_flags = NL80211_FLAG_NEED_NETDEV_UP |
+				  NL80211_FLAG_OWNER_ONLY |
 				  NL80211_FLAG_NEED_RTNL |
 				  NL80211_FLAG_CLEAR_SKB,
 	},
@@ -14028,6 +14058,7 @@ static const struct genl_ops nl80211_ops[] = {
 		.doit = nl80211_update_connect_params,
 		.flags = GENL_ADMIN_PERM,
 		.internal_flags = NL80211_FLAG_NEED_NETDEV_UP |
+				  NL80211_FLAG_OWNER_ONLY |
 				  NL80211_FLAG_NEED_RTNL |
 				  NL80211_FLAG_CLEAR_SKB,
 	},
@@ -14037,6 +14068,7 @@ static const struct genl_ops nl80211_ops[] = {
 		.doit = nl80211_disconnect,
 		.flags = GENL_UNS_ADMIN_PERM,
 		.internal_flags = NL80211_FLAG_NEED_NETDEV_UP |
+				  NL80211_FLAG_OWNER_ONLY |
 				  NL80211_FLAG_NEED_RTNL,
 	},
 	{
@@ -14083,6 +14115,7 @@ static const struct genl_ops nl80211_ops[] = {
 		.doit = nl80211_remain_on_channel,
 		.flags = GENL_UNS_ADMIN_PERM,
 		.internal_flags = NL80211_FLAG_NEED_WDEV_UP |
+				  NL80211_FLAG_OWNER_ONLY |
 				  NL80211_FLAG_NEED_RTNL,
 	},
 	{
@@ -14091,6 +14124,7 @@ static const struct genl_ops nl80211_ops[] = {
 		.doit = nl80211_cancel_remain_on_channel,
 		.flags = GENL_UNS_ADMIN_PERM,
 		.internal_flags = NL80211_FLAG_NEED_WDEV_UP |
+				  NL80211_FLAG_OWNER_ONLY |
 				  NL80211_FLAG_NEED_RTNL,
 	},
 	{
@@ -14115,6 +14149,7 @@ static const struct genl_ops nl80211_ops[] = {
 		.doit = nl80211_tx_mgmt,
 		.flags = GENL_UNS_ADMIN_PERM,
 		.internal_flags = NL80211_FLAG_NEED_WDEV_UP |
+				  NL80211_FLAG_OWNER_ONLY |
 				  NL80211_FLAG_NEED_RTNL,
 	},
 	{
@@ -14123,6 +14158,7 @@ static const struct genl_ops nl80211_ops[] = {
 		.doit = nl80211_tx_mgmt_cancel_wait,
 		.flags = GENL_UNS_ADMIN_PERM,
 		.internal_flags = NL80211_FLAG_NEED_WDEV_UP |
+				  NL80211_FLAG_OWNER_ONLY |
 				  NL80211_FLAG_NEED_RTNL,
 	},
 	{
@@ -14147,6 +14183,7 @@ static const struct genl_ops nl80211_ops[] = {
 		.doit = nl80211_set_cqm,
 		.flags = GENL_UNS_ADMIN_PERM,
 		.internal_flags = NL80211_FLAG_NEED_NETDEV |
+				  NL80211_FLAG_OWNER_ONLY |
 				  NL80211_FLAG_NEED_RTNL,
 	},
 	{
@@ -14221,6 +14258,7 @@ static const struct genl_ops nl80211_ops[] = {
 		.doit = nl80211_set_rekey_data,
 		.flags = GENL_UNS_ADMIN_PERM,
 		.internal_flags = NL80211_FLAG_NEED_NETDEV_UP |
+				  NL80211_FLAG_OWNER_ONLY |
 				  NL80211_FLAG_NEED_RTNL |
 				  NL80211_FLAG_CLEAR_SKB,
 	},
@@ -14278,6 +14316,7 @@ static const struct genl_ops nl80211_ops[] = {
 		.doit = nl80211_start_p2p_device,
 		.flags = GENL_UNS_ADMIN_PERM,
 		.internal_flags = NL80211_FLAG_NEED_WDEV |
+				  NL80211_FLAG_OWNER_ONLY |
 				  NL80211_FLAG_NEED_RTNL,
 	},
 	{
@@ -14286,6 +14325,7 @@ static const struct genl_ops nl80211_ops[] = {
 		.doit = nl80211_stop_p2p_device,
 		.flags = GENL_UNS_ADMIN_PERM,
 		.internal_flags = NL80211_FLAG_NEED_WDEV_UP |
+				  NL80211_FLAG_OWNER_ONLY |
 				  NL80211_FLAG_NEED_RTNL,
 	},
 	{
@@ -14371,6 +14411,7 @@ static const struct genl_ops nl80211_ops[] = {
 		.doit = nl80211_crit_protocol_start,
 		.flags = GENL_UNS_ADMIN_PERM,
 		.internal_flags = NL80211_FLAG_NEED_WDEV_UP |
+				  NL80211_FLAG_OWNER_ONLY |
 				  NL80211_FLAG_NEED_RTNL,
 	},
 	{
@@ -14379,6 +14420,7 @@ static const struct genl_ops nl80211_ops[] = {
 		.doit = nl80211_crit_protocol_stop,
 		.flags = GENL_UNS_ADMIN_PERM,
 		.internal_flags = NL80211_FLAG_NEED_WDEV_UP |
+				  NL80211_FLAG_OWNER_ONLY |
 				  NL80211_FLAG_NEED_RTNL,
 	},
 	{
-- 
2.21.0

[PATCH v2 3/3] nl80211: Include wiphy address setup in NEW_WIPHY

[Denis Kenzior]

Include wiphy address setup in wiphy dumps and new wiphy events.  The
wiphy permanent address is exposed as ATTR_MAC.  If addr_mask is setup,
then it is included as ATTR_MAC_MASK attribute.  If multiple addresses
are available, then their are exposed in a nested ATTR_MAC_ADDRS array.

This information is already exposed via sysfs, but it makes sense to
include it in the wiphy dump as well.

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

 Changes in v2:
  - Move from case 0 to 9

diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 26bab9560c0f..f4b3e6f1dfbf 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -2166,6 +2166,31 @@ static int nl80211_send_wiphy(struct cfg80211_registered_device *rdev,
 			    rdev->wiphy.vht_capa_mod_mask))
 			goto nla_put_failure;
 
+		if (nla_put(msg, NL80211_ATTR_MAC, ETH_ALEN,
+			    rdev->wiphy.perm_addr))
+			goto nla_put_failure;
+
+		if (!is_zero_ether_addr(rdev->wiphy.addr_mask) &&
+		    nla_put(msg, NL80211_ATTR_MAC_MASK, ETH_ALEN,
+			    rdev->wiphy.addr_mask))
+			goto nla_put_failure;
+
+		if (rdev->wiphy.n_addresses > 1) {
+			void *attr;
+
+			attr = nla_nest_start_noflag(msg,
+						     NL80211_ATTR_MAC_ADDRS);
+			if (!attr)
+				goto nla_put_failure;
+
+			for (i = 0; i < rdev->wiphy.n_addresses; i++)
+				if (nla_put(msg, i + 1, ETH_ALEN,
+					    rdev->wiphy.addresses[i].addr))
+					goto nla_put_failure;
+
+			nla_nest_end(msg, attr);
+		}
+
 		state->split_start++;
 		break;
 	case 10:
-- 
2.21.0

Re: [PATCH v2 2/3] nl80211: Limit certain commands to interface owner

[Arend Van Spriel]

On 6/21/2019 12:07 AM, Denis Kenzior wrote:
> If the wdev object has been created (via NEW_INTERFACE) with
> SOCKET_OWNER attribute set, then limit certain commands only to the
> process that created that wdev.
> 
> This can be used to make sure no other process on the system interferes
> by sending unwanted scans, action frames or any other funny business.

The flag is a good addition opposed to having handlers deal with it. 
However, earlier motivation for SOCKET_OWNER use was about netlink 
multicast being unreliable, which I can agree to. However, avoiding 
"funny business" is a different thing. Our testing infrastructure is 
doing all kind of funny business. Guess we will need to refrain from 
using any user-space wireless tools that use the SOCKET_OWNER attribute, 
but how do we know? Somehow I suspect iwd is one to avoid ;-) I have yet 
to give iwd a spin, but this SOCKET_OWNER strategy kept me from it. 
Maybe iwd could have a developer option which disables the use of the 
SOCKET_OWNER attribute.

Regards,
Arend

> This patch introduces a new internal flag, and checks that flag in the
> pre_doit hook.
> 
> Signed-off-by: Denis Kenzior <denkenz@gmail.com>
> ---
>   net/wireless/nl80211.c | 80 ++++++++++++++++++++++++++++++++----------
>   1 file changed, 61 insertions(+), 19 deletions(-)
> 
> diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
> index ff760ba83449..26bab9560c0f 100644
> --- a/net/wireless/nl80211.c
> +++ b/net/wireless/nl80211.c
> @@ -13587,6 +13587,7 @@ static int nl80211_probe_mesh_link(struct sk_buff *skb, struct genl_info *info)
>   #define NL80211_FLAG_NEED_WDEV_UP	(NL80211_FLAG_NEED_WDEV |\
>   					 NL80211_FLAG_CHECK_NETDEV_UP)
>   #define NL80211_FLAG_CLEAR_SKB		0x20
> +#define NL80211_FLAG_OWNER_ONLY		0x40
>   
>   static int nl80211_pre_doit(const struct genl_ops *ops, struct sk_buff *skb,
>   			    struct genl_info *info)
> @@ -13595,6 +13596,7 @@ static int nl80211_pre_doit(const struct genl_ops *ops, struct sk_buff *skb,
>   	struct wireless_dev *wdev;
>   	struct net_device *dev;
>   	bool rtnl = ops->internal_flags & NL80211_FLAG_NEED_RTNL;
> +	int ret;
>   
>   	if (rtnl)
>   		rtnl_lock();
> @@ -13602,10 +13604,10 @@ static int nl80211_pre_doit(const struct genl_ops *ops, struct sk_buff *skb,
>   	if (ops->internal_flags & NL80211_FLAG_NEED_WIPHY) {
>   		rdev = cfg80211_get_dev_from_info(genl_info_net(info), info);
>   		if (IS_ERR(rdev)) {
> -			if (rtnl)
> -				rtnl_unlock();
> -			return PTR_ERR(rdev);
> +			ret = PTR_ERR(rdev);
> +			goto done;
>   		}
> +
>   		info->user_ptr[0] = rdev;
>   	} else if (ops->internal_flags & NL80211_FLAG_NEED_NETDEV ||
>   		   ops->internal_flags & NL80211_FLAG_NEED_WDEV) {
> @@ -13614,32 +13616,33 @@ static int nl80211_pre_doit(const struct genl_ops *ops, struct sk_buff *skb,
>   		wdev = __cfg80211_wdev_from_attrs(genl_info_net(info),
>   						  info->attrs);
>   		if (IS_ERR(wdev)) {
> -			if (rtnl)
> -				rtnl_unlock();
> -			return PTR_ERR(wdev);
> +			ret = PTR_ERR(wdev);
> +			goto done;
>   		}
>   
>   		dev = wdev->netdev;
>   		rdev = wiphy_to_rdev(wdev->wiphy);
>   
> +		ret = -EINVAL;
>   		if (ops->internal_flags & NL80211_FLAG_NEED_NETDEV) {
> -			if (!dev) {
> -				if (rtnl)
> -					rtnl_unlock();
> -				return -EINVAL;
> -			}
> +			if (!dev)
> +				goto done;
>   
>   			info->user_ptr[1] = dev;
>   		} else {
>   			info->user_ptr[1] = wdev;
>   		}
>   
> +		ret = -ENETDOWN;
>   		if (ops->internal_flags & NL80211_FLAG_CHECK_NETDEV_UP &&
> -		    !wdev_running(wdev)) {
> -			if (rtnl)
> -				rtnl_unlock();
> -			return -ENETDOWN;
> -		}
> +		    !wdev_running(wdev))
> +			goto done;
> +
> +		ret = -EPERM;
> +		if (ops->internal_flags & NL80211_FLAG_OWNER_ONLY &&
> +		    wdev->owner_nlportid &&
> +		    wdev->owner_nlportid != info->snd_portid)
> +			goto done;
>   
>   		if (dev)
>   			dev_hold(dev);
> @@ -13647,7 +13650,13 @@ static int nl80211_pre_doit(const struct genl_ops *ops, struct sk_buff *skb,
>   		info->user_ptr[0] = rdev;
>   	}
>   
> -	return 0;
> +	ret = 0;
> +
> +done:
> +	if (rtnl && !ret)
> +		rtnl_unlock();
> +
> +	return ret;
>   }
>   
>   static void nl80211_post_doit(const struct genl_ops *ops, struct sk_buff *skb,
> @@ -13712,7 +13721,8 @@ static const struct genl_ops nl80211_ops[] = {
>   		.doit = nl80211_set_interface,
>   		.flags = GENL_UNS_ADMIN_PERM,
>   		.internal_flags = NL80211_FLAG_NEED_NETDEV |
> -				  NL80211_FLAG_NEED_RTNL,
> +				  NL80211_FLAG_NEED_RTNL |
> +				  NL80211_FLAG_OWNER_ONLY,
>   	},
>   	{
>   		.cmd = NL80211_CMD_NEW_INTERFACE,
> @@ -13728,7 +13738,8 @@ static const struct genl_ops nl80211_ops[] = {
>   		.doit = nl80211_del_interface,
>   		.flags = GENL_UNS_ADMIN_PERM,
>   		.internal_flags = NL80211_FLAG_NEED_WDEV |
> -				  NL80211_FLAG_NEED_RTNL,
> +				  NL80211_FLAG_NEED_RTNL |
> +				  NL80211_FLAG_OWNER_ONLY,
>   	},
>   	{
>   		.cmd = NL80211_CMD_GET_KEY,
> @@ -13745,6 +13756,7 @@ static const struct genl_ops nl80211_ops[] = {
>   		.flags = GENL_UNS_ADMIN_PERM,
>   		.internal_flags = NL80211_FLAG_NEED_NETDEV_UP |
>   				  NL80211_FLAG_NEED_RTNL |
> +				  NL80211_FLAG_OWNER_ONLY |
>   				  NL80211_FLAG_CLEAR_SKB,
>   	},
>   	{
> @@ -13754,6 +13766,7 @@ static const struct genl_ops nl80211_ops[] = {
>   		.flags = GENL_UNS_ADMIN_PERM,
>   		.internal_flags = NL80211_FLAG_NEED_NETDEV_UP |
>   				  NL80211_FLAG_NEED_RTNL |
> +				  NL80211_FLAG_OWNER_ONLY |
>   				  NL80211_FLAG_CLEAR_SKB,
>   	},
>   	{
> @@ -13762,6 +13775,7 @@ static const struct genl_ops nl80211_ops[] = {
>   		.doit = nl80211_del_key,
>   		.flags = GENL_UNS_ADMIN_PERM,
>   		.internal_flags = NL80211_FLAG_NEED_NETDEV_UP |
> +				  NL80211_FLAG_OWNER_ONLY |
>   				  NL80211_FLAG_NEED_RTNL,
>   	},
>   	{
> @@ -13778,6 +13792,7 @@ static const struct genl_ops nl80211_ops[] = {
>   		.flags = GENL_UNS_ADMIN_PERM,
>   		.doit = nl80211_start_ap,
>   		.internal_flags = NL80211_FLAG_NEED_NETDEV_UP |
> +				  NL80211_FLAG_OWNER_ONLY |
>   				  NL80211_FLAG_NEED_RTNL,
>   	},
>   	{
> @@ -13786,6 +13801,7 @@ static const struct genl_ops nl80211_ops[] = {
>   		.flags = GENL_UNS_ADMIN_PERM,
>   		.doit = nl80211_stop_ap,
>   		.internal_flags = NL80211_FLAG_NEED_NETDEV_UP |
> +				  NL80211_FLAG_OWNER_ONLY |
>   				  NL80211_FLAG_NEED_RTNL,
>   	},
>   	{
> @@ -13802,6 +13818,7 @@ static const struct genl_ops nl80211_ops[] = {
>   		.doit = nl80211_set_station,
>   		.flags = GENL_UNS_ADMIN_PERM,
>   		.internal_flags = NL80211_FLAG_NEED_NETDEV_UP |
> +				  NL80211_FLAG_OWNER_ONLY |
>   				  NL80211_FLAG_NEED_RTNL,
>   	},
>   	{
> @@ -13810,6 +13827,7 @@ static const struct genl_ops nl80211_ops[] = {
>   		.doit = nl80211_new_station,
>   		.flags = GENL_UNS_ADMIN_PERM,
>   		.internal_flags = NL80211_FLAG_NEED_NETDEV_UP |
> +				  NL80211_FLAG_OWNER_ONLY |
>   				  NL80211_FLAG_NEED_RTNL,
>   	},
>   	{
> @@ -13818,6 +13836,7 @@ static const struct genl_ops nl80211_ops[] = {
>   		.doit = nl80211_del_station,
>   		.flags = GENL_UNS_ADMIN_PERM,
>   		.internal_flags = NL80211_FLAG_NEED_NETDEV_UP |
> +				  NL80211_FLAG_OWNER_ONLY |
>   				  NL80211_FLAG_NEED_RTNL,
>   	},
>   	{
> @@ -13921,6 +13940,7 @@ static const struct genl_ops nl80211_ops[] = {
>   		.doit = nl80211_trigger_scan,
>   		.flags = GENL_UNS_ADMIN_PERM,
>   		.internal_flags = NL80211_FLAG_NEED_WDEV_UP |
> +				  NL80211_FLAG_OWNER_ONLY |
>   				  NL80211_FLAG_NEED_RTNL,
>   	},
>   	{
> @@ -13929,6 +13949,7 @@ static const struct genl_ops nl80211_ops[] = {
>   		.doit = nl80211_abort_scan,
>   		.flags = GENL_UNS_ADMIN_PERM,
>   		.internal_flags = NL80211_FLAG_NEED_WDEV_UP |
> +				  NL80211_FLAG_OWNER_ONLY |
>   				  NL80211_FLAG_NEED_RTNL,
>   	},
>   	{
> @@ -13942,6 +13963,7 @@ static const struct genl_ops nl80211_ops[] = {
>   		.doit = nl80211_start_sched_scan,
>   		.flags = GENL_UNS_ADMIN_PERM,
>   		.internal_flags = NL80211_FLAG_NEED_NETDEV_UP |
> +				  NL80211_FLAG_OWNER_ONLY |
>   				  NL80211_FLAG_NEED_RTNL,
>   	},
>   	{
> @@ -13950,6 +13972,7 @@ static const struct genl_ops nl80211_ops[] = {
>   		.doit = nl80211_stop_sched_scan,
>   		.flags = GENL_UNS_ADMIN_PERM,
>   		.internal_flags = NL80211_FLAG_NEED_NETDEV_UP |
> +				  NL80211_FLAG_OWNER_ONLY |
>   				  NL80211_FLAG_NEED_RTNL,
>   	},
>   	{
> @@ -13958,6 +13981,7 @@ static const struct genl_ops nl80211_ops[] = {
>   		.doit = nl80211_authenticate,
>   		.flags = GENL_UNS_ADMIN_PERM,
>   		.internal_flags = NL80211_FLAG_NEED_NETDEV_UP |
> +				  NL80211_FLAG_OWNER_ONLY |
>   				  NL80211_FLAG_NEED_RTNL |
>   				  NL80211_FLAG_CLEAR_SKB,
>   	},
> @@ -13967,6 +13991,7 @@ static const struct genl_ops nl80211_ops[] = {
>   		.doit = nl80211_associate,
>   		.flags = GENL_UNS_ADMIN_PERM,
>   		.internal_flags = NL80211_FLAG_NEED_NETDEV_UP |
> +				  NL80211_FLAG_OWNER_ONLY |
>   				  NL80211_FLAG_NEED_RTNL |
>   				  NL80211_FLAG_CLEAR_SKB,
>   	},
> @@ -13976,6 +14001,7 @@ static const struct genl_ops nl80211_ops[] = {
>   		.doit = nl80211_deauthenticate,
>   		.flags = GENL_UNS_ADMIN_PERM,
>   		.internal_flags = NL80211_FLAG_NEED_NETDEV_UP |
> +				  NL80211_FLAG_OWNER_ONLY |
>   				  NL80211_FLAG_NEED_RTNL,
>   	},
>   	{
> @@ -13984,6 +14010,7 @@ static const struct genl_ops nl80211_ops[] = {
>   		.doit = nl80211_disassociate,
>   		.flags = GENL_UNS_ADMIN_PERM,
>   		.internal_flags = NL80211_FLAG_NEED_NETDEV_UP |
> +				  NL80211_FLAG_OWNER_ONLY |
>   				  NL80211_FLAG_NEED_RTNL,
>   	},
>   	{
> @@ -13992,6 +14019,7 @@ static const struct genl_ops nl80211_ops[] = {
>   		.doit = nl80211_join_ibss,
>   		.flags = GENL_UNS_ADMIN_PERM,
>   		.internal_flags = NL80211_FLAG_NEED_NETDEV_UP |
> +				  NL80211_FLAG_OWNER_ONLY |
>   				  NL80211_FLAG_NEED_RTNL,
>   	},
>   	{
> @@ -14000,6 +14028,7 @@ static const struct genl_ops nl80211_ops[] = {
>   		.doit = nl80211_leave_ibss,
>   		.flags = GENL_UNS_ADMIN_PERM,
>   		.internal_flags = NL80211_FLAG_NEED_NETDEV_UP |
> +				  NL80211_FLAG_OWNER_ONLY |
>   				  NL80211_FLAG_NEED_RTNL,
>   	},
>   #ifdef CONFIG_NL80211_TESTMODE
> @@ -14019,6 +14048,7 @@ static const struct genl_ops nl80211_ops[] = {
>   		.doit = nl80211_connect,
>   		.flags = GENL_UNS_ADMIN_PERM,
>   		.internal_flags = NL80211_FLAG_NEED_NETDEV_UP |
> +				  NL80211_FLAG_OWNER_ONLY |
>   				  NL80211_FLAG_NEED_RTNL |
>   				  NL80211_FLAG_CLEAR_SKB,
>   	},
> @@ -14028,6 +14058,7 @@ static const struct genl_ops nl80211_ops[] = {
>   		.doit = nl80211_update_connect_params,
>   		.flags = GENL_ADMIN_PERM,
>   		.internal_flags = NL80211_FLAG_NEED_NETDEV_UP |
> +				  NL80211_FLAG_OWNER_ONLY |
>   				  NL80211_FLAG_NEED_RTNL |
>   				  NL80211_FLAG_CLEAR_SKB,
>   	},
> @@ -14037,6 +14068,7 @@ static const struct genl_ops nl80211_ops[] = {
>   		.doit = nl80211_disconnect,
>   		.flags = GENL_UNS_ADMIN_PERM,
>   		.internal_flags = NL80211_FLAG_NEED_NETDEV_UP |
> +				  NL80211_FLAG_OWNER_ONLY |
>   				  NL80211_FLAG_NEED_RTNL,
>   	},
>   	{
> @@ -14083,6 +14115,7 @@ static const struct genl_ops nl80211_ops[] = {
>   		.doit = nl80211_remain_on_channel,
>   		.flags = GENL_UNS_ADMIN_PERM,
>   		.internal_flags = NL80211_FLAG_NEED_WDEV_UP |
> +				  NL80211_FLAG_OWNER_ONLY |
>   				  NL80211_FLAG_NEED_RTNL,
>   	},
>   	{
> @@ -14091,6 +14124,7 @@ static const struct genl_ops nl80211_ops[] = {
>   		.doit = nl80211_cancel_remain_on_channel,
>   		.flags = GENL_UNS_ADMIN_PERM,
>   		.internal_flags = NL80211_FLAG_NEED_WDEV_UP |
> +				  NL80211_FLAG_OWNER_ONLY |
>   				  NL80211_FLAG_NEED_RTNL,
>   	},
>   	{
> @@ -14115,6 +14149,7 @@ static const struct genl_ops nl80211_ops[] = {
>   		.doit = nl80211_tx_mgmt,
>   		.flags = GENL_UNS_ADMIN_PERM,
>   		.internal_flags = NL80211_FLAG_NEED_WDEV_UP |
> +				  NL80211_FLAG_OWNER_ONLY |
>   				  NL80211_FLAG_NEED_RTNL,
>   	},
>   	{
> @@ -14123,6 +14158,7 @@ static const struct genl_ops nl80211_ops[] = {
>   		.doit = nl80211_tx_mgmt_cancel_wait,
>   		.flags = GENL_UNS_ADMIN_PERM,
>   		.internal_flags = NL80211_FLAG_NEED_WDEV_UP |
> +				  NL80211_FLAG_OWNER_ONLY |
>   				  NL80211_FLAG_NEED_RTNL,
>   	},
>   	{
> @@ -14147,6 +14183,7 @@ static const struct genl_ops nl80211_ops[] = {
>   		.doit = nl80211_set_cqm,
>   		.flags = GENL_UNS_ADMIN_PERM,
>   		.internal_flags = NL80211_FLAG_NEED_NETDEV |
> +				  NL80211_FLAG_OWNER_ONLY |
>   				  NL80211_FLAG_NEED_RTNL,
>   	},
>   	{
> @@ -14221,6 +14258,7 @@ static const struct genl_ops nl80211_ops[] = {
>   		.doit = nl80211_set_rekey_data,
>   		.flags = GENL_UNS_ADMIN_PERM,
>   		.internal_flags = NL80211_FLAG_NEED_NETDEV_UP |
> +				  NL80211_FLAG_OWNER_ONLY |
>   				  NL80211_FLAG_NEED_RTNL |
>   				  NL80211_FLAG_CLEAR_SKB,
>   	},
> @@ -14278,6 +14316,7 @@ static const struct genl_ops nl80211_ops[] = {
>   		.doit = nl80211_start_p2p_device,
>   		.flags = GENL_UNS_ADMIN_PERM,
>   		.internal_flags = NL80211_FLAG_NEED_WDEV |
> +				  NL80211_FLAG_OWNER_ONLY |
>   				  NL80211_FLAG_NEED_RTNL,
>   	},
>   	{
> @@ -14286,6 +14325,7 @@ static const struct genl_ops nl80211_ops[] = {
>   		.doit = nl80211_stop_p2p_device,
>   		.flags = GENL_UNS_ADMIN_PERM,
>   		.internal_flags = NL80211_FLAG_NEED_WDEV_UP |
> +				  NL80211_FLAG_OWNER_ONLY |
>   				  NL80211_FLAG_NEED_RTNL,
>   	},
>   	{
> @@ -14371,6 +14411,7 @@ static const struct genl_ops nl80211_ops[] = {
>   		.doit = nl80211_crit_protocol_start,
>   		.flags = GENL_UNS_ADMIN_PERM,
>   		.internal_flags = NL80211_FLAG_NEED_WDEV_UP |
> +				  NL80211_FLAG_OWNER_ONLY |
>   				  NL80211_FLAG_NEED_RTNL,
>   	},
>   	{
> @@ -14379,6 +14420,7 @@ static const struct genl_ops nl80211_ops[] = {
>   		.doit = nl80211_crit_protocol_stop,
>   		.flags = GENL_UNS_ADMIN_PERM,
>   		.internal_flags = NL80211_FLAG_NEED_WDEV_UP |
> +				  NL80211_FLAG_OWNER_ONLY |
>   				  NL80211_FLAG_NEED_RTNL,
>   	},
>   	{
> 

Re: [PATCH v2 2/3] nl80211: Limit certain commands to interface owner

[Marcel Holtmann]

Hi Arend,

>> If the wdev object has been created (via NEW_INTERFACE) with
>> SOCKET_OWNER attribute set, then limit certain commands only to the
>> process that created that wdev.
>> This can be used to make sure no other process on the system interferes
>> by sending unwanted scans, action frames or any other funny business.
> 
> The flag is a good addition opposed to having handlers deal with it. However, earlier motivation for SOCKET_OWNER use was about netlink multicast being unreliable, which I can agree to. However, avoiding "funny business" is a different thing. Our testing infrastructure is doing all kind of funny business. Guess we will need to refrain from using any user-space wireless tools that use the SOCKET_OWNER attribute, but how do we know? Somehow I suspect iwd is one to avoid ;-) I have yet to give iwd a spin, but this SOCKET_OWNER strategy kept me from it. Maybe iwd could have a developer option which disables the use of the SOCKET_OWNER attribute.

when running iwd, we expect reproducible behavior. Meaning we need to ensure that nobody else is messing with our interfaces behind our back. A testing infrastructure that does that is really no good in the first place since you yourself are introducing unclean behavior.

When testing with iwd, we are testing the D-Bus API of iwd and you can at the same time take PCAP traces with iwmon. If we are able to store trace-cmd information also in the same PCAP file, we can extend iwmon to do exactly that. So far iwmon allows you to grab the netlink communication and the PAE communication which means you can easily analyze what was happening without having ask nl80211.

If you require extra debug information or triggers, then this has to happen via a D-Bus debug interfaces. However I see no benefit of not using SOCKET_OWNER. As I said above, if the testing uses different options than the real environment, what good is the testing.

Regards

Marcel

Re: [PATCH v2 2/3] nl80211: Limit certain commands to interface owner

[Denis Kenzior]

Hi Arend,

On 06/21/2019 03:09 AM, Arend Van Spriel wrote:
> On 6/21/2019 12:07 AM, Denis Kenzior wrote:
>> If the wdev object has been created (via NEW_INTERFACE) with
>> SOCKET_OWNER attribute set, then limit certain commands only to the
>> process that created that wdev.
>>
>> This can be used to make sure no other process on the system interferes
>> by sending unwanted scans, action frames or any other funny business.
> 
> The flag is a good addition opposed to having handlers deal with it. 
> However, earlier motivation for SOCKET_OWNER use was about netlink 
> multicast being unreliable, which I can agree to. However, avoiding 

???  I can't agree to that as I have no idea what you're talking about 
:)  Explain?  SOCKET_OWNER was introduced mainly to bring down links / 
scans / whatever in case the initiating process died.  As a side effect 
it also helped in the beginning when users ran iwd + wpa_s 
simultaneously (by accident) and all sorts of fun ensued.  We then 
re-used SOCKET_OWNER for running EAPoL over NL80211.  But 'multicast 
unreliability' was never an issue that I recall?

> "funny business" is a different thing. Our testing infrastructure is 
> doing all kind of funny business. Guess we will need to refrain from 

So you're going behind the managing daemon's back and messing with the 
kernel state...  I guess the question is why?  But really, if wpa_s 
wants to tolerate that, that is their problem :)  iwd doesn't want to, 
nor do we want to deal with the various race conditions and corner cases 
associated with that.  Life is hard as it is ;)

> using any user-space wireless tools that use the SOCKET_OWNER attribute, 
> but how do we know? Somehow I suspect iwd is one to avoid ;-) I have yet 

I guess you will be avoiding wpa_s since that one uses SOCKET_OWNER too ;)

> to give iwd a spin, but this SOCKET_OWNER strategy kept me from it. 
> Maybe iwd could have a developer option which disables the use of the 
> SOCKET_OWNER attribute.

Okay?  Not sure what you're trying to say here?  I'd interpret this as 
"You guys suck.  I'm taking my ball and going home?" but I hope this 
isn't what you're saying?

Regards,
-Denis

Re: [PATCH v2 2/3] nl80211: Limit certain commands to interface owner

[Arend Van Spriel]

On 6/21/2019 7:14 PM, Denis Kenzior wrote:
> Hi Arend,
> 
> On 06/21/2019 03:09 AM, Arend Van Spriel wrote:
>> On 6/21/2019 12:07 AM, Denis Kenzior wrote:
>>> If the wdev object has been created (via NEW_INTERFACE) with
>>> SOCKET_OWNER attribute set, then limit certain commands only to the
>>> process that created that wdev.
>>>
>>> This can be used to make sure no other process on the system interferes
>>> by sending unwanted scans, action frames or any other funny business.
>>
>> The flag is a good addition opposed to having handlers deal with it. 
>> However, earlier motivation for SOCKET_OWNER use was about netlink 
>> multicast being unreliable, which I can agree to. However, avoiding 
> 
> ???  I can't agree to that as I have no idea what you're talking about 
> :)  Explain?  SOCKET_OWNER was introduced mainly to bring down links / 
> scans / whatever in case the initiating process died.  As a side effect 
> it also helped in the beginning when users ran iwd + wpa_s 
> simultaneously (by accident) and all sorts of fun ensued.  We then 
> re-used SOCKET_OWNER for running EAPoL over NL80211.  But 'multicast 
> unreliability' was never an issue that I recall?

hmm. I tried searching in memory... of my email client but to no avail. 
I somehow recalled that netlink multicast was not guaranteed to be 
delivered/seen by all listeners.

>> "funny business" is a different thing. Our testing infrastructure is 
>> doing all kind of funny business. Guess we will need to refrain from 
> 
> So you're going behind the managing daemon's back and messing with the 
> kernel state...  I guess the question is why?  But really, if wpa_s 
> wants to tolerate that, that is their problem :)  iwd doesn't want to, 
> nor do we want to deal with the various race conditions and corner cases 
> associated with that.  Life is hard as it is ;)

That's just it, right. This is what Marcel calls the real environment, 
but is it. The nl80211 is a kernel API and should that mean that there 
must be a managing daemon locking down APIs for other user-space tools 
to use. If I want a user-space app to show a radar screen with 
surrounding APs using scanning and FTM nl80211 commands it seems now it 
has to create a new interface and hope the resources are there for it to 
succeed. Where is my freedom in that? If I am using such an app don't 
you think I don't accept it could impact the managing daemon.

>> using any user-space wireless tools that use the SOCKET_OWNER 
>> attribute, but how do we know? Somehow I suspect iwd is one to avoid 
>> ;-) I have yet 
> 
> I guess you will be avoiding wpa_s since that one uses SOCKET_OWNER too ;)

Probably. One of my concerns was about NL80211_CMD_CONNECT event, but 
checking nl80211_send_connect_result() it seems to just send it to the 
mlme multicast group regardless SOCKET_OWNER use.

>> to give iwd a spin, but this SOCKET_OWNER strategy kept me from it. 
>> Maybe iwd could have a developer option which disables the use of the 
>> SOCKET_OWNER attribute.
> 
> Okay?  Not sure what you're trying to say here?  I'd interpret this as 
> "You guys suck.  I'm taking my ball and going home?" but I hope this 
> isn't what you're saying?

Not saying that. Just saying that the "real environment" is in the eye 
of the beholder and it would be nice if there was a way to opt out, but 
Marcel seems strongly opposed to it. So there seems no point in 
scratching that itch and come up with a patch.

Regards,
Arend

Re: [PATCH v2 2/3] nl80211: Limit certain commands to interface owner

[Denis Kenzior]

Hi Arend,

>>> "funny business" is a different thing. Our testing infrastructure is 
>>> doing all kind of funny business. Guess we will need to refrain from 
>>
>> So you're going behind the managing daemon's back and messing with the 
>> kernel state...  I guess the question is why?  But really, if wpa_s 
>> wants to tolerate that, that is their problem :)  iwd doesn't want to, 
>> nor do we want to deal with the various race conditions and corner 
>> cases associated with that.  Life is hard as it is ;)
> 
> That's just it, right. This is what Marcel calls the real environment, 
> but is it. The nl80211 is a kernel API and should that mean that there 
> must be a managing daemon locking down APIs for other user-space tools 
> to use. If I want a user-space app to show a radar screen with 
> surrounding APs using scanning and FTM nl80211 commands it seems now it 
> has to create a new interface and hope the resources are there for it to 
> succeed. Where is my freedom in that? If I am using such an app don't 
> you think I don't accept it could impact the managing daemon.

I get it.  But on the flip side, should the managing daemon accept you 
messing with it? I mean there is a definite associated cost here, 
whether it is stuff crashing, having to account for extra corner cases 
and race conditions, giving out erroneous results, etc.

As Marcel pointed out, the proper solution is to do this via some 
diagnostic interface on the managing daemon, so it can properly manage 
such requests to not interfere with whatever else is going on.

By the way, the above would be generally useful to many people, so if 
you have some code lying around... ;)

>>> to give iwd a spin, but this SOCKET_OWNER strategy kept me from it. 
>>> Maybe iwd could have a developer option which disables the use of the 
>>> SOCKET_OWNER attribute.
>>
>> Okay?  Not sure what you're trying to say here?  I'd interpret this as 
>> "You guys suck.  I'm taking my ball and going home?" but I hope this 
>> isn't what you're saying?
> 
> Not saying that. Just saying that the "real environment" is in the eye 
> of the beholder and it would be nice if there was a way to opt out, but 
> Marcel seems strongly opposed to it. So there seems no point in 
> scratching that itch and come up with a patch.
> 

I guess the question is, what do you want this for?  If you want this 
for pure manual testing and accept the consequence of the managing 
daemon crashing, giving erroneous results or being otherwise confused? 
If you're fine with the above, I don't see a problem with such a patch.

Regards,
-Denis

Re: [PATCH v2 2/3] nl80211: Limit certain commands to interface owner

[Marcel Holtmann]

Hi Arend,

>>>> If the wdev object has been created (via NEW_INTERFACE) with
>>>> SOCKET_OWNER attribute set, then limit certain commands only to the
>>>> process that created that wdev.
>>>> 
>>>> This can be used to make sure no other process on the system interferes
>>>> by sending unwanted scans, action frames or any other funny business.
>>> 
>>> The flag is a good addition opposed to having handlers deal with it. However, earlier motivation for SOCKET_OWNER use was about netlink multicast being unreliable, which I can agree to. However, avoiding 
>> ???  I can't agree to that as I have no idea what you're talking about :)  Explain?  SOCKET_OWNER was introduced mainly to bring down links / scans / whatever in case the initiating process died.  As a side effect it also helped in the beginning when users ran iwd + wpa_s simultaneously (by accident) and all sorts of fun ensued.  We then re-used SOCKET_OWNER for running EAPoL over NL80211.  But 'multicast unreliability' was never an issue that I recall?
> 
> hmm. I tried searching in memory... of my email client but to no avail. I somehow recalled that netlink multicast was not guaranteed to be delivered/seen by all listeners.
> 
>>> "funny business" is a different thing. Our testing infrastructure is doing all kind of funny business. Guess we will need to refrain from 
>> So you're going behind the managing daemon's back and messing with the kernel state...  I guess the question is why?  But really, if wpa_s wants to tolerate that, that is their problem :)  iwd doesn't want to, nor do we want to deal with the various race conditions and corner cases associated with that.  Life is hard as it is ;)
> 
> That's just it, right. This is what Marcel calls the real environment, but is it. The nl80211 is a kernel API and should that mean that there must be a managing daemon locking down APIs for other user-space tools to use. If I want a user-space app to show a radar screen with surrounding APs using scanning and FTM nl80211 commands it seems now it has to create a new interface and hope the resources are there for it to succeed. Where is my freedom in that? If I am using such an app don't you think I don't accept it could impact the managing daemon.

if you are operating on a shared radio resource you have to have some way of ensuring that nobody steals resources from you. Having an external application that will also use scanning and other off-channel operation will result in a bad experience. Especially if it involves scanning. Currently we still have 3 or more parties triggering scanning on nl80211. Essentially they are now fighting for radio time. You have wpa_supplicant scanning, you have NetworkManager scanning and you have the UI scanning. Now adding just another application that just scans at its decided time location / direction finding is not helping the situation.

If our kernel cfg80211 / nl80211 would be smart enough to handle these concurrent tasks, I would have little objection to let all clients do whatever they want, but we don’t have that. I do not want an external application messing with my planned radio time. And frankly if I am in the middle of roaming, I don’t want to be delayed because some fancy radar looking UI decides to start a full spectrum scan or blocks us via an action frame that times out.

With iwd we are moving towards the direction that we are utilizing the information from access points and surrounding networks to intelligently scan and reduce the time spent scanning to a minimum. For us that is the way to improve WiFi experience for Linux.

We have been through this with Bluetooth already years ago. You need a central daemon that watches out for your radio utilization. Doing anything behind the back of such a daemon is not going to work out long term. Same applies to 2G/3G/LTE where even more tasks need to be managed. And even wpa_supplicant has an internal mutex to control radio time.

Regards

Marcel

Re: [PATCH v2 2/3] nl80211: Limit certain commands to interface owner

[Arend Van Spriel]

On 6/22/2019 3:44 PM, Marcel Holtmann wrote:
> Hi Arend,
> 
>>>>> If the wdev object has been created (via NEW_INTERFACE) with
>>>>> SOCKET_OWNER attribute set, then limit certain commands only to the
>>>>> process that created that wdev.
>>>>>
>>>>> This can be used to make sure no other process on the system interferes
>>>>> by sending unwanted scans, action frames or any other funny business.
>>>>
>>>> The flag is a good addition opposed to having handlers deal with it. However, earlier motivation for SOCKET_OWNER use was about netlink multicast being unreliable, which I can agree to. However, avoiding
>>> ???  I can't agree to that as I have no idea what you're talking about :)  Explain?  SOCKET_OWNER was introduced mainly to bring down links / scans / whatever in case the initiating process died.  As a side effect it also helped in the beginning when users ran iwd + wpa_s simultaneously (by accident) and all sorts of fun ensued.  We then re-used SOCKET_OWNER for running EAPoL over NL80211.  But 'multicast unreliability' was never an issue that I recall?
>>
>> hmm. I tried searching in memory... of my email client but to no avail. I somehow recalled that netlink multicast was not guaranteed to be delivered/seen by all listeners.
>>
>>>> "funny business" is a different thing. Our testing infrastructure is doing all kind of funny business. Guess we will need to refrain from
>>> So you're going behind the managing daemon's back and messing with the kernel state...  I guess the question is why?  But really, if wpa_s wants to tolerate that, that is their problem :)  iwd doesn't want to, nor do we want to deal with the various race conditions and corner cases associated with that.  Life is hard as it is ;)
>>
>> That's just it, right. This is what Marcel calls the real environment, but is it. The nl80211 is a kernel API and should that mean that there must be a managing daemon locking down APIs for other user-space tools to use. If I want a user-space app to show a radar screen with surrounding APs using scanning and FTM nl80211 commands it seems now it has to create a new interface and hope the resources are there for it to succeed. Where is my freedom in that? If I am using such an app don't you think I don't accept it could impact the managing daemon.
> 
> if you are operating on a shared radio resource you have to have some way of ensuring that nobody steals resources from you. Having an external application that will also use scanning and other off-channel operation will result in a bad experience. Especially if it involves scanning. Currently we still have 3 or more parties triggering scanning on nl80211. Essentially they are now fighting for radio time. You have wpa_supplicant scanning, you have NetworkManager scanning and you have the UI scanning. Now adding just another application that just scans at its decided time location / direction finding is not helping the situation.

My app was just a hypothetical example. I understand your conundrum, but 
my point was that you can not know how a system is configured. Now for 
the SOCKET_OWNER I should say it does not provide you any guarantees. At 
best it improves your chances. With the nl80211 API being as it is, you 
can not rule out multiple application controlling the same device. The 
virtual interfaces can be guarded with SOCKET_OWNER, but in the end 
there is still one physical device and only if you are lucky you may 
come across a device with two physical radios, but most of them just 
have one. If you really want to be in control we should allow only one 
socket or at least only one "control" socket.

> If our kernel cfg80211 / nl80211 would be smart enough to handle these concurrent tasks, I would have little objection to let all clients do whatever they want, but we don’t have that. I do not want an external application messing with my planned radio time. And frankly if I am in the middle of roaming, I don’t want to be delayed because some fancy radar looking UI decides to start a full spectrum scan or blocks us via an action frame that times out.

The have been some efforts to handle concurrent use. For scheduled scan 
concurrency was added and critical proto primitives allow to temporarily 
disable scans when user-space needs it, eg. for EAPOL or DHCP exchange.

> With iwd we are moving towards the direction that we are utilizing the information from access points and surrounding networks to intelligently scan and reduce the time spent scanning to a minimum. For us that is the way to improve WiFi experience for Linux.
> 
> We have been through this with Bluetooth already years ago. You need a central daemon that watches out for your radio utilization. Doing anything behind the back of such a daemon is not going to work out long term. Same applies to 2G/3G/LTE where even more tasks need to be managed. And even wpa_supplicant has an internal mutex to control radio time.

Right. Given how nl80211 works today the only real control of radio time 
would need to be done in kernel space or go for the one "control" socket 
approach.

Regards,
Arend

Re: [PATCH v2 2/3] nl80211: Limit certain commands to interface owner

[Denis Kenzior]

Hi Arend,

On 06/24/2019 03:39 AM, Arend Van Spriel wrote:
> On 6/22/2019 3:44 PM, Marcel Holtmann wrote:
>> Hi Arend,
>>
>>>>>> If the wdev object has been created (via NEW_INTERFACE) with
>>>>>> SOCKET_OWNER attribute set, then limit certain commands only to the
>>>>>> process that created that wdev.
>>>>>>
>>>>>> This can be used to make sure no other process on the system 
>>>>>> interferes
>>>>>> by sending unwanted scans, action frames or any other funny business.
>>>>>
>>>>> The flag is a good addition opposed to having handlers deal with 
>>>>> it. However, earlier motivation for SOCKET_OWNER use was about 
>>>>> netlink multicast being unreliable, which I can agree to. However, 
>>>>> avoiding
>>>> ???  I can't agree to that as I have no idea what you're talking 
>>>> about :)  Explain?  SOCKET_OWNER was introduced mainly to bring down 
>>>> links / scans / whatever in case the initiating process died.  As a 
>>>> side effect it also helped in the beginning when users ran iwd + 
>>>> wpa_s simultaneously (by accident) and all sorts of fun ensued.  We 
>>>> then re-used SOCKET_OWNER for running EAPoL over NL80211.  But 
>>>> 'multicast unreliability' was never an issue that I recall?
>>>
>>> hmm. I tried searching in memory... of my email client but to no 
>>> avail. I somehow recalled that netlink multicast was not guaranteed 
>>> to be delivered/seen by all listeners.
>>>
>>>>> "funny business" is a different thing. Our testing infrastructure 
>>>>> is doing all kind of funny business. Guess we will need to refrain 
>>>>> from
>>>> So you're going behind the managing daemon's back and messing with 
>>>> the kernel state...  I guess the question is why?  But really, if 
>>>> wpa_s wants to tolerate that, that is their problem :)  iwd doesn't 
>>>> want to, nor do we want to deal with the various race conditions and 
>>>> corner cases associated with that.  Life is hard as it is ;)
>>>
>>> That's just it, right. This is what Marcel calls the real 
>>> environment, but is it. The nl80211 is a kernel API and should that 
>>> mean that there must be a managing daemon locking down APIs for other 
>>> user-space tools to use. If I want a user-space app to show a radar 
>>> screen with surrounding APs using scanning and FTM nl80211 commands 
>>> it seems now it has to create a new interface and hope the resources 
>>> are there for it to succeed. Where is my freedom in that? If I am 
>>> using such an app don't you think I don't accept it could impact the 
>>> managing daemon.
>>
>> if you are operating on a shared radio resource you have to have some 
>> way of ensuring that nobody steals resources from you. Having an 
>> external application that will also use scanning and other off-channel 
>> operation will result in a bad experience. Especially if it involves 
>> scanning. Currently we still have 3 or more parties triggering 
>> scanning on nl80211. Essentially they are now fighting for radio time. 
>> You have wpa_supplicant scanning, you have NetworkManager scanning and 
>> you have the UI scanning. Now adding just another application that 
>> just scans at its decided time location / direction finding is not 
>> helping the situation.
> 
> My app was just a hypothetical example. I understand your conundrum, but 
> my point was that you can not know how a system is configured. Now for 
> the SOCKET_OWNER I should say it does not provide you any guarantees. At 
> best it improves your chances. With the nl80211 API being as it is, you 
> can not rule out multiple application controlling the same device. The 
> virtual interfaces can be guarded with SOCKET_OWNER, but in the end 

+1, SOCKET_OWNER is a band-aid.  But in the end this is a fairly simple 
change.   So we can have the managing daemon destroy any existing wdevs 
and re-create them with SOCKET_OWNER set (can we get brcmfmac to support 
this please?).  This at least gives us a chance that nothing will 
inadvertently cause interference.  It won't stop other processes from 
creating other wdevs or other funny business, but that is currently much 
more rare anyway.  In case it really becomes a problem, we can at least 
say "Don't hold it that way."

> there is still one physical device and only if you are lucky you may 
> come across a device with two physical radios, but most of them just 
> have one. If you really want to be in control we should allow only one 
> socket or at least only one "control" socket.

+1.  I've been saying this for a while, there should only be a single 
controlling socket/process, or at least an option for something like 
that.  But the current nl80211 design makes this quite hard.  Hopefully 
the recognition that this is needed will gain traction, until then we're 
stuck with band-aids.

> 
>> If our kernel cfg80211 / nl80211 would be smart enough to handle these 
>> concurrent tasks, I would have little objection to let all clients do 
>> whatever they want, but we don’t have that. I do not want an external 
>> application messing with my planned radio time. And frankly if I am in 
>> the middle of roaming, I don’t want to be delayed because some fancy 
>> radar looking UI decides to start a full spectrum scan or blocks us 
>> via an action frame that times out.
> 
> The have been some efforts to handle concurrent use. For scheduled scan 
> concurrency was added and critical proto primitives allow to temporarily 
> disable scans when user-space needs it, eg. for EAPOL or DHCP exchange.

Which only a single driver seems to support, unfortunately.

Regards,
-Denis

> 
>> With iwd we are moving towards the direction that we are utilizing the 
>> information from access points and surrounding networks to 
>> intelligently scan and reduce the time spent scanning to a minimum. 
>> For us that is the way to improve WiFi experience for Linux.
>>
>> We have been through this with Bluetooth already years ago. You need a 
>> central daemon that watches out for your radio utilization. Doing 
>> anything behind the back of such a daemon is not going to work out 
>> long term. Same applies to 2G/3G/LTE where even more tasks need to be 
>> managed. And even wpa_supplicant has an internal mutex to control 
>> radio time.
> 
> Right. Given how nl80211 works today the only real control of radio time 
> would need to be done in kernel space or go for the one "control" socket 
> approach.
> 
> Regards,
> Arend