pull-request: mac80211-next next-2019-10-11
Hi, Let me try to be a bit "maintainer-of-the-day agnostic" ;-) I'll be going on vacation, but figured I'd at least get this stuff out. As usual, I ran the hwsim tests from wpa_s/hostapd and all looks fine, compilation also was OK. Kalle has agreed to help cover when I'm on vacation (though I'm home next week, so if there's any fallout I'll deal with it then), so if there's something urgent he may include some stack changes in his trees or ask you to or apply a patch. Please pull and let me know if there's any problem. Thanks, johannes The following changes since commit 9077f052abd5391a866dd99e27212213648becef: net: propagate errors correctly in register_netdevice() (2019-10-03 12:31:06 -0700) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211-next.git tags/mac80211-next-for-net-next-2019-10-11 for you to fetch changes up to 7dfd8ac327301f302b03072066c66eb32578e940: mac80211_hwsim: add support for OCB (2019-10-11 10:33:34 +0200) A few more small things, nothing really stands out: * minstrel improvements from Felix * a TX aggregation simplification * some additional capabilities for hwsim * minor cleanups & docs updates Denis Kenzior (1): nl80211: trivial: Remove redundant loop Felix Fietkau (3): mac80211: minstrel: remove divisions in tx status path mac80211: minstrel_ht: replace rate stats ewma with a better moving average mac80211: minstrel_ht: rename prob_ewma to prob_avg, use it for the new average Johannes Berg (2): mac80211: pass internal sta to ieee80211_tx_frags() mac80211: simplify TX aggregation start Koen Vandeputte (1): mac80211: IBSS: avoid unneeded return value processing Ramon Fontes (2): mac80211_hwsim: add more 5GHz channels, 5/10 MHz support mac80211_hwsim: add support for OCB Sunil Dutt (1): nl80211: Document the expectation for NL80211_ATTR_IE in NL80211_CMD_CONNECT drivers/net/wireless/ath/ath9k/htc_drv_main.c | 2 +- drivers/net/wireless/ath/ath9k/main.c | 2 +- drivers/net/wireless/ath/carl9170/main.c | 3 +- drivers/net/wireless/ath/wcn36xx/main.c| 5 +- .../broadcom/brcm80211/brcmsmac/mac80211_if.c | 3 +- drivers/net/wireless/intel/iwlegacy/4965-mac.c | 2 +- drivers/net/wireless/intel/iwlwifi/dvm/tx.c| 2 +- drivers/net/wireless/intel/iwlwifi/mvm/sta.c | 5 +- drivers/net/wireless/mac80211_hwsim.c | 37 +-- drivers/net/wireless/marvell/mwl8k.c | 2 +- drivers/net/wireless/mediatek/mt76/mt7603/main.c | 3 +- drivers/net/wireless/mediatek/mt76/mt7615/main.c | 3 +- drivers/net/wireless/mediatek/mt76/mt76x02_util.c | 3 +- drivers/net/wireless/mediatek/mt7601u/main.c | 3 +- drivers/net/wireless/ralink/rt2x00/rt2800lib.c | 4 +- drivers/net/wireless/realtek/rtlwifi/base.c| 3 +- drivers/net/wireless/realtek/rtw88/mac80211.c | 3 +- drivers/net/wireless/rsi/rsi_91x_mac80211.c| 3 +- include/net/mac80211.h | 11 +++- include/uapi/linux/nl80211.h | 8 +++ net/mac80211/agg-tx.c | 9 ++- net/mac80211/ibss.c| 9 +-- net/mac80211/rc80211_minstrel.c| 48 +++--- net/mac80211/rc80211_minstrel.h| 57 +++-- net/mac80211/rc80211_minstrel_debugfs.c| 8 +-- net/mac80211/rc80211_minstrel_ht.c | 73 -- net/mac80211/rc80211_minstrel_ht.h | 2 +- net/mac80211/rc80211_minstrel_ht_debugfs.c | 8 +-- net/mac80211/tx.c | 15 ++--- net/wireless/nl80211.c | 6 +- 30 files changed, 212 insertions(+), 130 deletions(-)
Re: [PATCH] mac80211: More strictly validate .abort_scan
On Tue, 2019-10-08 at 11:33 -0500, Denis Kenzior wrote: > nl80211 requires NL80211_CMD_ABORT_SCAN to have a wdev or netdev > attribute present and checks that if netdev is provided it is UP. > However, mac80211 does not check that an ongoing scan actually belongs > to the netdev/wdev provided by the user. In other words, it is possible > for an application to cancel scans on an interface it doesn't manage. I think you should do this in cfg80211. johannes
Re: [PATCH 2/2] mac80211: Support LIVE_ADDRESS_CHANGE feature
On Tue, 2019-10-08 at 15:55 -0500, Denis Kenzior wrote: > Right, so you're talking in the context of this implementation which > performs a remove/add interface. You're right about that. > > But I was asking more in general terms. If all we're doing is scanning, > can we just change the mac? Anyway, not important. Maybe I bring this > up once this set is accepted. Maybe, but honestly, I'm not convinced the complexity would be worth it. You'd have to push this all the way through to the driver, so it knows to do it, or defer it until the scan is done, or something? Not something you'd want to do on all hardware while a non-randomized scan is running, for example (iwlwifi might actually be OK). Or you could perhaps cache the MAC address change in mac80211 and apply it at the next possible point in time - but then again you have to be really careful to actually apply it and block all further operations, even if a bunch of remain-on-channel's are active and you request a new scan, that has to be blocked until the remain-on-channel is done *and* the MAC address change is applied? Seems rather complex for very little value. johannes
Re: [PATCH 4.4, 4.9, 4.14, 4.19] nl80211: validate beacon head
On Wed, 2019-10-09 at 11:43 +0200, Greg KH wrote:
>
> > + for_each_element(elem, data, len) {
> > + /* nothing */
> > + }
>
> for_each_element() is not in 4.4, 4.9, 4.14, or 4.19, so this breaks the
> build :(
Oh, right. I had previously also said:
You also need to cherry-pick
0f3b07f027f8 ("cfg80211: add and use strongly typed element iteration macros")
7388afe09143 ("cfg80211: Use const more consistently in for_each_element
macros")
as dependencies - the latter doesn't apply cleanly as it has a change
that doesn't apply, but that part of the change isn't necessary.
johannes
Re: [PATCH 4.4, 4.9, 4.14, 4.19] nl80211: validate beacon head
On Wed, 2019-10-09 at 11:27 +0200, Greg KH wrote:
> On Wed, Oct 09, 2019 at 08:41:09AM +0200, Johannes Berg wrote:
> > From: Johannes Berg
> >
> > Commit 8a3347aa110c76a7f87771999aed491d1d8779a8 upstream.
>
> I don't see that commit in Linus's tree :(
Hmmm. Not sure what happened there. I had created this in reply to
Sasha's bot, perhaps it told me a different commit ID or something.
Or maybe ... yes I think the bot had actually applied a *patch* rather
than picked one up, and I didn't really know what to do, but forgot
about this then.
> Is this f88eb7c0d002 ("nl80211: validate beacon head")?
Yes.
johannes
[PATCH 4.4, 4.9, 4.14, 4.19] nl80211: validate beacon head
From: Johannes Berg
Commit 8a3347aa110c76a7f87771999aed491d1d8779a8 upstream.
We currently don't validate the beacon head, i.e. the header,
fixed part and elements that are to go in front of the TIM
element. This means that the variable elements there can be
malformed, e.g. have a length exceeding the buffer size, but
most downstream code from this assumes that this has already
been checked.
Add the necessary checks to the netlink policy.
Cc: sta...@vger.kernel.org
Fixes: ed1b6cc7f80f ("cfg80211/nl80211: add beacon settings")
Link:
https://lore.kernel.org/r/1569009255-I7ac7fbe9436e9d8733439eab8acbbd35e55c74ef@changeid
Signed-off-by: Johannes Berg
---
net/wireless/nl80211.c | 38 ++
1 file changed, 38 insertions(+)
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 6168db3c35e4..4a10ab388e0b 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -200,6 +200,38 @@ cfg80211_get_dev_from_info(struct net *netns, struct
genl_info *info)
return __cfg80211_rdev_from_attrs(netns, info->attrs);
}
+static int validate_beacon_head(const struct nlattr *attr,
+ struct netlink_ext_ack *extack)
+{
+ const u8 *data = nla_data(attr);
+ unsigned int len = nla_len(attr);
+ const struct element *elem;
+ const struct ieee80211_mgmt *mgmt = (void *)data;
+ unsigned int fixedlen = offsetof(struct ieee80211_mgmt,
+u.beacon.variable);
+
+ if (len < fixedlen)
+ goto err;
+
+ if (ieee80211_hdrlen(mgmt->frame_control) !=
+ offsetof(struct ieee80211_mgmt, u.beacon))
+ goto err;
+
+ data += fixedlen;
+ len -= fixedlen;
+
+ for_each_element(elem, data, len) {
+ /* nothing */
+ }
+
+ if (for_each_element_completed(elem, data, len))
+ return 0;
+
+err:
+ NL_SET_ERR_MSG_ATTR(extack, attr, "malformed beacon head");
+ return -EINVAL;
+}
+
/* policy for the attributes */
static const struct nla_policy nl80211_policy[NUM_NL80211_ATTR] = {
[NL80211_ATTR_WIPHY] = { .type = NLA_U32 },
@@ -4014,6 +4046,12 @@ static int nl80211_parse_beacon(struct nlattr *attrs[],
memset(bcn, 0, sizeof(*bcn));
if (attrs[NL80211_ATTR_BEACON_HEAD]) {
+ int ret = validate_beacon_head(attrs[NL80211_ATTR_BEACON_HEAD],
+ NULL);
+
+ if (ret)
+ return ret;
+
bcn->head = nla_data(attrs[NL80211_ATTR_BEACON_HEAD]);
bcn->head_len = nla_len(attrs[NL80211_ATTR_BEACON_HEAD]);
if (!bcn->head_len)
--
2.20.1
Re: pull-request: mac80211 2019-10-08
Hi Jakub, > Pulled into net. Let me know if did it wrong :) Oops, didn't know it was your "turn" again, guess I haven't been reading netdev enough. Looks good, but I didn't think this could possibly go wrong :) > FWIW there was this little complaint from checkpatch: [...] > WARNING: Duplicate signature > #14: > Signed-off-by: Johannes Berg Hmm, yeah, so ... I was actually not sure about that and I guess it slipped by. Most of the time, I've been editing it out, but what happens is this: 1) I send a patch to our internal tree, to fix up some things. Unless it's really urgent, I don't necessarily post it externally at the same time. This obviously has my S-o-b. 2) Luca goes through our internal tree and sends out the patches to the list, adding his S-o-b. 3) For the patches to the stack, I apply them, and git-am adds my S-o-b again because it's not the last. So now we have S-o-b: Johannes S-o-b: Luca S-o-b: Johannes If I edit it just to be "S-o-b: Johannes", then it looks strange because I've applied a patch from the list and dropped an S-o-b. It's still my code, and Luca doesn't normally have to make any changes to it, but ... This is what I've normally been doing I think, but it always felt a bit weird because then it's not the patch I actually applied, it's like I pretend the whole process described above never happened. If I edit and remove my first S-o-b then it's weird because the Author isn't the first S-o-b, making it look like I didn't sign it off when I authored it? If I edit and remove the last S-o-b, how did it end up in my tree? So basically my first S-o-b is certifying (a) or maybe occasionally (b) under the DCO, while Luca's and my second are certifying (c) (and maybe occasionally also (a) or (b) if any changes were made.) Is there any convention on this that I could adhere to? :) johannes
Re: [PATCH 2/2] mac80211: Support LIVE_ADDRESS_CHANGE feature
On Tue, 2019-10-08 at 13:50 -0500, Denis Kenzior wrote: > Hi Johannes, > > > > But they shouldn't change due a mac address change? I wonder if we can > > > further relax the requirements to allow mac change if > > > NL80211_SCAN_FLAG_RANDOM_ADDR was used? > > > > No, at least with HW scan that would completely confuse the driver - > > since from the driver's POV we'd remove the interface it's currently > > managing the scan for. > > So help me understand this better. include/net/mac80211.h: int (*hw_scan)(struct ieee80211_hw *hw, struct ieee80211_vif *vif, struct ieee80211_scan_request *req); How is it difficult to understand that with an API like that, it might not be a good idea to remove the vif from the driver while it's scanning? > Just by virtue of copying the new > mac into sdata->vif.addr That's not what happens? johannes
Re: [PATCH 2/2] mac80211: Support LIVE_ADDRESS_CHANGE feature
> > > > No, this typically cannot be fixed, and it doesn't really make sense. > > > > The NIC cannot possibly do two scans at a time since it has only a > > > > single radio resource :-) > > > > > > So why is the scan request not per phy then? And should mac address > > > even affect the ongoing scan? Can we simply change it with the scan > > > ongoing? > > > > There are things that affect the scan from the interface, e.g. > > capability overrides, (extended) capabilities, the MAC address is used > > unless randomization is requested, etc. > > > > But they shouldn't change due a mac address change? I wonder if we can > further relax the requirements to allow mac change if > NL80211_SCAN_FLAG_RANDOM_ADDR was used? No, at least with HW scan that would completely confuse the driver - since from the driver's POV we'd remove the interface it's currently managing the scan for. johannes
Re: [PATCH 2/2] mac80211: Support LIVE_ADDRESS_CHANGE feature
Hi, > > The scan_req struct contains a reference to which interface is scanning, > > so it should very well be possible to have > > > > phy0: > > wlan0: IFF_UP & scanning > > wlan1: IFF_UP & change MAC address all the time > > > > just like it's possible to change the MAC address when wlan1 *isn't* > > IFF_UP even if wlan0 is scanning, right? > > > > Indeed. But that is not what you were suggesting earlier with just > checking local->scanning. So if scan_req contains a wdev, then yes it > should be possible to compare the scan_req->wdev to the interface being > changed. Well, yes, but only because I was incrementally going from James's patch, which was checking that only. Similar with the other local-> things being checked here, btw, though in some cases it might be harder to actually determine which wdev is doing something and which isn't. > > No, this typically cannot be fixed, and it doesn't really make sense. > > The NIC cannot possibly do two scans at a time since it has only a > > single radio resource :-) > > So why is the scan request not per phy then? And should mac address > even affect the ongoing scan? Can we simply change it with the scan > ongoing? There are things that affect the scan from the interface, e.g. capability overrides, (extended) capabilities, the MAC address is used unless randomization is requested, etc. johannes
Re: [PATCH 2/2] mac80211: Support LIVE_ADDRESS_CHANGE feature
Hi, > > You could have two interfaces, one which is scanning right now, right? > > And then theoretically you don't care about the other one - it *should* > > be OK to remove/re-add (with new MAC address) the one that *isn't* > > scanning, right? > > Actually, I don't think you can? Unless I'm missing something? All the > scan state is stored on struct ieee80211_local, so if that struct is > allocated per phy as you point out below, then what you suggest is > currently not possible? ? The scan_req struct contains a reference to which interface is scanning, so it should very well be possible to have phy0: wlan0: IFF_UP & scanning wlan1: IFF_UP & change MAC address all the time just like it's possible to change the MAC address when wlan1 *isn't* IFF_UP even if wlan0 is scanning, right? > > But we don't have that granularity here for anything - you're just > > checking "sdata->local->something", and by going from sdata to local > > you've now checked the whole NIC, not just a single interface on that > > NIC. > > Right. But that seems to be a limitation of mac80211 actually. We > can't run two scans concurrently on different interfaces. This is > rather unintuitive given that scan requests require an ifindex/wdev. > > Can this be changed / fixed in mac80211 actually? I would expect that > if a card supports p2p and station simultaneously, then it can scan / go > offchannel on two interfaces simultaneously? Or not? What can iwlwifi > do for example? No, this typically cannot be fixed, and it doesn't really make sense. The NIC cannot possibly do two scans at a time since it has only a single radio resource :-) > > But it's also completely confusing to do it this way because you go from > > "sdata" to "local", and at that point the data that you're working on is > > no longer specific to that one interface, it's actually for the whole > > NIC. > > I agree its confusing, but that seems to be how mac80211 works? See above. > Given the above, I'm not sure I see anything wrong? The switch/case can > probably be gotten rid of, but it actually makes things clear that only > station/p2p_device and adhoc are handled specially. I just don't think they *should* be handled specially. Given your code now, you can have phy0: wlan0: STATION, IFF_UP & something is doing remain-on-channel wlan1: STATION, IFF_UP --> cannot change wlan1 MAC address phy0: wlan0: STATION, IFF_UP & something is doing remain-on-channel wlan1: AP, IFF_UP --> *can* change wlan1 MAC address This doesn't really make much sense? johannes
Re: [PATCH 2/2] mac80211: Support LIVE_ADDRESS_CHANGE feature
Hi, > I concur that scanning should be checked as > if (sdata->local->scanning). So Johannes you're right that the polarity > is reversed. However, __ieee80211_start_scan seems to check for > local->scan_req instead to take deferred scans into account. So I > wonder if that is a better approach. Hmm. I don't think it's necessary. We basically only get to that kind of state if ieee80211_can_scan() returned false - then we assign local->scan_req and defer until ieee80211_run_deferred_scan() is called. But in the meantime, nothing in the scan requests references the MAC address. It does mean that we should grab local->mtx though for these checks, and then all around the interface change, so that we can be sure we don't actually start scanning in the middle of the changes here. johannes
pull-request: mac80211 2019-10-08
Hi Dave, Another week, another set of fixes. Please pull and let me know if there's any problem. Thanks, johannes The following changes since commit 3afb0961884046c8fb4acbce65139088959681c8: tcp: fix slab-out-of-bounds in tcp_zerocopy_receive() (2019-10-03 12:05:34 -0700) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211.git tags/mac80211-for-davem-2019-10-08 for you to fetch changes up to dc0c18ed229cdcca283dd78fefa38273ec37a42c: mac80211: fix scan when operating on DFS channels in ETSI domains (2019-10-07 22:10:50 +0200) A number of fixes: * allow scanning when operating on radar channels in ETSI regdomains * accept deauth frames in IBSS - we have code to parse and handle them, but were dropping them early * fix an allocation failure path in hwsim * fix a failure path memory leak in nl80211 FTM code * fix RCU handling & locking in multi-BSSID parsing * reject malformed SSID in mac80211 (this shouldn't really be able to happen, but defense in depth) * avoid userspace buffer overrun in ancient wext code if SSID was too long Aaron Komisar (1): mac80211: fix scan when operating on DFS channels in ETSI domains Johannes Berg (1): mac80211: accept deauth frames in IBSS mode Michael Vassernis (1): mac80211_hwsim: fix incorrect dev_alloc_name failure goto Navid Emamdoost (1): nl80211: fix memory leak in nl80211_get_ftm_responder_stats Sara Sharon (1): cfg80211: fix a bunch of RCU issues in multi-bssid code Will Deacon (2): mac80211: Reject malformed SSID elements cfg80211: wext: avoid copying malformed SSIDs drivers/net/wireless/mac80211_hwsim.c | 2 +- include/net/cfg80211.h| 8 net/mac80211/mlme.c | 5 +++-- net/mac80211/rx.c | 11 ++- net/mac80211/scan.c | 30 -- net/wireless/nl80211.c| 2 +- net/wireless/reg.c| 1 + net/wireless/reg.h| 8 net/wireless/scan.c | 23 +-- net/wireless/wext-sme.c | 8 ++-- 10 files changed, 71 insertions(+), 27 deletions(-)
Re: [PATCH 2/2] mac80211: Support LIVE_ADDRESS_CHANGE feature
Hi, > > > If you do care about this being more granular then you should check > > > *which* interface is scanning, and then you can still switch the > > > MAC > > > address for *other* interfaces - but I'd still argue it should be > > > independent of interface type. > > So yes these can scan, but this should be covered by the > netif_carrier_ok check which is done first. Not sure what you mean by that. You could have two interfaces, one which is scanning right now, right? And then theoretically you don't care about the other one - it *should* be OK to remove/re-add (with new MAC address) the one that *isn't* scanning, right? But we don't have that granularity here for anything - you're just checking "sdata->local->something", and by going from sdata to local you've now checked the whole NIC, not just a single interface on that NIC. Which is fine, no complaint from me, just in that case you end up failing when really there isn't much need to fail. In fact, in a case like this, actually clearing IFF_UP, changing address and setting IFF_UP would work, concurrently with another interface scanning. > We can just remove the > switch entirely, but the roc_list/scanning check only matters for > station/p2p_client so checking for the other interface types is kinda > pointless and redundant. But it's also completely confusing to do it this way because you go from "sdata" to "local", and at that point the data that you're working on is no longer specific to that one interface, it's actually for the whole NIC. Basically what I'm saying is this: it's confusing and makes no sense to do something like if (this_is_a_certain_netdev_type) check_some_global_data(); > Also I am not sure what you mean by *which* interface. This function is > called on a single interface, so checking what other interfaces are > doing seems strange... My point exactly - but that's what you're doing here in the code. Now I think perhaps without even realizing? johannes
Re: [PATCH 2/2] mac80211: Support LIVE_ADDRESS_CHANGE feature
On Fri, 2019-10-04 at 09:25 -0700, James Prestwood wrote: > > > I'm not even entirely sure it _is_ needed - if we've still not > > created > > the IBSS but are scanning for it or trying to merge the MAC address > > won't really matter yet? Probably? > > I guess its just paranoia, rather be safe than sorry. I can take this > out, but is "Probably?" a good reason? ;) Fair enough, nobody really cares about IBSS anyway ;-) I guess I was mostly wondering if you had noticed anything that would actually be a problem. > Ok so no switch statement, simply just check that we aren't offchannel > or scanning. I guess this would then cover the IBSS case too. Don't think it covers IBSS - that one is really specially accessing some IBSS data. > > If you do care about this being more granular then you should check > > *which* interface is scanning, and then you can still switch the MAC > > address for *other* interfaces - but I'd still argue it should be > > independent of interface type. > > I think maybe in the future we might want this, but for now lets not > worry about it. But just to make sure we are on the same page, your > talking about e.g. hardware with multiple radios so you could be doing > offchannel work/scanning/connecting simultaneously without having to > wait for the current operation to complete? Not really multiple radios, who cares? Just multiple interfaces would be sufficient. You're just removing/adding some interface (as far as the driver is concerned) - doesn't matter if you actually are scanning or something on another interface right? > > And, I'm confused, but isn't the polarity of the scanning check > > wrong? > > Ah yeah, after you pointed that out I realized 'scanning' is a bit > field. I should be doing: > > test_bit(SCAN_HW_SCANNING, &sdata->local->scanning) I think checking for all the bits is fine (and necessary, just HW scan is unlikely to be enough, changing the MAC address would also disrupt a software scan) - just need to invert the polarity? > Either way I'll send another patch with these things addressed. I'll wait, thanks. johannes
Re: [PATCH 1/2] mac80211: Implement Airtime-based Queue Limit (AQL)
On Mon, 2019-10-07 at 21:40 +0200, Toke Høiland-Jørgensen wrote: > > > So if and when we start supporting true multi-band devices we'll have to > > > change these things anyway. So might as well keep everything together so > > > it all gets fixed :) > > > > I guess I'm OK with that, but I'm pretty sure this will come up sooner > > rather than later ... > > > > What else is there though? > > By "it all" I meant "all the airtime fairness stuff". Other than that, I > didn't have anything in particular in mind. I just kinda assumed there > would be lots of places that had an implicit assumption that all devices > on the same phy shares a channel... Not _that_ much - we do have the channel contexts after all. But except for hwsim (*cough cough* I was lazy) nothing actually implements real concurrent multi-channel yet, obviously, but uses a single radio with channel hopping... johannes
Re: [PATCH v2 1/2] mac80211: Implement Airtime-based Queue Limit (AQL)
On Sun, 2019-10-06 at 21:31 -0700, Kan Yan wrote:
> +/**
> + * ieee80211_sta_update_pending_airtime - update txq's estimated airtime
> + *
> + * Update the estimated total airtime of frames queued in the lower layer
> queue.
> + *
> + * The airtime is estimated using frame length and the last reported data
> + * rate. The pending airtime for a txq is increased by the estimated
> + * airtime when the frame is relased to the lower layer, and decreased by the
typo - released.
> + * same amount at the tx completion event.
I think this isn't really all that clear, "The airtime is [...]
decreased by the same amount at the tx completion event." makes it sound
like that is implicit? But that's not true, this needs to be called at
that point, afaict?
I'm not sure why you decided to not add the inlines I suggested, but I
still think it'd be clearer to have them to indicate that both need to
be called.
Some note should probably also be there that we really want to decrease
later again with the same value that it was increased with, not with the
actual airtime that's now known due to the TX completion, right?
> + *
> + * @pubsta: the station
> + * @tid: the TID to register airtime for
s/register/update/ now, I guess
> +/**
> + * ieee80211_txq_aql_check - check if a txq can send more frames to firmware
s/firmware/device/ IMHO
> +static ssize_t aql_txq_limit_write(struct file *file,
> +const char __user *user_buf,
> +size_t count,
> +loff_t *ppos)
> +{
> + struct ieee80211_local *local = file->private_data;
> + char buf[100];
> + size_t len;
> + u32 ac, q_limit_low, q_limit_high;
use a space here please, not a tab
> + struct sta_info *sta;
> +
> + if (count > sizeof(buf))
> + return -EINVAL;
> +
> + if (copy_from_user(buf, user_buf, count))
> + return -EFAULT;
> +
> + buf[sizeof(buf) - 1] = '\0';
> + len = strlen(buf);
> + if (len > 0 && buf[len - 1] == '\n')
> + buf[len - 1] = 0;
You could use "0" and "'\0'" consistently - I'd prefer just plain 0, but
here you have two spellings within 4 lines ;-)
> @@ -245,7 +268,6 @@ static ssize_t sta_airtime_write(struct file *file, const
> char __user *userbuf,
> sta->airtime[ac].deficit = sta->airtime_weight;
> spin_unlock_bh(&local->active_txq_lock[ac]);
> }
> -
> return count;
better leave that
> +void ieee80211_sta_update_pending_airtime(struct ieee80211_sta *pubsta, u8
> tid,
> + s32 tx_airtime)
> +{
> + u8 ac = ieee80211_ac_from_tid(tid);
> + struct sta_info *sta = container_of(pubsta, struct sta_info, sta);
> + struct ieee80211_local *local = sta->local;
> +
> + spin_lock_bh(&local->active_txq_lock[ac]);
> + sta->airtime[ac].aql_tx_pending += tx_airtime;
> + local->aql_total_pending_airtime += tx_airtime;
> + WARN_ONCE(sta->airtime[ac].aql_tx_pending < 0, "STA pending airtime <
> 0");
> + WARN_ONCE(local->aql_total_pending_airtime < 0,
> + "Total pending airtime < 0");
I think you should reset them if the warning happens?
> +++ b/net/mac80211/sta_info.h
> @@ -127,11 +127,15 @@ enum ieee80211_agg_stop_reason {
> /* Debugfs flags to enable/disable use of RX/TX airtime in scheduler */
> #define AIRTIME_USE_TX BIT(0)
> #define AIRTIME_USE_RX BIT(1)
> +#define AIRTIME_USE_AQL BIT(2)
>
> struct airtime_info {
> u64 rx_airtime;
> u64 tx_airtime;
> s64 deficit;
> + s32 aql_tx_pending; /* Estimated airtime for frames pending in queue */
This doesn't make sense as an s32. I realize you need it above for the
warning, but you can check for underflow before doing the calculation
and keep the storage unsigned.
> +bool ieee80211_txq_aql_check(struct ieee80211_hw *hw,
> + struct ieee80211_txq *txq)
> +{
> + struct sta_info *sta;
> + struct ieee80211_local *local = hw_to_local(hw);
> +
> + if (!(local->airtime_flags & AIRTIME_USE_AQL))
> + return true;
> +
> + if (!txq->sta)
> + return true;
> +
> + sta = container_of(txq->sta, struct sta_info, sta);
> + if (sta->airtime[txq->ac].aql_tx_pending <
> + sta->airtime[txq->ac].aql_limit_low ||
> + (local->aql_total_pending_airtime < local->aql_threshold &&
> + sta->airtime[txq->ac].aql_tx_pending <
> + sta->airtime[txq->ac].aql_limit_high))
> + return true;
> + else
> + return false;
This is a massive expression ... IMHO it'd be clearer as either
splitting it up into pieces ("if (first) return true; if (second) return
true; return false;") or just returning the value of the expression?
if (x) return true; else return false;
is just
return x;
after all.
johannes
Re: [PATCH 1/2] mac80211: Implement Airtime-based Queue Limit (AQL)
On Sun, 2019-10-06 at 19:40 +0200, Toke Høiland-Jørgensen wrote: > > That's a good point. I haven't thought about real simultaneous dual > > band chipset and such chipset do exists now. Is RSDB support coming to > > mac80211 soon? Just curious if it will be just virtual interfaces or > > something else. I chose "local" instead of "sdata" thinking about the > > case of several virtual interfaces (AP, STA, MESH) operates in the > > same channel, then the interface total could be a better choice. > > > > I am ok with moving the "aql_total_pending_airtime" into sdata, but > > afraid that's not the most optimal choice for the case of multiple > > virtual interfaces operates in the same channel. > > Maybe we could leave it in "local" for now. What do you think? > > I'd lean towards keeping it in 'local' for consistency with all the > other airtime stuff. For now, I think having multiple SSIDs on the same > radio is more common than the reverse (multiple bands on a single > radio). > > In particular, the per-group airtime fairness stuff is definitely > designed on the assumption that all BSSes share the same band. s/band/channel/, presumably. > So if and when we start supporting true multi-band devices we'll have to > change these things anyway. So might as well keep everything together so > it all gets fixed :) I guess I'm OK with that, but I'm pretty sure this will come up sooner rather than later ... What else is there though? johannes
Re: [PATCH net v4 07/12] macvlan: use dynamic lockdep key instead of subclass
On Sat, 2019-10-05 at 18:13 +0900, Taehee Yoo wrote: > > If we place lockdep keys into "struct net_device", this macro would be a > little bit modified and reused. And driver code shape will not be huge > changed. I think this way is better than this v4 way. > So I will try it. What I was thinking was that if we can do this for every VLAN netdev, why shouldn't we do it for *every* netdev unconditionally? Some code could perhaps even be simplified if this was just a general part of netdev allocation. > > But it seems to me the whole nesting also has to be applied here? > > > > __dev_xmit_skb: > > * qdisc_run_begin() > > * sch_direct_xmit() > >* HARD_TX_LOCK(dev, txq, smp_processor_id()); > >* dev_hard_start_xmit() // say this is VLAN > > * dev_queue_xmit() // on real_dev > >* __dev_xmit_skb // recursion on another netdev > > > > Now if you have VLAN-in-VLAN the whole thing will recurse right? > > > > I have checked on this routine. > Only xmit_lock(HARD_TX_LOCK) could be nested. other > qdisc locks(runinng, busylock) will not be nested. OK, I still didn't check it too closely I guess, or got confused which lock I should look at. > This patch already > handles the _xmit_lock key. so I think there is no problem. Right > But I would like to place four lockdep keys(busylock, address, > running, _xmit_lock) into "struct net_device" because of code complexity. > > Let me know if I misunderstood anything. Nothing to misunderstand - I was just asking/wondering why the qdisc locks were not treated the same way. johannes
Re: [PATCH 1/2] mac80211: Implement Airtime-based Queue Limit (AQL)
On Thu, 2019-10-03 at 23:21 -0700, Kan Yan wrote: > > +/* The firmware's transmit queue size limit in airtime */ > +#define IEEE80211_DEFAULT_AQL_INTERFACE_LIMIT24000 I'm not sure I understand this completely, but IMHO rewording it to be like a "NIC limit" would be better. However, I'm not sure it *shouldn't* actually be per interface (i.e. moving from local->aql_total_pending_airtime to sdata->aql_total_pending_airtime because you could have multiple channels etc. involved and then using a single airtime limit across two interfaces that are actually on two different channels (e.g. 2.4 and 5 GHz) doesn't make that much sense. Actually, it does make some sense as long as the NIC is actually channel-hopping ... but that's in the process of changing now, there's going to be hardware really soon (or perhaps already exists) that has real dual-band capabilities... Maybe we can live with this now, but we'd probably expect to change this really soon. johannes
Re: [PATCH 1/2] mac80211: Implement Airtime-based Queue Limit (AQL)
Just took a very brief look so I can think about it while offline ;-)
But some (editorial) comments:
> +/**
> + * ieee80211_sta_register_pending_airtime - register the estimated airtime
> for
> + * the frames pending in lower layer (firmware/hardware) txq.
That doesn't work - the short description must be on a single line.
> + * Update the total pending airtime of the frames released to firmware. The
> + * pending airtime is used by AQL to control queue size in the lower layer.
What does "released to firmware" mean? I guess it should either be
something like "transmitted by the device" or "sitting on the hardware
queues" - I'm guessing the latter?
> + * The airtime is estimated using frame length and the last reported data
> + * rate. The pending airtime for a txq is increased by the estimated
> + * airtime when the frame is relased to the lower layer, and decreased by the
> + * same amount at the tx completion event.
> + *
> + * @pubsta: the station
> + * @tid: the TID to register airtime for
> + * @tx_airtime: the estimated airtime (in usec)
> + * @tx_commpleted: true if called from the tx completion event.
> + */
> +void ieee80211_sta_register_pending_airtime(struct ieee80211_sta *pubsta,
> + u8 tid, u32 tx_airtime,
> + bool tx_completed);
The "bool tx_completed" is a bit confusing IMHO.
Probably better to do something like this:
void ieee80211_sta_update_pending_airtime(sta, tid, *s32* airtime);
static inline void ieee80211_sta_register_pending_airtime(...)
{
ieee80211_sta_update_pending_airtime(..., airtime);
}
static inline void ieee80211_sta_release_pending_airtime(...)
{
ieee8021
1_sta_update_pending_airtime(..., -airtime);
}
or something like that?
> +/**
> + * ieee80211_txq_aritme_check - check if the airtime limit of AQL (Airtime
> based
> + * queue limit) has been reached.
same comment as above - and there's a typo
> + * @hw: pointer obtained from ieee80211_alloc_hw()
> + * @txq: pointer obtained from station or virtual interface
> + * Retrun
typo
> true if the queue limit has not been reached and txq can continue to
> + * release packets to the lower layer.
> + * Return false if the queue limit has been reached and the txq should not
> + * release more frames to the lower layer.
> + */
> +bool
> +ieee80211_txq_airtime_check(struct ieee80211_hw *hw, struct ieee80211_txq
> *txq);
Why is it necessary to call this, vs. just returning NULL when an SKB is
requested?
> +static ssize_t aql_txq_limit_read(struct file *file,
> + char __user *user_buf,
> + size_t count,
> + loff_t *ppos)
> +{
> + struct ieee80211_local *local = file->private_data;
> + char buf[400];
> + int len = 0;
> +
> + rcu_read_lock();
> + len = scnprintf(buf, sizeof(buf),
> + "AC AQL limit low AQL limit high\n"
> + "0 %u %u\n"
> + "1 %u %u\n"
> + "2 %u %u\n"
> + "3 %u %u\n",
> + local->aql_txq_limit_low[0],
> + local->aql_txq_limit_high[0],
> + local->aql_txq_limit_low[1],
> + local->aql_txq_limit_high[1],
> + local->aql_txq_limit_low[2],
> + local->aql_txq_limit_high[2],
> + local->aql_txq_limit_low[3],
> + local->aql_txq_limit_high[3]);
> + rcu_read_unlock();
I don't understand the RCI critical section here, you do nothing that
would require it.
> + return simple_read_from_buffer(user_buf, count, ppos,
> +buf, len);
> +}
> +
> +static ssize_t aql_txq_limit_write(struct file *file,
> +const char __user *user_buf,
> +size_t count,
> +loff_t *ppos)
> +{
> + struct ieee80211_local *local = file->private_data;
> + char buf[100];
> + size_t len;
> + u32 ac, q_limit_low, q_limit_high;
> + struct sta_info *sta;
> +
> + if (count > sizeof(buf))
> + return -EINVAL;
> +
> + if (copy_from_user(buf, user_buf, count))
> + return -EFAULT;
> +
> + buf[sizeof(buf) - 1] = '\0';
> + len = strlen(buf);
> + if (len > 0 && buf[len - 1] == '\n')
> + buf[len - 1] = 0;
> +
> + if (sscanf(buf, "%u %u %u", &ac, &q_limit_low, &q_limit_high) == 3) {
> + if (ac < IEEE80211_NUM_ACS) {
The double indentation is a bit odd here - why not return -EINVAL
immediately if any of the checks doesn't work?
if (sscanf(...) != 3)
return -EINVAL;
if (ac >= NUM_ACS)
return -EINVAL;
[...]
> + buf[sizeof(_buf) - 1] = '\0';
> + if (sscanf(buf, "queue limit %u
Re: [PATCH v3] mac80211: fix scan blocked on DFS channels in ETSI domains
On Thu, 2019-10-03 at 08:13 +, Aaron Komisar wrote: > > The real reason of scan failure is because mac80211 checks if it's DFS > > channel, but it doesn't check if CAC is done or not. > > The problem is that scan request is blocked in ETSI reg domains. In non-ETSI > reg domains the behavior is fine. > > cfg80211 blocks scan in non-ETSI reg domains and allows leaving the channel > in ETSI reg domains. I think that if we add a function in mac80211, which > checks if we can leave the operating channel this function should also take > into account the reg domain for completeness. > > So to solve the issue, the right approach should be "check if DFS > > channels and check if CAC is done". > > We can't scan while CAC is in progress but why must we verify that CAC was > done > in order to perform a scan operation? I agree that'd be weird - if CAC wasn't done we shouldn't be operating on that channel to start with? Peter, can you clarify your objection? Just to be sure - we're talking here about the channel we're currently operating on, not any channel that we might want to scan on. I also note that regulatory_pre_cac_allowed() is named a bit confusingly in this context, but I understand it - maybe a comment would be worthwhile where this function is used, saying e.g. /* * If pre-CAC is allowed, we can also briefly leave the channel * for scanning purposes. */ or something like that. I do wonder if we should pull up the check for "cac_started" into cfg80211? But then if we do that, we could maybe even pull *all* of the checks up? But maybe not because of the tracking which channels we're on etc. But at least the "cac_started" seems feasible? Thanks, johannes
Re: [PATCH 2/2] mac80211: Support LIVE_ADDRESS_CHANGE feature
Hi,
I was tempted to apply this (sans the feature advertisement part that I
don't think should be in nl80211), but:
>
> Signed-off-by: James Prestwood
Please add a commit log.
> +static int ieee80211_can_live_addr_change(struct ieee80211_sub_if_data
> *sdata)
> +{
> + if (netif_carrier_ok(sdata->dev))
> + return -EBUSY;
> +
> + switch (sdata->vif.type) {
> + case NL80211_IFTYPE_AP:
> + case NL80211_IFTYPE_P2P_GO:
> + case NL80211_IFTYPE_AP_VLAN:
> + case NL80211_IFTYPE_WDS:
> + case NL80211_IFTYPE_MESH_POINT:
> + case NL80211_IFTYPE_MONITOR:
> + case NL80211_IFTYPE_OCB:
> + /* No further checking required, when started or UP these
> + * interface types set carrier
> + */
> + break;
> + case NL80211_IFTYPE_ADHOC:
> + if (sdata->u.ibss.ssid_len != 0)
> + return -EBUSY;
Can you please document why this is there? Maybe all of the conditions,
for that matter.
I'm not even entirely sure it _is_ needed - if we've still not created
the IBSS but are scanning for it or trying to merge the MAC address
won't really matter yet? Probably?
> + break;
> + case NL80211_IFTYPE_STATION:
> + case NL80211_IFTYPE_P2P_CLIENT:
> + if (!list_empty(&sdata->local->roc_list) ||
> + !sdata->local->scanning)
> + return -EBUSY;
AP, mesh and other interfaces *can* scan, so that test should be pulled
out to be generic - but then in fact all of them should probably be
generic - ROC maybe can't be done on other interfaces yet, but unless
you're going to check *which* interface is actually doing the ROC, you
should just make that a generic check that applies to all interfaces.
If you do care about this being more granular then you should check
*which* interface is scanning, and then you can still switch the MAC
address for *other* interfaces - but I'd still argue it should be
independent of interface type.
And, I'm confused, but isn't the polarity of the scanning check wrong?
johannes
[PATCH v3] mac80211: simplify TX aggregation start
From: Johannes Berg There really is no need to make drivers call the ieee80211_start_tx_ba_cb_irqsafe() function and then schedule the worker if all we want is to set a bit. Add a new return value (that was previously considered invalid) to indicate that the driver is immediately ready for the session, and make drivers use it. The only drivers that remain different are the Intel ones as they need to negotiate more with the firmware. Signed-off-by: Johannes Berg --- v2: fix compilation v3: fix rt2800lib to not return 1 for -ENOSPC --- drivers/net/wireless/ath/ath9k/htc_drv_main.c | 2 +- drivers/net/wireless/ath/ath9k/main.c | 2 +- drivers/net/wireless/ath/carl9170/main.c | 3 +-- drivers/net/wireless/ath/wcn36xx/main.c | 5 +++-- .../broadcom/brcm80211/brcmsmac/mac80211_if.c | 3 +-- drivers/net/wireless/intel/iwlegacy/4965-mac.c| 2 +- drivers/net/wireless/intel/iwlwifi/dvm/tx.c | 2 +- drivers/net/wireless/intel/iwlwifi/mvm/sta.c | 5 ++--- drivers/net/wireless/mac80211_hwsim.c | 3 +-- drivers/net/wireless/marvell/mwl8k.c | 2 +- drivers/net/wireless/mediatek/mt76/mt7603/main.c | 3 +-- drivers/net/wireless/mediatek/mt76/mt7615/main.c | 3 +-- drivers/net/wireless/mediatek/mt76/mt76x02_util.c | 3 +-- drivers/net/wireless/mediatek/mt7601u/main.c | 3 +-- drivers/net/wireless/ralink/rt2x00/rt2800lib.c| 4 ++-- drivers/net/wireless/realtek/rtlwifi/base.c | 3 +-- drivers/net/wireless/realtek/rtw88/mac80211.c | 3 +-- drivers/net/wireless/rsi/rsi_91x_mac80211.c | 3 +-- include/net/mac80211.h| 11 +-- net/mac80211/agg-tx.c | 9 - 20 files changed, 39 insertions(+), 35 deletions(-) diff --git a/drivers/net/wireless/ath/ath9k/htc_drv_main.c b/drivers/net/wireless/ath/ath9k/htc_drv_main.c index a82ad739ab80..791f6633667c 100644 --- a/drivers/net/wireless/ath/ath9k/htc_drv_main.c +++ b/drivers/net/wireless/ath/ath9k/htc_drv_main.c @@ -1674,7 +1674,7 @@ static int ath9k_htc_ampdu_action(struct ieee80211_hw *hw, case IEEE80211_AMPDU_TX_START: ret = ath9k_htc_tx_aggr_oper(priv, vif, sta, action, tid); if (!ret) - ieee80211_start_tx_ba_cb_irqsafe(vif, sta->addr, tid); + ret = IEEE80211_AMPDU_TX_START_IMMEDIATE; break; case IEEE80211_AMPDU_TX_STOP_CONT: case IEEE80211_AMPDU_TX_STOP_FLUSH: diff --git a/drivers/net/wireless/ath/ath9k/main.c b/drivers/net/wireless/ath/ath9k/main.c index 34121fbf32e3..0548aa3702e3 100644 --- a/drivers/net/wireless/ath/ath9k/main.c +++ b/drivers/net/wireless/ath/ath9k/main.c @@ -1921,7 +1921,7 @@ static int ath9k_ampdu_action(struct ieee80211_hw *hw, ath9k_ps_wakeup(sc); ret = ath_tx_aggr_start(sc, sta, tid, ssn); if (!ret) - ieee80211_start_tx_ba_cb_irqsafe(vif, sta->addr, tid); + ret = IEEE80211_AMPDU_TX_START_IMMEDIATE; ath9k_ps_restore(sc); break; case IEEE80211_AMPDU_TX_STOP_FLUSH: diff --git a/drivers/net/wireless/ath/carl9170/main.c b/drivers/net/wireless/ath/carl9170/main.c index 40a8054f8aa6..5914926a5c5b 100644 --- a/drivers/net/wireless/ath/carl9170/main.c +++ b/drivers/net/wireless/ath/carl9170/main.c @@ -1449,8 +1449,7 @@ static int carl9170_op_ampdu_action(struct ieee80211_hw *hw, rcu_assign_pointer(sta_info->agg[tid], tid_info); spin_unlock_bh(&ar->tx_ampdu_list_lock); - ieee80211_start_tx_ba_cb_irqsafe(vif, sta->addr, tid); - break; + return IEEE80211_AMPDU_TX_START_IMMEDIATE; case IEEE80211_AMPDU_TX_STOP_CONT: case IEEE80211_AMPDU_TX_STOP_FLUSH: diff --git a/drivers/net/wireless/ath/wcn36xx/main.c b/drivers/net/wireless/ath/wcn36xx/main.c index 79998a3ddb7a..a276dae30887 100644 --- a/drivers/net/wireless/ath/wcn36xx/main.c +++ b/drivers/net/wireless/ath/wcn36xx/main.c @@ -1084,6 +1084,7 @@ static int wcn36xx_ampdu_action(struct ieee80211_hw *hw, enum ieee80211_ampdu_mlme_action action = params->action; u16 tid = params->tid; u16 *ssn = ¶ms->ssn; + int ret = 0; wcn36xx_dbg(WCN36XX_DBG_MAC, "mac ampdu action action %d tid %d\n", action, tid); @@ -1106,7 +1107,7 @@ static int wcn36xx_ampdu_action(struct ieee80211_hw *hw, sta_priv->ampdu_state[tid] = WCN36XX_AMPDU_START; spin_unlock_bh(&sta_priv->ampdu_lock); - ieee80211_start_tx_ba_cb_irqsafe(vif, sta->addr, tid); + ret = IEEE80211_AMPDU_TX_START_IMMEDIATE; break; case IEEE80211_AMPDU_TX_OPERATIONAL:
Re: [PATCH] mac80211: simplify TX aggregation start
On Wed, 2019-10-02 at 11:08 +0200, Stanislaw Gruszka wrote: > On Tue, Oct 01, 2019 at 10:06:29PM +0200, Johannes Berg wrote: > > index f1cdcd61c54a..b74e758cce09 100644 > > --- a/drivers/net/wireless/ralink/rt2x00/rt2800lib.c > > +++ b/drivers/net/wireless/ralink/rt2x00/rt2800lib.c > > @@ -10489,7 +10489,7 @@ int rt2800_ampdu_action(struct ieee80211_hw *hw, > > struct ieee80211_vif *vif, > > */ > > break; > > case IEEE80211_AMPDU_TX_START: > > - ieee80211_start_tx_ba_cb_irqsafe(vif, sta->addr, tid); > > + ret = IEEE80211_AMPDU_TX_START_IMMEDIATE; > > break; > > case IEEE80211_AMPDU_TX_STOP_CONT: > > case IEEE80211_AMPDU_TX_STOP_FLUSH: > > > +#define IEEE80211_AMPDU_TX_START_IMMEDIATE 1 > > + > > /** > > On rt2x00 we already return 1 for case we do not have free HW WCID > number for a remote station. > > if (sta_priv->wcid > WCID_END) > return 1; > > So we should change that to some other error code i.e. -ENOSPC. Hah, umm... that's kinda weird, but ok :) johannes
[PATCH v2] mac80211: simplify TX aggregation start
From: Johannes Berg There really is no need to make drivers call the ieee80211_start_tx_ba_cb_irqsafe() function and then schedule the worker if all we want is to set a bit. Add a new return value (that was previously considered invalid) to indicate that the driver is immediately ready for the session, and make drivers use it. The only drivers that remain different are the Intel ones as they need to negotiate more with the firmware. Signed-off-by: Johannes Berg --- drivers/net/wireless/ath/ath9k/htc_drv_main.c | 2 +- drivers/net/wireless/ath/ath9k/main.c | 2 +- drivers/net/wireless/ath/carl9170/main.c | 3 +-- drivers/net/wireless/ath/wcn36xx/main.c | 5 +++-- .../broadcom/brcm80211/brcmsmac/mac80211_if.c | 3 +-- drivers/net/wireless/intel/iwlegacy/4965-mac.c| 2 +- drivers/net/wireless/intel/iwlwifi/dvm/tx.c | 2 +- drivers/net/wireless/intel/iwlwifi/mvm/sta.c | 5 ++--- drivers/net/wireless/mac80211_hwsim.c | 3 +-- drivers/net/wireless/marvell/mwl8k.c | 2 +- drivers/net/wireless/mediatek/mt76/mt7603/main.c | 3 +-- drivers/net/wireless/mediatek/mt76/mt7615/main.c | 3 +-- drivers/net/wireless/mediatek/mt76/mt76x02_util.c | 3 +-- drivers/net/wireless/mediatek/mt7601u/main.c | 3 +-- drivers/net/wireless/ralink/rt2x00/rt2800lib.c| 2 +- drivers/net/wireless/realtek/rtlwifi/base.c | 3 +-- drivers/net/wireless/realtek/rtw88/mac80211.c | 3 +-- drivers/net/wireless/rsi/rsi_91x_mac80211.c | 3 +-- include/net/mac80211.h| 11 +-- net/mac80211/agg-tx.c | 9 - 20 files changed, 38 insertions(+), 34 deletions(-) diff --git a/drivers/net/wireless/ath/ath9k/htc_drv_main.c b/drivers/net/wireless/ath/ath9k/htc_drv_main.c index a82ad739ab80..791f6633667c 100644 --- a/drivers/net/wireless/ath/ath9k/htc_drv_main.c +++ b/drivers/net/wireless/ath/ath9k/htc_drv_main.c @@ -1674,7 +1674,7 @@ static int ath9k_htc_ampdu_action(struct ieee80211_hw *hw, case IEEE80211_AMPDU_TX_START: ret = ath9k_htc_tx_aggr_oper(priv, vif, sta, action, tid); if (!ret) - ieee80211_start_tx_ba_cb_irqsafe(vif, sta->addr, tid); + ret = IEEE80211_AMPDU_TX_START_IMMEDIATE; break; case IEEE80211_AMPDU_TX_STOP_CONT: case IEEE80211_AMPDU_TX_STOP_FLUSH: diff --git a/drivers/net/wireless/ath/ath9k/main.c b/drivers/net/wireless/ath/ath9k/main.c index 34121fbf32e3..0548aa3702e3 100644 --- a/drivers/net/wireless/ath/ath9k/main.c +++ b/drivers/net/wireless/ath/ath9k/main.c @@ -1921,7 +1921,7 @@ static int ath9k_ampdu_action(struct ieee80211_hw *hw, ath9k_ps_wakeup(sc); ret = ath_tx_aggr_start(sc, sta, tid, ssn); if (!ret) - ieee80211_start_tx_ba_cb_irqsafe(vif, sta->addr, tid); + ret = IEEE80211_AMPDU_TX_START_IMMEDIATE; ath9k_ps_restore(sc); break; case IEEE80211_AMPDU_TX_STOP_FLUSH: diff --git a/drivers/net/wireless/ath/carl9170/main.c b/drivers/net/wireless/ath/carl9170/main.c index 40a8054f8aa6..5914926a5c5b 100644 --- a/drivers/net/wireless/ath/carl9170/main.c +++ b/drivers/net/wireless/ath/carl9170/main.c @@ -1449,8 +1449,7 @@ static int carl9170_op_ampdu_action(struct ieee80211_hw *hw, rcu_assign_pointer(sta_info->agg[tid], tid_info); spin_unlock_bh(&ar->tx_ampdu_list_lock); - ieee80211_start_tx_ba_cb_irqsafe(vif, sta->addr, tid); - break; + return IEEE80211_AMPDU_TX_START_IMMEDIATE; case IEEE80211_AMPDU_TX_STOP_CONT: case IEEE80211_AMPDU_TX_STOP_FLUSH: diff --git a/drivers/net/wireless/ath/wcn36xx/main.c b/drivers/net/wireless/ath/wcn36xx/main.c index 79998a3ddb7a..a276dae30887 100644 --- a/drivers/net/wireless/ath/wcn36xx/main.c +++ b/drivers/net/wireless/ath/wcn36xx/main.c @@ -1084,6 +1084,7 @@ static int wcn36xx_ampdu_action(struct ieee80211_hw *hw, enum ieee80211_ampdu_mlme_action action = params->action; u16 tid = params->tid; u16 *ssn = ¶ms->ssn; + int ret = 0; wcn36xx_dbg(WCN36XX_DBG_MAC, "mac ampdu action action %d tid %d\n", action, tid); @@ -1106,7 +1107,7 @@ static int wcn36xx_ampdu_action(struct ieee80211_hw *hw, sta_priv->ampdu_state[tid] = WCN36XX_AMPDU_START; spin_unlock_bh(&sta_priv->ampdu_lock); - ieee80211_start_tx_ba_cb_irqsafe(vif, sta->addr, tid); + ret = IEEE80211_AMPDU_TX_START_IMMEDIATE; break; case IEEE80211_AMPDU_TX_OPERATIONAL: spin_lock_bh(&sta_priv->ampdu_lock); @@ -1131,7 +1132,7
[PATCH] mac80211: pass internal sta to ieee80211_tx_frags()
From: Johannes Berg
This simplifies the code somewhat, and if necessary would let
us access the sta itself in that code.
Signed-off-by: Johannes Berg
---
net/mac80211/tx.c | 15 ---
1 file changed, 4 insertions(+), 11 deletions(-)
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 1fa422782905..938c10f7955b 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -1617,7 +1617,7 @@ static bool ieee80211_queue_skb(struct ieee80211_local
*local,
static bool ieee80211_tx_frags(struct ieee80211_local *local,
struct ieee80211_vif *vif,
- struct ieee80211_sta *sta,
+ struct sta_info *sta,
struct sk_buff_head *skbs,
bool txpending)
{
@@ -1679,7 +1679,7 @@ static bool ieee80211_tx_frags(struct ieee80211_local
*local,
spin_unlock_irqrestore(&local->queue_stop_reason_lock, flags);
info->control.vif = vif;
- control.sta = sta;
+ control.sta = sta ? &sta->sta : NULL;
__skb_unlink(skb, skbs);
drv_tx(local, &control, skb);
@@ -1698,7 +1698,6 @@ static bool __ieee80211_tx(struct ieee80211_local *local,
struct ieee80211_tx_info *info;
struct ieee80211_sub_if_data *sdata;
struct ieee80211_vif *vif;
- struct ieee80211_sta *pubsta;
struct sk_buff *skb;
bool result = true;
__le16 fc;
@@ -1713,11 +1712,6 @@ static bool __ieee80211_tx(struct ieee80211_local *local,
if (sta && !sta->uploaded)
sta = NULL;
- if (sta)
- pubsta = &sta->sta;
- else
- pubsta = NULL;
-
switch (sdata->vif.type) {
case NL80211_IFTYPE_MONITOR:
if (sdata->u.mntr.flags & MONITOR_FLAG_ACTIVE) {
@@ -1744,8 +1738,7 @@ static bool __ieee80211_tx(struct ieee80211_local *local,
break;
}
- result = ieee80211_tx_frags(local, vif, pubsta, skbs,
- txpending);
+ result = ieee80211_tx_frags(local, vif, sta, skbs, txpending);
ieee80211_tpt_led_trig_tx(local, fc, led_len);
@@ -3529,7 +3522,7 @@ static bool ieee80211_xmit_fast(struct
ieee80211_sub_if_data *sdata,
struct ieee80211_sub_if_data, u.ap);
__skb_queue_tail(&tx.skbs, skb);
- ieee80211_tx_frags(local, &sdata->vif, &sta->sta, &tx.skbs, false);
+ ieee80211_tx_frags(local, &sdata->vif, sta, &tx.skbs, false);
return true;
}
--
2.20.1
[PATCH] mac80211: simplify TX aggregation start
From: Johannes Berg There really is no need to make drivers call the ieee80211_start_tx_ba_cb_irqsafe() function and then schedule the worker if all we want is to set a bit. Add a new return value (that was previously considered invalid) to indicate that the driver is immediately ready for the session, and make drivers use it. The only drivers that remain different are the Intel ones as they need to negotiate more with the firmware. Signed-off-by: Johannes Berg --- drivers/net/wireless/ath/ath9k/htc_drv_main.c | 2 +- drivers/net/wireless/ath/ath9k/main.c | 2 +- drivers/net/wireless/ath/carl9170/main.c | 3 +-- drivers/net/wireless/ath/wcn36xx/main.c | 5 +++-- .../broadcom/brcm80211/brcmsmac/mac80211_if.c | 3 +-- drivers/net/wireless/intel/iwlegacy/4965-mac.c| 2 +- drivers/net/wireless/intel/iwlwifi/dvm/tx.c | 2 +- drivers/net/wireless/intel/iwlwifi/mvm/sta.c | 5 ++--- drivers/net/wireless/mac80211_hwsim.c | 3 +-- drivers/net/wireless/marvell/mwl8k.c | 2 +- drivers/net/wireless/mediatek/mt76/mt7603/main.c | 3 +-- drivers/net/wireless/mediatek/mt76/mt7615/main.c | 3 +-- drivers/net/wireless/mediatek/mt76/mt76x02_util.c | 3 +-- drivers/net/wireless/mediatek/mt7601u/main.c | 3 +-- drivers/net/wireless/ralink/rt2x00/rt2800lib.c| 2 +- drivers/net/wireless/realtek/rtlwifi/base.c | 3 +-- drivers/net/wireless/realtek/rtw88/mac80211.c | 3 +-- drivers/net/wireless/rsi/rsi_91x_mac80211.c | 3 +-- include/net/mac80211.h| 11 +-- net/mac80211/agg-tx.c | 9 - 20 files changed, 38 insertions(+), 34 deletions(-) diff --git a/drivers/net/wireless/ath/ath9k/htc_drv_main.c b/drivers/net/wireless/ath/ath9k/htc_drv_main.c index a82ad739ab80..791f6633667c 100644 --- a/drivers/net/wireless/ath/ath9k/htc_drv_main.c +++ b/drivers/net/wireless/ath/ath9k/htc_drv_main.c @@ -1674,7 +1674,7 @@ static int ath9k_htc_ampdu_action(struct ieee80211_hw *hw, case IEEE80211_AMPDU_TX_START: ret = ath9k_htc_tx_aggr_oper(priv, vif, sta, action, tid); if (!ret) - ieee80211_start_tx_ba_cb_irqsafe(vif, sta->addr, tid); + ret = IEEE80211_AMPDU_TX_START_IMMEDIATE; break; case IEEE80211_AMPDU_TX_STOP_CONT: case IEEE80211_AMPDU_TX_STOP_FLUSH: diff --git a/drivers/net/wireless/ath/ath9k/main.c b/drivers/net/wireless/ath/ath9k/main.c index 34121fbf32e3..0548aa3702e3 100644 --- a/drivers/net/wireless/ath/ath9k/main.c +++ b/drivers/net/wireless/ath/ath9k/main.c @@ -1921,7 +1921,7 @@ static int ath9k_ampdu_action(struct ieee80211_hw *hw, ath9k_ps_wakeup(sc); ret = ath_tx_aggr_start(sc, sta, tid, ssn); if (!ret) - ieee80211_start_tx_ba_cb_irqsafe(vif, sta->addr, tid); + ret = IEEE80211_AMPDU_TX_START_IMMEDIATE; ath9k_ps_restore(sc); break; case IEEE80211_AMPDU_TX_STOP_FLUSH: diff --git a/drivers/net/wireless/ath/carl9170/main.c b/drivers/net/wireless/ath/carl9170/main.c index 40a8054f8aa6..5914926a5c5b 100644 --- a/drivers/net/wireless/ath/carl9170/main.c +++ b/drivers/net/wireless/ath/carl9170/main.c @@ -1449,8 +1449,7 @@ static int carl9170_op_ampdu_action(struct ieee80211_hw *hw, rcu_assign_pointer(sta_info->agg[tid], tid_info); spin_unlock_bh(&ar->tx_ampdu_list_lock); - ieee80211_start_tx_ba_cb_irqsafe(vif, sta->addr, tid); - break; + return IEEE80211_AMPDU_TX_START_IMMEDIATE; case IEEE80211_AMPDU_TX_STOP_CONT: case IEEE80211_AMPDU_TX_STOP_FLUSH: diff --git a/drivers/net/wireless/ath/wcn36xx/main.c b/drivers/net/wireless/ath/wcn36xx/main.c index 79998a3ddb7a..a276dae30887 100644 --- a/drivers/net/wireless/ath/wcn36xx/main.c +++ b/drivers/net/wireless/ath/wcn36xx/main.c @@ -1084,6 +1084,7 @@ static int wcn36xx_ampdu_action(struct ieee80211_hw *hw, enum ieee80211_ampdu_mlme_action action = params->action; u16 tid = params->tid; u16 *ssn = ¶ms->ssn; + int ret = 0; wcn36xx_dbg(WCN36XX_DBG_MAC, "mac ampdu action action %d tid %d\n", action, tid); @@ -1106,7 +1107,7 @@ static int wcn36xx_ampdu_action(struct ieee80211_hw *hw, sta_priv->ampdu_state[tid] = WCN36XX_AMPDU_START; spin_unlock_bh(&sta_priv->ampdu_lock); - ieee80211_start_tx_ba_cb_irqsafe(vif, sta->addr, tid); + ret = IEEE80211_AMPDU_TX_START_IMMEDIATE; break; case IEEE80211_AMPDU_TX_OPERATIONAL: spin_lock_bh(&sta_priv->ampdu_lock); @@ -1131,7 +1132,7
Re: [PATCH v2] Can't scan on ETSI domains when operating ch is DFS
On Wed, 2019-09-18 at 15:33 +, Aaron Komisar wrote:
> In non-ETSI reg domains scan is blocked when operating channel is a DFS ch.
> For ETSI domains, however, once DFS channel is marked as available afer
> the CAC, this channel will remain available even after leaving this channel.
> Therefore a new CAC will not be required when scan is done.
>
> In cfg80211 scan is not blocked for ETSI reg domains.
> This patch enables scan in mac80211 as well when operating channel is a radar
> channel for ETSI reg domains (unless CAC is in progress).
Please fix the commit message - the subject needs a prefix ("mac80211:")
and "can't scan" is a bug report, not a changelog entry ...
I think I've understood after staring at this for far too long what
you're trying to do and what the problem is, but that really needs to be
described more accurately.
Thanks,
johannes
pull-request: mac80211 2019-10-01
Hi Dave, Here's a list of fixes - the BHs disabled one has been reported multiple times, and the SSID/MBSSID ordering one has over-the-air security implementations. Please pull and let me know if there's any problem. Thanks, johannes The following changes since commit 68ce6688a5baefde30914fc07fc27292dbbe8320: net: sched: taprio: Fix potential integer overflow in taprio_set_picos_per_byte (2019-09-30 18:32:20 -0700) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211.git tags/mac80211-for-davem-2019-10-01 for you to fetch changes up to d8dec42b5c2d2b273bc30b0e073cfbe832d69902: mac80211: keep BHs disabled while calling drv_tx_wake_queue() (2019-10-01 17:56:19 +0200) A small list of fixes this time: * two null pointer dereference fixes * a fix for preempt-enabled/BHs-enabled (lockdep) splats (that correctly pointed out a bug) * a fix for multi-BSSID ordering assumptions * a fix for the EDMG support, on-stack chandefs need to be initialized properly (now that they're bigger) * beacon (head) data from userspace should be validated ---- Johannes Berg (4): nl80211: validate beacon head cfg80211: validate SSID/MBSSID element ordering assumption cfg80211: initialize on-stack chandefs mac80211: keep BHs disabled while calling drv_tx_wake_queue() Miaoqing Pan (2): nl80211: fix null pointer dereference mac80211: fix txq null pointer dereference net/mac80211/debugfs_netdev.c | 11 +-- net/mac80211/util.c | 13 - net/wireless/nl80211.c| 44 --- net/wireless/reg.c| 2 +- net/wireless/scan.c | 7 ++- net/wireless/wext-compat.c| 2 +- 6 files changed, 66 insertions(+), 13 deletions(-)
Re: [PATCH net v4 01/12] net: core: limit nested device depth
Hi, (jumping out now, forgive me for being so brief) > If I understand correctly, you said about the alignment of > "lower_level" and "upper_level". > I thought this place is a fine position for variables as regards the > alignment and I didn't try to put each variable in different places. > > If I misunderstood your mention, please let me know. Not sure what you mean, alignment doesn't matter for them (they're u8). I was thinking of the packing for the overall struct, we have: unsigned intmax_mtu; unsigned short type; unsigned short hard_header_len; unsigned char min_header_len; + unsigned char upper_level, lower_level; unsigned short needed_headroom; unsigned short needed_tailroom; Previously, there was a one byte hole at that spot due to a single "unsigned char" (after something aligned at least 4 bytes) followed by "unsigned short" - now you push that out a bit. If you place the variables a bit lower, below "name_assign_type", you probably fill a hole instead. Check out the 'pahole' tool. johannes
Re: mac80211_hwsim (kernel 4.18+): wmediumd + 2.4Ghz
On Tue, 2019-10-01 at 10:19 -0300, Ramon Fontes wrote:
> > Do they also have the same version of wmediumd?
>
> Yes, they have the same version. As you can also see through the
> video, I only change the kernel and all the packages have the same
> version. I also tried to check the latest commits of mac80211_hwsim,
> but haven't found a reason for the problem yet.
I had been looking, but didn't consider that where you say "4.18" you
probably meant "4.18.xyz" for some stable version of 4.18 ...
Now that I look at that, I think most likely the reason is commit
119f94a6fefc ("cfg80211: Address some corner cases in scan result
channel updating") which was backported to the 4.18 series.
Before this commit, we'd have used the DS element all the time, after
this commit we'd trust the RX channel on 5 GHz...
Here's a test to reproduce this:
https://p.sipsolutions.net/63b37d07fd52179c.txt
However, I think the only reasonable way to fix this is in wmediumd:
https://p.sipsolutions.net/6c52392b5e31d9d1.txt
Bob, what do you think?
johannes
[PATCH v2] mac80211: keep BHs disabled while calling drv_tx_wake_queue()
From: Johannes Berg Drivers typically expect this, as it's the case for almost all cases where this is called (i.e. from the TX path). Also, the code in mac80211 itself (if the driver calls ieee80211_tx_dequeue()) expects this as it uses this_cpu_ptr() without additional protection. This should fix various reports of the problem: https://bugzilla.kernel.org/show_bug.cgi?id=204127 https://lore.kernel.org/linux-wireless/can5hydrwb3o_fe6a1xdnp1e+xs66d5kieuhhfigkklnqokx...@mail.gmail.com/ https://lore.kernel.org/lkml/nycvar.yfh.7.76.1909111238470@cbobk.fhfr.pm/ Reported-by: Jiri Kosina Reported-by: Aaron Hill Reported-by: Lukas Redlinger Reported-by: Oleksii Shevchuk Signed-off-by: Johannes Berg --- v2: * use local_bh_enable/disable to capture the last occurrence * split spin_lock_bh() into local_bh_disable()/spin_lock() to make it clearer while we unlock them separately --- net/mac80211/util.c | 13 - 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/net/mac80211/util.c b/net/mac80211/util.c index 051a02ddcb85..32a7a53833c0 100644 --- a/net/mac80211/util.c +++ b/net/mac80211/util.c @@ -247,7 +247,8 @@ static void __ieee80211_wake_txqs(struct ieee80211_sub_if_data *sdata, int ac) struct sta_info *sta; int i; - spin_lock_bh(&fq->lock); + local_bh_disable(); + spin_lock(&fq->lock); if (sdata->vif.type == NL80211_IFTYPE_AP) ps = &sdata->bss->ps; @@ -273,9 +274,9 @@ static void __ieee80211_wake_txqs(struct ieee80211_sub_if_data *sdata, int ac) &txqi->flags)) continue; - spin_unlock_bh(&fq->lock); + spin_unlock(&fq->lock); drv_wake_tx_queue(local, txqi); - spin_lock_bh(&fq->lock); + spin_lock(&fq->lock); } } @@ -288,12 +289,14 @@ static void __ieee80211_wake_txqs(struct ieee80211_sub_if_data *sdata, int ac) (ps && atomic_read(&ps->num_sta_ps)) || ac != vif->txq->ac) goto out; - spin_unlock_bh(&fq->lock); + spin_unlock(&fq->lock); drv_wake_tx_queue(local, txqi); + local_bh_enable(); return; out: - spin_unlock_bh(&fq->lock); + spin_unlock(&fq->lock); + local_bh_enable(); } static void -- 2.20.1
Re: [PATCH 2/2] mac80211: minstrel_ht: replace rate stats ewma with a better moving average
On Tue, 2019-10-01 at 13:06 +0200, Johannes Berg wrote: > On Tue, 2019-10-01 at 12:52 +0200, Felix Fietkau wrote: > > Might be useful, yes. The main issue here is that the period / window > > size has to be hardcoded through the coefficient values, unless we find > > a way to do floating point math, including exp() and cos() at compile > > time, including conversion to fixed point. > > Works fine for me? > [snip] I guess really the question is - how do we ensure that happened and fail if it didn't? johannes
Re: [PATCH 2/2] mac80211: minstrel_ht: replace rate stats ewma with a better moving average
On Tue, 2019-10-01 at 12:52 +0200, Felix Fietkau wrote:
>
> Might be useful, yes. The main issue here is that the period / window
> size has to be hardcoded through the coefficient values, unless we find
> a way to do floating point math, including exp() and cos() at compile
> time, including conversion to fixed point.
Works fine for me?
#include
#include
#include
#define E(x) ((int)(exp(x) * (1<<20)))
#define C(x) ((int)(cos(x) * (1<<20)))
int main()
{
int e2 = E(2);
int c2 = C(2);
printf("e2: %d\n", e2);
printf("whole: %d frac: %d\n", e2 >> 20, e2 & ((1 << 20) - 1));
printf("c2: %d\n", c2);
printf("sign: %s whole: %d frac: %d\n", c2 < 0 ? "-" : "", abs(c2) >>
20, abs(c2) & ((1 << 20) - 1));
}
objdump -dr test:
[...]
00401040 :
401040: 48 83 ec 08 sub$0x8,%rsp
401044: be 92 39 76 00 mov$0x763992,%esi //
exp(2) * (1<<20)
401049: bf 10 20 40 00 mov$0x402010,%edi
40104e: 31 c0 xor%eax,%eax
401050: e8 db ff ff ff callq 401030
401055: ba 92 39 06 00 mov$0x63992,%edx
40105a: be 07 00 00 00 mov$0x7,%esi
40105f: 31 c0 xor%eax,%eax
401061: bf 2b 20 40 00 mov$0x40202b,%edi
401066: e8 c5 ff ff ff callq 401030
40106b: be 77 57 f9 ff mov$0xfff95777,%esi
401070: bf 18 20 40 00 mov$0x402018,%edi
401075: 31 c0 xor%eax,%eax
401077: e8 b4 ff ff ff callq 401030
40107c: b9 89 a8 06 00 mov$0x6a889,%ecx//
cos(2) * (1<<20)
401081: 31 d2 xor%edx,%edx
401083: 31 c0 xor%eax,%eax
401085: be 20 20 40 00 mov$0x402020,%esi
40108a: bf 22 20 40 00 mov$0x402022,%edi
40108f: e8 9c ff ff ff callq 401030
[...]
johannes
Re: [PATCH] mac80211: keep BHs disabled while calling drv_tx_wake_queue()
On Tue, 2019-10-01 at 12:56 +0200, Jiri Kosina wrote:
> On Tue, 1 Oct 2019, Toke Høiland-Jørgensen wrote:
>
> > > - spin_unlock_bh(&fq->lock);
> > > + spin_unlock(&fq->lock);
> > > drv_wake_tx_queue(local, txqi);
> > > - spin_lock_bh(&fq->lock);
> > > + spin_lock(&fq->lock);
> >
> > Okay, so this will mean that the drv_wake_tx_queue() entry point will be
> > called with bhs disabled. But there are lots of uses of
> > spin_{,un}lock_bh() in tx.c:
>
> I am fine with whatever fix for this you guys settle on :) Just for the
> record, I proposed this back then:
>
>
> http://lore.kernel.org/r/nycvar.yfh.7.76.1904151300160.9...@cbobk.fhfr.pm
>
> maybe it could be resurected, as I believe it'd fix this one as well?
Yes, it would, but it wouldn't address any other driver that likely has
the same issue :)
johannes
Re: [PATCH] mac80211: keep BHs disabled while calling drv_tx_wake_queue()
On Tue, 2019-10-01 at 12:53 +0200, Toke Høiland-Jørgensen wrote:
>
> > - spin_unlock_bh(&fq->lock);
> > + spin_unlock(&fq->lock);
> > drv_wake_tx_queue(local, txqi);
> > - spin_lock_bh(&fq->lock);
> > + spin_lock(&fq->lock);
>
> Okay, so this will mean that the drv_wake_tx_queue() entry point will be
> called with bhs disabled.
Right.
> But there are lots of uses of
> spin_{,un}lock_bh() in tx.c:
[snip]
> so won't that mean that the driver still gets bhs re-enabled after (for
> instance) the first call to ieee80211_tx_dequeue()?
No, local_bh_disable()/local_bh_enable() is re-entrant and nests fine.
johannes
Re: [PATCH 2/2] mac80211: minstrel_ht: replace rate stats ewma with a better moving average
> This change replaces the EWMA implementation with a moving average that's > designed to significantly reduce lag while keeping a bigger window size > by being better at filtering out noise. > > It is only slightly more expensive than the simple EWMA and still avoids > divisions in its calculation. > > The algorithm is adapted from an implementation intended for a completely > different field (stock market trading), where the tradeoff of lag vs > noise filtering is equally important. It is based on the "smoothing filter" > from http://www.stockspotter.com/files/PredictiveIndicators.pdf. > > I have adapted it to fixed-point math with some constants so that it uses > only addition, bit shifts and multiplication > Would it be worth pulling that out into similar helpers to EWMA in average.h, perhaps even in the same file? You need to keep a bit more state, but essentially the same API should work since EWMA already declares the "struct ewma_something" once you use the DECLARE_EWMA(). johannes
[PATCH] mac80211: keep BHs disabled while calling drv_tx_wake_queue()
From: Johannes Berg Drivers typically expect this, as it's the case for almost all cases where this is called (i.e. from the TX path). Also, the code in mac80211 itself (if the driver calls ieee80211_tx_dequeue()) expects this as it uses this_cpu_ptr() without additional protection. This should fix various reports of the problem: https://bugzilla.kernel.org/show_bug.cgi?id=204127 https://lore.kernel.org/linux-wireless/can5hydrwb3o_fe6a1xdnp1e+xs66d5kieuhhfigkklnqokx...@mail.gmail.com/ https://lore.kernel.org/lkml/nycvar.yfh.7.76.1909111238470@cbobk.fhfr.pm/ Reported-by: Jiri Kosina Reported-by: Aaron Hill Reported-by: Lukas Redlinger Reported-by: Oleksii Shevchuk Signed-off-by: Johannes Berg --- net/mac80211/util.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/net/mac80211/util.c b/net/mac80211/util.c index 051a02ddcb85..ad1e88958da2 100644 --- a/net/mac80211/util.c +++ b/net/mac80211/util.c @@ -273,9 +273,9 @@ static void __ieee80211_wake_txqs(struct ieee80211_sub_if_data *sdata, int ac) &txqi->flags)) continue; - spin_unlock_bh(&fq->lock); + spin_unlock(&fq->lock); drv_wake_tx_queue(local, txqi); - spin_lock_bh(&fq->lock); + spin_lock(&fq->lock); } } -- 2.20.1
Re: [RFCv3 2/3] nl80211: Support >4096 byte NEW_WIPHY event nlmsg
Hi, > > Yeah, that does seem reasonable, especially if we're moving to bigger > > messages anyway. If we do add something huge to each channel, we can > > recover that code I suppose. > > So do you want me to drop the channel splitting logic and only allow > this for bands? Or just keep this since it is already done? I guess I'm saying I don't really care. If we have it now might as well keep it? Actually, don't we already have up to 240 bytes per channel, counting the channel nesting itself and all the flags and WMM data that can be inside? Am I counting this wrong? If this is right, we really do need to be able to split this, because we have ~60 channels in the 6/7 GHz band ... > > > The current logic uses last_channel_pos for some of the trickery in > > > addition to last_good_pos. So nla_put_failure would have to be made > > > aware of that. Perhaps we can store last_good_pos in the stack, but the > > > split mechanism only allows the splits to be done at certain points... > > > > Hmm, not sure I understand. last_channel_pos and last_good_pos are > > basically equivalent, no? In fact, I'm not sure why you need > > last_channel_pos vs. just using last_good_pos instead? Maybe I'm missing > > something. > > Sort of. The way I did it, last_channel_pos keeps track of whether any > channel info was added or not. So if NULL, we simply backtrack to > last_good_pos in nla_put_failure. You can probably use last_good_pos for > everything and an extra variable for the channel info tracking. Right. > > To me, conceptually, the "state->band_start" and "state->chan_start" is > > basically a sub-state of "state->split", so this is underneath state- > > > split == 3 (I think?), you basically get 3.0.0, 3.0.1, 3.0.2, ..., > > 3.1.0, 3.1.1 ... for the state? Which you have to unwind in terms of > > message formatting, but the last_good_pos should be sufficient? > > Right. And as I mentioned above, this could be done, but you probably > need another state variable.. > > > IOW, the only difference I see between the normal split states 1, 2, ... > > and the band/channel split states 3.0.0, 3.0.1, ... is the fact that we > > have to also fix up the nested attributes after we trim to last_good_pos > > on failures. Where am I wrong? > > > > Didn't say that you were ;) No, you didn't, but I thought I probably was missing something :) > To be clear, I think it is a good approach and can be made to work. My > main hesitation is whether doing it now is worth it given the discussion > at the very top. But I can see what I can come up with if you want. See above - unless I'm doing something completely wrong, each channel with all the WMM data and stuff can really be big, no? (Now, why did we put the WMM data into each channel? Maybe we could've done per band? I think it can differ on the UNII subbands though) johannes
Re: [PATCH 1/2] nl80211: Add LIVE_ADDR_CHANGE feature
Hi, > > > Because userspace needs to know if this is supported? > > > IFF_LIVE_ADDR_CHANGE is a private flag... AFAIK userspace has no > > > way of > > > obtaining this. > > > > Oh, annoying. > > > > But that doesn't really mean that nl80211 is an appropriate place to > > advertise it, IMHO? > > The intention of the flag was not soley related to > CMD_CONNECT/CMD_AUTHENTICATE. Its an indication that the > hardware/driver supports a live address change. If you don't want it > here could you suggest a better location for it? I guess RTNL would be the right place? This can hardly be specific to wireless, the flag comes from elsewhere. johannes
Re: [PATCH RFC/RFT 1/4] mac80211: Rearrange ieee80211_tx_info to make room for tx_time_est
On Tue, 2019-10-01 at 11:08 +0200, Toke Høiland-Jørgensen wrote: > > Awesome! Any idea for how to make it work on big-endian systems? I got a > splat from the kbuild robot that triggered the BUILD_BUG_ON when > building for m68k. I assume it's the union with codel_time_t that ends > up being aligned wrong... Hmm. Pad out the u16 part of the union by putting it into a struct, or perhaps it's enough to make the union __packed? johannes
Re: [PATCH 1/2] nl80211: validate beacon head
Hi,
> This commit has been processed because it contains a "Fixes:" tag,
> fixing commit: ed1b6cc7f80f cfg80211/nl80211: add beacon settings.
[...]
> NOTE: The patch will not be queued to stable trees until it is upstream.
>
> How should we proceed with this patch?
You tell me! I can make a backport (below), but I don't know what to do
with it until the real commit actually makes it upstream ... where did
you even find it?
You also need to cherry-pick
0f3b07f027f8 ("cfg80211: add and use strongly typed element iteration macros")
7388afe09143 ("cfg80211: Use const more consistently in for_each_element
macros")
as dependencies - the latter doesn't apply cleanly as it has a change
that doesn't apply, but that part of the change isn't necessary.
johannes
>From 35b3085c0087933b77e36e28776cffac9cf27338 Mon Sep 17 00:00:00 2001
From: Johannes Berg
Date: Fri, 20 Sep 2019 22:31:24 +0300
Subject: [PATCH] nl80211: validate beacon head
Commit 8a3347aa110c76a7f87771999aed491d1d8779a8 upstream.
We currently don't validate the beacon head, i.e. the header,
fixed part and elements that are to go in front of the TIM
element. This means that the variable elements there can be
malformed, e.g. have a length exceeding the buffer size, but
most downstream code from this assumes that this has already
been checked.
Add the necessary checks to the netlink policy.
Cc: sta...@vger.kernel.org
Fixes: ed1b6cc7f80f ("cfg80211/nl80211: add beacon settings")
Link:
https://lore.kernel.org/r/1569009255-I7ac7fbe9436e9d8733439eab8acbbd35e55c74ef@changeid
Signed-off-by: Johannes Berg
---
net/wireless/nl80211.c | 38 ++
1 file changed, 38 insertions(+)
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 2a85bff6a8f3..58d1b0d5cc84 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -200,6 +200,38 @@ cfg80211_get_dev_from_info(struct net *netns, struct
genl_info *info)
return __cfg80211_rdev_from_attrs(netns, info->attrs);
}
+static int validate_beacon_head(const struct nlattr *attr,
+ struct netlink_ext_ack *extack)
+{
+ const u8 *data = nla_data(attr);
+ unsigned int len = nla_len(attr);
+ const struct element *elem;
+ const struct ieee80211_mgmt *mgmt = (void *)data;
+ unsigned int fixedlen = offsetof(struct ieee80211_mgmt,
+u.beacon.variable);
+
+ if (len < fixedlen)
+ goto err;
+
+ if (ieee80211_hdrlen(mgmt->frame_control) !=
+ offsetof(struct ieee80211_mgmt, u.beacon))
+ goto err;
+
+ data += fixedlen;
+ len -= fixedlen;
+
+ for_each_element(elem, data, len) {
+ /* nothing */
+ }
+
+ if (for_each_element_completed(elem, data, len))
+ return 0;
+
+err:
+ NL_SET_ERR_MSG_ATTR(extack, attr, "malformed beacon head");
+ return -EINVAL;
+}
+
/* policy for the attributes */
static const struct nla_policy nl80211_policy[NUM_NL80211_ATTR] = {
[NL80211_ATTR_WIPHY] = { .type = NLA_U32 },
@@ -4014,6 +4046,12 @@ static int nl80211_parse_beacon(struct nlattr *attrs[],
memset(bcn, 0, sizeof(*bcn));
if (attrs[NL80211_ATTR_BEACON_HEAD]) {
+ int ret = validate_beacon_head(attrs[NL80211_ATTR_BEACON_HEAD],
+ NULL);
+
+ if (ret)
+ return ret;
+
bcn->head = nla_data(attrs[NL80211_ATTR_BEACON_HEAD]);
bcn->head_len = nla_len(attrs[NL80211_ATTR_BEACON_HEAD]);
if (!bcn->head_len)
--
2.20.1
Re: [PATCH RFC/RFT 2/4] mac80211: Add API function to set the last TX bitrate for a station
On Thu, 2019-09-19 at 14:22 +0200, Toke Høiland-Jørgensen wrote:
Given a ULL constant:
> +/* constants for calculating reciprocals to avoid division in fast path */
> +#define IEEE80211_RECIPROCAL_DIVISOR 0x1ULL
[...]
> +void ieee80211_sta_set_last_tx_bitrate(struct ieee80211_sta *pubsta,
> +u32 rate)
> +{
> + struct sta_info *sta = container_of(pubsta, struct sta_info, sta);
> +
> + sta->last_tx_bitrate = rate;
> + sta->last_tx_bitrate_reciprocal = ((u64)IEEE80211_RECIPROCAL_DIVISOR /
> rate);
that cast seems unnecessary?
johannes
Re: [PATCH RFC/RFT 1/4] mac80211: Rearrange ieee80211_tx_info to make room for tx_time_est
Hi, Sorry for the long time to review here ... On Thu, 2019-09-19 at 14:22 +0200, Toke Høiland-Jørgensen wrote: > From: Toke Høiland-Jørgensen > > To implement airtime queue limiting, we need to keep a running account of > the estimated airtime of all skbs queued into the device. Do to this > correctly, we need to store the airtime estimate into the skb so we can > decrease the outstanding balance when the skb is freed. This means that the > time estimate must be stored somewhere that will survive for the lifetime > of the skb. > > Fortunately, we had a couple of bytes left in the 'status' field in the > ieee80211_tx_info; and since we only plan to calculate the airtime estimate > after the skb is dequeued from the FQ structure, on the control side we can > share the space with the codel enqueue time. And by rearranging the order > of elements it is possible to have the position of the new tx_time_est line > up between the control and status structs, so the value will survive from > when mac80211 hands the packet to the driver, and until the driver either > frees it, or hands it back through TX status. Seems reasonable to me, if we end up needing it and don't have an out- of-band path (that you seem to have been discussing in this long thread too) johannes
Re: mac80211_hwsim (kernel 4.18+): wmediumd + 2.4Ghz
On Mon, 2019-09-30 at 15:28 -0300, Ramon Fontes wrote: > > Based on this info, looks like hostapd/wpa_s versions might be causing > > the difference, > > can you please confirm the versions on both? > > They have the same hostap (hostapd + wpa_s) version: > Hostapd v2.10-devel-hostap_2_9-102-g12de14907+ > wpa_supplicant v2.10-devel-hostap_2_9-102-g12de14907+ Do they also have the same version of wmediumd? > I've tested with v2.8-devel-hostap_2_7-313-g3e949655c+ too. > > In this short video (https://www.youtube.com/watch?v=f6rrHIGpePA - > running on VirtualBox) you can see the problem in action. Through this > video you can see that it works with 4.16 and 4.17, and doesn't work > with 4.19 (I forgot to repeat the test with 4.18, but I confirm that > it has the same behaviour as 4.19). Regardless, we should fix this stuff ... johannes
Re: [PATCH net v4 00/12] net: fix nested device bugs
On Sun, 2019-09-29 at 17:31 +0900, Taehee Yoo wrote: > virt_wifi case is a little bit different case. Well, arguably, it was also just missing this - it just looks different :) > I add the last patch that is to fix refcnt leaks in the virt_wifi module. > The way to fix this is to add notifier routine. > The notifier routine could delete lower device before deleting > virt_wifi device. > If virt_wifi devices are nested, notifier would work recursively. > At that time, it would make stack memory overflow. > > Actually, before this patch, virt_wifi doesn't have the same problem. > So, I will update a comment in a v5 patch. OK, sure. > Many other devices use this way to avoid wrong nesting configuration. > And I think it's a good way. > But we should think about the below configuration. > > vlan5 >| > virt_wifi4 >| > vlan3 >| > virt_wifi2 >| > vlan1 >| > dummy0 > > That code wouldn't avoid this configuration. > And all devices couldn't avoid this config. Good point, so then really that isn't useful to check - most people won't try to set it up that way (since it's completely useless) and if they do anyway too much nesting would be caught by your patchset here. > I have been considering this case, but I couldn't make a decision yet. > Maybe common netdev function is needed to find the same device type > in their graph. I don't think it's worthwhile just to prevent somebody from making a configuration that we think now is nonsense. Perhaps they do have some kind of useful use-case for it ... > This is a little bit different question for you. > I found another bug in virt_wifi after my last patch. > Please test below commands > ip link add dummy0 type dummy > ip link add vw1 link dummy0 type virt_wifi > ip link add vw2 link vw1 type virt_wifi > modprobe -rv virt_wifi > > Then, you can see the warning messages. > If SET_NETDEV_DEV() is deleted in the virt_wifi_newlink(), > you can avoid that warning message. > But I'm not sure about it's safe to remove that. > I would really appreciate it if you let me know about that. Hmm, I don't see any warnings. SET_NETDEV_DEV() should be there though. Do you see the same if you stack it with something else inbetween? If not, I guess preventing virt_wifi from stacking on top of itself would be sufficient ... johannes
Re: bug: nl80211 / brcmfmac broken for bcm4329/bcm4330 sdio in linux-next
Hi,
> Since 5.3 landed, brcmfmac has been broken both on bcm4329 and bcm4330
> sdio devices.
Yep, thanks for the report. I don't think you mean 5.3, as that doesn't
contain the problematic commit as far as I can tell? That was commit
2a38075cd0be ("nl80211: Add support for EDMG channels").
I have a fix for this already pending I believe:
https://patchwork.kernel.org/patch/11156631/
johannes
Re: [PATCH net v4 07/12] macvlan: use dynamic lockdep key instead of subclass
Hi, > > I didn't see any discussion on this, but perhaps I missed it? The cost > > would be a bigger netdev struct (when lockdep is enabled), but we > > already have that for all the VLANs etc. it's just in the private data, > > so it's not a _huge_ difference really I'd think, and this is quite a > > bit of code for each device type now. > > Actually I agree with your opinion. > The benefits of this way are to be able to make common helper functions. > That would reduce duplicate codes and we can maintain this more easily. > But I'm not sure about the overhead of this way. So I would like to ask > maintainers and more reviewers about this. :-) > Using "struct nested_netdev_lockdep" looks really good. > I will make common codes such as "struct nested_netdev_lockdep" > and "netdev_devinit_nested_lockdep" and others in a v5 patch. That makes *sense*, but it seems to me that for example in virt_wifi we just missed this part completely, so addressing it in the generic code would still reduce overall code and complexity? Actually, looking at net-next, we already have netdev_lockdep_set_classes() as a macro there that handles all this. I guess having it as a macro makes some sense so it "evaporates" when lockdep isn't enabled. I'd probably try that but maybe somebody else can chime in and say what they think about applying that to *every* netdev instead though. > > What's not really clear to me is why the qdisc locks can actually stay > > the same at all levels? Can they just never nest? But then why are they > > different per device type? > > I didn't test about qdisc so I didn't modify code related to qdisc code. > If someone reviews this, I would really appreciate. I didn't really think hard about it when I wrote this ... But it seems to me the whole nesting also has to be applied here? __dev_xmit_skb: * qdisc_run_begin() * sch_direct_xmit() * HARD_TX_LOCK(dev, txq, smp_processor_id()); * dev_hard_start_xmit() // say this is VLAN * dev_queue_xmit() // on real_dev * __dev_xmit_skb // recursion on another netdev Now if you have VLAN-in-VLAN the whole thing will recurse right? johannes
Re: [PATCH net v4 01/12] net: core: limit nested device depth
Hi,
Sorry for the delay.
> These functions are used as a callback function of
> netdev_walk_all_{upper/lower}_dev(). So these return types are needed.
Ah yes, I missed that, sorry.
> Without storing level storing, a walking graph routine is needed only
> once. The routine would work as a nesting depth validator.
> So that the detach routine doesn't need to walk the graph.
> Whereas, in this patch, both attach and detach routine need to
> walk graph. So, storing nesting variable way is slower than without
> storing nesting variable way because of the detach routine's updating
> upper and lower level routine.
Right, that's what I thought.
> But I'm sure that storing nesting variables is useful because other
> modules already using nesting level values.
> Please look at vlan_get_encap_level() and usecases.
Indeed, I noticed that later.
> If we don't provide nesting level variables, they should calculate
> every time when they need it and this way is easier way to get a
> nesting level. There are use-cases of lower_level variable
> in the 11th patch.
Yes, makes sense, agree. One could argue that you only ever need the
"lower_level" stored, not the "upper_level", but I guess that doesn't
really make a difference.
Placing these in a better position in the struct might make sense - a
cursory look suggested that they weren't filling any of the many holes
there, did you pay attention to that or was the placement more or less
random?
johannes
Re: [PATCH net v4 11/12] net: remove unnecessary variables and callback
On Sat, 2019-09-28 at 16:48 +, Taehee Yoo wrote:
> This patch removes variables and callback these are related to the nested
> device structure.
> devices that can be nested have their own nest_level variable that
> represents the depth of nested devices.
> In the previous patch, new {lower/upper}_level variables are added and
> they replace old private nest_level variable.
> So, this patch removes all 'nest_level' variables.
Ah, well, I see at least this patch also needs the nesting level tracked
in the netdev, at least the "lower_level".
johannes
Re: [PATCH net v4 01/12] net: core: limit nested device depth
Hi,
> int netdev_walk_all_upper_dev_rcu(struct net_device *dev,
> int (*fn)(struct net_device *dev,
> void *data),
> void *data)
> {
[...]
> }
>
> return 0;
> +
> }
that seems like an oversight, probably from editing the patch in
different versions?
> +static int __netdev_update_upper_level(struct net_device *dev, void *data)
> +{
> + dev->upper_level = __netdev_upper_depth(dev) + 1;
> + return 0;
> +}
> +
> +static int __netdev_update_lower_level(struct net_device *dev, void *data)
> +{
> + dev->lower_level = __netdev_lower_depth(dev) + 1;
> + return 0;
> +}
Is there any point in the return value here? You don't really use it,
afaict? I guess I might see the point if it was used for tail-call
optimisation or such?
Also, I dunno, I guess netdevs aren't as much under pressure as SKBs :-)
but do we actually gain much from storing the nesting level at all? You
have to maintain it all the time anyway when adding/removing and that's
the only place where you also check it, so perhaps it wouldn't be that
bad to just count at that time?
But then again the counting would probably be recursive again ...
> return 0;
> +
> }
> EXPORT_SYMBOL_GPL(netdev_walk_all_lower_dev_rcu);
same nit as above
> + __netdev_update_upper_level(dev, NULL);
> + netdev_walk_all_lower_dev(dev, __netdev_update_upper_level, NULL);
> +
> + __netdev_update_lower_level(upper_dev, NULL);
> + netdev_walk_all_upper_dev(upper_dev, __netdev_update_lower_level, NULL);
Actually, if I'm reading this correctly you already walk all the levels
anyway? Then couldn't you calculate the depth at this time and return
it, instead of storing it? Though, if it actually overflowed then you'd
have to walk *again* to undo that?
Hmm, actually, if you don't store the value you don't even need to walk
here I guess, or at least you would only have to do it to verify you
*can* attach, but wouldn't have to in detach?
So it looks to me like on attach (i.e. this code, quoted from
__netdev_upper_dev_link) you're already walking the entire graph to
update the level values, and could probably instead calculate the
nesting depth to validate it?
And then on netdev_upper_dev_unlink() you wouldn't even have to walk the
graph at all, since you only need that to update the values that you
stored.
But maybe I'm misinterpreting this completely?
Thanks,
johannes
Re: [PATCH net v4 00/12] net: fix nested device bugs
> VLAN, BONDING, TEAM, MACSEC, MACVLAN, IPVLAN, VIRT_WIFI and VXLAN.
> But I couldn't test all interface types so there could be more device
> types which have similar problems.
Did you test virt_wifi? I don't see how it *doesn't* have the nesting
problem, and you didn't change it?
No, I see. You're limiting the nesting generally now in patch 1, and the
others are just lockdep fixups (I guess it's surprising virt_wifi
doesn't do this at all?).
FWIW I don't think virt_wifi really benefits at all from stacking, so we
could just do something like
--- a/drivers/net/wireless/virt_wifi.c
+++ b/drivers/net/wireless/virt_wifi.c
@@ -508,6 +508,9 @@ static int virt_wifi_newlink(struct net *src_net, struct
net_device *dev,
else if (dev->mtu > priv->lowerdev->mtu)
return -EINVAL;
+ if (priv->lowerdev->ieee80211_ptr)
+ return -EINVAL;
+
err = netdev_rx_handler_register(priv->lowerdev, virt_wifi_rx_handler,
priv);
if (err) {
IMHO, but of course generally limiting the stack depth is needed anyway
and solves the problem well enough for virt_wifi.
johannes
Re: [PATCH net v4 07/12] macvlan: use dynamic lockdep key instead of subclass
Hi,
I hadn't seen the previous patchsets of this, and looking briefly in the
archives didn't really seem to say anything about this.
However, I'm wondering now: patches 2-7 of this patchset look basically
all identical in a way:
* you set the addr_list_lock's class to a newly registered key inside
the netdev (or rather the private struct, but doesn't make a big
difference)
* you set each TX queue's _xmit_lock's class similarly
* you set the qdisc_tx_busylock/qdisc_running_key
The first two of these look pretty much completely identical.
Would it perhaps make sense to just do that for *every* netdev? Many of
those netdevs won't ever nest so it wouldn't really be useful, but I'm
not convinced it would put that much more strain on lockdep - if
anything, people are probably creating more VLANs than regular PF/VF
netdevs anyway?
I didn't see any discussion on this, but perhaps I missed it? The cost
would be a bigger netdev struct (when lockdep is enabled), but we
already have that for all the VLANs etc. it's just in the private data,
so it's not a _huge_ difference really I'd think, and this is quite a
bit of code for each device type now.
Alternatively, maybe there could just be some common helper code:
struct nested_netdev_lockdep {
struct lock_class_key xmit_lock_key;
struct lock_class_key addr_lock_key;
};
void netdev_init_nested_lockdep(struct net_device *dev,
struct netsted_netdev_lockdep *l)
{
/* ... */
}
so you just have to embed a "struct nested_netdev_lockdep" in your
private data structure and call the common code.
Or maybe make that
void netdev_init_nested_lockdep(
struct net_device *dev,
struct
netsted_netdev_lockdep *l,
struct lock_class_key
*qdisc_tx_busylock_key,
struct lock_class_key *qdisc_running_key)
so you can't really get that part wrong either?
> @@ -922,6 +938,9 @@ static void macvlan_uninit(struct net_device *dev)
> port->count -= 1;
> if (!port->count)
> macvlan_port_destroy(port->dev);
> +
> + lockdep_unregister_key(&vlan->addr_lock_key);
> + lockdep_unregister_key(&vlan->xmit_lock_key);
> }
OK, so I guess you need an equivalent "deinit" function too -
netdev_deinit_nested_lockdep() or so.
What's not really clear to me is why the qdisc locks can actually stay
the same at all levels? Can they just never nest? But then why are they
different per device type?
Thanks,
johannes
Re: [PATCH net v4 12/12] virt_wifi: fix refcnt leak in module exit routine
On Sat, 2019-09-28 at 16:48 +, Taehee Yoo wrote: > virt_wifi_newlink() calls netdev_upper_dev_link() and it internally > holds reference count of lower interface. [...] > This patch adds notifier routine to delete upper interface before deleting > lower interface. Good catch, thanks! For now I'll assume this will go in through net together with the whole series (once ready), shout if you want something else. johannes
Re: mac80211_hwsim: packets being transmitted through the monitor interface
On Fri, 2019-09-27 at 10:01 -0300, Ramon Fontes wrote: > > A monitor interface created on one of the (hwsim or other!) wiphys shows > > all packets seen by that device. > > Weird. When I try to reproduce the same with a physical network > interface, I can see no packets. It only shows the 802.11 protocol. > With mac80211_hwsim, in turn, I can see, for example, TCP packets and > their headers include the 802.11 header. At least with mac80211 drivers that should work everywhere, though you may get encrypted packets if software crypto is used, or crypto headers if hardware crypto is used etc. johannes
Re: mac80211_hwsim: packets being transmitted through the monitor interface
[please quote properly] On Fri, 2019-09-27 at 09:30 -0300, Ramon Fontes wrote: > Yes, I agree that they are different things, but I can also see the > packets through the monitor interface created via iw. Is this expected > too? The hwsim0 interface shows *all* packets on the virtual air. A monitor interface created on one of the (hwsim or other!) wiphys shows all packets seen by that device. johannes
Re: mac80211_hwsim: packets being transmitted through the monitor interface
On Thu, 2019-09-26 at 22:54 -0300, Ramon Fontes wrote: > Hello, > > I've noticed that packets transmitted between two clients connected to > a hostapd AP are also transmitted (injected) through the monitor > interface. Is this expected behavior? You mean on 'hwsim0'? That interface is just for monitoring what's happening on the 'virtual air', so yes. > I can easily modify such > behavior by changing mac80211_hwsim, but it works only with hwsim0. On > the other hand, if I create a monitor interface via iw it doesn't > work. The two are completely different/unrelated things. johannes
Re: Virtual interfaces intel AX200
On Fri, 2019-09-20 at 16:34 +0100, Bruno Antunes wrote: > Hello, > > Does anyone knows if the new intel AX200 or intel AX201 have support > for multiple virtual interfaces? > Is there any intel Wi-Fi device that does support it? TBH I'm not really sure what exactly it does now, but the driver does contain code to advertise the limits. I think it's basically P2P + normal client. johannes
Re: [PATCH] cfg80211: initialize on-stack chandefs
On Mon, 2019-09-23 at 15:12 +0300, Dmitry Osipenko wrote: > > Tested-by: Dmitry Osipenko That was quick, heh! Thanks, johannes
[PATCH] cfg80211: initialize on-stack chandefs
From: Johannes Berg
In a few places we don't properly initialize on-stack chandefs,
resulting in EDMG data to be non-zero, which broke things.
Additionally, in a few places we rely on the driver to init the
data completely, but perhaps we shouldn't as non-EDMG drivers
may not initialize the EDMG data, also initialize it there.
Fixes: 2a38075cd0be ("nl80211: Add support for EDMG channels")
Reported-by: Dmitry Osipenko
Signed-off-by: Johannes Berg
---
net/wireless/nl80211.c | 4 +++-
net/wireless/reg.c | 2 +-
net/wireless/wext-compat.c | 2 +-
3 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index d21b1581a665..b9797a3c110a 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -2636,6 +2636,8 @@ int nl80211_parse_chandef(struct
cfg80211_registered_device *rdev,
control_freq = nla_get_u32(attrs[NL80211_ATTR_WIPHY_FREQ]);
+ memset(chandef, 0, sizeof(*chandef));
+
chandef->chan = ieee80211_get_channel(&rdev->wiphy, control_freq);
chandef->width = NL80211_CHAN_WIDTH_20_NOHT;
chandef->center_freq1 = control_freq;
@@ -3176,7 +3178,7 @@ static int nl80211_send_iface(struct sk_buff *msg, u32
portid, u32 seq, int flag
if (rdev->ops->get_channel) {
int ret;
- struct cfg80211_chan_def chandef;
+ struct cfg80211_chan_def chandef = {};
ret = rdev_get_channel(rdev, wdev, &chandef);
if (ret == 0) {
diff --git a/net/wireless/reg.c b/net/wireless/reg.c
index 5311d0ae2454..420c4207ab59 100644
--- a/net/wireless/reg.c
+++ b/net/wireless/reg.c
@@ -2108,7 +2108,7 @@ static void reg_call_notifier(struct wiphy *wiphy,
static bool reg_wdev_chan_valid(struct wiphy *wiphy, struct wireless_dev *wdev)
{
- struct cfg80211_chan_def chandef;
+ struct cfg80211_chan_def chandef = {};
struct cfg80211_registered_device *rdev = wiphy_to_rdev(wiphy);
enum nl80211_iftype iftype;
diff --git a/net/wireless/wext-compat.c b/net/wireless/wext-compat.c
index 7b6529d81c61..cac9e28d852b 100644
--- a/net/wireless/wext-compat.c
+++ b/net/wireless/wext-compat.c
@@ -798,7 +798,7 @@ static int cfg80211_wext_giwfreq(struct net_device *dev,
{
struct wireless_dev *wdev = dev->ieee80211_ptr;
struct cfg80211_registered_device *rdev = wiphy_to_rdev(wdev->wiphy);
- struct cfg80211_chan_def chandef;
+ struct cfg80211_chan_def chandef = {};
int ret;
switch (wdev->iftype) {
--
2.20.1
[PATCH 2/2] cfg80211: validate SSID/MBSSID element ordering assumption
From: Johannes Berg
The code copying the data assumes that the SSID element is
before the MBSSID element, but since the data is untrusted
from the AP, this cannot be guaranteed.
Validate that this is indeed the case and ignore the MBSSID
otherwise, to avoid having to deal with both cases for the
copy of data that should be between them.
Cc: sta...@vger.kernel.org
Fixes: 0b8fb8235be8 ("cfg80211: Parsing of Multiple BSSID information in
scanning")
Signed-off-by: Johannes Berg
---
net/wireless/scan.c | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/net/wireless/scan.c b/net/wireless/scan.c
index d66e6d4b7555..27d76c4c5cea 100644
--- a/net/wireless/scan.c
+++ b/net/wireless/scan.c
@@ -1711,7 +1711,12 @@ cfg80211_update_notlisted_nontrans(struct wiphy *wiphy,
return;
new_ie_len -= trans_ssid[1];
mbssid = cfg80211_find_ie(WLAN_EID_MULTIPLE_BSSID, ie, ielen);
- if (!mbssid)
+ /*
+* It's not valid to have the MBSSID element before SSID
+* ignore if that happens - the code below assumes it is
+* after (while copying things inbetween).
+*/
+ if (!mbssid || mbssid < trans_ssid)
return;
new_ie_len -= mbssid[1];
rcu_read_lock();
--
2.20.1
[PATCH 1/2] nl80211: validate beacon head
From: Johannes Berg
We currently don't validate the beacon head, i.e. the header,
fixed part and elements that are to go in front of the TIM
element. This means that the variable elements there can be
malformed, e.g. have a length exceeding the buffer size, but
most downstream code from this assumes that this has already
been checked.
Add the necessary checks to the netlink policy.
Cc: sta...@vger.kernel.org
Fixes: ed1b6cc7f80f ("cfg80211/nl80211: add beacon settings")
Signed-off-by: Johannes Berg
---
net/wireless/nl80211.c | 37 +++--
1 file changed, 35 insertions(+), 2 deletions(-)
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index fd05ae1437a9..932854a0c38b 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -201,6 +201,38 @@ cfg80211_get_dev_from_info(struct net *netns, struct
genl_info *info)
return __cfg80211_rdev_from_attrs(netns, info->attrs);
}
+static int validate_beacon_head(const struct nlattr *attr,
+ struct netlink_ext_ack *extack)
+{
+ const u8 *data = nla_data(attr);
+ unsigned int len = nla_len(attr);
+ const struct element *elem;
+ const struct ieee80211_mgmt *mgmt = (void *)data;
+ unsigned int fixedlen = offsetof(struct ieee80211_mgmt,
+u.beacon.variable);
+
+ if (len < fixedlen)
+ goto err;
+
+ if (ieee80211_hdrlen(mgmt->frame_control) !=
+ offsetof(struct ieee80211_mgmt, u.beacon))
+ goto err;
+
+ data += fixedlen;
+ len -= fixedlen;
+
+ for_each_element(elem, data, len) {
+ /* nothing */
+ }
+
+ if (for_each_element_completed(elem, data, len))
+ return 0;
+
+err:
+ NL_SET_ERR_MSG_ATTR(extack, attr, "malformed beacon head");
+ return -EINVAL;
+}
+
static int validate_ie_attr(const struct nlattr *attr,
struct netlink_ext_ack *extack)
{
@@ -322,8 +354,9 @@ const struct nla_policy nl80211_policy[NUM_NL80211_ATTR] = {
[NL80211_ATTR_BEACON_INTERVAL] = { .type = NLA_U32 },
[NL80211_ATTR_DTIM_PERIOD] = { .type = NLA_U32 },
- [NL80211_ATTR_BEACON_HEAD] = { .type = NLA_BINARY,
- .len = IEEE80211_MAX_DATA_LEN },
+ [NL80211_ATTR_BEACON_HEAD] =
+ NLA_POLICY_VALIDATE_FN(NLA_BINARY, validate_beacon_head,
+ IEEE80211_MAX_DATA_LEN),
[NL80211_ATTR_BEACON_TAIL] =
NLA_POLICY_VALIDATE_FN(NLA_BINARY, validate_ie_attr,
IEEE80211_MAX_DATA_LEN),
--
2.20.1
Re: Can Intel AX200 sniff UL OFDMA ?
On Fri, 2019-09-20 at 11:09 -0400, Tim Higgins wrote: > On 9/19/2019 6:05 PM, Johannes Berg wrote: > > Hi Tim, > > > > > I have been using the debug hw_sniffer_params file to tune the AX200 to a > > > specific AID. This > > > works well for capturing OFDMA DL. But I have yet to capture any UL OFDMA > > > frames, or at least I > > > don't think I have. > > > > > > I am looking for QoS data frames that have HE_MU PPDU format. Is this > > > correct? > > > I can see plenty of HE_SU PPDU frames from STA to AP, but no HE_MU uplink. > > > > > > Am I looking for the wrong thing or can the AX200 not sniff OFDMA UL? > > UL OFDMA frames should be HE_TB PPDU format, not HE_MU. They can only be > > sent as a response to trigger frames, so all the participants are > > synchronized etc. > > > > johannes > > Thanks, Johannes. I believe Wireshark shows HE_TB as HE_TRIG (0x3) > under HE information > HE Data 1, correct? Right, see http://www.radiotap.org/fields/HE johannes
Re: Can Intel AX200 sniff UL OFDMA ?
Hi Tim, > I have been using the debug hw_sniffer_params file to tune the AX200 to a > specific AID. This > works well for capturing OFDMA DL. But I have yet to capture any UL OFDMA > frames, or at least I > don't think I have. > > I am looking for QoS data frames that have HE_MU PPDU format. Is this correct? > I can see plenty of HE_SU PPDU frames from STA to AP, but no HE_MU uplink. > > Am I looking for the wrong thing or can the AX200 not sniff OFDMA UL? UL OFDMA frames should be HE_TB PPDU format, not HE_MU. They can only be sent as a response to trigger frames, so all the participants are synchronized etc. johannes
Re: [PATCH 1/2] nl80211: Add LIVE_ADDR_CHANGE feature
On Fri, 2019-09-13 at 13:56 -0700, James Prestwood wrote: > Hi, > > On Fri, 2019-09-13 at 22:48 +0200, Johannes Berg wrote: > > On Fri, 2019-09-13 at 12:59 -0700, James Prestwood wrote: > > > Add a new feature bit signifying that the wireless hardware > > > supports > > > changing the mac address while the underlying net_device is > > > UP. Note > > > that this is slightly different from IFF_LIVE_ADDR_CHANGE as > > > additional > > > restrictions might be imposed by the hardware. Typical > > > restrictions > > > are: > > > - No connection is active on this interface, e.g. carrier is > > > off > > > - No scan is in progress > > > - No offchannel operation is in progress > > > > Hmm, what do you need this patch for? > > > > IFF_LIVE_ADDR_CHANGE should be sufficient to discover it? > > Because userspace needs to know if this is supported? > IFF_LIVE_ADDR_CHANGE is a private flag... AFAIK userspace has no way of > obtaining this. Oh, annoying. But that doesn't really mean that nl80211 is an appropriate place to advertise it, IMHO? And in nl80211 you'd need the flag for if you actually have the "change MAC address during connect" attribute. johannes
Re: [PATCH 1/2] nl80211: Add LIVE_ADDR_CHANGE feature
On Fri, 2019-09-13 at 12:59 -0700, James Prestwood wrote: > Add a new feature bit signifying that the wireless hardware supports > changing the mac address while the underlying net_device is UP. Note > that this is slightly different from IFF_LIVE_ADDR_CHANGE as additional > restrictions might be imposed by the hardware. Typical restrictions > are: > - No connection is active on this interface, e.g. carrier is off > - No scan is in progress > - No offchannel operation is in progress Hmm, what do you need this patch for? IFF_LIVE_ADDR_CHANGE should be sufficient to discover it? johannes
Re: [RFC 0/4] Allow live MAC address change
On Wed, 2019-09-11 at 12:20 -0700, James Prestwood wrote: > I see what your saying, but theses kind of state changes are all over > the place in other APIs, and undocumented: One example is > SCAN_FLAG_FLUSH clearing out the scanning state for all other > processes. Scanning always changes scan list state? > I'm sure I could find more. If we documented this attribute > and behavior I don't see an issue. But I'm sure you could actually find an example :-) That doesn't really mean it's the *right* thing to do though, IMHO. Also, who says that this is the only thing? Next up, somebody wants to randomize the MTU? Ok, probably not, but you could pick a random other rtnetlink attribute and have nl80211 set it. Where do we stop? Thinking this to the extreme - why not add an rtnetlink message interpreter into this code? ;-) Sure, none of that is really seriously likely to happen, but I'm really not convinced we (more or less arbitrarily) need many ways of doing the same thing in the kernel. Either way, regardless of that discussion, I think it'd be good if you could repost the patches for just the "quick win" that we can all agree on, and then we can get those reviewed and into the tree before we need to continue this discussion; after all, while we're discussing saving about 3 milliseconds, you're still wasting around 280 :-) (and the easy one can be done without affecting the other, just need to reorder the patches and split them a bit differently) johannes
Re: [RFC 0/4] Allow live MAC address change
On Wed, 2019-09-11 at 08:54 -0700, James Prestwood wrote:
>
> I could have made this a bit more clear. I initially did measure the
> time to a full connection, including EAPoL, but the more I timed the
> more chance there was for scheduling delays that really threw off the
> results. Not that these results weren't valid, I just would have needed
> to time many many more runs to get a decent averaged time. The method
> of timings I took just isolated things a bit better.
Sure, makes sense, and I didn't think you were doing that, I was just
wondering what exactly you did measure.
> For the three methods below I measured the time from the connection
> initiation (either powering down via RTNL, changing MAC via RTNL, or
> sending CMD_CONNECT) until we got a success callback from CMD_CONNECT,
> including changing the MAC via RTNL in those cases.
Ah, ok.
> Out of curiosity how this behavior is different than the power down +
> RTNL MAC change (the current way of doing things)? If you power down
> the device, change the MAC, then power up does that MAC get reset after
> a disconnection/failure?
No, of course not? But then you're explicitly issuing a command ("change
the MAC address") that is supposed to affect state indefinitely, vs.
issuing a command ("please connect") that isn't really meant to.
If there was one thing that we learned from wext, IMHO it was that
keeping all the state in the kernel is bad for you, and it's much better
to handle things if the state gets reset when you disconnect etc. In
most places that's what we do now and I think it has served us well, so
I'm very reluctant to mix things that need state in the kernel with
those that don't.
(You might not remember wext, but you'd have to issue a bunch of
commands in the right order, and it would keep all the state inbetween;
if you forgot to clear the BSSID after setting it, it'd be remembered
and you couldn't connect to a new AP unless you reset it, etc.)
Thanks,
johannes
Re: [RFCv3 2/3] nl80211: Support >4096 byte NEW_WIPHY event nlmsg
On Wed, 2019-09-11 at 07:41 -0500, Denis Kenzior wrote:
>
> For my dual band cards the channel dump all fits into a ~1k attribute if
> I recall correctly. So there may not really be a need for this. Or at
> the very least we could keep things simple(r) and only split at the band
> level, not at the individual channel level.
Yeah, that does seem reasonable, especially if we're moving to bigger
messages anyway. If we do add something huge to each channel, we can
recover that code I suppose.
> > > [snip]
> > > +chan_put_failure:
> > > + if (!last_channel_pos)
> > > + goto nla_put_failure;
> > > +
> > > + nlmsg_trim(msg, last_channel_pos);
> > > + nla_nest_end(msg, nl_freqs);
> > > nla_nest_end(msg, nl_band);
> > >
> > > - if (state->split) {
> > > - /* start again here */
> > > - if (state->chan_start)
> > > - band--;
> > > + if (state->chan_start < sband->n_channels)
> > > break;
> > > - }
> > > +
> > > + state->chan_start = 0;
> > > + state->band_start++;
> > > }
> > > - nla_nest_end(msg, nl_bands);
> > >
> > > - if (band < NUM_NL80211_BANDS)
> > > - state->band_start = band + 1;
> > > - else
> > > - state->band_start = 0;
> > > +band_put_failure:
> > > + if (!last_channel_pos)
> > > + goto nla_put_failure;
> > > +
> > > + nla_nest_end(msg, nl_bands);
> > >
> > > - /* if bands & channels are done, continue outside */
> > > - if (state->band_start == 0 && state->chan_start == 0)
> > > - state->split_start++;
> > > - if (state->split)
> > > + if (state->band_start < NUM_NL80211_BANDS)
> > > break;
> >
> > Thinking out loud, maybe we could simplify this by just having a small
> > "stack" of nested attributes to end?
> >
> > I mean, essentially, you have here similar code to the nla_put_failure
> > label, in that it finishes and sends out the message, except here you
> > have to end a bunch of nested attributes.
> >
> > What if we did something like
> >
> > #define dump_nest_start(msg, attr) ({ \
> > struct nlattr r = nla_nest_start_noflag(msg, attr); \
> > BUG_ON(nest_stack_depth >= ARRAY_SIZE(nest_stack); \
> > nest_stack[nest_stack_depth++] = r; \
> > r; \
> > })
> >
> > #define dump_nest_end(msg, r) do { \
> > BUG_ON(nest_stack_depth > 0); \
> > nest_stack_depth--; \
> > BUG_ON(nest_stack[nest_stack_depth] == r); \
> > nla_nest_end(msg, r); \
> > } while (0)
> >
> >
> > or something like that (we probably don't want to use
> > nla_nest_start_noflag() for future attributes, etc. but anyway).
> >
> > Then we could unwind any nesting at the end in the common code at the
> > nla_put_failure label very easily, I think?
>
> I see where you're going with this, I think I do anyway...
I'm not sure I am going anywhere with this ;-)
> The current logic uses last_channel_pos for some of the trickery in
> addition to last_good_pos. So nla_put_failure would have to be made
> aware of that. Perhaps we can store last_good_pos in the stack, but the
> split mechanism only allows the splits to be done at certain points...
Hmm, not sure I understand. last_channel_pos and last_good_pos are
basically equivalent, no? In fact, I'm not sure why you need
last_channel_pos vs. just using last_good_pos instead? Maybe I'm missing
something.
To me, conceptually, the "state->band_start" and "state->chan_start" is
basically a sub-state of "state->split", so this is underneath state-
>split == 3 (I think?), you basically get 3.0.0, 3.0.1, 3.0.2, ...,
3.1.0, 3.1.1 ... for the state? Which you have to unwind in terms of
message formatting, but the last_good_pos should be sufficient?
IOW, the only difference I see between the normal split states 1, 2, ...
and the band/channel split states 3.0.0, 3.0.1, ... is the fact that we
have to also fix up the nested attributes after we trim to last_good_pos
on failures. Where am I wrong?
> Right now only the channel dump uses this (and I'm still not fully
> convinced we should go to all the trouble), so one argument would be not
> to introduce something this generic until another user of it manifests
> itself?
I was thinking it'd actually be less complex, but I guess I have to try
to write it to see for myself.
johannes
Re: [RFCv3 3/3] nl80211: Send large new_wiphy events
On Wed, 2019-09-11 at 07:20 -0500, Denis Kenzior wrote:
> > I'm not sure I see how the applications could do buffers that are
> > "inherently" large enough, there's no practical message size limit, is
> > there (32-bits for the size).
>
> The kernel caps this to 32k right now if I read the code correctly. But
> fair point.
The kernel caps this for dumps only, no? We can allocate here ourselves
for multicasting a message as large as we like I think.
> > > + if (WARN_ON(nl80211_send_wiphy(rdev, cmd, msg, 0, 0, 0, &state) < 0)) {
> > > + nlmsg_free(msg);
> > > + goto legacy;
> > > + }
> > > +
> > > + genlmsg_multicast_netns(&nl80211_fam, wiphy_net(&rdev->wiphy), msg, 0,
> > > + NL80211_MCGRP_CONFIG2, GFP_KERNEL);
> > > +
> > > +legacy:
> >
> > nit: just use "else" instead of the goto?
>
> I'm not sure I understand? We want to send both messages here...
It's equivalent to:
-
if (WARN_ON(nl80211_send_wiphy(...) < 0)
nlmsg_free(msg);
else
genlmsg_multicast_netns(...);
... code for legacy ...
-
no?
johannes
pull-request: mac80211-next 2019-09-11
Hi Dave, As detailed below, here are some more changes for -next, almost certainly the final round since the merge window is around the corner now. Please pull and let me know if there's any problem. Thanks, johannes The following changes since commit c76c992525245ec1c7b6738bf887c42099abab02: nexthops: remove redundant assignment to variable err (2019-08-22 12:14:05 -0700) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211-next.git tags/mac80211-next-for-davem-2019-09-11 for you to fetch changes up to c1d3ad84eae35414b6b334790048406bd6301b12: cfg80211: Purge frame registrations on iftype change (2019-09-11 10:45:10 +0200) We have a number of changes, but things are settling down: * a fix in the new 6 GHz channel support * a fix for recent minstrel (rate control) updates for an infinite loop * handle interface type changes better wrt. management frame registrations (for management frames sent to userspace) * add in-BSS RX time to survey information * handle HW rfkill properly if !CONFIG_RFKILL * send deauth on IBSS station expiry, to avoid state mismatches * handle deferred crypto tailroom updates in mac80211 better when device restart happens * fix a spectre-v1 - really a continuation of a previous patch * advertise NL80211_CMD_UPDATE_FT_IES as supported if so * add some missing parsing in VHT extended NSS support * support HE in mac80211_hwsim * let mac80211 drivers determine the max MTU themselves along with the usual cleanups etc. Arend van Spriel (1): cfg80211: fix boundary value in ieee80211_frequency_to_channel() Colin Ian King (1): mac80211: minstrel_ht: fix infinite loop because supported is not being shifted Denis Kenzior (1): cfg80211: Purge frame registrations on iftype change Felix Fietkau (1): cfg80211: add local BSS receive time to survey information Johannes Berg (4): cfg80211: always shut down on HW rfkill mac80211: list features in WEP/TKIP disable in better order mac80211: remove unnecessary key condition mac80211: IBSS: send deauth when expiring inactive STAs Lior Cohen (1): mac80211: clear crypto tx tailroom counter upon keys enable Luca Coelho (1): mac80211: don't check if key is NULL in ieee80211_key_link() Masashi Honma (1): nl80211: Fix possible Spectre-v1 for CQM RSSI thresholds Matthew Wang (1): nl80211: add NL80211_CMD_UPDATE_FT_IES to supported commands Mordechay Goodstein (1): mac80211: vht: add support VHT EXT NSS BW in parsing VHT Sven Eckelmann (1): mac80211_hwsim: Register support for HE meshpoint Wen Gong (1): mac80211: allow drivers to set max MTU zhong jiang (1): cfg80211: Do not compare with boolean in nl80211_common_reg_change_event drivers/net/wireless/mac80211_hwsim.c | 283 +++--- include/net/cfg80211.h| 4 + include/net/mac80211.h| 3 + include/uapi/linux/nl80211.h | 3 + net/mac80211/ibss.c | 8 + net/mac80211/ieee80211_i.h| 3 +- net/mac80211/iface.c | 2 +- net/mac80211/key.c| 48 ++ net/mac80211/key.h| 4 +- net/mac80211/main.c | 1 + net/mac80211/mlme.c | 13 +- net/mac80211/rc80211_minstrel_ht.c| 2 +- net/mac80211/util.c | 11 +- net/mac80211/vht.c| 10 +- net/wireless/core.c | 13 +- net/wireless/core.h | 2 +- net/wireless/nl80211.c| 17 +- net/wireless/util.c | 3 +- net/wireless/wext-compat.c| 5 +- 19 files changed, 274 insertions(+), 161 deletions(-)
Re: [PATCH] mac80211: Do not send Layer 2 Update frame before authorization
On Wed, 2019-09-11 at 16:03 +0300, Jouni Malinen wrote: > The Layer 2 Update frame is used to update bridges when a station roams > to another AP even if that STA does not transmit any frames after the > reassociation. This behavior was described in IEEE Std 802.11F-2003 as > something that would happen based on MLME-ASSOCIATE.indication, i.e., > before completing 4-way handshake. However, this IEEE trial-use > recommended practice document was published before RSN (IEEE Std > 802.11i-2004) and as such, did not consider RSN use cases. Furthermore, > IEEE Std 802.11F-2003 was withdrawn in 2006 and as such, has not been > maintained amd should not be used anymore. > > Sending out the Layer 2 Update frame immediately after association is > fine for open networks (and also when using SAE, FT protocol, or FILS > authentication when the station is actually authenticated by the time > association completes). However, it is not appropriate for cases where > RSN is used with PSK or EAP authentication since the station is actually > fully authenticated only once the 4-way handshake completes after > authentication and attackers might be able to use the unauthenticated > triggering of Layer 2 Update frame transmission to disrupt bridge > behavior. > > Fix this by postponing transmission of the Layer 2 Update frame from > station entry addition to the point when the station entry is marked > authorized. Similarly, send out the VLAN binding update only if the STA > entry has already been authorized. Reviewed-by: Johannes Berg Dave, if you were still planning to send a pull request to Linus before he closes the tree on Sunday this would be good to include (and we should also backport it to stable later). If not, I can pick it up afterwards, let me know. Thanks, johannes
Re: [PATCH] cfg80211: Purge frame registrations on iftype change
Hi, On Fri, 2019-08-30 at 01:32 -0500, Denis Kenzior wrote: > Hi Johannes, > > On 8/30/19 3:53 AM, Johannes Berg wrote: > > On Wed, 2019-08-28 at 16:11 -0500, Denis Kenzior wrote: > > > Currently frame registrations are not purged, even when changing the > > > interface type. This can lead to potentially weird / dangerous > > > situations where frames possibly not relevant to a given interface > > > type remain registered and mgmt_frame_register is not called for the > > > no-longer-relevant frame types. > > > > I'd argue really just "weird and non-working", hardly dangerous. I think I may just have found a way that's sort of "dangerous" in the sense of breaking all of our tests, but hey. > > Even in > > the mac80211 design where we want to not let you intercept e.g. AUTH > > frames in client mode - if you did, then you'd just end up with a non- > > working interface. Not sure I see any "dangerous situation". Not really > > an all that important distinction though. > > Fair enough, I'm happy to drop / reword this language. It seemed fishy > to me since the unregistration operation was not called at all, and the > driver does go to some lengths to set up the valid frame registration > types. Sure. > > However, I do wonder if we should make this more transactional, and hang > > on to them if the type switching fails. We're not notifying userspace > > that the registrations have disappeared, so if type switching fails and > > it continues to work with the old type rather than throwing its hands up > > and quitting or something, it'd make a possibly bigger mess to just > > silently have removed them already. > > I do like that idea, not sure how to go about implementing it though? > The failure case is a bit hard to deal with. Something like > NL80211_EXT_FEATURE_LIVE_IFTYPE_CHANGE would help, particularly if > nl80211/cfg80211 actually checked it prior to doing anything (e.g. > disconnecting, etc). That would then take care of the majority of the > 'typical' failure paths. I didn't add such checking in the other patch > set since I felt you might find it overly intrusive on userspace. But > maybe we really should do this? As I just said on the other patch, I think we probably should do that there, if just to be able to advertise a correct set of interface types that you can switch between there. I don't see how it'd be more intrusive to userspace than failing later? :-) > So playing devil's advocate, another argument might be that by the time > we got here, we've already tore down a bunch of state. E.g. > disconnected the station, stopped AP, etc. So we've already > side-effected state in a bunch of ways, what's one more? True, fair point. > > I *think* it should be safe to just move this after the switching > > succeeds, since the switching can pretty much only be done at a point > > where nothing is happening on the interface anyway, though that might > > confuse the driver when the remove happens. > > > > I would concur as that is what happens today. But should it? Well, dunno, what should happen? If you ask drivers they might want to remove & re-register after, for those registrations that are still possible. > It isn't currently clear to me if there are any guarantees on the driver > operation call sequence that cfg80211 provides. E.g. can the driver > expect rdev_change_virtual_intf to be called only once all the old > registrations are purged and the new registrations are performed after > the fact? Or should it expect things to just happen in any order? Well, evidently it cannot rely on anything today, and for the most part I guess this is implemented in the software paths where it doesn't really matter (the same way that mac80211 implements it). But it probably should be defined better. > > What do you think? > > > > A big part of me thinks that just wiping the slate clean and having > userspace set it up from scratch isn't that much to ask and it might > want to do that anyway. It might (a big maybe?) also make the driver's > life easier if it can rely on certain guarantees from cfg80211. E.g. > that all invalid registrations are purged. Yeah, fair point. > I have seen wpa_s perform a bunch of register commands which bounce off > with an -EALREADY. So it may already be erring on the side of caution > and assuming that it needs to reset the state fully? Not sure. I'm pretty sure that it does in fact go through a full reset (re-setup) after switching things around. > But if the kernel wants to be nice and spends some cycles figuring out > which frame registrations to keep and re-register them, that is also > fine with me. Let's not then. I've applied this patch now. johannes
Re: [RFCv3 3/3] nl80211: Send large new_wiphy events
On Fri, 2019-09-06 at 10:43 -0500, Denis Kenzior wrote:
>
> + * There are no limits (outside of netlink protocol limits) on
> + * message sizes that can be sent over the "config2" multicast group. It
> + * is assumed that applications utilizing "config2" multicast group
> + * utilize buffers that are inherently large enough or can utilize
> + * MSG_PEEK/MSG_TRUNC in the netlink transport in order to allocate big
> + * enough buffers.
I'm not sure I see how the applications could do buffers that are
"inherently" large enough, there's no practical message size limit, is
there (32-bits for the size).
I'd argue this should just say that applications should use large
buffers and still use MSG_PEEK/handle MSG_TRUNC, but I can also edit it
later.
> + msg = nlmsg_new(alloc_size, GFP_KERNEL);
> + if (!msg)
> + goto legacy;
> +
> + if (WARN_ON(nl80211_send_wiphy(rdev, cmd, msg, 0, 0, 0, &state) < 0)) {
> + nlmsg_free(msg);
> + goto legacy;
> + }
> +
> + genlmsg_multicast_netns(&nl80211_fam, wiphy_net(&rdev->wiphy), msg, 0,
> + NL80211_MCGRP_CONFIG2, GFP_KERNEL);
> +
> +legacy:
nit: just use "else" instead of the goto?
johannes
Re: [RFCv3 2/3] nl80211: Support >4096 byte NEW_WIPHY event nlmsg
Hi,
The first patch looks good, couple of nits/comments on this one below.
On Fri, 2019-09-06 at 10:43 -0500, Denis Kenzior wrote:
> For historical reasons, NEW_WIPHY messages generated by dumps or
> GET_WIPHY commands were limited to 4096 bytes. This was due to the
> fact that the kernel only allocated 4k buffers prior to commit
> 9063e21fb026 ("netlink: autosize skb lengthes"). Once the sizes of
> NEW_WIPHY messages exceeded these sizes, split dumps were introduced.
Actually, userspace prior to around the same time *also* only used 4k
buffers (old libnl), and so even with that kernel we still could
possibly have to deal with userspace that had 4k messages only ... but
we could have solved that part trivially instead of adding code to split
it, just the kernel part was still in the way then.
Anyway, I can reword this per my understanding (but will have to reread
all my messages I guess).
> findings. E.g. the kernel was at fault for the 4096 byte buffer size
> limits and not userspace.
Nit: I think most of the time when you write "e.g." ("exempli gratia",
"for example") you really mean "i.e." ("id est", "which is").
> - The code in case '3' is quite complex, but it does try to support a
> message running out of room in the middle of a channel dump and
> restarting from where it left off in the next split message. Perhaps
> this can be simplified, but it seems this capability is useful.
> Please take extra care when reviewing this.
Is it useful? You say it basically all fits today, and that means the
channels will either fit into a single message or not ... Then again, if
we add a lot of channels or a lot more data to each channel. Hmm. OK, I
guess better if we do have it.
> + void *last_good_pos = 0;
Use NULL.
> + last_good_pos = nlmsg_get_pos(msg);
> state->split_start++;
Maybe we're better off having a local macro for these two lines? That
way, we don't risk updating one without the other, which would be
confusing.
> @@ -2004,81 +2004,78 @@ static int nl80211_send_wiphy(struct
> cfg80211_registered_device *rdev,
> if (!nl_bands)
> goto nla_put_failure;
>
> - for (band = state->band_start;
> - band < NUM_NL80211_BANDS; band++) {
> + /* Position in the buffer if we added a set of channel info */
> + last_channel_pos = 0;
NULL
> [snip]
> +chan_put_failure:
> + if (!last_channel_pos)
> + goto nla_put_failure;
> +
> + nlmsg_trim(msg, last_channel_pos);
> + nla_nest_end(msg, nl_freqs);
> nla_nest_end(msg, nl_band);
>
> - if (state->split) {
> - /* start again here */
> - if (state->chan_start)
> - band--;
> + if (state->chan_start < sband->n_channels)
> break;
> - }
> +
> + state->chan_start = 0;
> + state->band_start++;
> }
> - nla_nest_end(msg, nl_bands);
>
> - if (band < NUM_NL80211_BANDS)
> - state->band_start = band + 1;
> - else
> - state->band_start = 0;
> +band_put_failure:
> + if (!last_channel_pos)
> + goto nla_put_failure;
> +
> + nla_nest_end(msg, nl_bands);
>
> - /* if bands & channels are done, continue outside */
> - if (state->band_start == 0 && state->chan_start == 0)
> - state->split_start++;
> - if (state->split)
> + if (state->band_start < NUM_NL80211_BANDS)
> break;
Thinking out loud, maybe we could simplify this by just having a small
"stack" of nested attributes to end?
I mean, essentially, you have here similar code to the nla_put_failure
label, in that it finishes and sends out the message, except here you
have to end a bunch of nested attributes.
What if we did something like
#define dump_nest_start(msg, attr) ({ \
struct nlattr r = nla_nest_start_noflag(msg, attr); \
BUG_ON(nest_stack_depth >= ARRAY_SIZE(nest_stack); \
nest_stack[nest_stack_depth++] = r; \
r; \
})
#define dump_nest_end(msg, r) do { \
BUG_ON(nest_stack_depth > 0); \
nest_stack_depth--; \
BUG_ON(nest_stack[nest_stack_depth] == r); \
nla_nest_end(msg, r); \
} while (0)
or something like that (we probably don't want to use
nla_nest_start_noflag() for future attributes, etc. but anyway).
Then we could unwin
Re: [RFC 0/4] Allow live MAC address change
Hi James, TBH, I'm still not really convinced. > I have taken some timings for all 3 ways of changing the MAC. Powered > change via RTNL, non powered via RTNL, and changing through > CMD_CONNECT. All times were taken in microseconds and tested on an > Intel 7260 PCI wireless adapter: >From where to where did you measure? I mean, clearly you cannot have counted all the way to the connection? > Powered via RTNL: > > Average: 294508.9 > Min: 284523 > Max: 300345 > > == > Non-Powered via RTNL: > > Average: 14824.7 > Min: 11037 > Max: 17812 > > Speedup from powered change: 19.87x (average) I'm assuming that this version is the IFF_LIVE_ADDR_CHANGE + setting the MAC address via RTNL? If so, yeah, obviously not powering off the firmware will be much faster than powering it off. That's an easy win really. > == > via CMD_CONNECT: > > Average: 11848.7 > Min: 9748 > Max: 17152 > > Speedup from powered change: 24.86x (average) And this really only gives you a gain of 3ms. That'd be nice, but like I said before, it's not the only thing we/you should be thinking about. One fundamental issue I have with this is that you're now combining together temporary with persistent state changes. After a disconnection (or connection failure), the interface usually goes back to its previous state. With this change, you're keeping the MAC address modified though. Sure, you don't care (because you're probably going use a new random address later anyway), but these are still things we should consider in an API. I'll happily take the subset of the patches that implements the IFF_LIVE_ADDR_CHANGE in mac80211, but I don't think the 3ms win there wins over having a well-defined API. johannes
Re: [PATCH] nl80211: Support mgmt frame unregistrations
On Wed, 2019-09-04 at 11:22 -0500, Denis Kenzior wrote: > To state another way, it is > currently not possible to write a userspace application that utilizes a > single nl80211 genl socket, instead it must open multiple genl sockets > for multiple wdevs on multiple phys. I don't see how this is too onerous for the application, every application is basically going to have an event loop anyway. Thus, I don't really see any reason for us to add a bunch of code just to make an application track fewer file descriptors - we need to have the cleanup on close already anyway, so why not actually exercise those code paths? I do note that with the "unregister on iftype change" patch you could switch to an unsupported type and reach this, but I don't think you'd want to rely on that :-) Possibly I could imagine a reason for this if you needed a single socket for functional reason, but you're not really giving any such reason. I could imagine that there might be races, but I'm having a hard time coming up with a scenario where they actually matter ... if you really really get a race between e.g. RX-AUTH and INTERFACE-DEL you'll try to do some operations that will just fail, but so what? johannes
Re: [PATCH 1/2] nl80211: Add NL80211_EXT_FEATURE_LIVE_IFTYPE_CHANGE
Hi, > Agreed. I guess I just view nl80211.h as a sort of combination between > a uapi file and an actual manpage. And manpages do mention which kernel > version a certain feature/flag/whatever was added. Such info can be > useful in many ways, e.g. figuring out which kernel version might be > required for a certain piece of hardware, etc. Yeah, fair point, I just think it's better to document that behaviour as dependent on the flag, not the kernel version; this will continue to be true for drivers that don't set the flag in future kernels after all. IOW, I don't see how it does you any good to know that you're running on kernel version 5.4, when the flag isn't set you still have to follow the 4 steps you outlined. > > > + * @NL80211_EXT_FEATURE_LIVE_IFTYPE_CHANGE: This device supports > > > switching > > > + * the IFTYPE of an interface without having to bring the device > > > DOWN > > > + * first via RTNL. Exact semantics of this feature is driver > > > + * implementation dependent. > > > > That's not really nice. > > Sorry. This came from some doc changes I have pending. I think I wrote > this after looking at some fullmac drivers and how they handle mode > changes and the wording reflects the exasperation I felt at the time. > > Do you want to suggest some alternate wording? I think it is worth it > to have some fair warning in the docs stating that prior to version so > and so the semantics are completely driver dependent. Well, but you're trying to document/advertise the restrictions now. So this feels a bit insufficient, by advertising the feature flag the device is now saying "it's possible I can switch, but don't ask me how or when". (Cont'd below) So I don't really think it's the wording that bothers me so much as the fact that you're basically going only half the way documenting this. We have nothing now, which I can agree isn't a good thing, but adding a flag that says "maybe you can do it on this device" doesn't really change the status quo *much*, since that was already true before. It seems to me you'd rather want a definitive statement. > > > For mac80211, the following restrictions > > > + * apply: > > > + * - Only devices currently in IFTYPE AP, P2P_GO, > > > P2P_CLIENT, > > > + * STATION, ADHOC and OCB can be switched. > > > + * - The target IFTYPE must be one of: AP, P2P_GO, > > > P2P_CLIENT, > > > + * STATION, ADHOC or OCB. > > > + * Other drivers are expected to follow similar restrictions. > > > > Maybe we should instead have a "bitmask of interface types that can be > > switched while live" or something like that? > > > > I'm fine with that, but this would only apply to newer kernels, no? Sure, but all that you're doing here does. > Don't we at least want to attempt to state what the rules are for older > ones? That's what you did above for the NL80211_CMD_SET_INTERFACE documentation update, I don't think it would belong into the documentation for NL80211_EXT_FEATURE_LIVE_IFTYPE_CHANGE? And you wrote that this is what happens when the flag is *set* which by definition cannot happen in older kernels. > An alternative might be to simply state what the restrictions are and > just enforce those at the cfg80211 level. Sounds good to me, we do that for a lot of things. Basically that just takes it one step further - I said above we could advertise the restrictions, but once we do that cfg80211 knows and can also enforce it. johannes
Re: [PATCH 4/8] mac80211: Allow user space to register for station Rx authentication
On Fri, 2019-08-30 at 14:24 +0300, Luca Coelho wrote: > From: Ilan Peer > > To support Pre Association Security Negotiation (PASN) while already > associated to one AP, allow user space to register to Rx authentication > frames, so that the user space logic would be able to receive/handle > authentication frames from a different AP as part of PASN. > > Note that it is expected that user space would intelligently register > for Rx authentication frames, i.e., only when PASN is used and configure > a match filter only for PASN authentication algorithm, as otherwise > the MLME functionality of mac80211 would be broken. This literally broke hundreds of wpa_s tests, so I guess it's not "intelligently register[ing]" for them... johannes
Re: intel AX200 crash on 5.2.7+
On Mon, 2019-09-09 at 10:03 -0700, Ben Greear wrote: > Hello, > > Looks like we managed to crash the AX200 firmware. This was running 5.2.7+ > kernel > in an apu2 platform. > > Is there a better place to report/discuss this? This is OK for first reports, but usually we'll ask to file a bug on bugzilla.kernel.org (and assign to linuxw...@intel.com if you can? Not sure it's possible - or add that to CC at least) > [ 6066.180908] iwlwifi :01:00.0: 0x0942 | ADVANCED_SYSASSERT Hmm, that's a calibration failure. Did you do anything special in that environment? I guess filing a bug would be best, so we have a record for it and can hand it off to the firmware team or similar. johannes
Re: [RFC] cfg80211: Allow self managed devices to update global regulatory
On Fri, 2019-09-06 at 08:45 +0530, Sriram R wrote: > Currently, self managed drivers cannot update the global regulatory > using a regulatory hint from driver if the wiphy regd is already set > from other sources. > Due to this, when a regulatory hint is provided to cfg80211 from > self managed devices, the request gets ignored and global reg is > always at default, i.e World reg, DFS-UNSET. > Hence in such systems, the result of 'iw reg get' does not indicate a > valid global regd. Yeah, but ... if you have a self-managed PHY you should anyway use iw phy0 reg get instead of plain iw reg get so I'm not sure I understand? johannes
Re: iw scan dump for /AX attributes?
On Thu, 2019-09-05 at 11:20 -0700, Ben Greear wrote: > Is anyone working on getting iw to print out /AX (HE) related > info? Good question, I wonder why we didn't do that. But no, we're not working on it as far as I know, and I haven't seen anything from anyone else either. johannes
Re: [PATCH 31/49] ath11k: add mac.c
On Thu, 2019-09-05 at 15:29 +0300, Kalle Valo wrote: > > Yeah, I was supposed to write: > > "maybe we should change mac80211 to not require this op to be present" > > But of course I could have just misunderstood, let's see what Johannes > says :) :-) Yes, that's what I meant. johannes
Re: [PATCH v6] mac80211_hwsim: Register support for HE meshpoint
On Fri, 2019-08-30 at 16:37 +0300, Jouni Malinen wrote: > On Fri, Aug 30, 2019 at 12:38:20PM +0200, Johannes Berg wrote: > > > > mesh_secure_ocv_mix_legacy > > > > wpas_mesh_open_ht40 > > > > mesh_secure_ocv_mix_ht > > > No, these also failed for me without the mac80211_hwsim patch [3] in a > > > full > > > test run. And thus not analyzed further by me. > > > > I also see these fail if (and only if) I have this patch applied. > > > > I'm going to drop this patch (again) for now, even if the situation is > > now better I still don't want to knowingly break things there. > > > > Please resend once that's all sorted out. > > I pushed the relevant wpa_supplicant changes into hostap.git and all the > hwsim test cases pass now with the current snapshot when this > mac80211_hwsim patch is included. Great, thanks. I've resurrected the patch in patchwork, so no need to resend. johannes
Re: [PATCH] cfg80211: Convert 6 GHz channel frequency to channel number
On Fri, 2019-08-30 at 12:47 +0200, Arend Van Spriel wrote: > On 8/30/2019 12:32 PM, Johannes Berg wrote: > > On Thu, 2019-08-29 at 15:21 -0700, Amar Singhal wrote: > > > Enhance function ieee80211_frequency_to_channel by adding 6 GHz > > > channels. > > > > Wait, this is already supported, no? Just implemented slightly > > differently? > > It is Johannes, but I was unaware as well. Did you forget to email that > it was applied or is there some automated stuff that failed on you? ;-) Truth be told, I've been very lazy (mostly due to being busy) and haven't responded manually - and also haven't managed to set up anything that would automate a response, I tried Kalle's tool at one point but it didn't work for me yet. Right now, your best bet is probably to poll patchwork, sorry about that. johannes
Re: [PATCH] cfg80211: Add new helper function for channels
On Fri, 2019-08-30 at 12:40 +0200, Arend Van Spriel wrote: > > +EXPORT_SYMBOL(ieee80211_channel_op_class_to_frequency); > > The function ieee80211_operating_class_to_band() uses ranges within > switch statement, eg.: > > case 128 ... 130: > *band = NL80211_BAND_5GHZ; > return true; No that you remind me - how is this new function not just a composition of the existing ones? i.e. just convert the op_class to band first, and then (band, channel) to freq? johannes
Re: [PATCH] cfg80211: inspect off channel operation only when off channel given
On Fri, 2018-07-06 at 14:31 +0200, Johannes Berg wrote:
> On Tue, 2018-07-03 at 16:04 -0700, peter...@bowerswilkins.com wrote:
> > From: Peter Oh
> >
> > NL80211_ATTR_OFFCHANNEL_TX_OK does not mean given channel is always
> > off channel, but it means the channel given could be off channel.
> > Hence it should not block the given channel to be used if given
> > channel does not require off channel mgmt tx although regulatory
> > domain is non-ETSI.
> >
> > Signed-off-by: Peter Oh
> > ---
> > net/wireless/nl80211.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
> > index 4eece06..991042b 100644
> > --- a/net/wireless/nl80211.c
> > +++ b/net/wireless/nl80211.c
> > @@ -9915,7 +9915,9 @@ static int nl80211_tx_mgmt(struct sk_buff *skb,
> > struct genl_info *info)
> > return -EINVAL;
> >
> > wdev_lock(wdev);
> > - if (params.offchan && !cfg80211_off_channel_oper_allowed(wdev)) {
> > + if (params.offchan &&
> > + !cfg80211_chandef_identical(&chandef, &wdev->chandef) &&
> > + !cfg80211_off_channel_oper_allowed(wdev)) {
> > wdev_unlock(wdev);
>
> Hmm. That seems fine, but can we be sure that wdev->chandef is always
> valid? ISTR that it isn't necessarily updated all the time, but I can't
> really say right now.
For the record, in addition to this question, the commit log might need
some rewording since the whole regulatory/non-ETSI part isn't really
obvious (and not clear to me right now).
I've had this patch waiting for about a year now, I'll drop it. Please
resend if it's still relevant.
johannes
Re: [PATCH v6] mac80211_hwsim: Register support for HE meshpoint
Hi, > > mesh_secure_ocv_mix_legacy > > wpas_mesh_open_ht40 > > mesh_secure_ocv_mix_ht > No, these also failed for me without the mac80211_hwsim patch [3] in a full > test run. And thus not analyzed further by me. I also see these fail if (and only if) I have this patch applied. I'm going to drop this patch (again) for now, even if the situation is now better I still don't want to knowingly break things there. Please resend once that's all sorted out. johannes
Re: [PATCH] cfg80211: Convert 6 GHz channel frequency to channel number
On Thu, 2019-08-29 at 15:21 -0700, Amar Singhal wrote: > Enhance function ieee80211_frequency_to_channel by adding 6 GHz > channels. Wait, this is already supported, no? Just implemented slightly differently? johannes
Re: [PATCH 1/2] nl80211: Add NL80211_EXT_FEATURE_LIVE_IFTYPE_CHANGE
On Mon, 2019-08-26 at 11:26 -0500, Denis Kenzior wrote: > > + * Prior to Kernel 5.4, userspace applications should implement the > + * following behavior: I'm not sure mentioning the kernel version here does us any good? I mean, you really need to implement that behaviour regardless of kernel version, if NL80211_EXT_FEATURE_LIVE_IFTYPE_CHANGE isn't set. > + * @NL80211_EXT_FEATURE_LIVE_IFTYPE_CHANGE: This device supports switching > + * the IFTYPE of an interface without having to bring the device DOWN > + * first via RTNL. Exact semantics of this feature is driver > + * implementation dependent. That's not really nice. > For mac80211, the following restrictions > + * apply: > + * - Only devices currently in IFTYPE AP, P2P_GO, P2P_CLIENT, > + * STATION, ADHOC and OCB can be switched. > + * - The target IFTYPE must be one of: AP, P2P_GO, P2P_CLIENT, > + * STATION, ADHOC or OCB. > + * Other drivers are expected to follow similar restrictions. Maybe we should instead have a "bitmask of interface types that can be switched while live" or something like that? johannes
Re: [RFCv2 4/4] nl80211: Send large new_wiphy events
On Fri, 2019-08-16 at 14:27 -0500, Denis Kenzior wrote:
> Send large NEW_WIPHY events on a new multicast group so that clients
> that can accept larger messages do not need to round-trip to the kernel
> and perform extra filtered wiphy dumps.
>
> A new multicast group is introduced and the large message is sent before
> the legacy message. This way clients that listen on both multicast
> groups can ignore duplicate legacy messages if needed.
Since I just did the digging, it seems that this would affect (old)
applications with libnl up to 3.2.22, unless they changed the default
recvmsg() buffer size.
I think this is a pretty decent approach, but I'm slightly worried about
hitting the new limits (16k) eventually. It seems far off now, but who
knows what kind of data we'll add. HE is (and likely will be) adding
quite a bit since it has everything for each interface type - something
drivers have for the most part not implemented yet. That trend will only
continue, as complexity in the spec doesn't seem to be going down.
And I don't really want to see "config3" a couple of years down the
road...
So can we at least mandate (document) that "config2" basically has no
message limit, and you will use MSG_PEEK/handle MSG_TRUNC with it?
That way, we can later bump the 8192 even beyond 16k if needed, and not
run into problems.
> + if (cmd == NL80211_CMD_NEW_WIPHY) {
> + state.large_message = true;
> + alloc_size = 8192UL;
> + } else
> + alloc_size = NLMSG_DEFAULT_SIZE;
> +
nit: there should be braces on both branches
> + if (nl80211_send_wiphy(rdev, cmd, msg, 0, 0, 0, &state) < 0) {
> + nlmsg_free(msg);
> + goto legacy;
> + }
I think that'd be worth a WARN_ON(), it should never happen that you
actually run out of space, it means that the above wasn't big enough.
Now, on the previous patches I actually thought that you could set
"state->split" (and you should) and not need "state->large_message" in
order to indicate that the sub-functions are allowed to create larger
data - just keep filling the SKBs as much as possible for the dump.
Here, it seems like we do need it. It might be possible to get away
without it (by setting split here, and then having some special code to
handle the case of it not getting to the end), but that doesn't seem
worth it.
> @@ -14763,6 +14787,8 @@ void nl80211_notify_iface(struct
> cfg80211_registered_device *rdev,
> return;
> }
>
> + genlmsg_multicast_netns(&nl80211_fam, wiphy_net(&rdev->wiphy), msg, 0,
> + NL80211_MCGRP_CONFIG2, GFP_KERNEL);
Hmm. That seems only needed if you don't want to listen on "config" at
all, but in the patch description you explicitly said that you send it
on "config2" *before* "config" for compatibility reasons (which makes
sense) - so what is it?
I'm having a hard time seeing anyone get away with only listening on
config2 since that'd basically require very recent (as of now future)
kernel. Are you planning this for a world where you can ditch support
for kernel<5.4 (or so)?
johannes
Re: [RFCv2 1/4] nl80211: Fix broken non-split wiphy dumps
On Fri, 2019-08-30 at 11:40 +0200, Johannes Berg wrote:
> On Fri, 2019-08-30 at 11:10 +0200, Johannes Berg wrote:
> > On Fri, 2019-08-30 at 11:03 +0200, Johannes Berg wrote:
> > > On Fri, 2019-08-16 at 14:27 -0500, Denis Kenzior wrote:
> > > > If a (legacy) client requested a wiphy dump but did not provide the
> > > > NL80211_ATTR_SPLIT_WIPHY_DUMP attribute, the dump was supposed to be
> > > > composed of purely non-split NEW_WIPHY messages, with 1 wiphy per
> > > > message. At least this was the intent after commit:
> > > > 3713b4e364ef ("nl80211: allow splitting wiphy information in dumps")
> > > >
> > > > However, in reality the non-split dumps were broken very shortly after.
> > > > Perhaps around commit:
> > > > fe1abafd942f ("nl80211: re-add channel width and extended capa
> > > > advertising")
> > >
> > > Fun. I guess we updated all userspace quickly enough to not actually
> > > have any issues there. As far as I remember, nobody ever complained, so
> > > I guess people just updated their userspace.
> >
> > Actually, going back in time to the code there (e.g. iw and hostap), it
> > seems that it quite possibly never was a userspace issue, just an issue
> > with netlink allocating a 4k SKB by default for dumps.
> >
> > Even then, libnl would've defaulted to a 16k recvmsg() buffer size, and
> > we didn't override that anywhere.
>
> Ah, also not quite true, at the time it still had a 4k default, until
> commit 807fddc4cd9e ("nl: Increase receive buffer size to 4 pages")
> dated May 8, 2013.
However, even before that, it would have supported responding to
MSG_TRUNC by retrying the recvmsg(), but hostap/iw wouldn't have set
nl_socket_enable_msg_peek()... oh well.
johannes
Re: [RFCv2 1/4] nl80211: Fix broken non-split wiphy dumps
On Fri, 2019-08-30 at 11:10 +0200, Johannes Berg wrote:
> On Fri, 2019-08-30 at 11:03 +0200, Johannes Berg wrote:
> > On Fri, 2019-08-16 at 14:27 -0500, Denis Kenzior wrote:
> > > If a (legacy) client requested a wiphy dump but did not provide the
> > > NL80211_ATTR_SPLIT_WIPHY_DUMP attribute, the dump was supposed to be
> > > composed of purely non-split NEW_WIPHY messages, with 1 wiphy per
> > > message. At least this was the intent after commit:
> > > 3713b4e364ef ("nl80211: allow splitting wiphy information in dumps")
> > >
> > > However, in reality the non-split dumps were broken very shortly after.
> > > Perhaps around commit:
> > > fe1abafd942f ("nl80211: re-add channel width and extended capa
> > > advertising")
> >
> > Fun. I guess we updated all userspace quickly enough to not actually
> > have any issues there. As far as I remember, nobody ever complained, so
> > I guess people just updated their userspace.
>
> Actually, going back in time to the code there (e.g. iw and hostap), it
> seems that it quite possibly never was a userspace issue, just an issue
> with netlink allocating a 4k SKB by default for dumps.
>
> Even then, libnl would've defaulted to a 16k recvmsg() buffer size, and
> we didn't override that anywhere.
Ah, also not quite true, at the time it still had a 4k default, until
commit 807fddc4cd9e ("nl: Increase receive buffer size to 4 pages")
dated May 8, 2013.
johannes
Re: [RFCv2 2/4] nl80211: Support >4096 byte NEW_WIPHY event nlmsg
On Fri, 2019-08-16 at 14:27 -0500, Denis Kenzior wrote:
> For historical reasons, NEW_WIPHY messages generated by dumps or
> GET_WIPHY commands were limited to 4096 bytes due to userspace tools
> using limited buffers.
I think now that I've figured out why, it'd be good to note that it
wasn't due to userspace tools, but rather due to the default netlink
dump skb allocation at the time, prior to commit 9063e21fb026
("netlink: autosize skb lengthes").
> Once the sizes NEW_WIPHY messages exceeded these
> sizes, split dumps were introduced. All any non-legacy data was added
> only to messages using split-dumps (including filtered dumps).
>
> However, split-dumping has quite a significant overhead. On cards
> tested, split dumps generated message sizes 1.7-1.8x compared to
> non-split dumps, while still comfortably fitting into an 8k buffer. The
> kernel now expects userspace to provide 16k buffers by default, and 32k
> buffers are possible.
>
> Introduce a concept of a large message, so that if the kernel detects
> that userspace has provided a buffer of sufficient size, a non-split
> message could be generated.
So, there's still a wrinkle with this. Larger SKB allocation can fail,
and instead of returning an error to userspace, the kernel will allocate
a smaller SKB instead.
With genetlink, we currently don't even have a way of controlling the
minimum allocation that's always required.
Since we already have basically all of the mechanics, I'd say perhaps a
better concept would be to "split when necessary", aborting if split
isn't supported.
IOW, do something like
... nl80211_send_wiphy(...)
{
[...]
switch (state->split_start) {
[...]
case :
[...] // put stuff
state->split_start++;
state->skb_end = nlmsg_get_pos(skb);
/* fall through */
case :
[...]
}
finish:
genlmsg_end(msg, hdr);
return 0;
nla_put_failure:
if (state->split_start < 9) {
genlmsg_cancel(msg, hdr);
return -EMSGSIZE;
}
nlmsg_trim(msg, state->skb_end);
goto finish;
}
That way, we fill each SKB as much as possible, up to 32k if userspace
provided big enough buffers *and* we could allocate the SKB.
Your userspace would still set the split flag, and thus be compatible
with all kinds of options:
* really old kernel not supporting split
* older kernel sending many messages
* kernel after this change packing more into one message
* even if allocating big SKBs failed
johannes
Re: [RFCv2 1/4] nl80211: Fix broken non-split wiphy dumps
On Fri, 2019-08-30 at 11:03 +0200, Johannes Berg wrote:
> On Fri, 2019-08-16 at 14:27 -0500, Denis Kenzior wrote:
> > If a (legacy) client requested a wiphy dump but did not provide the
> > NL80211_ATTR_SPLIT_WIPHY_DUMP attribute, the dump was supposed to be
> > composed of purely non-split NEW_WIPHY messages, with 1 wiphy per
> > message. At least this was the intent after commit:
> > 3713b4e364ef ("nl80211: allow splitting wiphy information in dumps")
> >
> > However, in reality the non-split dumps were broken very shortly after.
> > Perhaps around commit:
> > fe1abafd942f ("nl80211: re-add channel width and extended capa advertising")
>
> Fun. I guess we updated all userspace quickly enough to not actually
> have any issues there. As far as I remember, nobody ever complained, so
> I guess people just updated their userspace.
Actually, going back in time to the code there (e.g. iw and hostap), it
seems that it quite possibly never was a userspace issue, just an issue
with netlink allocating a 4k SKB by default for dumps.
Even then, libnl would've defaulted to a 16k recvmsg() buffer size, and
we didn't override that anywhere.
So more likely, this was all fixed by kernel 9063e21fb026 ("netlink:
autosize skb lengthes") about a year after we ran into the problem.
johannes
Re: [RFCv2 1/4] nl80211: Fix broken non-split wiphy dumps
On Fri, 2019-08-16 at 14:27 -0500, Denis Kenzior wrote:
> If a (legacy) client requested a wiphy dump but did not provide the
> NL80211_ATTR_SPLIT_WIPHY_DUMP attribute, the dump was supposed to be
> composed of purely non-split NEW_WIPHY messages, with 1 wiphy per
> message. At least this was the intent after commit:
> 3713b4e364ef ("nl80211: allow splitting wiphy information in dumps")
>
> However, in reality the non-split dumps were broken very shortly after.
> Perhaps around commit:
> fe1abafd942f ("nl80211: re-add channel width and extended capa advertising")
Fun. I guess we updated all userspace quickly enough to not actually
have any issues there. As far as I remember, nobody ever complained, so
I guess people just updated their userspace.
Given that it's been 6+ years, maybe we're better off just removing the
whole non-split thing then, instead of fixing it. Seems even less likely
now that somebody would run a 6+yo supplicant (from before its commit
c30a4ab045ce ("nl80211: Fix mode settings with split wiphy dump")).
OTOH, this is a simple fix, would removing the non-split mode result in
any appreciable cleanups? Perhaps not, and we'd have to insert something
instead to reject non-split and log a warning, or whatnot.
johannes
Re: [PATCH] cfg80211: Purge frame registrations on iftype change
On Wed, 2019-08-28 at 16:11 -0500, Denis Kenzior wrote: > Currently frame registrations are not purged, even when changing the > interface type. This can lead to potentially weird / dangerous > situations where frames possibly not relevant to a given interface > type remain registered and mgmt_frame_register is not called for the > no-longer-relevant frame types. I'd argue really just "weird and non-working", hardly dangerous. Even in the mac80211 design where we want to not let you intercept e.g. AUTH frames in client mode - if you did, then you'd just end up with a non- working interface. Not sure I see any "dangerous situation". Not really an all that important distinction though. Depending on the design, it may also just be that those registrations are *ignored*, because e.g. firmware intercepts the AUTH frame already, which would just (maybe) confuse userspace - but that seems unlikely since it switched interface type and has no real need for those frames then. > The kernel currently relies on userspace apps to actually purge the > registrations themselves, e.g. by closing the nl80211 socket associated > with those frames. However, this requires multiple nl80211 sockets to > be open by the userspace app, and for userspace to be aware of all state > changes. This is not something that the kernel should rely on. I tend to agree with that the kernel shouldn't rely on it. > This commit adds a call to cfg80211_mlme_purge_registrations() to > forcefully remove any registrations left over prior to switching the > iftype. However, I do wonder if we should make this more transactional, and hang on to them if the type switching fails. We're not notifying userspace that the registrations have disappeared, so if type switching fails and it continues to work with the old type rather than throwing its hands up and quitting or something, it'd make a possibly bigger mess to just silently have removed them already. I *think* it should be safe to just move this after the switching succeeds, since the switching can pretty much only be done at a point where nothing is happening on the interface anyway, though that might confuse the driver when the remove happens. Also, perhaps it'd be better to actually hang on to those registrations that *are* still possible afterwards? But to not confuse the driver I guess that might require unregister/re-register to happen, all of which requires hanging on to the list and going through it after the type switch completed? What do you think? johannes
Re: mac80211_hwsim (kernel 4.18+): wmediumd + 2.4Ghz
On Fri, 2019-08-30 at 00:35 +0530, Krishna Chaitanya wrote: > > Is this supposed to work at all? AFAICS, in hwsim channel matching > checks are only done in non-mediumd path (no_nl), and wmediumd also > doesn't have any checks? So, hostapd responds to all probe requests in all > channels. Am I missing something? Hmm. Interesting observation, I wasn't aware of that. That certainly explains the situation though - on 2.4 GHz we'd prefer using the DS Element, and thus not use the scan result, while on 5 GHz we assume that the reported RX frequency is correct (there's no channel overlap). Still doesn't explain why it should work in 4.17 and not in 4.18, there aren't a lot of wifi changes there at all. I guess we should fix that in hwsim, anyone esle want to? :-) johannes
Re: [PATCH] cfg80211: Add new fields to wiphy structure
On Thu, 2019-08-29 at 15:09 -0700, Amar Singhal wrote: > A channel is completely defined by (oper_class, channel number) tuple, > and not just by center frequency. Operating class also tells about the > bandwidth supported by the channel. Therefore add the operating class and > channel number to the wiphy structure. We don't split out the channels that way, so this doesn't seem like the right approach. Instead, we list the *frequencies*, and then have flags for the permitted bandwidths. We already support things like "no-HT40+" and could possibly extend that to others, if it were _really_ possible, though in practice those limitations are usually not present in devices, just in the spec, and we can rely on hostapd/wpa_s to take care of them. Even if you do have those limitations, this isn't the right way to go about it, because it'll be very confusing to userspace to see the same frequency multiple times. It'd also cause a bunch of problems with scanning (listing the same channel twice) etc. Since you haven't explained why you want to do this I cannot offer any further guidance, but this cannot be the right approach. johannes
