On Tue, Jun 03, 2025 at 11:37:30AM -0600, Ahmed Zaki wrote:
>
>
> 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.
Thanks for the clarification. And I agree that this is a good approach.
And sorry for my somewhat nonsensical question earlier, my mind had
gone off on a tangent.
Reviewed-by: Simon Horman <[email protected]>
