lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <165a166c-d402-48d9-a190-44a710fa4d1c@bootlin.com>
Date: Tue, 14 Jan 2025 13:56:05 +0100
From: Alexis Lothoré <alexis.lothore@...tlin.com>
To: Kalle Valo <kvalo@...nel.org>
Cc: Ajay Singh <ajay.kathat@...rochip.com>,
 Claudiu Beznea <claudiu.beznea@...on.dev>, Marek Vasut <marex@...x.de>,
 Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
 linux-wireless@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] wifi: wilc1000: unregister wiphy only after netdev
 registration

On 1/14/25 13:20, Kalle Valo wrote:
> Alexis Lothoré <alexis.lothore@...tlin.com> writes:
> 
>> wiphy_unregister/wiphy_free has been recently decoupled from
>> wilc_netdev_cleanup to fix a faulty error path in sdio/spi probe
>> functions. However this change introduced a new failure when simply
>> loading then unloading the driver:
>>   $ modprobe wilc1000-sdio; modprobe -r wilc1000-sdio
>>   WARNING: CPU: 0 PID: 115 at net/wireless/core.c:1145 wiphy_unregister+0x904/0xc40 [cfg80211]
>>   Modules linked in: wilc1000_sdio(-) wilc1000 cfg80211 bluetooth ecdh_generic ecc
>>   CPU: 0 UID: 0 PID: 115 Comm: modprobe Not tainted 6.13.0-rc6+ #45
>>   Hardware name: Atmel SAMA5
>>   Call trace:
>>    unwind_backtrace from show_stack+0x18/0x1c
>>    show_stack from dump_stack_lvl+0x44/0x70
>>    dump_stack_lvl from __warn+0x118/0x27c
>>    __warn from warn_slowpath_fmt+0xcc/0x140
>>    warn_slowpath_fmt from wiphy_unregister+0x904/0xc40 [cfg80211]
>>    wiphy_unregister [cfg80211] from wilc_sdio_remove+0xb0/0x15c [wilc1000_sdio]
>>    wilc_sdio_remove [wilc1000_sdio] from sdio_bus_remove+0x104/0x3f0
>>    sdio_bus_remove from device_release_driver_internal+0x424/0x5dc
>>    device_release_driver_internal from driver_detach+0x120/0x224
>>    driver_detach from bus_remove_driver+0x17c/0x314
>>    bus_remove_driver from sys_delete_module+0x310/0x46c
>>    sys_delete_module from ret_fast_syscall+0x0/0x1c
>>   Exception stack(0xd0acbfa8 to 0xd0acbff0)
>>   bfa0:                   0044b210 0044b210 0044b24c 00000800 00000000 00000000
>>   bfc0: 0044b210 0044b210 00000000 00000081 00000000 0044b210 00000000 00000000
>>   bfe0: 00448e24 b6af99c4 0043ea0d aea2e12c
>>   irq event stamp: 0
>>   hardirqs last  enabled at (0): [<00000000>] 0x0
>>   hardirqs last disabled at (0): [<c01588f0>] copy_process+0x1c4c/0x7bec
>>   softirqs last  enabled at (0): [<c0158944>] copy_process+0x1ca0/0x7bec
>>   softirqs last disabled at (0): [<00000000>] 0x0
>>
>> The warning is triggered by the fact that there is still a
>> wireless_device linked to the wiphy we are unregistering, due to
>> wiphy_unregister now being called after net device unregister (performed
>> in wilc_netdev_cleanup). Fix this warning by moving wiphy_unregister
>> after wilc_netdev_cleanup is nominal paths (ie: driver removal).
>> wilc_netdev_cleanup ordering is left untouched in error paths in probe
>> function because net device is not registered in those paths (so the
>> warning can not trigger), yet the wiphy can still be registered, and we
>> still some cleanup steps from wilc_netdev_cleanup.
>>
>> Signed-off-by: Alexis Lothoré <alexis.lothore@...tlin.com>
>> ---
>> I clearly overlooked this simple scenario/misunderstood expected
>> unregistration order when fixing some spi probe error path, my bad (see
>> commit 89a7616e1715 ("ARM: dts: at91-sama5d27_wlsom1: update bluetooth
>> chip description") in wireless-next)
> 
> No worries, bugs are business as usual.
> 
>> @Kalle: 89a7616e1715 (the faulty commit) is only in wireless-next for
>> now IIUC, so I did not provide any Fixes: tag to prevent any faulty SHA1
>> if those commits end up being picked in stable tree (however, the faulty
>> commit _has_ a Fixes tag). Please let me know if we should proceed
>> differently.
> 
> Hmm, I don't really follow you here. I feel that always adding the Fixes
> tag is the safest option, that way it's clear for everyone what commit
> we are fixing.

I was thinking about the fact that the faulty commit SHA1 may change because of
a merge, and then break the Fixes: tag, but maybe I am overthinking.

So if it's ok for you, I would like to add the Fixes tag
> but I can't find commit 89a7616e1715 anywhere.

Gaah, indeed that's not the correct SHA1. The faulty commit in wireless-next is
in fact commit 1be94490b6b8 ("wifi: wilc1000: unregister wiphy only if it has
been registered")

Thanks,

Alexis

-- 
Alexis Lothoré, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ