Re: [PATCH] wil6210: disallow changing RSN in beacon change
On 10/18/2017 1:13 PM, Johannes Berg wrote: >> hostapd uses change_beacon to change the security of the AP so this >> needs to be supported. > > I didn't think this made sense - Jouni? Does hostapd kick off all > stations in this case? > >> We do need to restart the AP in this case which will >> disconnect existing clients, but this cannot be helped... > > Why not restart the AP entirely then from userspace? Hmm. I wonder what > would happen with mac80211 - I guess keys would have to removed etc? > Does this just work by accident because mac80211 removes the keys with > stations? What about GTK(s) though? > Not sure what happens when the privacy stays the same (secure) but keys change, maybe Jouni can comment. >> As a side note, hostapd can also use change_beacon to change the >> SSID. > > When does that happen? By chance I worked on a WPS certification test last week which used a shell script to perform various operations. The AP started secure but the script could change its configuration to unsecure. It used the wps_config CLI command to change both the security and SSID and hostapd used change_beacon to perform this operation. We got this script from WIFI team so there is good chance it is in use by existing certification test beds.
[PATCH V2] bcma: Use bcma_debug and not pr_cont in MIPS driver
Commit 66cc04424960 ("bcma: use bcma_debug and pr_cont in MIPS driver")
converted a printk(KERN_DEBUG to bcma_debug.
bcma_debug is guarded by a #define DEBUG via pr_debug.
This means that the bcma_debug will generally not be emitted
but any pr_cont following the bcma_debug will be emitted.
Correct this by removing the uses of pr_cont by using a temporary.
Signed-off-by: Joe Perches
---
v2: Fix whitespace damage
I hear there's this checkpatch tool...
drivers/bcma/driver_mips.c | 11 +++
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/bcma/driver_mips.c b/drivers/bcma/driver_mips.c
index 5904ef1aa624..f040aba48d50 100644
--- a/drivers/bcma/driver_mips.c
+++ b/drivers/bcma/driver_mips.c
@@ -184,11 +184,14 @@ static void bcma_core_mips_print_irq(struct bcma_device
*dev, unsigned int irq)
{
int i;
static const char *irq_name[] = {"2(S)", "3", "4", "5", "6", "D", "I"};
+ char interrupts[20];
+ char *ints = interrupts;
- bcma_debug(dev->bus, "core 0x%04x, irq :", dev->id.id);
- for (i = 0; i <= 6; i++)
- pr_cont(" %s%s", irq_name[i], i == irq ? "*" : " ");
- pr_cont("\n");
+ for (i = 0; i < ARRAY_SIZE(irq_name); i++)
+ ints += sprintf(ints, " %s%c",
+ irq_name[i], i == irq ? '*' : ' ');
+
+ bcma_debug(dev->bus, "core 0x%04x, irq:%s\n", dev->id.id, interrupts);
}
static void bcma_core_mips_dump_irq(struct bcma_bus *bus)
--
2.10.0.rc2.1.g053435c
Re: [PATCH] bcma: Use bcma_debug and not pr_cont in MIPS driver
On Wed, Oct 18, 2017 at 10:12:18PM -0700, Joe Perches wrote:
> Commit 66cc04424960 ("bcma: use bcma_debug and pr_cont in MIPS driver")
> converted a printk(KERN_DEBUG to bcma_debug.
>
> bcma_debug is guarded by a #define DEBUG via pr_debug.
>
> This means that the bcma_debug will generally not be emitted
> but any pr_cont following the bcma_debug will be emitted.
>
> Correct this by removing the uses of pr_cont by using a temporary.
>
> Signed-off-by: Joe Perches
> ---
> drivers/bcma/driver_mips.c | 11 +++
> 1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/bcma/driver_mips.c b/drivers/bcma/driver_mips.c
> index 5904ef1aa624..a929956150eb 100644
> --- a/drivers/bcma/driver_mips.c
> +++ b/drivers/bcma/driver_mips.c
> @@ -184,11 +184,14 @@ static void bcma_core_mips_print_irq(struct bcma_device
> *dev, unsigned int irq)
> {
> int i;
> static const char *irq_name[] = {"2(S)", "3", "4", "5", "6", "D", "I"};
> +char interrupts[20];
> +char *ints = interrupts;
Tabs were changed to spaces.
>
> - bcma_debug(dev->bus, "core 0x%04x, irq :", dev->id.id);
> - for (i = 0; i <= 6; i++)
> - pr_cont(" %s%s", irq_name[i], i == irq ? "*" : " ");
> - pr_cont("\n");
> +for (i = 0; i < ARRAY_SIZE(irq_name); i++)
> +ints += sprintf(ints, " %s%c",
> + irq_name[i], i == irq ? '*' : ' ');
But not on this line.
> +
> +bcma_debug(dev->bus, "core 0x%04x, irq:%s\n", dev->id.id,
> interrupts);
> }
>
> static void bcma_core_mips_dump_irq(struct bcma_bus *bus)
> --
> 2.10.0.rc2.1.g053435c
>
--
James Cameron
http://quozl.netrek.org/
[PATCH] bcma: Use bcma_debug and not pr_cont in MIPS driver
Commit 66cc04424960 ("bcma: use bcma_debug and pr_cont in MIPS driver")
converted a printk(KERN_DEBUG to bcma_debug.
bcma_debug is guarded by a #define DEBUG via pr_debug.
This means that the bcma_debug will generally not be emitted
but any pr_cont following the bcma_debug will be emitted.
Correct this by removing the uses of pr_cont by using a temporary.
Signed-off-by: Joe Perches
---
drivers/bcma/driver_mips.c | 11 +++
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/bcma/driver_mips.c b/drivers/bcma/driver_mips.c
index 5904ef1aa624..a929956150eb 100644
--- a/drivers/bcma/driver_mips.c
+++ b/drivers/bcma/driver_mips.c
@@ -184,11 +184,14 @@ static void bcma_core_mips_print_irq(struct bcma_device
*dev, unsigned int irq)
{
int i;
static const char *irq_name[] = {"2(S)", "3", "4", "5", "6", "D", "I"};
+char interrupts[20];
+char *ints = interrupts;
- bcma_debug(dev->bus, "core 0x%04x, irq :", dev->id.id);
- for (i = 0; i <= 6; i++)
- pr_cont(" %s%s", irq_name[i], i == irq ? "*" : " ");
- pr_cont("\n");
+for (i = 0; i < ARRAY_SIZE(irq_name); i++)
+ints += sprintf(ints, " %s%c",
+ irq_name[i], i == irq ? '*' : ' ');
+
+bcma_debug(dev->bus, "core 0x%04x, irq:%s\n", dev->id.id, interrupts);
}
static void bcma_core_mips_dump_irq(struct bcma_bus *bus)
--
2.10.0.rc2.1.g053435c
Re: [PATCH] bcma: use bcma_debug and pr_cont in MIPS driver
Joe Perches writes:
> On Mon, 2017-10-16 at 23:21 +0200, Hauke Mehrtens wrote:
>> On 10/16/2017 02:54 PM, Rafał Miłecki wrote:
>> > From: Rafał Miłecki
>> >
>> > Using bcma_debug gives a device-specific prefix for messages and pr_cont
>> > is a common helper for continuing a line.
>> >
>> > Signed-off-by: Rafał Miłecki
>>
>> Acked-By: Hauke Mehrtens
>>
>> > ---
>> > drivers/bcma/driver_mips.c | 7 ---
>> > 1 file changed, 4 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/drivers/bcma/driver_mips.c b/drivers/bcma/driver_mips.c
>> > index 89af807cf29c..5904ef1aa624 100644
>> > --- a/drivers/bcma/driver_mips.c
>> > +++ b/drivers/bcma/driver_mips.c
>> > @@ -184,10 +184,11 @@ static void bcma_core_mips_print_irq(struct
>> > bcma_device *dev, unsigned int irq)
>> > {
>> >int i;
>> >static const char *irq_name[] = {"2(S)", "3", "4", "5", "6", "D", "I"};
>> > - printk(KERN_DEBUG KBUILD_MODNAME ": core 0x%04x, irq :", dev->id.id);
>> > +
>> > + bcma_debug(dev->bus, "core 0x%04x, irq :", dev->id.id);
>> >for (i = 0; i <= 6; i++)
>> > - printk(" %s%s", irq_name[i], i == irq ? "*" : " ");
>> > - printk("\n");
>> > + pr_cont(" %s%s", irq_name[i], i == irq ? "*" : " ");
>> > + pr_cont("\n");
>> > }
>> >
>> > static void bcma_core_mips_dump_irq(struct bcma_bus *bus)
>> >
>
> This isn't the same code as it depends on #define DEBUG
> and will not output the first line in most cases.
>
> I'd suggest a nack.
Too late, I already applied this. Please submit a followup patch if
something needs to be changed.
--
Kalle Valo
Re: rtlwifi oops
On 10/18/2017 08:40 PM, nirinA wrote: i checked my dmesg and have this similar log, i think when i unplugged the device. [ 5640.100541] usb 2-1.4: USB disconnect, device number 5 [ 5640.104108] rtl_usb: reg 0x102, usbctrl_vendorreq TimeOut! status:0xffed value=0x0 [ 5640.104110] rtl_usb: reg 0x422, usbctrl_vendorreq TimeOut! status:0xffed value=0x0 [ 5640.104113] rtl_usb: reg 0x542, usbctrl_vendorreq TimeOut! status:0xffed value=0x0 [ 5640.104127] rtl_usb: reg 0x102, usbctrl_vendorreq TimeOut! status:0xffed value=0xd38000 i will apply the patch and will see if i still get this. As I said in the response to your private message: No, that is a different error. You need to check for the USB disconnect when starting, but no rtl_usb errors. Larry
Re: [PATCH] bcma: use bcma_debug and pr_cont in MIPS driver
On Mon, 2017-10-16 at 23:21 +0200, Hauke Mehrtens wrote:
> On 10/16/2017 02:54 PM, Rafał Miłecki wrote:
> > From: Rafał Miłecki
> >
> > Using bcma_debug gives a device-specific prefix for messages and pr_cont
> > is a common helper for continuing a line.
> >
> > Signed-off-by: Rafał Miłecki
>
> Acked-By: Hauke Mehrtens
>
> > ---
> > drivers/bcma/driver_mips.c | 7 ---
> > 1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/bcma/driver_mips.c b/drivers/bcma/driver_mips.c
> > index 89af807cf29c..5904ef1aa624 100644
> > --- a/drivers/bcma/driver_mips.c
> > +++ b/drivers/bcma/driver_mips.c
> > @@ -184,10 +184,11 @@ static void bcma_core_mips_print_irq(struct
> > bcma_device *dev, unsigned int irq)
> > {
> > int i;
> > static const char *irq_name[] = {"2(S)", "3", "4", "5", "6", "D", "I"};
> > - printk(KERN_DEBUG KBUILD_MODNAME ": core 0x%04x, irq :", dev->id.id);
> > +
> > + bcma_debug(dev->bus, "core 0x%04x, irq :", dev->id.id);
> > for (i = 0; i <= 6; i++)
> > - printk(" %s%s", irq_name[i], i == irq ? "*" : " ");
> > - printk("\n");
> > + pr_cont(" %s%s", irq_name[i], i == irq ? "*" : " ");
> > + pr_cont("\n");
> > }
> >
> > static void bcma_core_mips_dump_irq(struct bcma_bus *bus)
> >
This isn't the same code as it depends on #define DEBUG
and will not output the first line in most cases.
I'd suggest a nack.
Perhaps it'd be better to use a temporary and avoid the
pr_cont uses like:
{
int i;
static const char *irq_name[] = {"2(S)", "3", "4", "5", "6", "D", "I"};
char interrupts[20];
char *ints = interrupts;
for (i = 0; i < ARRAY_SIZE(irq_name), i++)
ints += sprintf(ints, " %s%c", irq_name[i], i == irq ? '*' : '
');
bcma_debug(dev->bus, "core 0x04x, irq: %s\n", dev->id.id, interrupts);
}
Re: rtlwifi oops
i checked my dmesg and have this similar log, i think when i unplugged the device. [ 5640.100541] usb 2-1.4: USB disconnect, device number 5 [ 5640.104108] rtl_usb: reg 0x102, usbctrl_vendorreq TimeOut! status:0xffed value=0x0 [ 5640.104110] rtl_usb: reg 0x422, usbctrl_vendorreq TimeOut! status:0xffed value=0x0 [ 5640.104113] rtl_usb: reg 0x542, usbctrl_vendorreq TimeOut! status:0xffed value=0x0 [ 5640.104127] rtl_usb: reg 0x102, usbctrl_vendorreq TimeOut! status:0xffed value=0xd38000 i will apply the patch and will see if i still get this. thanks, -- nirinA Larry Finger wrote: On 10/18/2017 06:18 PM, nirinA wrote: not really a rtlwifi oops then. maybe issue with udev as that also disabled the usb mouse. but i have no problem since. i will report if i will catch it again. thanks, -- nirinA James Cameron wrote: On Thu, Oct 19, 2017 at 01:31:43AM +0300, nirinA wrote: hello there, i got the oops below with a rtl8192cu:0bda:8178 and kernel 4.13.6, but cannot reproduce it. i use this device since 4.3 or so without noticing any issue. nirinA [ 239.338040] usb 2-1.3: new high-speed USB device number 4 using ehci-pci [ 239.417728] usb 2-1.3: New USB device found, idVendor=0bda, idProduct=8178 [ 239.417730] usb 2-1.3: New USB device strings: Mfr=1, Product=2, SerialNumber=3 [ 239.417731] usb 2-1.3: Product: 802.11n WLAN Adapter [ 239.417732] usb 2-1.3: Manufacturer: Realtek [ 239.417733] usb 2-1.3: SerialNumber: 00e04c01 [ 239.578100] rtl8192cu: Chip version 0x11 [ 239.678225] usb 2-1-port3: disabled by hub (EMI?), re-enabling... Just prior to the oops, your USB hub disabled the port being used by the wireless device. While the response of the driver seems wrong, it is a difficult condition to reproduce; one must either force or forge the disabling by the hub. [ 239.678230] usb 2-1.3: USB disconnect, device number 4 [ 239.679128] rtl_usb: reg 0x30, usbctrl_vendorreq TimeOut! status:0xffed value=0x0 This sequence of events is unusual. A driver should be able to trust that the platform is behaving correctly and normally. In reviewing the USB probe routine, the only thing I could see is that one returned value is not being checked. If the problem happens again, please try the following patch: diff --git a/drivers/net/wireless/realtek/rtlwifi/usb.c b/drivers/net/wireless/realtek/rtlwifi/usb.c index 5590d07d0918..092cd2da15f6 100644 --- a/drivers/net/wireless/realtek/rtlwifi/usb.c +++ b/drivers/net/wireless/realtek/rtlwifi/usb.c @@ -1082,6 +1082,8 @@ int rtl_usb_probe(struct usb_interface *intf, init_completion(&rtlpriv->firmware_loading_complete); SET_IEEE80211_DEV(hw, &intf->dev); udev = interface_to_usbdev(intf); + if (!udev) + return -ENODEV; usb_get_dev(udev); usb_priv = rtl_usbpriv(hw); memset(usb_priv, 0, sizeof(*usb_priv)); I will test and push this patch because that value should be checked, but I'm not sure it would correct your problem. Larry
Re: rtlwifi oops
On 10/18/2017 06:18 PM, nirinA wrote: not really a rtlwifi oops then. maybe issue with udev as that also disabled the usb mouse. but i have no problem since. i will report if i will catch it again. thanks, -- nirinA James Cameron wrote: On Thu, Oct 19, 2017 at 01:31:43AM +0300, nirinA wrote: hello there, i got the oops below with a rtl8192cu:0bda:8178 and kernel 4.13.6, but cannot reproduce it. i use this device since 4.3 or so without noticing any issue. nirinA [ 239.338040] usb 2-1.3: new high-speed USB device number 4 using ehci-pci [ 239.417728] usb 2-1.3: New USB device found, idVendor=0bda, idProduct=8178 [ 239.417730] usb 2-1.3: New USB device strings: Mfr=1, Product=2, SerialNumber=3 [ 239.417731] usb 2-1.3: Product: 802.11n WLAN Adapter [ 239.417732] usb 2-1.3: Manufacturer: Realtek [ 239.417733] usb 2-1.3: SerialNumber: 00e04c01 [ 239.578100] rtl8192cu: Chip version 0x11 [ 239.678225] usb 2-1-port3: disabled by hub (EMI?), re-enabling... Just prior to the oops, your USB hub disabled the port being used by the wireless device. While the response of the driver seems wrong, it is a difficult condition to reproduce; one must either force or forge the disabling by the hub. [ 239.678230] usb 2-1.3: USB disconnect, device number 4 [ 239.679128] rtl_usb: reg 0x30, usbctrl_vendorreq TimeOut! status:0xffed value=0x0 This sequence of events is unusual. A driver should be able to trust that the platform is behaving correctly and normally. In reviewing the USB probe routine, the only thing I could see is that one returned value is not being checked. If the problem happens again, please try the following patch: diff --git a/drivers/net/wireless/realtek/rtlwifi/usb.c b/drivers/net/wireless/realtek/rtlwifi/usb.c index 5590d07d0918..092cd2da15f6 100644 --- a/drivers/net/wireless/realtek/rtlwifi/usb.c +++ b/drivers/net/wireless/realtek/rtlwifi/usb.c @@ -1082,6 +1082,8 @@ int rtl_usb_probe(struct usb_interface *intf, init_completion(&rtlpriv->firmware_loading_complete); SET_IEEE80211_DEV(hw, &intf->dev); udev = interface_to_usbdev(intf); + if (!udev) + return -ENODEV; usb_get_dev(udev); usb_priv = rtl_usbpriv(hw); memset(usb_priv, 0, sizeof(*usb_priv)); I will test and push this patch because that value should be checked, but I'm not sure it would correct your problem. Larry
Re: [PATCH V6 0/5] Add TDLS feature for ath10k
On 10/15/2017 11:00 PM, yint...@qti.qualcomm.com wrote: From: Yingying Tang This patchset is for Rome PCIE chip, it will not affect other hardware Please add your patchset history here until V6. That's what cover letter is supposed to have. Yingying Tang (5): mac80211: Enable TDLS peer buffer STA feature ath10k: Enable TDLS peer buffer STA feature ath10k: Enable TDLS peer inactivity detection ath10k: Avoid to set WEP key for TDLS peer ath10k: Fix TDLS peer TX data failure issue on encryped AP drivers/net/wireless/ath/ath10k/mac.c |9 - drivers/net/wireless/ath/ath10k/wmi-tlv.c | 55 + drivers/net/wireless/ath/ath10k/wmi-tlv.h |9 + drivers/net/wireless/ath/ath10k/wmi.h |1 + include/net/mac80211.h|4 +++ net/mac80211/tdls.c |5 ++- 6 files changed, 81 insertions(+), 2 deletions(-)
Re: rtlwifi oops
not really a rtlwifi oops then. maybe issue with udev as that also disabled the usb mouse. but i have no problem since. i will report if i will catch it again. thanks, -- nirinA James Cameron wrote: On Thu, Oct 19, 2017 at 01:31:43AM +0300, nirinA wrote: hello there, i got the oops below with a rtl8192cu:0bda:8178 and kernel 4.13.6, but cannot reproduce it. i use this device since 4.3 or so without noticing any issue. nirinA [ 239.338040] usb 2-1.3: new high-speed USB device number 4 using ehci-pci [ 239.417728] usb 2-1.3: New USB device found, idVendor=0bda, idProduct=8178 [ 239.417730] usb 2-1.3: New USB device strings: Mfr=1, Product=2, SerialNumber=3 [ 239.417731] usb 2-1.3: Product: 802.11n WLAN Adapter [ 239.417732] usb 2-1.3: Manufacturer: Realtek [ 239.417733] usb 2-1.3: SerialNumber: 00e04c01 [ 239.578100] rtl8192cu: Chip version 0x11 [ 239.678225] usb 2-1-port3: disabled by hub (EMI?), re-enabling... Just prior to the oops, your USB hub disabled the port being used by the wireless device. While the response of the driver seems wrong, it is a difficult condition to reproduce; one must either force or forge the disabling by the hub. [ 239.678230] usb 2-1.3: USB disconnect, device number 4 [ 239.679128] rtl_usb: reg 0x30, usbctrl_vendorreq TimeOut! status:0xffed value=0x0 [ 239.679131] rtl_usb: reg 0x32, usbctrl_vendorreq TimeOut! status:0xffed value=0x20 [ 239.679133] rtl_usb: reg 0x33, usbctrl_vendorreq TimeOut! status:0xffed value=0x80 [ 239.679134] rtl_usb: reg 0x30, usbctrl_vendorreq TimeOut! status:0xffed value=0x80206f30 [ 239.700640] rtl_usb: rx_max_size 15360, rx_urb_num 8, in_ep 1 [ 239.700648] BUG: unable to handle kernel NULL pointer dereference at (null) [ 239.700682] IP: rtl_deinit_core+0x2e/0x90 [rtlwifi] [ 239.700693] PGD 1c5d5d067 [ 239.700694] P4D 1c5d5d067 [ 239.700701] PUD 1c5d9b067 [ 239.700708] PMD 0 [ 239.700727] Oops: [#1] SMP [ 239.700735] Modules linked in: rtl8192cu(+) rtl_usb rtl8192c_common rtlwifi mac80211 cfg80211 rfkill appletalk ipx p8023 p8022 psnap llc nct6775 hwmon_vid ipv6 hid_generic usbhid hid coretemp hwmon x86_pkg_temp_thermal intel_powerclamp snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic kvm_intel i915 kvm drm_kms_helper evdev i2c_dev irqbypass syscopyarea sysfillrect sysimgblt r8169 crc32_pclmul serio_raw crc32c_intel mii snd_pcsp fb_sys_fops drm fan thermal button battery 8250 8250_base serial_core mei_me snd_hda_intel mei intel_gtt snd_hda_codec video agpgart ehci_pci ehci_hcd lpc_ich snd_hwdep snd_hda_core snd_pcm i2c_algo_bit snd_timer i2c_i801 i2c_core snd soundcore fuse [ 239.700875] CPU: 0 PID: 1174 Comm: udevd Not tainted 4.13.6.20171012 #1 [ 239.700889] Hardware name: To be filled by O.E.M. To be filled by O.E.M./ONDA H61V Ver:4.01, BIOS 4.6.5 01/07/2013 [ 239.700909] task: 8801c5fbc980 task.stack: c99a [ 239.700924] RIP: 0010:rtl_deinit_core+0x2e/0x90 [rtlwifi] [ 239.700936] RSP: :c99a3b40 EFLAGS: 00010207 [ 239.700947] RAX: 0282 RBX: 8801b9440780 RCX: ffea [ 239.700962] RDX: 0001 RSI: 0282 RDI: [ 239.700977] RBP: 8801b944b070 R08: 0001 R09: 02b0 [ 239.700991] R10: ea0008473480 R11: R12: 8801b9441560 [ 239.701006] R13: 8801b944b880 R14: R15: 8801b9441560 [ 239.701021] FS: 7f3eb636e7c0() GS:88021f20() knlGS: [ 239.701037] CS: 0010 DS: ES: CR0: 80050033 [ 239.701050] CR2: CR3: 0001c5d99000 CR4: 001406f0 [ 239.701064] Call Trace: [ 239.701074] ? rtl_usb_probe+0x6b6/0xc50 [rtl_usb] [ 239.701088] ? usb_probe_interface+0xe2/0x2a0 [ 239.701100] ? driver_probe_device+0x21d/0x2d0 [ 239.70] ? __driver_attach+0x8a/0x90 [ 239.701121] ? driver_probe_device+0x2d0/0x2d0 [ 239.701133] ? bus_for_each_dev+0x5c/0x90 [ 239.701143] ? bus_add_driver+0x196/0x220 [ 239.701154] ? driver_register+0x57/0xc0 [ 239.701165] ? usb_register_driver+0x7c/0x140 [ 239.701175] ? 0xa064e000 [ 239.701185] ? do_one_initcall+0x4b/0x190 [ 239.701197] ? kmem_cache_alloc_trace+0xe4/0x1c0 [ 239.701208] ? do_init_module+0x22/0x1e1 [ 239.701219] ? do_init_module+0x5b/0x1e1 [ 239.701229] ? load_module+0x21ff/0x2760 [ 239.701239] ? SYSC_finit_module+0x90/0xb0 [ 239.701249] ? SYSC_finit_module+0x90/0xb0 [ 239.701261] ? entry_SYSCALL_64_fastpath+0x1a/0xa5 [ 239.701272] Code: 00 00 41 57 31 f6 41 56 41 55 41 54 55 53 48 89 fb e8 e7 fe ff ff 4c 8b 7b 48 49 8b bf 10 9b 00 00 49 8d af 10 9b 00 00 48 39 ef <48> 8b 1f 74 44 49 bd 00 01 00 00 00 00 ad de 49 89 de 49 bc 00 [ 239.701327] RIP: rtl_deinit_core+0x2e/0x90 [rtlwifi] RSP: c99a3b40 [ 239.702028] CR2: [ 239.705370] ---[ end trace 6ec9029c0d9c0e13 ]--- [ 239.706311] udevd[528]: worker [1174] failed while handling '/devices/pci
Re: [PATCH] staging: wilc1000: replace redundant computations with 0
On Tue, 2017-10-10 at 15:05 +0100, Colin King wrote:
> From: Colin Ian King
>
> Shifting and masking strHostIfSetMulti->enabled is redundant since
> enabled is a bool and so all the shifted and masked values will be
> zero. Replace them with zero to simplify the code.
>
> Detected by CoverityScan, CID#1339458 ("Bad shift operation") and
> CID#1339506 ("Operands don't affect result").
>
> Signed-off-by: Colin Ian King
> ---
> drivers/staging/wilc1000/host_interface.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/staging/wilc1000/host_interface.c
> b/drivers/staging/wilc1000/host_interface.c
> index 7b620658ec38..94477dd08c85 100644
> --- a/drivers/staging/wilc1000/host_interface.c
> +++ b/drivers/staging/wilc1000/host_interface.c
> @@ -2417,9 +2417,9 @@ static void Handle_SetMulticastFilter(struct wilc_vif
> *vif,
>
> pu8CurrByte = wid.val;
> *pu8CurrByte++ = (strHostIfSetMulti->enabled & 0xFF);
> - *pu8CurrByte++ = ((strHostIfSetMulti->enabled >> 8) & 0xFF);
> - *pu8CurrByte++ = ((strHostIfSetMulti->enabled >> 16) & 0xFF);
> - *pu8CurrByte++ = ((strHostIfSetMulti->enabled >> 24) & 0xFF);
> + *pu8CurrByte++ = 0;
> + *pu8CurrByte++ = 0;
> + *pu8CurrByte++ = 0;
This might be more an indication of another defect
Perhaps this is just supposed to be
*pu8CurrByte++ = strHostIfSetMulti->enabled;
without the three byte sets to zero after that.
> *pu8CurrByte++ = (strHostIfSetMulti->cnt & 0xFF);
> *pu8CurrByte++ = ((strHostIfSetMulti->cnt >> 8) & 0xFF);
Re: rtlwifi oops
On Thu, Oct 19, 2017 at 01:31:43AM +0300, nirinA wrote: > hello there, > i got the oops below with a rtl8192cu:0bda:8178 and kernel 4.13.6, but > cannot reproduce it. > i use this device since 4.3 or so without noticing any issue. > > nirinA > > [ 239.338040] usb 2-1.3: new high-speed USB device number 4 using ehci-pci > [ 239.417728] usb 2-1.3: New USB device found, idVendor=0bda, > idProduct=8178 > [ 239.417730] usb 2-1.3: New USB device strings: Mfr=1, Product=2, > SerialNumber=3 > [ 239.417731] usb 2-1.3: Product: 802.11n WLAN Adapter > [ 239.417732] usb 2-1.3: Manufacturer: Realtek > [ 239.417733] usb 2-1.3: SerialNumber: 00e04c01 > [ 239.578100] rtl8192cu: Chip version 0x11 > [ 239.678225] usb 2-1-port3: disabled by hub (EMI?), re-enabling... Just prior to the oops, your USB hub disabled the port being used by the wireless device. While the response of the driver seems wrong, it is a difficult condition to reproduce; one must either force or forge the disabling by the hub. > [ 239.678230] usb 2-1.3: USB disconnect, device number 4 > [ 239.679128] rtl_usb: reg 0x30, usbctrl_vendorreq TimeOut! > status:0xffed value=0x0 > [ 239.679131] rtl_usb: reg 0x32, usbctrl_vendorreq TimeOut! > status:0xffed value=0x20 > [ 239.679133] rtl_usb: reg 0x33, usbctrl_vendorreq TimeOut! > status:0xffed value=0x80 > [ 239.679134] rtl_usb: reg 0x30, usbctrl_vendorreq TimeOut! > status:0xffed value=0x80206f30 > [ 239.700640] rtl_usb: rx_max_size 15360, rx_urb_num 8, in_ep 1 > [ 239.700648] BUG: unable to handle kernel NULL pointer dereference at > (null) > [ 239.700682] IP: rtl_deinit_core+0x2e/0x90 [rtlwifi] > [ 239.700693] PGD 1c5d5d067 > [ 239.700694] P4D 1c5d5d067 > [ 239.700701] PUD 1c5d9b067 > [ 239.700708] PMD 0 > > [ 239.700727] Oops: [#1] SMP > [ 239.700735] Modules linked in: rtl8192cu(+) rtl_usb rtl8192c_common > rtlwifi mac80211 cfg80211 rfkill appletalk ipx p8023 p8022 psnap llc nct6775 > hwmon_vid ipv6 hid_generic usbhid hid coretemp hwmon x86_pkg_temp_thermal > intel_powerclamp snd_hda_codec_hdmi snd_hda_codec_realtek > snd_hda_codec_generic kvm_intel i915 kvm drm_kms_helper evdev i2c_dev > irqbypass syscopyarea sysfillrect sysimgblt r8169 crc32_pclmul serio_raw > crc32c_intel mii snd_pcsp fb_sys_fops drm fan thermal button battery 8250 > 8250_base serial_core mei_me snd_hda_intel mei intel_gtt snd_hda_codec video > agpgart ehci_pci ehci_hcd lpc_ich snd_hwdep snd_hda_core snd_pcm > i2c_algo_bit snd_timer i2c_i801 i2c_core snd soundcore fuse > [ 239.700875] CPU: 0 PID: 1174 Comm: udevd Not tainted 4.13.6.20171012 #1 > [ 239.700889] Hardware name: To be filled by O.E.M. To be filled by > O.E.M./ONDA H61V Ver:4.01, BIOS 4.6.5 01/07/2013 > [ 239.700909] task: 8801c5fbc980 task.stack: c99a > [ 239.700924] RIP: 0010:rtl_deinit_core+0x2e/0x90 [rtlwifi] > [ 239.700936] RSP: :c99a3b40 EFLAGS: 00010207 > [ 239.700947] RAX: 0282 RBX: 8801b9440780 RCX: > ffea > [ 239.700962] RDX: 0001 RSI: 0282 RDI: > > [ 239.700977] RBP: 8801b944b070 R08: 0001 R09: > 02b0 > [ 239.700991] R10: ea0008473480 R11: R12: > 8801b9441560 > [ 239.701006] R13: 8801b944b880 R14: R15: > 8801b9441560 > [ 239.701021] FS: 7f3eb636e7c0() GS:88021f20() > knlGS: > [ 239.701037] CS: 0010 DS: ES: CR0: 80050033 > [ 239.701050] CR2: CR3: 0001c5d99000 CR4: > 001406f0 > [ 239.701064] Call Trace: > [ 239.701074] ? rtl_usb_probe+0x6b6/0xc50 [rtl_usb] > [ 239.701088] ? usb_probe_interface+0xe2/0x2a0 > [ 239.701100] ? driver_probe_device+0x21d/0x2d0 > [ 239.70] ? __driver_attach+0x8a/0x90 > [ 239.701121] ? driver_probe_device+0x2d0/0x2d0 > [ 239.701133] ? bus_for_each_dev+0x5c/0x90 > [ 239.701143] ? bus_add_driver+0x196/0x220 > [ 239.701154] ? driver_register+0x57/0xc0 > [ 239.701165] ? usb_register_driver+0x7c/0x140 > [ 239.701175] ? 0xa064e000 > [ 239.701185] ? do_one_initcall+0x4b/0x190 > [ 239.701197] ? kmem_cache_alloc_trace+0xe4/0x1c0 > [ 239.701208] ? do_init_module+0x22/0x1e1 > [ 239.701219] ? do_init_module+0x5b/0x1e1 > [ 239.701229] ? load_module+0x21ff/0x2760 > [ 239.701239] ? SYSC_finit_module+0x90/0xb0 > [ 239.701249] ? SYSC_finit_module+0x90/0xb0 > [ 239.701261] ? entry_SYSCALL_64_fastpath+0x1a/0xa5 > [ 239.701272] Code: 00 00 41 57 31 f6 41 56 41 55 41 54 55 53 48 89 fb e8 > e7 fe ff ff 4c 8b 7b 48 49 8b bf 10 9b 00 00 49 8d af 10 9b 00 00 48 39 ef > <48> 8b 1f 74 44 49 bd 00 01 00 00 00 00 ad de 49 89 de 49 bc 00 > [ 239.701327] RIP: rtl_deinit_core+0x2e/0x90 [rtlwifi] RSP: > c99a3b40 > [ 239.702028] CR2: > [ 239.705370] ---[ end trace 6ec9029c0d9c0e13 ]--- > [ 239.706311] udevd[528]: worker [1174] failed while handling > '/devices/pci:00/:00:1
rtlwifi oops
hello there, i got the oops below with a rtl8192cu:0bda:8178 and kernel 4.13.6, but cannot reproduce it. i use this device since 4.3 or so without noticing any issue. nirinA [ 239.338040] usb 2-1.3: new high-speed USB device number 4 using ehci-pci [ 239.417728] usb 2-1.3: New USB device found, idVendor=0bda, idProduct=8178 [ 239.417730] usb 2-1.3: New USB device strings: Mfr=1, Product=2, SerialNumber=3 [ 239.417731] usb 2-1.3: Product: 802.11n WLAN Adapter [ 239.417732] usb 2-1.3: Manufacturer: Realtek [ 239.417733] usb 2-1.3: SerialNumber: 00e04c01 [ 239.578100] rtl8192cu: Chip version 0x11 [ 239.678225] usb 2-1-port3: disabled by hub (EMI?), re-enabling... [ 239.678230] usb 2-1.3: USB disconnect, device number 4 [ 239.679128] rtl_usb: reg 0x30, usbctrl_vendorreq TimeOut! status:0xffed value=0x0 [ 239.679131] rtl_usb: reg 0x32, usbctrl_vendorreq TimeOut! status:0xffed value=0x20 [ 239.679133] rtl_usb: reg 0x33, usbctrl_vendorreq TimeOut! status:0xffed value=0x80 [ 239.679134] rtl_usb: reg 0x30, usbctrl_vendorreq TimeOut! status:0xffed value=0x80206f30 [ 239.700640] rtl_usb: rx_max_size 15360, rx_urb_num 8, in_ep 1 [ 239.700648] BUG: unable to handle kernel NULL pointer dereference at (null) [ 239.700682] IP: rtl_deinit_core+0x2e/0x90 [rtlwifi] [ 239.700693] PGD 1c5d5d067 [ 239.700694] P4D 1c5d5d067 [ 239.700701] PUD 1c5d9b067 [ 239.700708] PMD 0 [ 239.700727] Oops: [#1] SMP [ 239.700735] Modules linked in: rtl8192cu(+) rtl_usb rtl8192c_common rtlwifi mac80211 cfg80211 rfkill appletalk ipx p8023 p8022 psnap llc nct6775 hwmon_vid ipv6 hid_generic usbhid hid coretemp hwmon x86_pkg_temp_thermal intel_powerclamp snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic kvm_intel i915 kvm drm_kms_helper evdev i2c_dev irqbypass syscopyarea sysfillrect sysimgblt r8169 crc32_pclmul serio_raw crc32c_intel mii snd_pcsp fb_sys_fops drm fan thermal button battery 8250 8250_base serial_core mei_me snd_hda_intel mei intel_gtt snd_hda_codec video agpgart ehci_pci ehci_hcd lpc_ich snd_hwdep snd_hda_core snd_pcm i2c_algo_bit snd_timer i2c_i801 i2c_core snd soundcore fuse [ 239.700875] CPU: 0 PID: 1174 Comm: udevd Not tainted 4.13.6.20171012 #1 [ 239.700889] Hardware name: To be filled by O.E.M. To be filled by O.E.M./ONDA H61V Ver:4.01, BIOS 4.6.5 01/07/2013 [ 239.700909] task: 8801c5fbc980 task.stack: c99a [ 239.700924] RIP: 0010:rtl_deinit_core+0x2e/0x90 [rtlwifi] [ 239.700936] RSP: :c99a3b40 EFLAGS: 00010207 [ 239.700947] RAX: 0282 RBX: 8801b9440780 RCX: ffea [ 239.700962] RDX: 0001 RSI: 0282 RDI: [ 239.700977] RBP: 8801b944b070 R08: 0001 R09: 02b0 [ 239.700991] R10: ea0008473480 R11: R12: 8801b9441560 [ 239.701006] R13: 8801b944b880 R14: R15: 8801b9441560 [ 239.701021] FS: 7f3eb636e7c0() GS:88021f20() knlGS: [ 239.701037] CS: 0010 DS: ES: CR0: 80050033 [ 239.701050] CR2: CR3: 0001c5d99000 CR4: 001406f0 [ 239.701064] Call Trace: [ 239.701074] ? rtl_usb_probe+0x6b6/0xc50 [rtl_usb] [ 239.701088] ? usb_probe_interface+0xe2/0x2a0 [ 239.701100] ? driver_probe_device+0x21d/0x2d0 [ 239.70] ? __driver_attach+0x8a/0x90 [ 239.701121] ? driver_probe_device+0x2d0/0x2d0 [ 239.701133] ? bus_for_each_dev+0x5c/0x90 [ 239.701143] ? bus_add_driver+0x196/0x220 [ 239.701154] ? driver_register+0x57/0xc0 [ 239.701165] ? usb_register_driver+0x7c/0x140 [ 239.701175] ? 0xa064e000 [ 239.701185] ? do_one_initcall+0x4b/0x190 [ 239.701197] ? kmem_cache_alloc_trace+0xe4/0x1c0 [ 239.701208] ? do_init_module+0x22/0x1e1 [ 239.701219] ? do_init_module+0x5b/0x1e1 [ 239.701229] ? load_module+0x21ff/0x2760 [ 239.701239] ? SYSC_finit_module+0x90/0xb0 [ 239.701249] ? SYSC_finit_module+0x90/0xb0 [ 239.701261] ? entry_SYSCALL_64_fastpath+0x1a/0xa5 [ 239.701272] Code: 00 00 41 57 31 f6 41 56 41 55 41 54 55 53 48 89 fb e8 e7 fe ff ff 4c 8b 7b 48 49 8b bf 10 9b 00 00 49 8d af 10 9b 00 00 48 39 ef <48> 8b 1f 74 44 49 bd 00 01 00 00 00 00 ad de 49 89 de 49 bc 00 [ 239.701327] RIP: rtl_deinit_core+0x2e/0x90 [rtlwifi] RSP: c99a3b40 [ 239.702028] CR2: [ 239.705370] ---[ end trace 6ec9029c0d9c0e13 ]--- [ 239.706311] udevd[528]: worker [1174] failed while handling '/devices/pci:00/:00:1d.0/usb2/2-1/2-1.3/2-1.3:1.0'
Re: Setting single rate in ath10k broken by "reject/clear user rate mask if not usable"
On 10/18/2017 02:02 PM, Johannes Berg wrote: On Wed, 2017-10-18 at 13:51 -0700, Ben Greear wrote: " I guess you could implement this part? I.e. iterating the clients and checking that they all support the rate that is set. However, then you also need to implement that this gets reset when a new client that doesn't support this rate connects. " The first part seems OK, but the second seems like a pain. Maybe just keep a new client from being able to connect at all if it doesn't support any available rates? I suppose if you reject the NEW_STATION command, then hostapd will reject the association though, so it's probably not hard to do. However, I'm not really sure why you'd want that? If you do want that then basically you're just implemented a very roundabout way of adding this rate to the basic rates, so you might as well just add it and work with the current basic rates check? If a user specifies a specific rate or rate-set, then I do not think we should quietly change it out from under them. So, we could check at the time the rate-set is applied (all current peers). We can reject the change at that point if one of the peers does not support any common rates. And, whenever a peer tries to be associated, we can check that there is some common rateset in place. If there is no common rateset, then reject the association one way or another. We'll need to be a little careful what happens with client-mode interfaces, which is where we actually observed hitting the WARN_ON() about not having any rates at all, but I think I already put a reset of the rates there anyway if they're not compatible with the AP. At least that's easier because there's only one client. It would be easy to configure a station to do VHT MCS 8 only, and then it would never be able to associate with an HT AP, for instance. I don't think this should be a WARN_ON event, just a failure. It would be great if we could get a useful error message back to the caller, but I am not sure how feasible that is with the current netlink and mac80211 code. If your main concern is hitting a WARN_ON, maybe just change it to a quieter error message or remove it entirely and just return a failure code? Thanks, Ben johannes -- Ben Greear Candela Technologies Inc http://www.candelatech.com
Re: [PATCH] mac80211: aggregation: Convert timers to use timer_setup()
On Wed, Oct 18, 2017 at 1:50 PM, Johannes Berg wrote: > On Wed, 2017-10-18 at 07:19 -0700, Kees Cook wrote: >> On Wed, Oct 18, 2017 at 3:29 AM, Johannes Berg >> wrote: >> > > This has been the least trivial timer conversion yet. Given the use of >> > > RCU and other things I may not even know about, I'd love to get a close >> > > look at this. I *think* this is correct, as it will re-lookup the tid >> > > entries when firing the timer. >> > >> > I'm not really sure why you're doing the lookup again? That seems >> > pointless, since you already have the right structure, and already rely >> > on it being valid. You can't really get a new struct assigned to the >> > same TID without the old one being destroyed. >> >> I couldn't tell what the lifetime expectation was, so I left the >> re-lookup. I assumed it was possible to have a timer fire after the >> structure had been removed from the station structure. > > Oh, right, I guess that would've been possible in theory. It's not > actually possible though since the aggregation sessions can't live on > without a station, so I've already made a follow-up patch to remove the > indirection. Okay, cool, thanks. It seems like I have a similar case in the iwlwifi driver too, but it's different enough that I'm not sure about that one either. I'll send that when I've got it ready. -Kees -- Kees Cook Pixel Security
Re: Setting single rate in ath10k broken by "reject/clear user rate mask if not usable"
On Wed, 2017-10-18 at 13:51 -0700, Ben Greear wrote: > " > I guess you could implement this part? I.e. iterating the clients and > checking that they all support the rate that is set. However, then you > also need to implement that this gets reset when a new client that > doesn't support this rate connects. > " > > The first part seems OK, but the second seems like a pain. Maybe just > keep a new client from being able to connect at all if it doesn't support > any available rates? I suppose if you reject the NEW_STATION command, then hostapd will reject the association though, so it's probably not hard to do. However, I'm not really sure why you'd want that? If you do want that then basically you're just implemented a very roundabout way of adding this rate to the basic rates, so you might as well just add it and work with the current basic rates check? We'll need to be a little careful what happens with client-mode interfaces, which is where we actually observed hitting the WARN_ON() about not having any rates at all, but I think I already put a reset of the rates there anyway if they're not compatible with the AP. At least that's easier because there's only one client. johannes
Re: Setting single rate in ath10k broken by "reject/clear user rate mask if not usable"
On 10/18/2017 01:34 PM, Johannes Berg wrote: Hi, On Wed, 2017-10-18 at 19:56 +0200, Oleksij Rempel wrote: People trying to do regulatory testing want this feature, and other people that are not me also like to test with specific rates. Still a small-ish set of people, but bigger than just me at least. Till now i was interviewing different people who was asking for this for ath9k-htc. So I would say we have: - academical researchers - testers - R&D - exploit and penetration testers - HAM - just hackers As for me, it sounds a s lot. Making (literally) millions of devices in the field hit a WARN_ON() is not really acceptable either though. You can argue that this introduced a regression, but putting the old behaviour back would equally be a regression, for more systems by a few orders of magnitude. In any case, I've already suggested a way to fix this, but you've both completely ignored that part of my email. All I've been reading is that you're demanding that I fix this, and arguments about how much people are allowed to shoot themselves in the foot, none of which is very constructive. You mean this part? It wasn't clear to me that you thought this was a good solution that should be implemented. " I guess you could implement this part? I.e. iterating the clients and checking that they all support the rate that is set. However, then you also need to implement that this gets reset when a new client that doesn't support this rate connects. " The first part seems OK, but the second seems like a pain. Maybe just keep a new client from being able to connect at all if it doesn't support any available rates? Thanks, Ben I might even fix it myself eventually, if only to appease the people who say we have a zero tolerance no regressions rule, but it's not exactly the most important thing I'm doing right now (also, I'll be going on vacation for a few days, and you can probably implement my suggestion in that time, and then I can review it when I get back on Monday.) Let's just say that I think we're discussing the wrong thing here - we ought to be discussing how it can be fixed, and perhaps you can even be constructive in suggesting (and testing, which I can't really do) changes. johannes -- Ben Greear Candela Technologies Inc http://www.candelatech.com
Re: [PATCH] mac80211: aggregation: Convert timers to use timer_setup()
On Wed, 2017-10-18 at 07:19 -0700, Kees Cook wrote: > On Wed, Oct 18, 2017 at 3:29 AM, Johannes Berg > wrote: > > > This has been the least trivial timer conversion yet. Given the use of > > > RCU and other things I may not even know about, I'd love to get a close > > > look at this. I *think* this is correct, as it will re-lookup the tid > > > entries when firing the timer. > > > > I'm not really sure why you're doing the lookup again? That seems > > pointless, since you already have the right structure, and already rely > > on it being valid. You can't really get a new struct assigned to the > > same TID without the old one being destroyed. > > I couldn't tell what the lifetime expectation was, so I left the > re-lookup. I assumed it was possible to have a timer fire after the > structure had been removed from the station structure. Oh, right, I guess that would've been possible in theory. It's not actually possible though since the aggregation sessions can't live on without a station, so I've already made a follow-up patch to remove the indirection. johannes
Re: Setting single rate in ath10k broken by "reject/clear user rate mask if not usable"
Hi, On Wed, 2017-10-18 at 19:56 +0200, Oleksij Rempel wrote: > > > People trying to do regulatory testing want this feature, and other people > > that are not me also like to test with specific rates. Still a > > small-ish set of people, but bigger than just me at least. > > Till now i was interviewing different people who was asking for this for > ath9k-htc. So I would say we have: > - academical researchers > - testers > - R&D > - exploit and penetration testers > - HAM > - just hackers > > As for me, it sounds a s lot. Making (literally) millions of devices in the field hit a WARN_ON() is not really acceptable either though. You can argue that this introduced a regression, but putting the old behaviour back would equally be a regression, for more systems by a few orders of magnitude. In any case, I've already suggested a way to fix this, but you've both completely ignored that part of my email. All I've been reading is that you're demanding that I fix this, and arguments about how much people are allowed to shoot themselves in the foot, none of which is very constructive. I might even fix it myself eventually, if only to appease the people who say we have a zero tolerance no regressions rule, but it's not exactly the most important thing I'm doing right now (also, I'll be going on vacation for a few days, and you can probably implement my suggestion in that time, and then I can review it when I get back on Monday.) Let's just say that I think we're discussing the wrong thing here - we ought to be discussing how it can be fixed, and perhaps you can even be constructive in suggesting (and testing, which I can't really do) changes. johannes
Re: [PATCH 00/58] networking: Convert timers to use timer_setup()
On Tue, Oct 17, 2017 at 10:44 PM, Kalle Valo wrote: > Kees Cook writes: >> Which split is preferred? I had been trying to separate wireless from >> the rest of net (but missed some cases). > > So what we try to follow is that I apply all patches for > drivers/net/wireless to my wireless-drivers trees, with exception of > Johannes taking mac80211_hwsim.c patches to his mac80211 trees. And > Johannes of course takes all patches for net/wireless and net/mac80211. > > So in general I prefer that I take all drivers/net/wireless patches and > make it obvious for Dave that he can ignore those patches (not mix > wireless-drivers and net patches into same set etc). But like I said, > it's ok to push API changes like these via Dave's net trees as well if > you want (and if Dave is ok with that). The chances of conflicts is low, > and if there are be any those would be easy to fix either by me or Dave. Okay, great. That'll help. I'll wait for the dust to settle and rebase against -next, and then I'll see what's outstanding and double-check where they need to be sent (and I'll queue new conversions up accordingly too). >>> For now I'll just drop all your timer_setup() related patches from my >>> queue and I'll assume Dave will take those. Ok? >>> >>> [1] https://patchwork.kernel.org/project/linux-wireless/list/ >> >> I guess I'll wait to see what Dave says. > > Ok, I don't drop the patches from my queue quite yet then. Alright, thanks very much! -Kees -- Kees Cook Pixel Security
Re: [PATCH] MAINTAINERS: update Johannes Berg's entries
On Tue, 2017-10-10 at 09:57 +0200, Johannes Berg wrote: > From: Johannes Berg > > Update my MAINTAINERS file entries to list all the right files. > Since I'm also the de-facto wireless extensions maintainer, > there's little point in excluding those. > > Signed-off-by: Johannes Berg > --- > MAINTAINERS | 13 ++--- > 1 file changed, 10 insertions(+), 3 deletions(-) > > diff --git a/MAINTAINERS b/MAINTAINERS > index f0c37be4e04a..e90cdecd7b5d 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -3329,17 +3329,22 @@ S:Maintained > F: drivers/auxdisplay/cfag12864bfb.c > F: include/linux/cfag12864b.h > > -CFG80211 and NL80211 > +802.11 (including CFG80211/NL80211) You should also move this block in MAINTAINERS to the appropriate alphabetic order location. > M: Johannes Berg > L: linux-wireless@vger.kernel.org > W: http://wireless.kernel.org/ > T: git git://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211.git > T: git > git://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211-next.git > S: Maintained > +F: net/wireless/ > F: include/uapi/linux/nl80211.h > +F: include/linux/ieee80211.h > +F: include/net/wext.h > F: include/net/cfg80211.h > -F: net/wireless/* > -X: net/wireless/wext* > +F: include/net/iw_handler.h > +F: include/net/ieee80211_radiotap.h > +F: Documentation/driver-api/80211/cfg80211.rst > +F: Documentation/networking/regulatory.txt > > CHAR and MISC DRIVERS > M: Arnd Bergmann > @@ -8207,6 +8212,7 @@ F: Documentation/networking/mac80211-injection.txt > F: include/net/mac80211.h > F: net/mac80211/ > F: drivers/net/wireless/mac80211_hwsim.[ch] > +F: Documentation/networking/mac80211_hwsim/README > > MAILBOX API > M: Jassi Brar > @@ -11491,6 +11497,7 @@ T:git > git://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211.git > T: git > git://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211-next.git > S: Maintained > F: Documentation/rfkill.txt > +F: Documentation/ABI/stable/sysfs-class-rfkill > F: net/rfkill/ > > RHASHTABLE
Re: Setting single rate in ath10k broken by "reject/clear user rate mask if not usable"
Am 18.10.2017 um 16:50 schrieb Ben Greear:
On 10/18/2017 12:33 AM, Johannes Berg wrote:
Hi,
The call to set the rate in the driver comes before the error
check.
if (ieee80211_hw_check(&local->hw, HAS_RATE_CONTROL)) {
ret = drv_set_bitrate_mask(local, sdata, mask);
if (ret) {
pr_err("%s: drv-set-bitrate-mask had error
return: %d\n",
sdata->dev->name, ret);
return ret;
}
}
/*
* If active validate the setting and reject it if it doesn't
leave
* at least one basic rate usable, since we really have to be
able
* to send something, and if we're an AP we have to be able to
do
* so at a basic rate so that all clients can receive it.
*/
if (rcu_access_pointer(sdata->vif.chanctx_conf) &&
sdata->vif.bss_conf.chandef.chan) {
u32 basic_rates = sdata->vif.bss_conf.basic_rates;
enum nl80211_band band = sdata-
vif.bss_conf.chandef.chan->band;
if (!(mask->control[band].legacy & basic_rates)) {
I changed this code so I could set a
single rate... --Ben
pr_err("%s: WARNING: no legacy rates for
band[%d] in set-bitrate-mask.\n",
sdata->dev->name, band);
}
}
Heh, that's just dumb. I guess I'll fix that by putting the test first,
no idea how that happened.
So, I think we should relax this check, at least for ath10k.
Well, yes and no. I don't think we should make ath10k special here,
and
this fixes a real problem - namely that you can set up the system
so
that you have no usable rates at all, and then you just get a
WARN_ON
and start using the lowest possible rate...
Well, there are a million ways to set up a broken system,
True, but this one actually happened in practice, for some reason, and
stopping the user from constantly shooting themselves in the foot seems
like a good idea to me. Especially if the user (or application) can't
really even know what they're getting into.
Now, the case in question was _client_ mode, but still.
and setting a single rate has a useful purpose, especially with
ath10k since it has such limited rate-setting capabilities.
You're stretching the definition of "useful purpose" a bit here though,
you're about the only one who's ever going to need to set a single
rate.
People trying to do regulatory testing want this feature, and other people
that are not me also like to test with specific rates. Still a
small-ish set
of people, but bigger than just me at least.
Till now i was interviewing different people who was asking for this for
ath9k-htc. So I would say we have:
- academical researchers
- testers
- R&D
- exploit and penetration testers
- HAM
- just hackers
As for me, it sounds a s lot.
--
Regards,
Oleksij
Re: using verifier to ensure a BPF program uses certain metadata?
On Wed, Oct 18, 2017 at 08:56:31AM +0200, Johannes Berg wrote:
> > > Now, I realize that people could trivially just work around this in
> > > their program if they wanted, but I think most will take the
> > > reminder
> > > and just implement
> > >
> > > if (ctx->is_data_ethernet)
> > > return DROP_FRAME;
> > >
> > > instead, since mostly data frames will not be very relevant to
> > > them.
> > >
> > > What do you think?
> >
> > sounds fine and considering new verifier ops after Jakub refactoring
> > a check that is_data_ethernet was accessed would fit nicely.
> > Without void** hack.
>
> Ok, thanks! I'll have to check what Jakub is doing there, do you have a
> pointer to that refactoring?
something similar to
commit 4f9218aaf8a4 ("bpf: move knowledge about post-translation offsets out of
verifier")
Re: Setting single rate in ath10k broken by "reject/clear user rate mask if not usable"
On 10/18/2017 12:33 AM, Johannes Berg wrote:
Hi,
The call to set the rate in the driver comes before the error
check.
if (ieee80211_hw_check(&local->hw, HAS_RATE_CONTROL)) {
ret = drv_set_bitrate_mask(local, sdata, mask);
if (ret) {
pr_err("%s: drv-set-bitrate-mask had error
return: %d\n",
sdata->dev->name, ret);
return ret;
}
}
/*
* If active validate the setting and reject it if it doesn't
leave
* at least one basic rate usable, since we really have to be
able
* to send something, and if we're an AP we have to be able to
do
* so at a basic rate so that all clients can receive it.
*/
if (rcu_access_pointer(sdata->vif.chanctx_conf) &&
sdata->vif.bss_conf.chandef.chan) {
u32 basic_rates = sdata->vif.bss_conf.basic_rates;
enum nl80211_band band = sdata-
vif.bss_conf.chandef.chan->band;
if (!(mask->control[band].legacy & basic_rates)) {
I changed this code so I could set a
single rate... --Ben
pr_err("%s: WARNING: no legacy rates for
band[%d] in set-bitrate-mask.\n",
sdata->dev->name, band);
}
}
Heh, that's just dumb. I guess I'll fix that by putting the test first,
no idea how that happened.
So, I think we should relax this check, at least for ath10k.
Well, yes and no. I don't think we should make ath10k special here,
and
this fixes a real problem - namely that you can set up the system
so
that you have no usable rates at all, and then you just get a
WARN_ON
and start using the lowest possible rate...
Well, there are a million ways to set up a broken system,
True, but this one actually happened in practice, for some reason, and
stopping the user from constantly shooting themselves in the foot seems
like a good idea to me. Especially if the user (or application) can't
really even know what they're getting into.
Now, the case in question was _client_ mode, but still.
and setting a single rate has a useful purpose, especially with
ath10k since it has such limited rate-setting capabilities.
You're stretching the definition of "useful purpose" a bit here though,
you're about the only one who's ever going to need to set a single
rate.
People trying to do regulatory testing want this feature, and other people
that are not me also like to test with specific rates. Still a small-ish set
of people, but bigger than just me at least.
This used to work, and now is broken, so it is a regression of at least somewhat
useful feature. I think we need a way to re-enable this, even if it requires
poking some sysfs or debugfs file to allow this check to be relaxed.
And for that matter, it might be nice to allow purposefully broken ratesets
(with part of basic missing, for instance), in order to test peer devices
(including
other Linux stacks).
There is basically never going to be a case where setting a single
tx-rate on an AP is a good idea in a general production environment,
so maybe a possible WARN-ON is fine?
A WARN_ON() for a user configuration is never fine.
You're assuming that there's actually a user sitting there and doing
this, which is not necessarily the case.
Even rejecting a single rate setting wouldn't be enough because you can
get into problems even when you enable multiple rates, e.g. if you
enable all the CCK rates while connected on 5 GHz.
If you *do* set up an AP with a limited rateset, then it should
simply fail to allow a station to connect if it does not have any
matching rates.
That's what requiring at least one basic rate to remain does. If you
want to have basic rates 6,9,12 and then configure only 18, how would
the client get rejected? Just configure basic rates differently
beforehand, and then you can do this easily, and the right thing with
rejecting clients will happen automatically (in fact, clients might not
even attempt to connect - even better!)
This might go back to a previous idea I had (and patches I posted and
carry in my tree) to allow setting a different advertise rateset vs
usable tx-rateset.
You still have the same problem with which clients support them and
require them etc.
For regulatory testing purposes: You can advertise normal basic rateset,
and you can accept those (and even transmit mgt and bcast frames on them),
but for data tx, you use a single fixed rate. That is one reason it is nice
to have the advertised rateset different from the tx rateset.
For existing stations that might not match the new fixed rate, we
could purposefully kick them off I guess, but seems like a lot of
work for a case that seems pretty irrelevant.
For better or worse, there are people using this API programmatically
without a user baby-sitting it, so we need to make it work in all
Re: [PATCH] mac80211: aggregation: Convert timers to use timer_setup()
On Wed, Oct 18, 2017 at 3:29 AM, Johannes Berg wrote: >> This has been the least trivial timer conversion yet. Given the use of >> RCU and other things I may not even know about, I'd love to get a close >> look at this. I *think* this is correct, as it will re-lookup the tid >> entries when firing the timer. > > I'm not really sure why you're doing the lookup again? That seems > pointless, since you already have the right structure, and already rely > on it being valid. You can't really get a new struct assigned to the > same TID without the old one being destroyed. I couldn't tell what the lifetime expectation was, so I left the re-lookup. I assumed it was possible to have a timer fire after the structure had been removed from the station structure. -Kees -- Kees Cook Pixel Security
Re: [PATCH] mac80211: aggregation: Convert timers to use timer_setup()
On Wed, Oct 18, 2017 at 4:37 AM, Johannes Berg wrote: > On Wed, 2017-10-18 at 13:31 +0200, Johannes Berg wrote: >> On Wed, 2017-10-18 at 12:29 +0200, Johannes Berg wrote: >> >> > Anyway, the change here looks correct to me, so I'll apply it and then >> > perhaps clean up more. I've only changed "u16 tid" to "u8 tid" since >> > the valid range is 0-15 (in theory, in practice 0-7). I started with u8 tid, but I saw it cast to u16 and in a few other places it was u16, so I went with that ultimately. >> Well, I guess I'm clearly wrong - it's crashing our test suite left and >> right. >> >> I'll dig a little bit, but I don't have much time today, and will be >> out for a few days starting tomorrow. > > Ok, it's pretty obvious - you never initialize the new fields in tid_tx > (sta and tid), only in tid_rx. Let's see if it passes with that fixed. Argh, whoops, thanks for working on this. -Kees -- Kees Cook Pixel Security
Re: wireless-regdb: Update regulatory rules for Kazakhstan (KZ) on 5GHz
On Wed, Oct 18, 2017 at 02:26:17PM +0300, Андрей Иванов wrote: > Please add support for 5 GHz in Kazakhstan > (5170 - 5250 @ 80), (20), AUTO-BW > (5250 - 5330 @ 80), (20), DFS, AUTO-BW > (5650 - 5730 @ 80), (30), DFS > (5735 - 5835 @ 80), (30) We got a very similar submission not that long ago, and I had some questions which were never answered. http://lists.infradead.org/pipermail/wireless-regdb/2017-August/001086.html That one provided a link to some documentation for Kazakhstan, but the information there seemed incomplete to me. Can you answer the questions I asked there, or provide a link to a more complete source of information for the regulations in Kazakhstan? Thanks, Seth
[PATCH] mac80211: avoid looking up tid_tx/tid_rx from timers
From: Johannes Berg
There's no need to re-lookup the data structures now that
we actually get them immediately with from_timer(), just
avoid that. The struct has to be valid anyway, otherwise
the timer object itself would no longer be valid, and we
can't have a different version of the struct since only a
single session per TID is permitted.
Signed-off-by: Johannes Berg
---
net/mac80211/agg-rx.c | 17 +++--
net/mac80211/agg-tx.c | 31 ---
2 files changed, 11 insertions(+), 37 deletions(-)
diff --git a/net/mac80211/agg-rx.c b/net/mac80211/agg-rx.c
index d444752dbf40..35e94483fb8c 100644
--- a/net/mac80211/agg-rx.c
+++ b/net/mac80211/agg-rx.c
@@ -153,27 +153,16 @@ EXPORT_SYMBOL(ieee80211_stop_rx_ba_session);
*/
static void sta_rx_agg_session_timer_expired(struct timer_list *t)
{
- struct tid_ampdu_rx *tid_rx_timer =
- from_timer(tid_rx_timer, t, session_timer);
- struct sta_info *sta = tid_rx_timer->sta;
- u8 tid = tid_rx_timer->tid;
- struct tid_ampdu_rx *tid_rx;
+ struct tid_ampdu_rx *tid_rx = from_timer(tid_rx, t, session_timer);
+ struct sta_info *sta = tid_rx->sta;
+ u8 tid = tid_rx->tid;
unsigned long timeout;
- rcu_read_lock();
- tid_rx = rcu_dereference(sta->ampdu_mlme.tid_rx[tid]);
- if (!tid_rx) {
- rcu_read_unlock();
- return;
- }
-
timeout = tid_rx->last_rx + TU_TO_JIFFIES(tid_rx->timeout);
if (time_is_after_jiffies(timeout)) {
mod_timer(&tid_rx->session_timer, timeout);
- rcu_read_unlock();
return;
}
- rcu_read_unlock();
ht_dbg(sta->sdata, "RX session timer expired on %pM tid %d\n",
sta->sta.addr, tid);
diff --git a/net/mac80211/agg-tx.c b/net/mac80211/agg-tx.c
index 3680b380e70c..1d0bf857a3b5 100644
--- a/net/mac80211/agg-tx.c
+++ b/net/mac80211/agg-tx.c
@@ -424,18 +424,12 @@ int ___ieee80211_stop_tx_ba_session(struct sta_info *sta,
u16 tid,
*/
static void sta_addba_resp_timer_expired(struct timer_list *t)
{
- struct tid_ampdu_tx *tid_tx_timer =
- from_timer(tid_tx_timer, t, addba_resp_timer);
- struct sta_info *sta = tid_tx_timer->sta;
- u8 tid = tid_tx_timer->tid;
- struct tid_ampdu_tx *tid_tx;
+ struct tid_ampdu_tx *tid_tx = from_timer(tid_tx, t, addba_resp_timer);
+ struct sta_info *sta = tid_tx->sta;
+ u8 tid = tid_tx->tid;
/* check if the TID waits for addBA response */
- rcu_read_lock();
- tid_tx = rcu_dereference(sta->ampdu_mlme.tid_tx[tid]);
- if (!tid_tx ||
- test_bit(HT_AGG_STATE_RESPONSE_RECEIVED, &tid_tx->state)) {
- rcu_read_unlock();
+ if (test_bit(HT_AGG_STATE_RESPONSE_RECEIVED, &tid_tx->state)) {
ht_dbg(sta->sdata,
"timer expired on %pM tid %d not expecting addBA
response\n",
sta->sta.addr, tid);
@@ -446,7 +440,6 @@ static void sta_addba_resp_timer_expired(struct timer_list
*t)
sta->sta.addr, tid);
ieee80211_stop_tx_ba_session(&sta->sta, tid);
- rcu_read_unlock();
}
void ieee80211_tx_ba_session_handle_start(struct sta_info *sta, int tid)
@@ -524,29 +517,21 @@ void ieee80211_tx_ba_session_handle_start(struct sta_info
*sta, int tid)
*/
static void sta_tx_agg_session_timer_expired(struct timer_list *t)
{
- struct tid_ampdu_tx *tid_tx_timer =
- from_timer(tid_tx_timer, t, session_timer);
- struct sta_info *sta = tid_tx_timer->sta;
- u8 tid = tid_tx_timer->tid;
- struct tid_ampdu_tx *tid_tx;
+ struct tid_ampdu_tx *tid_tx = from_timer(tid_tx, t, session_timer);
+ struct sta_info *sta = tid_tx->sta;
+ u8 tid = tid_tx->tid;
unsigned long timeout;
- rcu_read_lock();
- tid_tx = rcu_dereference(sta->ampdu_mlme.tid_tx[tid]);
- if (!tid_tx || test_bit(HT_AGG_STATE_STOPPING, &tid_tx->state)) {
- rcu_read_unlock();
+ if (test_bit(HT_AGG_STATE_STOPPING, &tid_tx->state)) {
return;
}
timeout = tid_tx->last_tx + TU_TO_JIFFIES(tid_tx->timeout);
if (time_is_after_jiffies(timeout)) {
mod_timer(&tid_tx->session_timer, timeout);
- rcu_read_unlock();
return;
}
- rcu_read_unlock();
-
ht_dbg(sta->sdata, "tx session timer expired on %pM tid %d\n",
sta->sta.addr, tid);
--
2.14.2
Re: [PATCH] mac80211: aggregation: Convert timers to use timer_setup()
On Wed, 2017-10-18 at 13:31 +0200, Johannes Berg wrote: > On Wed, 2017-10-18 at 12:29 +0200, Johannes Berg wrote: > > > Anyway, the change here looks correct to me, so I'll apply it and then > > perhaps clean up more. I've only changed "u16 tid" to "u8 tid" since > > the valid range is 0-15 (in theory, in practice 0-7). > > Well, I guess I'm clearly wrong - it's crashing our test suite left and > right. > > I'll dig a little bit, but I don't have much time today, and will be > out for a few days starting tomorrow. Ok, it's pretty obvious - you never initialize the new fields in tid_tx (sta and tid), only in tid_rx. Let's see if it passes with that fixed. johannes
Re: [PATCH] mac80211: aggregation: Convert timers to use timer_setup()
On Wed, 2017-10-18 at 12:29 +0200, Johannes Berg wrote: > Anyway, the change here looks correct to me, so I'll apply it and then > perhaps clean up more. I've only changed "u16 tid" to "u8 tid" since > the valid range is 0-15 (in theory, in practice 0-7). Well, I guess I'm clearly wrong - it's crashing our test suite left and right. I'll dig a little bit, but I don't have much time today, and will be out for a few days starting tomorrow. johannes
Re: [PATCH] netlink: fix netlink_ack() extack race
From: Johannes Berg
Date: Mon, 16 Oct 2017 17:09:53 +0200
> From: Johannes Berg
>
> It seems that it's possible to toggle NETLINK_F_EXT_ACK
> through setsockopt() while another thread/CPU is building
> a message inside netlink_ack(), which could then trigger
> the WARN_ON()s I added since if it goes from being turned
> off to being turned on between allocating and filling the
> message, the skb could end up being too small.
>
> Avoid this whole situation by storing the value of this
> flag in a separate variable and using that throughout the
> function instead.
>
> Fixes: 2d4bc93368f5 ("netlink: extended ACK reporting")
> Signed-off-by: Johannes Berg
Applied and queued up for -stable.
Re: [PATCH] netlink: use NETLINK_CB(in_skb).sk instead of looking it up
From: Johannes Berg
Date: Mon, 16 Oct 2017 16:57:49 +0200
> From: Johannes Berg
>
> When netlink_ack() reports an allocation error to the sending
> socket, there's no need to look up the sending socket since
> it's available in the SKB's CB. Use that instead of going to
> the trouble of looking it up.
>
> Note that the pointer is only available since Eric Biederman's
> commit 3fbc290540a1 ("netlink: Make the sending netlink socket availabe in
> NETLINK_CB")
> which is far newer than the original lookup code (Oct 2003)
> (though the field was called 'ssk' in that commit and only got
> renamed to 'sk' later, I'd actually argue 'ssk' was better - or
> perhaps it should've been 'source_sk' - since there are so many
> different 'sk's involved.)
>
> Signed-off-by: Johannes Berg
Applied to net-next.
Re: [PATCH V6 1/5] mac80211: Enable TDLS peer buffer STA feature
On Mon, 2017-10-16 at 14:00 +0800, yint...@qti.qualcomm.com wrote: > From: Yingying Tang > > Enable TDLS peer buffer STA feature. > Set extended capability bit to enable buffer STA when driver > support it. It would help if you could (build) test your changes :-) johannes
pull-request: iwlwifi-next 2017-10-18
Hi Kalle, Here's the second batch of patches intended for v4.15. It contains the last patch set I send out with v2 of the lq_color patch. I have sent this out before and kbuildbot reported success. Please let me know if there are any issues. Cheers, Luca. The following changes since commit 66cc044249603e12e1dbba347f03bdbc9f171fdf: bcma: use bcma_debug and pr_cont in MIPS driver (2017-10-17 17:22:07 +0300) are available in the git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/iwlwifi/iwlwifi-next.git tags/iwlwifi-next-for-kalle-2017-10-18 for you to fetch changes up to 3c798a45318e098e9937b0fee1e0cf986174fbbe: iwlwifi: pcie: remove set but not used variable tcph (2017-10-18 13:02:01 +0300) Second batch of iwlwifi patches for 4.15 * Allocate reorder buffer dynamically to save memory; * Fix a FW dump problem in the A000 family; * Fix for a statistics gathering issue (v2); * Sort the list of 9000 devices to make it easier to find entries; * A couple of cleanups in the FW dump code; * Remove some unnecessary variables and fields and calculations; Beni Lev (1): iwlwifi: mvm: allow reading UMAC error data from SMEM in A000 devices Johannes Berg (3): iwlwifi: mvm: allocate reorder buffer according to need iwlwifi: mvm: pass baid_data to iwl_mvm_release_frames() iwlwifi: pcie: remove set but not used variable tcph Liad Kaufman (1): iwlwifi: mvm: add missing lq_color Luca Coelho (3): iwlwifi: mvm: move umac_error_event_table validity check to where it's set iwlwifi: define minimum valid address for umac_error_event_table in cfg iwlwifi: pcie: sort IDs for the 9000 series for easier comparisons Sara Sharon (1): iwlwifi: mvm: remove duplicated fields in mvm reorder buffer drivers/net/wireless/intel/iwlwifi/cfg/8000.c | 3 ++- drivers/net/wireless/intel/iwlwifi/cfg/9000.c | 3 ++- drivers/net/wireless/intel/iwlwifi/cfg/a000.c | 3 ++- drivers/net/wireless/intel/iwlwifi/fw/api/tx.h| 4 ++-- drivers/net/wireless/intel/iwlwifi/iwl-config.h | 1 + drivers/net/wireless/intel/iwlwifi/mvm/fw.c | 20 +--- drivers/net/wireless/intel/iwlwifi/mvm/mvm.h | 43 ++- drivers/net/wireless/intel/iwlwifi/mvm/rxmq.c | 49 +++-- drivers/net/wireless/intel/iwlwifi/mvm/sta.c | 44 drivers/net/wireless/intel/iwlwifi/mvm/tx.c | 10 ++ drivers/net/wireless/intel/iwlwifi/mvm/utils.c| 17 - drivers/net/wireless/intel/iwlwifi/pcie/drv.c | 84 ++-- drivers/net/wireless/intel/iwlwifi/pcie/tx-gen2.c | 5 + 13 files changed, 184 insertions(+), 102 deletions(-) signature.asc Description: This is a digitally signed message part
Re: [PATCH] mac80211: aggregation: Convert timers to use timer_setup()
Hi,
[quoting your other email:]
> This has been the least trivial timer conversion yet. Given the use of
> RCU and other things I may not even know about, I'd love to get a close
> look at this. I *think* this is correct, as it will re-lookup the tid
> entries when firing the timer.
I'm not really sure why you're doing the lookup again? That seems
pointless, since you already have the right structure, and already rely
on it being valid. You can't really get a new struct assigned to the
same TID without the old one being destroyed.
> -static void sta_rx_agg_session_timer_expired(unsigned long data)
> +static void sta_rx_agg_session_timer_expired(struct timer_list *t)
> {
> - /* not an elegant detour, but there is no choice as the timer passes
> - * only one argument, and various sta_info are needed here, so init
> - * flow in sta_info_create gives the TID as data, while the timer_to_id
> - * array gives the sta through container_of */
> - u8 *ptid = (u8 *)data;
> - u8 *timer_to_id = ptid - *ptid;
> - struct sta_info *sta = container_of(timer_to_id, struct sta_info,
> - timer_to_tid[0]);
> + struct tid_ampdu_rx *tid_rx_timer =
> + from_timer(tid_rx_timer, t, session_timer);
> + struct sta_info *sta = tid_rx_timer->sta;
> + u16 tid = tid_rx_timer->tid;
> struct tid_ampdu_rx *tid_rx;
> unsigned long timeout;
>
> rcu_read_lock();
> - tid_rx = rcu_dereference(sta->ampdu_mlme.tid_rx[*ptid]);
> + tid_rx = rcu_dereference(sta->ampdu_mlme.tid_rx[tid]);
> if (!tid_rx) {
> rcu_read_unlock();
So through all of this, I'm pretty sure we can just use tid_rx_timer
instead of tid_rx.
(Same for TX)
Anyway, the change here looks correct to me, so I'll apply it and then
perhaps clean up more. I've only changed "u16 tid" to "u8 tid" since
the valid range is 0-15 (in theory, in practice 0-7).
johannes
Re: After upgrading to 4.11.1, wifi driver refuses to load after being unloaded once.
On Wed, 2017-10-18 at 12:50 +0300, Kalle Valo wrote: > Luca Coelho writes: > > > On Wed, 2017-10-18 at 07:59 +0300, Kalle Valo wrote: > > > Luca Coelho writes: > > > > > > > On Tue, 2017-10-17 at 14:23 -0700, Marc MERLIN wrote: > > > > > > > > > I don't know how or why, but I seem to: > > > > > saruman:~# grep IWLWIFI /boot/config-4.12.10-amd64-preempt- > > > > > sysrq- > > > > > 20170406 > > > > > CONFIG_IWLWIFI=m > > > > > CONFIG_IWLWIFI_LEDS=y > > > > > CONFIG_IWLWIFI_OPMODE_MODULAR=y > > > > > # CONFIG_IWLWIFI_BCAST_FILTERING is not set > > > > > CONFIG_IWLWIFI_PCIE_RTPM=y > > > > > CONFIG_IWLWIFI_DEBUG=y > > > > > CONFIG_IWLWIFI_DEVICE_TRACING=y > > > > > > > > > > I'll remove that, thanks. > > > > > > > > Cool, I think that might help. If it doesn't, please report a > > > > bug > > > > in > > > > buzilla. ;) > > > > > > But a Kconfig option should never break functionality, so IMHO > > > this > > > still sounds like a bug in iwlwifi. > > > > The problem is that to get this to work, some changes need to be > > made > > in the platform side. In this case, the rootport is not configured > > properly so it won't work. > > Yeah, but users or distros might accidentally enable this Kconfig > option and break the driver unintentionally. And subtle bugs like > this > are even worse as the user will not realise that it's because of a > new > Kconfig option. > > So I guess you can't automatically detect it the platform supports > RTPM, > right? Maybe there should be a module parameter which has to be set > to > enable this? And at least a big fat warning to the user that RTPM is > enabled, bugs are likely and the user has to know what she's doing. I thought this was what EXPERT was used for: menuconfig EXPERT bool "Configure standard kernel features (expert users)" # Unhide debug options, to make the on-by-default options visible select DEBUG_KERNEL help This option allows certain base kernel options and settings to be disabled or tweaked. This is for specialized environments which can tolerate a "non-standard" kernel. Only use this if you really know what you are doing. But it seems that it's widely used even by people/distros who don't know what they are doing. :( Would it be okay if we just add a printk(KERN_ERR, ...)? > > We discussed this before and that's why this option now depends on > > EXPERT. > > Heh, we did? I have no recollection of whatsoever about that :) I'm not sure you were involved in the discussion, but that discussion was the reason we introduced EXPERT as a dependency. -- Luca.
Re: [PATCH] fq_impl: Properly enforce memory limit
On Mon, 2017-10-16 at 17:05 +0200, Toke Høiland-Jørgensen wrote: > The fq structure would fail to properly enforce the memory limit in > the case > where the packet being enqueued was bigger than the packet being > removed to > bring the memory usage down. So keep dropping packets until the > memory usage is > back below the limit. Also, fix the statistics for memory limit > violations. > Applied. johannes
Re: [PATCH] mac80211: use constant time comparison with keys
On Tue, 2017-10-17 at 20:32 +0200, Jason A. Donenfeld wrote: > Otherwise we risk leaking information via timing side channel. > Applied. johannes
Re: [PATCH] wil6210: disallow changing RSN in beacon change
Hi, > This is not dead code, we reach it in several scenarios, mainly WPS > tests. Interesting. > hostapd uses change_beacon to change the security of the AP so this > needs to be supported. I didn't think this made sense - Jouni? Does hostapd kick off all stations in this case? > We do need to restart the AP in this case which will > disconnect existing clients, but this cannot be helped... Why not restart the AP entirely then from userspace? Hmm. I wonder what would happen with mac80211 - I guess keys would have to removed etc? Does this just work by accident because mac80211 removes the keys with stations? What about GTK(s) though? > As a side note, hostapd can also use change_beacon to change the > SSID. When does that happen? > It does so by updating the SSID IE in the probe response frame. We > have a pending patch that detects this and updates the FW but we also > need to update wdev->ssid otherwise the wireless_dev will be out of > date (not sure if it will cause any problems...) Logic-wise it won't, but we do expose this to userspace and that'd be confusing, so we have to update it I guess. johannes
Re: After upgrading to 4.11.1, wifi driver refuses to load after being unloaded once.
Luca Coelho writes: > On Wed, 2017-10-18 at 07:59 +0300, Kalle Valo wrote: >> Luca Coelho writes: >> >> > On Tue, 2017-10-17 at 14:23 -0700, Marc MERLIN wrote: >> > >> > > I don't know how or why, but I seem to: >> > > saruman:~# grep IWLWIFI /boot/config-4.12.10-amd64-preempt-sysrq- >> > > 20170406 >> > > CONFIG_IWLWIFI=m >> > > CONFIG_IWLWIFI_LEDS=y >> > > CONFIG_IWLWIFI_OPMODE_MODULAR=y >> > > # CONFIG_IWLWIFI_BCAST_FILTERING is not set >> > > CONFIG_IWLWIFI_PCIE_RTPM=y >> > > CONFIG_IWLWIFI_DEBUG=y >> > > CONFIG_IWLWIFI_DEVICE_TRACING=y >> > > >> > > I'll remove that, thanks. >> > >> > Cool, I think that might help. If it doesn't, please report a bug >> > in >> > buzilla. ;) >> >> But a Kconfig option should never break functionality, so IMHO this >> still sounds like a bug in iwlwifi. > > The problem is that to get this to work, some changes need to be made > in the platform side. In this case, the rootport is not configured > properly so it won't work. Yeah, but users or distros might accidentally enable this Kconfig option and break the driver unintentionally. And subtle bugs like this are even worse as the user will not realise that it's because of a new Kconfig option. So I guess you can't automatically detect it the platform supports RTPM, right? Maybe there should be a module parameter which has to be set to enable this? And at least a big fat warning to the user that RTPM is enabled, bugs are likely and the user has to know what she's doing. > We discussed this before and that's why this option now depends on > EXPERT. Heh, we did? I have no recollection of whatsoever about that :) -- Kalle Valo
pull-request: wireless-drivers-next 2017-10-18
Hi Dave, this for 4.15 stream to net-next tree. Please let me know if there are any problems. Kalle The following changes since commit 3e747fa18202896b5be66b88478352d5880fb8eb: Merge ath-current from ath.git (2017-09-25 10:06:12 +0300) are available in the git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/kvalo/wireless-drivers-next.git tags/wireless-drivers-next-for-davem-2017-10-18 for you to fetch changes up to 66cc044249603e12e1dbba347f03bdbc9f171fdf: bcma: use bcma_debug and pr_cont in MIPS driver (2017-10-17 17:22:07 +0300) wireless-drivers-next patches for 4.15 The first pull request for 4.15, unusually late this time but still relatively small. Also includes merge from wireless-drivers to fix conflicts in iwlwifi. Major changes: rsi * add P2P mode support * sdio suspend and resume support iwlwifi * A fix and an addition for PCI devices for the A000 family * Dump PCI registers when an error occurs, to make it easier to debug rtlwifi * add support for 64 bit DMA, enabled with a module parameter * add module parameter to enable ASPM Adam Borowski (1): rtl8xxxu: Don't printk raw binary if serial number is not burned in. Allen Pais (1): brcmfmac: use setup_timer() helper Andrey Konovalov (1): p54: don't unregister leds when they are not initialized Arnd Bergmann (2): brcmsmac: make some local variables 'static const' to reduce stack size rsi: fix integer overflow warning Chaya Rachel Ivgi (2): iwlwifi: nvm: set the correct offsets to 3168 series iwlwifi: remove redundant reading from NVM file Christoph Böhmwalder (1): iwlwifi: fix minor code style issues Christos Gkekas (1): rtlwifi: Remove unused cur_rfstate variables Colin Ian King (8): rsi: fix a dereference on adapter before it has been null checked b43: fix unitialized reads of ret by initializing the array to zero b43legacy: fix unitialized reads of ret by initializing the array to zero mwifiex: make const arrays static to shink object code size brcmsmac: make const array ucode_ofdm_rates static, reduces object code size mwifiex: make const array tos_to_ac static, reduces object code size iwlegacy: make const array static to shink object code size b43: make const arrays static, reduces object code size Dan Carpenter (1): rtlwifi: silence underflow warning David Spinadel (1): iwlwifi: mvm: Add new quota command API Douglas Anderson (2): mwifiex: kill useless list_empty checks mwifiex: minor cleanups w/ sta_list_spinlock in cfg80211.c Emmanuel Grumbach (3): iwlwifi: mvm: remove support for Link Quality Measurements iwlwifi: mvm: support firmware debug trigger on frame reorder timeout iwlwifi: mvm: don't send identical PHY_CTXT_CMD Ganapathi Bhat (4): mwifiex: notify cfg80211 about scan abort mwifiex: check for mfg_mode in add_virtual_intf mwifiex: avoid storing random_mac in private mwifiex: use get_random_mask_addr() helper Golan Ben Ami (1): iwlwifi: stop dbgc recording before stopping DMA Himanshu Jha (2): mwifiex: remove unnecessary call to memset mwifiex: Use put_unaligned_le32 Igor Mitsyanko (17): qtnfmac: convert channel width from bitfiled to simple enum qtnfmac: make "Channel change" event report full channel info qtnfmac: retrieve current channel info from EP qtnfmac: do not cache channel info from "connect" command qtnfmac: let wifi card handle channel switch request to the same chan qtnfmac: pass VIF info to SendChannel command qtnfmac: do not cache CSA chandef info qtnfmac: remove unused mac::status field qtnfmac: do not report channel changes until wiphy is registered qtnfmac: do not cache AP settings in driver structures qtnfmac: pass all AP settings to wireless card for processing qtnfmac: pass channel definition to WiFi card on START_AP command qtnfmac: get rid of QTNF_STATE_AP_CONFIG qtnfmac: get rid of QTNF_STATE_AP_START flag qtnfmac: do not cache BSS state in per-VIF structure qtnfmac: make encryption info a part of CONNECT command. qtnfmac: do not cache current channel info in driver's state Ilan Peer (1): iwlwifi: Add few debug prints to the WRT dump flow Johannes Berg (4): iwlwifi: nvm-parse: unify channel flags printing iwlwifi: fw: api: remove excess enum value documentation iwlwifi: fix indentation in a000 family configuration iwlwifi: mvm: warn on invalid statistics size Kalle Valo (3): Merge tag 'iwlwifi-for-kalle-2017-10-06' of git://git.kernel.org/.../iwlwifi/iwlwifi-fixes Merge tag 'iwlwifi-next-for-kalle-2017-10-06-2' of git://git.kernel.org/.../iwlwifi/iwlwifi-next Merge git://git.kernel.org/.../
Re: [PATCH] wil6210: disallow changing RSN in beacon change
Hi Johannes, On 10/17/2017 10:42 PM, Johannes Berg wrote: > From: Johannes Berg > > This is a code path that will never really get hit anyway, since > it's nonsense to change the beacon of an existing BSS to suddenly > include or no longer include the RSN IE. Reject this instead of > having the dead code, and get rid of accessing wdev->ssid/_len by > way of that. > This is not dead code, we reach it in several scenarios, mainly WPS tests. hostapd uses change_beacon to change the security of the AP so this needs to be supported. We do need to restart the AP in this case which will disconnect existing clients, but this cannot be helped... As a side note, hostapd can also use change_beacon to change the SSID. It does so by updating the SSID IE in the probe response frame. We have a pending patch that detects this and updates the FW but we also need to update wdev->ssid otherwise the wireless_dev will be out of date (not sure if it will cause any problems...)
[PATCH] mac80211: validate user rate mask before configuring driver
From: Johannes Berg
Ben reported that when the user rate mask is rejected for not
matching any basic rate, the driver had already been configured.
This is clearly an oversight in my original change, fix this by
doing the validation before calling the driver.
Reported-by: Ben Greear
Fixes: e8e4f5280ddd ("mac80211: reject/clear user rate mask if not usable")
Signed-off-by: Johannes Berg
---
net/mac80211/cfg.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index a354f1939e49..fb15d3b97cb2 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -2727,12 +2727,6 @@ static int ieee80211_set_bitrate_mask(struct wiphy
*wiphy,
if (!ieee80211_sdata_running(sdata))
return -ENETDOWN;
- if (ieee80211_hw_check(&local->hw, HAS_RATE_CONTROL)) {
- ret = drv_set_bitrate_mask(local, sdata, mask);
- if (ret)
- return ret;
- }
-
/*
* If active validate the setting and reject it if it doesn't leave
* at least one basic rate usable, since we really have to be able
@@ -2748,6 +2742,12 @@ static int ieee80211_set_bitrate_mask(struct wiphy
*wiphy,
return -EINVAL;
}
+ if (ieee80211_hw_check(&local->hw, HAS_RATE_CONTROL)) {
+ ret = drv_set_bitrate_mask(local, sdata, mask);
+ if (ret)
+ return ret;
+ }
+
for (i = 0; i < NUM_NL80211_BANDS; i++) {
struct ieee80211_supported_band *sband = wiphy->bands[i];
int j;
--
2.14.2
Re: Setting single rate in ath10k broken by "reject/clear user rate mask if not usable"
Hi,
> The call to set the rate in the driver comes before the error
> check.
>
> if (ieee80211_hw_check(&local->hw, HAS_RATE_CONTROL)) {
> ret = drv_set_bitrate_mask(local, sdata, mask);
> if (ret) {
> pr_err("%s: drv-set-bitrate-mask had error
> return: %d\n",
> sdata->dev->name, ret);
> return ret;
> }
> }
>
> /*
>* If active validate the setting and reject it if it doesn't
> leave
>* at least one basic rate usable, since we really have to be
> able
>* to send something, and if we're an AP we have to be able to
> do
>* so at a basic rate so that all clients can receive it.
>*/
> if (rcu_access_pointer(sdata->vif.chanctx_conf) &&
> sdata->vif.bss_conf.chandef.chan) {
> u32 basic_rates = sdata->vif.bss_conf.basic_rates;
> enum nl80211_band band = sdata-
> >vif.bss_conf.chandef.chan->band;
>
> if (!(mask->control[band].legacy & basic_rates)) {
> I changed this code so I could set a
> single rate... --Ben
> pr_err("%s: WARNING: no legacy rates for
> band[%d] in set-bitrate-mask.\n",
> sdata->dev->name, band);
> }
> }
Heh, that's just dumb. I guess I'll fix that by putting the test first,
no idea how that happened.
> >
> > > So, I think we should relax this check, at least for ath10k.
> >
> > Well, yes and no. I don't think we should make ath10k special here,
> > and
> > this fixes a real problem - namely that you can set up the system
> > so
> > that you have no usable rates at all, and then you just get a
> > WARN_ON
> > and start using the lowest possible rate...
>
> Well, there are a million ways to set up a broken system,
True, but this one actually happened in practice, for some reason, and
stopping the user from constantly shooting themselves in the foot seems
like a good idea to me. Especially if the user (or application) can't
really even know what they're getting into.
Now, the case in question was _client_ mode, but still.
> and setting a single rate has a useful purpose, especially with
> ath10k since it has such limited rate-setting capabilities.
You're stretching the definition of "useful purpose" a bit here though,
you're about the only one who's ever going to need to set a single
rate.
> There is basically never going to be a case where setting a single
> tx-rate on an AP is a good idea in a general production environment,
> so maybe a possible WARN-ON is fine?
A WARN_ON() for a user configuration is never fine.
You're assuming that there's actually a user sitting there and doing
this, which is not necessarily the case.
Even rejecting a single rate setting wouldn't be enough because you can
get into problems even when you enable multiple rates, e.g. if you
enable all the CCK rates while connected on 5 GHz.
> If you *do* set up an AP with a limited rateset, then it should
> simply fail to allow a station to connect if it does not have any
> matching rates.
That's what requiring at least one basic rate to remain does. If you
want to have basic rates 6,9,12 and then configure only 18, how would
the client get rejected? Just configure basic rates differently
beforehand, and then you can do this easily, and the right thing with
rejecting clients will happen automatically (in fact, clients might not
even attempt to connect - even better!)
> This might go back to a previous idea I had (and patches I posted and
> carry in my tree) to allow setting a different advertise rateset vs
> usable tx-rateset.
You still have the same problem with which clients support them and
require them etc.
> For existing stations that might not match the new fixed rate, we
> could purposefully kick them off I guess, but seems like a lot of
> work for a case that seems pretty irrelevant.
For better or worse, there are people using this API programmatically
without a user baby-sitting it, so we need to make it work in all
cases. We can let the user shoot themselves in the foot and have only a
single usable rate left, but we can't let them hang themselves and have
no rate left at all.
johannes
