On Sat, 20 Sept 2025 at 21:00, Andrew Lunn [off-list ref] wrote:

On Sat, Sep 20, 2025 at 10:20:59AM +0530, I Viswanath wrote:

quoted

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