* Re: [PATCH 3/3] nl80211: Include wiphy address setup in NEW_WIPHY
From: Denis Kenzior @ 2019-06-20 20:35 UTC (permalink / raw)
To: Johannes Berg; +Cc: linux-wireless
In-Reply-To: <144f36779085498bdc1b2f7ac0d0c267d431f51d.camel@sipsolutions.net>
Hi Johannes,
On 06/20/2019 03:09 PM, Johannes Berg wrote:
> On Thu, 2019-06-20 at 15:05 -0500, Denis Kenzior wrote:
>>
>> Ugh. So, if I understand this correctly, NEW_WIPHY events that are
>> generated when a new wiphy is plugged would only send the old 'legacy'
>> info and any info we add in cases 9+ would be 'lost' and the application
>> is forced into re-dumping the phy.
>
> Yes.
>
>> This is pretty much counter to what we want.
>
> Well, you want the info, shouldn't matter how you get it?
>
Well, it kind of does. You're asking userspace to introduce extra
complexity, extra round trips, extra stuff to go wrong just because the
kernel API has painted itself into a corner.
>> If you want to keep your sanity in userspace, you need proper 'object
>> appeared' / 'object disappeared' events from the kernel.
>
> Sure, but you don't really need to know *everything* about the events
> right there ... you can already filter which ones you care about
> (perhaps you know you never want to bind hwsim ones for example) and
> then request data on those that you do need.
Sure, but it would be nice to have all the info available if we do not
want to filter it...
>
>> And those
>> events should have all or nearly all info to not bother the kernel going
>> forward.
>
> That's what you wish for, but ...
Well, it is a pretty basic requirement for any event driven API, no?
>
>> It sounds like nl80211 API has run into the extend-ability
>> wall, no?
>
> I don't really see it that way.
>
>> Any suggestions on how to resolve this? Should NEW_WIPHY events also do
>> the whole split_dump semantic and generate 15+ or whatever messages?
>
> No, that'd be awful, and anyway you'd have to send a new command because
> otherwise old applications might be completely confused (not that I know
> of any other than "iw event" that would event listen to this, but who
> knows)
Well, given that we're the only ones that seem to care about this right
now, I don't see sending a new command as much of a big deal. I welcome
other ideas, but having the kernel send us an event, then us turning
around and requesting the *same* info is just silly.
Regards,
-Denis
^ permalink raw reply
* [PATCH v2 1/3] nl80211: Update uapi for CMD_FRAME_WAIT_CANCEL
From: Denis Kenzior @ 2019-06-20 22:07 UTC (permalink / raw)
To: Johannes Berg; +Cc: linux-wireless, 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
^ permalink raw reply related
* [PATCH v2 2/3] nl80211: Limit certain commands to interface owner
From: Denis Kenzior @ 2019-06-20 22:07 UTC (permalink / raw)
To: Johannes Berg; +Cc: linux-wireless, Denis Kenzior
In-Reply-To: <20190620220749.10071-1-denkenz@gmail.com>
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
^ permalink raw reply related
* [PATCH v2 3/3] nl80211: Include wiphy address setup in NEW_WIPHY
From: Denis Kenzior @ 2019-06-20 22:07 UTC (permalink / raw)
To: Johannes Berg; +Cc: linux-wireless, Denis Kenzior
In-Reply-To: <20190620220749.10071-1-denkenz@gmail.com>
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
^ permalink raw reply related
* Re: [PATCH 3/3] nl80211: Include wiphy address setup in NEW_WIPHY
From: Denis Kenzior @ 2019-06-20 23:51 UTC (permalink / raw)
To: Johannes Berg; +Cc: linux-wireless
In-Reply-To: <cc4f1755ee5f27c72cbac344988bcb91a1a835f3.camel@sipsolutions.net>
Hi Johannes,
On 06/20/2019 03:21 PM, Johannes Berg wrote:
> On Thu, 2019-06-20 at 22:09 +0200, Johannes Berg wrote:
>>
>> Sure, but you don't really need to know *everything* about the events
>> right there ... you can already filter which ones you care about
>> (perhaps you know you never want to bind hwsim ones for example) and
>> then request data on those that you do need.
>
> Btw, you can send a filter down when you do request the data, so you
> only get the data for the new wiphy you actually just discovered.
Yes, I know that. I did help fix this ~3 years ago in commit
b7fb44dacae04. Nobody was using that prior, which really leads me to
wonder what other userspace tools are doing for hotplug and how broken
they are...
>
> So realistically, vs. your suggestion of sending all of the data in
> multiple events, that just adds 2 messages (the request and the data you
> already had), which isn't nearly as bad as you paint it.
I never 'painted' the message overhead as 'bad'. The performance
overhead of this ping-pong is probably irrelevant in the grand scheme of
things. But I find the approach inelegant.
But really I'm more worried about race conditions that userspace has to
deal with. We already have the weird case of ATTR_GENERATION (which
nobody actually uses btw). And then we also need to dump both the
wiphys and the interfaces separately, cross-reference them while dealing
with the possibility of a wiphy or interface going away or being added
at any point. Then there's the fact that some drivers always add a
default netdev, others that (possibly) don't and the possibility that
the system was left in a weird state.
So from that standpoint it is far better to have the kernel generate
atomic change events with all the info present than having userspace
re-poll it when stuff might have changed in the meantime.
Going back to your 2 message point. What about sending the 'NEW_WIPHY'
event with the info in labels 0-8. And then another event type with the
'rest' of the info. And perhaps another feature bit to tell userspace
to expect multiple events. That would still end up being 2 messages and
still be more efficient than the ping-pong you suggest.
Regards,
-Denis
^ permalink raw reply
* Re: [PATCH v2 2/3] nl80211: Limit certain commands to interface owner
From: Arend Van Spriel @ 2019-06-21 8:09 UTC (permalink / raw)
To: Denis Kenzior, Johannes Berg; +Cc: linux-wireless
In-Reply-To: <20190620220749.10071-2-denkenz@gmail.com>
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,
> },
> {
>
^ permalink raw reply
* Re: [PATCH] rt2x00: fix rx queue hang
From: Soeren Moch @ 2019-06-21 11:30 UTC (permalink / raw)
To: Stanislaw Gruszka
Cc: Helmut Schaa, Kalle Valo, David S. Miller, linux-wireless, netdev,
linux-kernel, stable
In-Reply-To: <20190618093431.GA2577@redhat.com>
Hi!
On 18.06.19 11:34, Stanislaw Gruszka wrote:
> Hi
>
> On Mon, Jun 17, 2019 at 11:46:56AM +0200, Soeren Moch wrote:
>> Since commit ed194d136769 ("usb: core: remove local_irq_save() around
>> ->complete() handler") the handlers rt2x00usb_interrupt_rxdone() and
>> rt2x00usb_interrupt_txdone() are not running with interrupts disabled
>> anymore. So these handlers are not guaranteed to run completely before
>> workqueue processing starts. So only mark entries ready for workqueue
>> processing after proper accounting in the dma done queue.
> It was always the case on SMP machines that rt2x00usb_interrupt_{tx/rx}done
> can run concurrently with rt2x00_work_{rx,tx}done, so I do not
> understand how removing local_irq_save() around complete handler broke
> things.
I think because completion handlers can be interrupted now and scheduled
away
in the middle of processing.
> Have you reverted commit ed194d136769 and the revert does solve the problem ?
Yes, I already sent a patch for this, see [1]. But this was not considered
an acceptablesolution. Especially RT folks do not like code running with
interrupts disabled,particularly when trying to acquire spinlocks then.
[1] https://lkml.org/lkml/2019/5/31/863
> Between 4.19 and 4.20 we have some quite big changes in rt2x00 driver:
>
> 0240564430c0 rt2800: flush and txstatus rework for rt2800mmio
> adf26a356f13 rt2x00: use different txstatus timeouts when flushing
> 5022efb50f62 rt2x00: do not check for txstatus timeout every time on tasklet
> 0b0d556e0ebb rt2800mmio: use txdone/txstatus routines from lib
> 5c656c71b1bf rt2800: move usb specific txdone/txstatus routines to rt2800lib
>
> so I'm a bit afraid that one of those changes is real cause of
> the issue not ed194d136769 .
I tested 4.20 and 5.1 and see the exact same behavior. Reverting this
usb core patchsolves the problem.
4.19.x (before this usb core patch) is running fine.
>> Note that rt2x00usb_work_rxdone() processes all available entries, not
>> only such for which queue_work() was called.
>>
>> This fixes a regression on a RT5370 based wifi stick in AP mode, which
>> suddenly stopped data transmission after some period of heavy load. Also
>> stopping the hanging hostapd resulted in the error message "ieee80211
>> phy0: rt2x00queue_flush_queue: Warning - Queue 14 failed to flush".
>> Other operation modes are probably affected as well, this just was
>> the used testcase.
> Do you know what actually make the traffic stop,
> TX queue hung or RX queue hung?
I think RX queue hang, as stated in the patch title. "Queue 14" means QID_RX
(rt2x00queue.h, enum data_queue_qid).
I also tried to re-add local_irq_save() in only one of the handlers. Adding
this tort2x00usb_interrupt_rxdone() alone solved the issue, while doing so
for tx alonedid not.
Note that this doesn't mean there is no problem for tx, that's maybe
just more
difficult to trigger.
>> diff --git a/drivers/net/wireless/ralink/rt2x00/rt2x00dev.c b/drivers/net/wireless/ralink/rt2x00/rt2x00dev.c
>> index 1b08b01db27b..9c102a501ee6 100644
>> --- a/drivers/net/wireless/ralink/rt2x00/rt2x00dev.c
>> +++ b/drivers/net/wireless/ralink/rt2x00/rt2x00dev.c
>> @@ -263,9 +263,9 @@ EXPORT_SYMBOL_GPL(rt2x00lib_dmastart);
>>
>> void rt2x00lib_dmadone(struct queue_entry *entry)
>> {
>> - set_bit(ENTRY_DATA_STATUS_PENDING, &entry->flags);
>> clear_bit(ENTRY_OWNER_DEVICE_DATA, &entry->flags);
>> rt2x00queue_index_inc(entry, Q_INDEX_DMA_DONE);
>> + set_bit(ENTRY_DATA_STATUS_PENDING, &entry->flags);
> Unfortunately I do not understand how this suppose to fix the problem,
> could you elaborate more about this change?
>
Re-adding local_irq_save() around thisrt2x00lib_dmadone()solved
the issue. So I also tried to reverse the order of these calls.
It seems totally plausible to me, that the correct sequence is to
first clear the device assignment, then to set the status to dma_done,
then to trigger the workqueue processing for this entry. When the handler
is scheduled away in the middle of this sequence, now there is no
strange state where the entry can be processed by the workqueue while
not declared dma_done for it.
With this changed sequence there is no need anymore to disable interrupts
for solving the hang issue.
Regards,
Soeren
^ permalink raw reply
* Re: [PATCH v2 2/3] nl80211: Limit certain commands to interface owner
From: Marcel Holtmann @ 2019-06-21 13:27 UTC (permalink / raw)
To: Arend Van Spriel; +Cc: Denis Kenzior, Johannes Berg, linux-wireless
In-Reply-To: <11852f40-67e5-9122-7d82-077bdd0b014a@broadcom.com>
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
^ permalink raw reply
* [PATCH] mwifiex: ignore processing invalid command response
From: Ganapathi Bhat @ 2019-06-21 14:14 UTC (permalink / raw)
To: linux-wireless
Cc: Brian Norris, Cathy Luo, Zhiyuan Yang, James Cao, Rakesh Parmar,
Swati Kushwaha, Ganapathi Bhat
From: Swati Kushwaha <swatiuma@marvell.com>
Firmware can send invalid command response, the processing of
which can attempt to modify unexpected context and cause issues.
To fix this, driver should check that the command response ID is
same as the one it downloaded, and ignore processing of invalid
response.
Signed-off-by: Swati Kushwaha <swatiuma@marvell.com>
Signed-off-by: Ganapathi Bhat <gbhat@marvell.com>
---
drivers/net/wireless/marvell/mwifiex/cmdevt.c | 27 ++++++++++++++++--------
drivers/net/wireless/marvell/mwifiex/main.h | 2 +-
2 files changed, 19 insertions(+), 10 deletions(-)
diff --git a/drivers/net/wireless/marvell/mwifiex/cmdevt.c b/drivers/net/wireless/marvell/mwifiex/cmdevt.c
index 8c35441..133b03d 100644
--- a/drivers/net/wireless/marvell/mwifiex/cmdevt.c
+++ b/drivers/net/wireless/marvell/mwifiex/cmdevt.c
@@ -39,10 +39,11 @@
static void
mwifiex_init_cmd_node(struct mwifiex_private *priv,
struct cmd_ctrl_node *cmd_node,
- u32 cmd_oid, void *data_buf, bool sync)
+ u32 cmd_no, void *data_buf, bool sync)
{
cmd_node->priv = priv;
- cmd_node->cmd_oid = cmd_oid;
+ cmd_node->cmd_no = cmd_no;
+
if (sync) {
cmd_node->wait_q_enabled = true;
cmd_node->cmd_wait_q_woken = false;
@@ -92,7 +93,7 @@
mwifiex_clean_cmd_node(struct mwifiex_adapter *adapter,
struct cmd_ctrl_node *cmd_node)
{
- cmd_node->cmd_oid = 0;
+ cmd_node->cmd_no = 0;
cmd_node->cmd_flag = 0;
cmd_node->data_buf = NULL;
cmd_node->wait_q_enabled = false;
@@ -201,6 +202,7 @@ static int mwifiex_dnld_cmd_to_fw(struct mwifiex_private *priv,
}
cmd_code = le16_to_cpu(host_cmd->command);
+ cmd_node->cmd_no = cmd_code;
cmd_size = le16_to_cpu(host_cmd->size);
if (adapter->hw_status == MWIFIEX_HW_STATUS_RESET &&
@@ -621,7 +623,7 @@ int mwifiex_send_cmd(struct mwifiex_private *priv, u16 cmd_no,
}
/* Initialize the command node */
- mwifiex_init_cmd_node(priv, cmd_node, cmd_oid, data_buf, sync);
+ mwifiex_init_cmd_node(priv, cmd_node, cmd_no, data_buf, sync);
if (!cmd_node->cmd_skb) {
mwifiex_dbg(adapter, ERROR,
@@ -822,9 +824,6 @@ int mwifiex_process_cmdresp(struct mwifiex_adapter *adapter)
uint16_t cmdresp_result;
unsigned long flags;
- /* Now we got response from FW, cancel the command timer */
- del_timer_sync(&adapter->cmd_timer);
-
if (!adapter->curr_cmd || !adapter->curr_cmd->resp_skb) {
resp = (struct host_cmd_ds_command *) adapter->upld_buf;
mwifiex_dbg(adapter, ERROR,
@@ -833,9 +832,20 @@ int mwifiex_process_cmdresp(struct mwifiex_adapter *adapter)
return -1;
}
+ resp = (struct host_cmd_ds_command *)adapter->curr_cmd->resp_skb->data;
+ orig_cmdresp_no = le16_to_cpu(resp->command);
+ cmdresp_no = (orig_cmdresp_no & HostCmd_CMD_ID_MASK);
+
+ if (adapter->curr_cmd->cmd_no != cmdresp_no) {
+ mwifiex_dbg(adapter, ERROR,
+ "cmdresp error: cmd=0x%x cmd_resp=0x%x\n",
+ adapter->curr_cmd->cmd_no, cmdresp_no);
+ return -1;
+ }
+ /* Now we got response from FW, cancel the command timer */
+ del_timer_sync(&adapter->cmd_timer);
clear_bit(MWIFIEX_IS_CMD_TIMEDOUT, &adapter->work_flags);
- resp = (struct host_cmd_ds_command *) adapter->curr_cmd->resp_skb->data;
if (adapter->curr_cmd->cmd_flag & CMD_F_HOSTCMD) {
/* Copy original response back to response buffer */
struct mwifiex_ds_misc_cmd *hostcmd;
@@ -849,7 +859,6 @@ int mwifiex_process_cmdresp(struct mwifiex_adapter *adapter)
memcpy(hostcmd->cmd, resp, size);
}
}
- orig_cmdresp_no = le16_to_cpu(resp->command);
/* Get BSS number and corresponding priv */
priv = mwifiex_get_priv_by_id(adapter,
diff --git a/drivers/net/wireless/marvell/mwifiex/main.h b/drivers/net/wireless/marvell/mwifiex/main.h
index b025ba1..3e442c7 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.h
+++ b/drivers/net/wireless/marvell/mwifiex/main.h
@@ -747,7 +747,7 @@ struct mwifiex_bss_prio_tbl {
struct cmd_ctrl_node {
struct list_head list;
struct mwifiex_private *priv;
- u32 cmd_oid;
+ u32 cmd_no;
u32 cmd_flag;
struct sk_buff *cmd_skb;
struct sk_buff *resp_skb;
--
1.7.1
^ permalink raw reply related
* Re: [PATCH v2 2/3] nl80211: Limit certain commands to interface owner
From: Denis Kenzior @ 2019-06-21 17:14 UTC (permalink / raw)
To: Arend Van Spriel, Johannes Berg; +Cc: linux-wireless
In-Reply-To: <11852f40-67e5-9122-7d82-077bdd0b014a@broadcom.com>
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
^ permalink raw reply
* [PATCH] mt76: mt7615: remove cfg80211_chan_def from mt7615_set_channel signature
From: Lorenzo Bianconi @ 2019-06-21 17:15 UTC (permalink / raw)
To: nbd; +Cc: lorenzo.bianconi, linux-wireless, ryder.lee, royluo
Simplify mt7615_set_channel signature removing cfg80211_chan_def
parameter since it is not actually used
Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
drivers/net/wireless/mediatek/mt76/mt7615/main.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/net/wireless/mediatek/mt76/mt7615/main.c b/drivers/net/wireless/mediatek/mt76/mt7615/main.c
index d21407ddda31..ea6b2315c6e5 100644
--- a/drivers/net/wireless/mediatek/mt76/mt7615/main.c
+++ b/drivers/net/wireless/mediatek/mt76/mt7615/main.c
@@ -130,8 +130,7 @@ static void mt7615_remove_interface(struct ieee80211_hw *hw,
mutex_unlock(&dev->mt76.mutex);
}
-static int mt7615_set_channel(struct mt7615_dev *dev,
- struct cfg80211_chan_def *def)
+static int mt7615_set_channel(struct mt7615_dev *dev)
{
int ret;
@@ -196,7 +195,7 @@ static int mt7615_config(struct ieee80211_hw *hw, u32 changed)
if (changed & IEEE80211_CONF_CHANGE_CHANNEL) {
ieee80211_stop_queues(hw);
- ret = mt7615_set_channel(dev, &hw->conf.chandef);
+ ret = mt7615_set_channel(dev);
ieee80211_wake_queues(hw);
}
--
2.21.0
^ permalink raw reply related
* Re: [RFC 0/8] nl80211: add 6GHz band support
From: Arend Van Spriel @ 2019-06-21 19:41 UTC (permalink / raw)
To: Johannes Berg; +Cc: linux-wireless
In-Reply-To: <6d40e69c-0cea-cc92-7974-c54d0f70812e@broadcom.com>
On 6/3/2019 12:39 PM, Arend Van Spriel wrote:
> On 5/27/2019 10:46 PM, Arend Van Spriel wrote:
>> On 5/24/2019 8:38 PM, Arend Van Spriel wrote:
>>> On May 24, 2019 1:56:43 PM Johannes Berg <johannes@sipsolutions.net>
>>> wrote:
>>>
>>>> Hi Arend,
>>>>
>>>> On Mon, 2019-05-20 at 14:00 +0200, Arend van Spriel wrote:
>>>>> In 802.11ax D4.0 a new band has been proposed. This series contains
>>>>> changes to cfg80211 for supporting this band. With 2GHz and 5GHz there
>>>>> was no overlap in channel number. However, this new band has channel
>>>>> numbers with a range from 1 up to 253.
>>>>
>>>> At the wireless workshop in Prague, we looked at this and sort of
>>>> decided that it'd be better to put all the 6 GHz channels into the 5
>>>> GHz
>>>> "band" in nl80211, to avoid all the "5 || 6" since they're really the
>>>> same except for very specific places like scanning.
>>>
>>> Would have liked to be there, but attending is no longer an option
>>> for me. We now have two autistic, non-verbal children and I am the
>>> primary caregiver for the oldest because my wife can't handle him.
>>> Guess I should have checked the workshop notes before working on this
>>> :-) Do you have URL?
>>
>> Found the netdev wifi workshop page and looked over the slides
>> quickly, but the notes page is pretty empty ;-)
>>
>>> Agree that most functional requirements for 6 GHz are same as 5 GHz.
>>> There are some 6 GHz specifics about beaconing as well.
>>
>> This came up in discussion with my colleagues today and I would say
>> from mac80211 perspective there is more to it than just scanning. In
>> short the 6GHz band is for HE-only operation so for example only HE
>> rates may be used. As the bitrates are in ieee80211_supported_band
>> having a separate 6GHz band seems to have a (slight?) advantage.
>
> Hi, Johannes
>
> Any thoughts on this?
Hi Johannes,
It has been a while so maybe your thoughts are more concrete? ;-p
I really would like this to move forward as I also noticed hostapd
changes being posted for 6GHz support yesterday.
Thanks,
Arend
^ permalink raw reply
* Re: iwlwifi/brcmfmac public action frames crash (RESENDING)
From: Denis Kenzior @ 2019-06-21 20:09 UTC (permalink / raw)
To: linux-wireless, Johannes Berg; +Cc: Arend Van Spriel
In-Reply-To: <45805272aaf8b872a90cf0c364164b5fc1b20272.camel@linux.intel.com>
Ping, is anyone looking into these crashes?
On 06/13/2019 11:45 AM, James Prestwood wrote:
> Sorry if this comes in twice, I sent it ~12 hours ago but never saw it
> hit the list, nor in the archives so I am resending it.
>
> Hi,
>
> Both iwlwifi/brcmfmac seem to be unable to send public action frames to
> an unassociated AP. I am attempting to do a GAS ANQP request with a
> public action frame (via CMD_FRAME). Immediately after CMD_FRAME any of
> the following happens depending on the card:
>
> Intel 7260 (iwlwifi) - System lockup freeze (must hard reboot)
> Intel 3160 (iwlwifi) - CMD_FRAME returns -EINVAL
> BCM43602 (brcmfmac) - Kernel crash (below)
> AR9462 (ath9k) - works
> Random USB adapter (rt2800usb) - works
>
> iwlwifi (on 7260) completely locks the system, where the only way to
> recover is hard reboot. I have reproduced this on two separate systems,
> both with a 7260. I *have* seen it not lock the system once although
> lately it seems to happen every time. The 3160 did not cause a hang
> with my limited testing, though it did not accept CMD_FRAME which is
> likely why it never hung.
>
> Not sure how I can get any more info about the iwlwifi problem as the
> system is completely hung, but if there is a way I'll be happy to do
> that.
>
> Here is the brcmfmac crash:
>
> [19735.643941] BUG: unable to handle kernel NULL pointer dereference at
> 0000000000000000
> [19735.643965] PGD 80000001874aa067 P4D 80000001874aa067 PUD 2735fe067
> PMD 0
> [19735.643984] Oops: 0000 [#1] SMP PTI
> [19735.643993] CPU: 7 PID: 5051 Comm: iwd Tainted: G W
> I 4.19.0-rc2-custom #27
> [19735.644002] Hardware name: System manufacturer System Product
> Name/SABERTOOTH X58, BIOS 1402 08/09/2012
> [19735.644027] RIP: 0010:brcmf_p2p_send_action_frame+0x23a/0x850
> [brcmfmac]
> [19735.644037] Code: 41 c7 86 e0 00 00 00 00 00 00 00 f0 41 80 66 20 bf
> f0 41 80 66 20 7f 49 8b 46 48 b9 24 07 00 00 48 89 da 48 c7 c6 3d 00 8f
> c0 <48> 8b 38 e8 3e d7 ff ff 85 c0 41 89 c5 0f 85 c4 00 00 00 8b 03 49
> [19735.644051] RSP: 0018:ffffa879c8477a00 EFLAGS: 00010246
> [19735.644059] RAX: 0000000000000000 RBX: ffff954a2e059000 RCX:
> 0000000000000724
> [19735.644067] RDX: ffff954a2e059000 RSI: ffffffffc08f003d RDI:
> 0000000000000002
> [19735.644075] RBP: ffffa879c8477a50 R08: 000000000000001c R09:
> 0000000000000999
> [19735.644083] R10: ffff954b157a2f00 R11: ffffffffc0720000 R12:
> ffff954c32f26021
> [19735.644091] R13: ffff954a2e059000 R14: ffff954c32f26000 R15:
> 00000000ffffffff
> [19735.644099] FS: 00007f8d5aa30740(0000) GS:ffff954c369c0000(0000)
> knlGS:0000000000000000
> [19735.644108] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [19735.644115] CR2: 0000000000000000 CR3: 00000001845c8000 CR4:
> 00000000000006e0
> [19735.644123] Call Trace:
> [19735.644133] ? _cond_resched+0x19/0x40
> [19735.644153] brcmf_cfg80211_mgmt_tx+0x170/0x2f0 [brcmfmac]
> [19735.644192] cfg80211_mlme_mgmt_tx+0x115/0x2f0 [cfg80211]
> [19735.644219] nl80211_tx_mgmt+0x24d/0x3d0 [cfg80211]
> [19735.644228] genl_family_rcv_msg+0x1fe/0x3f0
> [19735.644237] ? nlmon_xmit+0x2c/0x30
> [19735.644246] ? dev_hard_start_xmit+0xa8/0x210
> [19735.644254] genl_rcv_msg+0x4c/0x90
> [19735.644261] ? genl_family_rcv_msg+0x3f0/0x3f0
> [19735.644268] netlink_rcv_skb+0x54/0x130
> [19735.644275] genl_rcv+0x28/0x40
> [19735.644281] netlink_unicast+0x1ab/0x250
> [19735.644288] netlink_sendmsg+0x2d1/0x3d0
> [19735.644297] sock_sendmsg+0x3e/0x50
> [19735.644304] __sys_sendto+0x13f/0x180
> [19735.644313] ? do_epoll_wait+0xb0/0xc0
> [19735.644321] __x64_sys_sendto+0x28/0x30
> [19735.644329] do_syscall_64+0x5a/0x120
> [19735.644336] entry_SYSCALL_64_after_hwframe+0x44/0xa9
> [19735.644344] RIP: 0033:0x7f8d5a352c4d
> [19735.644350] Code: ff ff ff ff eb b6 0f 1f 80 00 00 00 00 48 8d 05 c1
> dc 2c 00 41 89 ca 8b 00 85 c0 75 20 45 31 c9 45 31 c0 b8 2c 00 00 00 0f
> 05 <48> 3d 00 f0 ff ff 77 6b f3 c3 66 0f 1f 84 00 00 00 00 00 41 56 41
> [19735.644365] RSP: 002b:00007ffc9a618048 EFLAGS: 00000246 ORIG_RAX:
> 000000000000002c
> [19735.644374] RAX: ffffffffffffffda RBX: 00000000007077d0 RCX:
> 00007f8d5a352c4d
> [19735.644382] RDX: 0000000000000068 RSI: 000000000072bc40 RDI:
> 0000000000000004
> [19735.644390] RBP: 0000000000733510 R08: 0000000000000000 R09:
> 0000000000000000
> [19735.644397] R10: 0000000000000000 R11: 0000000000000246 R12:
> 00007ffc9a618094
> [19735.644405] R13: 00007ffc9a61809c R14: 0000000000000000 R15:
> 0000000000000000
> [19735.644414] Modules linked in: ccm algif_aead snd_hda_codec_realtek
> snd_hda_codec_generic snd_hda_codec_hdmi binfmt_misc arc4 nouveau
> gpio_ich ath9k mxm_wmi ath9k_common video rt2800usb intel_powerclamp
> snd_hda_intel ath9k_hw rt2x00usb iwlmvm rt2800lib snd_hda_codec
> rt2x00lib ath snd_seq_midi snd_seq_midi_event coretemp ttm mac80211
> snd_hda_core brcmfmac snd_hwdep snd_rawmidi iwlwifi intel_cstate
> drm_kms_helper brcmutil snd_seq drm snd_pcm input_leds serio_raw
> lpc_ich cfg80211 snd_seq_device i2c_algo_bit snd_timer fb_sys_fops
> syscopyarea sysfillrect snd sysimgblt i5500_temp wmi asus_atk0110
> soundcore mac_hid i7core_edac sch_fq_codel kvm_intel kvm vfio_pci
> vfio_virqfd irqbypass vfio_iommu_type1 vfio pci_stub parport_pc ppdev
> lp parport ip_tables x_tables autofs4 pata_acpi hid_generic usbhid hid
> firewire_ohci
> [19735.644521] realtek psmouse firewire_core crc_itu_t r8169 i2c_i801
> ahci libahci
> [19735.644538] CR2: 0000000000000000
> [19735.653612] ---[ end trace 30dbecd734da3b73 ]---
> [19735.653641] RIP: 0010:brcmf_p2p_send_action_frame+0x23a/0x850
> [brcmfmac]
> [19735.653651] Code: 41 c7 86 e0 00 00 00 00 00 00 00 f0 41 80 66 20 bf
> f0 41 80 66 20 7f 49 8b 46 48 b9 24 07 00 00 48 89 da 48 c7 c6 3d 00 8f
> c0 <48> 8b 38 e8 3e d7 ff ff 85 c0 41 89 c5 0f 85 c4 00 00 00 8b 03 49
> [19735.653659] RSP: 0018:ffffa879c8477a00 EFLAGS: 00010246
> [19735.653672] RAX: 0000000000000000 RBX: ffff954a2e059000 RCX:
> 0000000000000724
> [19735.653680] RDX: ffff954a2e059000 RSI: ffffffffc08f003d RDI:
> 0000000000000002
> [19735.653688] RBP: ffffa879c8477a50 R08: 000000000000001c R09:
> 0000000000000999
> [19735.653697] R10: ffff954b157a2f00 R11: ffffffffc0720000 R12:
> ffff954c32f26021
> [19735.653705] R13: ffff954a2e059000 R14: ffff954c32f26000 R15:
> 00000000ffffffff
> [19735.653714] FS: 00007f8d5aa30740(0000) GS:ffff954c369c0000(0000)
> knlGS:0000000000000000
> [19735.653725] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [19735.653731] CR2: 0000000000000000 CR3: 00000001845c8000 CR4:
> 00000000000006e0
>
> Thanks,
> James
>
^ permalink raw reply
* [PATCH] mt76: move nl80211_dfs_regions in mt76_dev data structure
From: Lorenzo Bianconi @ 2019-06-21 20:31 UTC (permalink / raw)
To: nbd; +Cc: lorenzo.bianconi, linux-wireless
Move dfs region field in mt76_dev data structure since it is
used by all drivers. This is a preliminary patch to add DFS support to
mt7615 driver
Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
drivers/net/wireless/mediatek/mt76/mt76.h | 2 ++
.../net/wireless/mediatek/mt76/mt7603/debugfs.c | 2 +-
drivers/net/wireless/mediatek/mt76/mt7603/init.c | 4 ++--
.../net/wireless/mediatek/mt76/mt7603/mt7603.h | 2 --
.../net/wireless/mediatek/mt76/mt76x02_debugfs.c | 2 +-
drivers/net/wireless/mediatek/mt76/mt76x02_dfs.c | 16 ++++++++--------
drivers/net/wireless/mediatek/mt76/mt76x02_dfs.h | 2 --
7 files changed, 14 insertions(+), 16 deletions(-)
diff --git a/drivers/net/wireless/mediatek/mt76/mt76.h b/drivers/net/wireless/mediatek/mt76/mt76.h
index 9110a895604a..2c107817610c 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76.h
+++ b/drivers/net/wireless/mediatek/mt76/mt76.h
@@ -486,6 +486,8 @@ struct mt76_dev {
int txpower_conf;
int txpower_cur;
+ enum nl80211_dfs_regions region;
+
u32 debugfs_reg;
struct led_classdev led_cdev;
diff --git a/drivers/net/wireless/mediatek/mt76/mt7603/debugfs.c b/drivers/net/wireless/mediatek/mt76/mt7603/debugfs.c
index 9c0bea489e1f..a1bc3103cbe9 100644
--- a/drivers/net/wireless/mediatek/mt76/mt7603/debugfs.c
+++ b/drivers/net/wireless/mediatek/mt76/mt7603/debugfs.c
@@ -49,7 +49,7 @@ mt7603_edcca_set(void *data, u64 val)
dev->ed_monitor_enabled = !!val;
dev->ed_monitor = dev->ed_monitor_enabled &&
- dev->region == NL80211_DFS_ETSI;
+ dev->mt76.region == NL80211_DFS_ETSI;
mt7603_init_edcca(dev);
mutex_unlock(&dev->mt76.mutex);
diff --git a/drivers/net/wireless/mediatek/mt76/mt7603/init.c b/drivers/net/wireless/mediatek/mt76/mt7603/init.c
index bce51997ff3b..75e3da452585 100644
--- a/drivers/net/wireless/mediatek/mt76/mt7603/init.c
+++ b/drivers/net/wireless/mediatek/mt76/mt7603/init.c
@@ -437,9 +437,9 @@ mt7603_regd_notifier(struct wiphy *wiphy,
struct ieee80211_hw *hw = wiphy_to_ieee80211_hw(wiphy);
struct mt7603_dev *dev = hw->priv;
- dev->region = request->dfs_region;
+ dev->mt76.region = request->dfs_region;
dev->ed_monitor = dev->ed_monitor_enabled &&
- dev->region == NL80211_DFS_ETSI;
+ dev->mt76.region == NL80211_DFS_ETSI;
}
static int
diff --git a/drivers/net/wireless/mediatek/mt76/mt7603/mt7603.h b/drivers/net/wireless/mediatek/mt76/mt7603/mt7603.h
index 944dc9a11a15..eb5c4880342b 100644
--- a/drivers/net/wireless/mediatek/mt76/mt7603/mt7603.h
+++ b/drivers/net/wireless/mediatek/mt76/mt7603/mt7603.h
@@ -118,8 +118,6 @@ struct mt7603_dev {
u8 mcu_running;
- enum nl80211_dfs_regions region;
-
u8 ed_monitor_enabled;
u8 ed_monitor;
s8 ed_trigger;
diff --git a/drivers/net/wireless/mediatek/mt76/mt76x02_debugfs.c b/drivers/net/wireless/mediatek/mt76/mt76x02_debugfs.c
index ffdba5ffc22d..1b1e424ccbb2 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76x02_debugfs.c
+++ b/drivers/net/wireless/mediatek/mt76/mt76x02_debugfs.c
@@ -120,7 +120,7 @@ static int
mt76_edcca_set(void *data, u64 val)
{
struct mt76x02_dev *dev = data;
- enum nl80211_dfs_regions region = dev->dfs_pd.region;
+ enum nl80211_dfs_regions region = dev->mt76.region;
mutex_lock(&dev->mt76.mutex);
diff --git a/drivers/net/wireless/mediatek/mt76/mt76x02_dfs.c b/drivers/net/wireless/mediatek/mt76/mt76x02_dfs.c
index 84b845647881..50e9b310e496 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76x02_dfs.c
+++ b/drivers/net/wireless/mediatek/mt76/mt76x02_dfs.c
@@ -283,7 +283,7 @@ static bool mt76x02_dfs_check_hw_pulse(struct mt76x02_dev *dev,
if (!pulse->period || !pulse->w1)
return false;
- switch (dev->dfs_pd.region) {
+ switch (dev->mt76.region) {
case NL80211_DFS_FCC:
if (pulse->engine > 3)
break;
@@ -457,7 +457,7 @@ static int mt76x02_dfs_create_sequence(struct mt76x02_dev *dev,
with_sum = event->width + cur_event->width;
sw_params = &dfs_pd->sw_dpd_params;
- switch (dev->dfs_pd.region) {
+ switch (dev->mt76.region) {
case NL80211_DFS_FCC:
case NL80211_DFS_JP:
if (with_sum < 600)
@@ -685,7 +685,7 @@ static void mt76x02_dfs_init_sw_detector(struct mt76x02_dev *dev)
{
struct mt76x02_dfs_pattern_detector *dfs_pd = &dev->dfs_pd;
- switch (dev->dfs_pd.region) {
+ switch (dev->mt76.region) {
case NL80211_DFS_FCC:
dfs_pd->sw_dpd_params.max_pri = MT_DFS_FCC_MAX_PRI;
dfs_pd->sw_dpd_params.min_pri = MT_DFS_FCC_MIN_PRI;
@@ -725,7 +725,7 @@ static void mt76x02_dfs_set_bbp_params(struct mt76x02_dev *dev)
break;
}
- switch (dev->dfs_pd.region) {
+ switch (dev->mt76.region) {
case NL80211_DFS_FCC:
radar_specs = &fcc_radar_specs[shift];
break;
@@ -836,7 +836,7 @@ void mt76x02_dfs_init_params(struct mt76x02_dev *dev)
struct cfg80211_chan_def *chandef = &dev->mt76.chandef;
if ((chandef->chan->flags & IEEE80211_CHAN_RADAR) &&
- dev->dfs_pd.region != NL80211_DFS_UNSET) {
+ dev->mt76.region != NL80211_DFS_UNSET) {
mt76x02_dfs_init_sw_detector(dev);
mt76x02_dfs_set_bbp_params(dev);
/* enable debug mode */
@@ -869,7 +869,7 @@ void mt76x02_dfs_init_detector(struct mt76x02_dev *dev)
INIT_LIST_HEAD(&dfs_pd->sequences);
INIT_LIST_HEAD(&dfs_pd->seq_pool);
- dfs_pd->region = NL80211_DFS_UNSET;
+ dev->mt76.region = NL80211_DFS_UNSET;
dfs_pd->last_sw_check = jiffies;
tasklet_init(&dfs_pd->dfs_tasklet, mt76x02_dfs_tasklet,
(unsigned long)dev);
@@ -882,14 +882,14 @@ mt76x02_dfs_set_domain(struct mt76x02_dev *dev,
struct mt76x02_dfs_pattern_detector *dfs_pd = &dev->dfs_pd;
mutex_lock(&dev->mt76.mutex);
- if (dfs_pd->region != region) {
+ if (dev->mt76.region != region) {
tasklet_disable(&dfs_pd->dfs_tasklet);
dev->ed_monitor = dev->ed_monitor_enabled &&
region == NL80211_DFS_ETSI;
mt76x02_edcca_init(dev);
- dfs_pd->region = region;
+ dev->mt76.region = region;
mt76x02_dfs_init_params(dev);
tasklet_enable(&dfs_pd->dfs_tasklet);
}
diff --git a/drivers/net/wireless/mediatek/mt76/mt76x02_dfs.h b/drivers/net/wireless/mediatek/mt76/mt76x02_dfs.h
index 70b394e17340..0408613b45a4 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76x02_dfs.h
+++ b/drivers/net/wireless/mediatek/mt76/mt76x02_dfs.h
@@ -118,8 +118,6 @@ struct mt76x02_dfs_seq_stats {
};
struct mt76x02_dfs_pattern_detector {
- enum nl80211_dfs_regions region;
-
u8 chirp_pulse_cnt;
u32 chirp_pulse_ts;
--
2.21.0
^ permalink raw reply related
* Re: [PATCH v2 2/3] nl80211: Limit certain commands to interface owner
From: Arend Van Spriel @ 2019-06-21 21:16 UTC (permalink / raw)
To: Denis Kenzior, Johannes Berg; +Cc: linux-wireless
In-Reply-To: <af810765-ba1a-c7ae-abe5-35eef72eb8ce@gmail.com>
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
^ permalink raw reply
* Re: [PATCH v2 2/3] nl80211: Limit certain commands to interface owner
From: Denis Kenzior @ 2019-06-21 22:33 UTC (permalink / raw)
To: Arend Van Spriel, Johannes Berg; +Cc: linux-wireless
In-Reply-To: <b1ae8df6-c8a7-e453-aad3-e31bb2e3bd60@broadcom.com>
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
^ permalink raw reply
* Re: [PATCH v2 2/3] nl80211: Limit certain commands to interface owner
From: Marcel Holtmann @ 2019-06-22 13:44 UTC (permalink / raw)
To: Arend Van Spriel; +Cc: Denis Kenzior, Johannes Berg, linux-wireless
In-Reply-To: <b1ae8df6-c8a7-e453-aad3-e31bb2e3bd60@broadcom.com>
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
^ permalink raw reply
* Re: iwlwifi/brcmfmac public action frames crash (RESENDING)
From: Arend Van Spriel @ 2019-06-23 6:06 UTC (permalink / raw)
To: Denis Kenzior, linux-wireless, Johannes Berg
In-Reply-To: <07c7b5fc-bc1c-d49f-1c1e-d0b67899e755@gmail.com>
On June 21, 2019 10:09:10 PM Denis Kenzior <denkenz@gmail.com> wrote:
> Ping, is anyone looking into these crashes?
Did not see this message before.
>
> On 06/13/2019 11:45 AM, James Prestwood wrote:
>> Sorry if this comes in twice, I sent it ~12 hours ago but never saw it
>> hit the list, nor in the archives so I am resending it.
>>
>>
>> Hi,
[snip]
>>
>>
>>
>>
>> Here is the brcmfmac crash:
>>
>>
>> [19735.643941] BUG: unable to handle kernel NULL pointer dereference at
>> 0000000000000000
>> [19735.643965] PGD 80000001874aa067 P4D 80000001874aa067 PUD 2735fe067
>> PMD 0
>> [19735.643984] Oops: 0000 [#1] SMP PTI
>> [19735.643993] CPU: 7 PID: 5051 Comm: iwd Tainted: G W
>> I 4.19.0-rc2-custom #27
>> [19735.644002] Hardware name: System manufacturer System Product
>> Name/SABERTOOTH X58, BIOS 1402 08/09/2012
>> [19735.644027] RIP: 0010:brcmf_p2p_send_action_frame+0x23a/0x850
>> [brcmfmac]
As the name suggest this was implemented for P2P support. Will look into this.
Regards,
Arend
^ permalink raw reply
* Re: iwlwifi module crash
From: b.K.il.h.u+tigbuh @ 2019-06-23 9:08 UTC (permalink / raw)
To: Balakrishnan Balasubramanian; +Cc: Emmanuel Grumbach, linux-wireless
In-Reply-To: <CAPuHQ=Ffq_Gw_KbyjpzR07MWz=+LxmGVEP2-Tn5zDxrUEuxrZQ@mail.gmail.com>
devices/ is probably just a symlink. Try to find it manually:
find /sys -iname remove
lspci
The interesting thing is that my iwlwifi card started to do the same
thing just recently (some weeks ago). However, I do suspend a lot and
it only happens after resuming, but not after every resume (maybe
5-10%). It always came back after restarting except on one day when it
needed three restarts, so maybe mine would be more about needing to
reseat the card.
> On Fri, Jun 14, 2019 at 4:54 AM Balakrishnan Balasubramanian <linux-wireless-list@balki.me> wrote:
>>
>> The issue occured again today. I tried to restart the module
>>
>> > echo 1 > /sys/module/iwlwifi/devices/0000\:02\:00.0/remove
>>
>> There is no folder 'devices'
>>
>> zadesk% ls /sys/module/iwlwifi
>> coresize drivers holders initsize initstate notes parameters refcnt
>> sections srcversion taint uevent
>>
>> > echo 1 > /sys/bus/pci/rescan
>>
>> Attached the error when trying to rescan.
>>
>> Thanks,
>> Bala
>>
>>
>>
>>
^ permalink raw reply
* Re: [PATCH] wireless: wil6210: no need to check return value of debugfs_create functions
From: merez @ 2019-06-23 12:55 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Kalle Valo, David S. Miller, linux-wireless, wil6210,
linux-wireless-owner
In-Reply-To: <20190611191024.GA17227@kroah.com>
On 2019-06-11 22:10, Greg Kroah-Hartman wrote:
> When calling debugfs functions, there is no need to ever check the
> return value. The function can work or not, but the code logic should
> never do something different based on this.
>
> Cc: Maya Erez <merez@codeaurora.org>
> Cc: Kalle Valo <kvalo@codeaurora.org>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: linux-wireless@vger.kernel.org
> Cc: wil6210@qti.qualcomm.com
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
> drivers/net/wireless/ath/wil6210/debugfs.c | 80 ++++++----------------
> 1 file changed, 22 insertions(+), 58 deletions(-)
>
Reviewed-by: Maya Erez <merez@codeaurora.org>
--
Maya Erez
Qualcomm Israel, Inc. on behalf of Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a
Linux Foundation Collaborative Project
^ permalink raw reply
* [PATCH] mt76: mt76u: get rid of {out,in}_max_packet
From: Lorenzo Bianconi @ 2019-06-23 20:25 UTC (permalink / raw)
To: nbd; +Cc: lorenzo.bianconi, linux-wireless, sgruszka
Remove {out,in}_max_packet from mt76_usb data structure since
they just track last usb endpoint and they are not actually used
Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
drivers/net/wireless/mediatek/mt76/mt76.h | 2 --
drivers/net/wireless/mediatek/mt76/usb.c | 2 --
2 files changed, 4 deletions(-)
diff --git a/drivers/net/wireless/mediatek/mt76/mt76.h b/drivers/net/wireless/mediatek/mt76/mt76.h
index 27f5dfb8c77f..2c107817610c 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76.h
+++ b/drivers/net/wireless/mediatek/mt76/mt76.h
@@ -397,9 +397,7 @@ struct mt76_usb {
struct delayed_work stat_work;
u8 out_ep[__MT_EP_OUT_MAX];
- u16 out_max_packet;
u8 in_ep[__MT_EP_IN_MAX];
- u16 in_max_packet;
bool sg_en;
struct mt76u_mcu {
diff --git a/drivers/net/wireless/mediatek/mt76/usb.c b/drivers/net/wireless/mediatek/mt76/usb.c
index ecc1aa59f5c1..fb87ce7fbdf6 100644
--- a/drivers/net/wireless/mediatek/mt76/usb.c
+++ b/drivers/net/wireless/mediatek/mt76/usb.c
@@ -267,12 +267,10 @@ mt76u_set_endpoints(struct usb_interface *intf,
if (usb_endpoint_is_bulk_in(ep_desc) &&
in_ep < __MT_EP_IN_MAX) {
usb->in_ep[in_ep] = usb_endpoint_num(ep_desc);
- usb->in_max_packet = usb_endpoint_maxp(ep_desc);
in_ep++;
} else if (usb_endpoint_is_bulk_out(ep_desc) &&
out_ep < __MT_EP_OUT_MAX) {
usb->out_ep[out_ep] = usb_endpoint_num(ep_desc);
- usb->out_max_packet = usb_endpoint_maxp(ep_desc);
out_ep++;
}
}
--
2.21.0
^ permalink raw reply related
* Re: [PATCH v2 2/3] nl80211: Limit certain commands to interface owner
From: Arend Van Spriel @ 2019-06-24 8:39 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: Denis Kenzior, Johannes Berg, linux-wireless
In-Reply-To: <011C968F-507F-4646-B206-C28BDE7EB4A0@holtmann.org>
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
^ permalink raw reply
* Re: [PATCH] Add SPDX identifiers
From: Kalle Valo @ 2019-06-24 9:01 UTC (permalink / raw)
To: yegorslists; +Cc: linux-wireless, johannes
In-Reply-To: <20190620130148.1674-1-yegorslists@googlemail.com>
yegorslists@googlemail.com writes:
> From: Yegor Yefremov <yegorslists@googlemail.com>
>
> Software Package Data Exchange identifiers help to detect source file
> licenses and hence simplify the FOSS compliance process.
>
> Signed-off-by: Yegor Yefremov <yegorslists@googlemail.com>
For patches to iw it's good practise to prefix the title with "iw: ",
that way people can immediately see for which component it is. But no
need to resend because of this.
--
Kalle Valo
^ permalink raw reply
* Re: [PATCH] Add SPDX identifiers
From: Johannes Berg @ 2019-06-24 9:45 UTC (permalink / raw)
To: yegorslists, linux-wireless
In-Reply-To: <20190620130148.1674-1-yegorslists@googlemail.com>
On Thu, 2019-06-20 at 15:01 +0200, yegorslists@googlemail.com wrote:
> From: Yegor Yefremov <yegorslists@googlemail.com>
>
> Software Package Data Exchange identifiers help to detect source file
> licenses and hence simplify the FOSS compliance process.
Well that's nice and all, but it's also wrong.
You haven't included any documentation that says what the SPDX
identifier, and specifically the "ISC" tag means in the context of the
project, and it's not even the same license text as on spdx.org.
johannes
^ permalink raw reply
* Re: [PATCH] Add SPDX identifiers
From: Yegor Yefremov @ 2019-06-24 10:07 UTC (permalink / raw)
To: Johannes Berg; +Cc: linux-wireless
In-Reply-To: <90ccc515bb26b212b537fc1b0287afaa0f86fdf8.camel@sipsolutions.net>
On Mon, Jun 24, 2019 at 11:45 AM Johannes Berg
<johannes@sipsolutions.net> wrote:
>
> On Thu, 2019-06-20 at 15:01 +0200, yegorslists@googlemail.com wrote:
> > From: Yegor Yefremov <yegorslists@googlemail.com>
> >
> > Software Package Data Exchange identifiers help to detect source file
> > licenses and hence simplify the FOSS compliance process.
>
> Well that's nice and all, but it's also wrong.
>
> You haven't included any documentation that says what the SPDX
> identifier, and specifically the "ISC" tag means in the context of the
> project, and it's not even the same license text as on spdx.org.
What about such definition?
SPDX short-form identifiers provide information about licenses that
apply to the source file.
As for the exact license I wasn't sure myself. Buildroot identifies it
as ISC [1]. How do you define its license in SPDX terms?
[1] https://git.busybox.net/buildroot/tree/package/iw/iw.mk
Yegor
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox