netdev - Re: [Intel-wired-lan] [PATCH iwl-net] idpf: convert control queue mutex to a spinlock

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: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <06e4f649-0442-42cf-b1db-d88bd8556d39@intel.com>
Date: Tue, 3 Jun 2025 11:37:30 -0600
From: Ahmed Zaki <ahmed.zaki@...el.com>
To: Simon Horman <horms@...nel.org>
CC: <intel-wired-lan@...ts.osuosl.org>, <netdev@...r.kernel.org>, "Aleksandr
 Loktionov" <aleksandr.loktionov@...el.com>
Subject: Re: [Intel-wired-lan] [PATCH iwl-net] idpf: convert control queue
 mutex to a spinlock



On 2025-05-28 3:55 a.m., Simon Horman wrote:
> On Fri, May 23, 2025 at 02:55:37PM -0600, Ahmed Zaki wrote:
>> With VIRTCHNL2_CAP_MACFILTER enabled, the following warning is generated
>> on module load:
>>
>> [  324.701677] BUG: sleeping function called from invalid context at kernel/locking/mutex.c:578
>> [  324.701684] in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 1582, name: NetworkManager
>> [  324.701689] preempt_count: 201, expected: 0
>> [  324.701693] RCU nest depth: 0, expected: 0
>> [  324.701697] 2 locks held by NetworkManager/1582:
>> [  324.701702]  #0: ffffffff9f7be770 (rtnl_mutex){....}-{3:3}, at: rtnl_newlink+0x791/0x21e0
>> [  324.701730]  #1: ff1100216c380368 (_xmit_ETHER){....}-{2:2}, at: __dev_open+0x3f0/0x870
>> [  324.701749] Preemption disabled at:
>> [  324.701752] [<ffffffff9cd23b9d>] __dev_open+0x3dd/0x870
>> [  324.701765] CPU: 30 UID: 0 PID: 1582 Comm: NetworkManager Not tainted 6.15.0-rc5+ #2 PREEMPT(voluntary)
>> [  324.701771] Hardware name: Intel Corporation M50FCP2SBSTD/M50FCP2SBSTD, BIOS SE5C741.86B.01.01.0001.2211140926 11/14/2022
>> [  324.701774] Call Trace:
>> [  324.701777]  <TASK>
>> [  324.701779]  dump_stack_lvl+0x5d/0x80
>> [  324.701788]  ? __dev_open+0x3dd/0x870
>> [  324.701793]  __might_resched.cold+0x1ef/0x23d
>> <..>
>> [  324.701818]  __mutex_lock+0x113/0x1b80
>> <..>
>> [  324.701917]  idpf_ctlq_clean_sq+0xad/0x4b0 [idpf]
>> [  324.701935]  ? kasan_save_track+0x14/0x30
>> [  324.701941]  idpf_mb_clean+0x143/0x380 [idpf]
>> <..>
>> [  324.701991]  idpf_send_mb_msg+0x111/0x720 [idpf]
>> [  324.702009]  idpf_vc_xn_exec+0x4cc/0x990 [idpf]
>> [  324.702021]  ? rcu_is_watching+0x12/0xc0
>> [  324.702035]  idpf_add_del_mac_filters+0x3ed/0xb50 [idpf]
>> <..>
>> [  324.702122]  __hw_addr_sync_dev+0x1cf/0x300
>> [  324.702126]  ? find_held_lock+0x32/0x90
>> [  324.702134]  idpf_set_rx_mode+0x317/0x390 [idpf]
>> [  324.702152]  __dev_open+0x3f8/0x870
>> [  324.702159]  ? __pfx___dev_open+0x10/0x10
>> [  324.702174]  __dev_change_flags+0x443/0x650
>> <..>
>> [  324.702208]  netif_change_flags+0x80/0x160
>> [  324.702218]  do_setlink.isra.0+0x16a0/0x3960
>> <..>
>> [  324.702349]  rtnl_newlink+0x12fd/0x21e0
>>
>> The sequence is as follows:
>> 	rtnl_newlink()->
>> 	__dev_change_flags()->
>> 	__dev_open()->
>> 	dev_set_rx_mode() - >  # disables BH and grabs "dev->addr_list_lock"
>> 	idpf_set_rx_mode() ->  # proceed only if VIRTCHNL2_CAP_MACFILTER is ON
>> 	__dev_uc_sync() ->
>> 	idpf_add_mac_filter ->
>> 	idpf_add_del_mac_filters ->
>> 	idpf_send_mb_msg() ->
>> 	idpf_mb_clean() ->
>> 	idpf_ctlq_clean_sq()   # mutex_lock(cq_lock)
>>
>> Fix by converting cq_lock to a spinlock. All operations under the new
>> lock are safe except freeing the DMA memory, which may use vunmap(). Fix
>> by requesting a contiguous physical memory for the DMA mapping.
> 
> Hi Ahmed,

Hi Simon, Sorry for the late reply, I was off last week.

> 
> If I understand things correctly, then by safe you mean won't sleep.  But

correct, that is what I meant.

> if so my question is if the path that frees DMA memory which is updated by
> this patch is run in a context where sleeping is not allowed.

I am not sure I understand the question, but the current freeing path 
runs in process context and sleeping is allowed (hence the previous use 
of mutex).

With the new spinlock, we need to make sure all code in-between the new 
spin lock/unlock cannot sleep. All was safe except DMA buffer freeing 
which called vunmap(). That is avoided in this patch by requesting 
contiguous DMA memory via DMA_ATTR_FORCE_CONTIGUOUS.