Re: [PATCH] net: usb: Remove disruptive netif_wake_queue in rtl8150_set_multicast

viswanath
2025-09-20T16:52:25Z

On Sat, 20 Sept 2025 at 21:00, Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Sat, Sep 20, 2025 at 10:20:59AM +0530, I Viswanath wrote:
> > syzbot reported WARNING in rtl8150_start_xmit/usb_submit_urb.
> > This is a possible sequence of events:
> >
> > CPU0 (in rtl8150_start_xmit) CPU1 (in rtl8150_start_xmit) CPU2 (in rtl8150_set_multicast)
> > netif_stop_queue();
> > netif_stop_queue();
> > usb_submit_urb();
> > netif_wake_queue(); <-- Wakes up TX queue before it's ready
> > netif_stop_queue();
> > usb_submit_urb(); <-- Warning
> > freeing urb
> >
> > Remove netif_wake_queue and corresponding netif_stop_queue in rtl8150_set_multicast to
> > prevent this sequence of events
>
> Please expand this sentence with an explanation of why this is
> safe. Why are these two calls not needed? The original author of this
> code thought they where needed, so you need to explain why they are
> not needed.
>
> Andrew
>
> ---
> pw-bot: cr

Hello,

Thanks for pointing that out. I wasn't thinking from that point of view.

According to Documentation, rtl8150_set_multicast (the
ndo_set_rx_mode callback) should
rely on the netif_addr_lock spinlock, not the netif_tx_lock
manipulated by netif
stop/start/wake queue functions.

However, There is no need to use the netif_addr_lock in the driver
directly because
the core function (dev_set_rx_mode) invoking this function locks
and unlocks the lock
correctly.

Synchronization is therefore handled by the core, making it safe
to remove that lock.

From what I have seen, every network driver assumes this for the
ndo_set_rx_mode callback.

I am not sure what the historical context was for using the
tx_lock as the synchronization
mechanism here but it's definitely not valid in the modern networking stack.

Thanks
Viswanath