Re: [PATCH net-next 12/12] net: mctp: usb: Allow multiple urbs in flight

[Paolo Abeni <pabeni@redhat.com>]

From: AI Reviewer <ai@example.com>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev/
---
net: mctp: usb: Allow multiple urbs in flight

Currently, we stop tx queues when we have one urb submitted. This means
we will immediately hit dev_hard_start_xmit's tx-queues-off ->
NETDEV_TX_BUSY case, and revert to the requeue -> gso_skb single-dequeue
path, and no longer be able to pack skbs without an xmit_more
indication.

Instead, allow a few urbs to be in-flight, with a limit of 16kB of data
outstanding (after which we will disable queues). With this, the tx path
will cause fewer requeues (and therefore non-packed transfers) under
normal loads.

> diff --git a/drivers/net/mctp/mctp-usb.c b/drivers/net/mctp/mctp-usb.c
> index 5739c87da1095..28aeb5b258723 100644
> --- a/drivers/net/mctp/mctp-usb.c
> +++ b/drivers/net/mctp/mctp-usb.c
[ ... ]
> @@ -48,22 +51,38 @@ enum {
>  	MCTP_USB_SUBCLASS_SPAN = 0x02,
>  };
>  
> +/* We use a total-size limit for outstanding URBs, as the transfer counts
> + * may vary a lot between spanning- and non-spanning modes. In spanning mode,
> + * this will allow for a couple of max-sized transfers to be in flight. In
> + * non-spanning mode, 32.
> + *
> + * We want to avoid disabling the tx queue if possible; doing so will end up
> + * requeueing to gso_skb, and we only dequeue from that one skb at a time,
> + * so can no longer perform transfer packing.
> + */
> +static const unsigned int TX_QMEM_MAX = 16384;
> +
>  static void mctp_usb_out_complete(struct urb *urb)
>  {
>  	struct mctp_usblib_tx_ctx *tx_ctx = urb->context;
>  	struct mctp_usb *mctp_usb = mctp_usblib_tx_ctx_priv(tx_ctx);
>  	struct net_device *netdev = mctp_usb->netdev;
>  	unsigned long flags;
> +	bool wake = false;
>  
>  	mctp_usblib_tx_send_complete(tx_ctx, netdev, urb->status == 0);
>  
>  	spin_lock_irqsave(&mctp_usb->tx_lock, flags);
> +	mctp_usb->tx_qmem -= urb->transfer_buffer_length;

[Severity: High]
Does this code underflow tx_qmem if called concurrently with or after
mctp_usb_stop() zeroes it?

If a user downs the interface while TX URBs are in flight, the USB core
(__usb_hcd_giveback_urb) unanchors the URB before invoking this completion
handler.

Because the URB is unanchored, usb_kill_anchored_urbs() in mctp_usb_stop()
can see an empty anchor list and return immediately, without waiting for
the completion handler.

> +	if (mctp_usb->tx_qmem < TX_QMEM_MAX)
> +		wake = true;
>  	usb_unanchor_urb(urb);
>  	spin_unlock_irqrestore(&mctp_usb->tx_lock, flags);
>  
>  	usb_free_urb(urb);
>  
> -	netif_wake_queue(netdev);
> +	if (wake)
> +		netif_wake_queue(netdev);
>  }
[ ... ]
> @@ -221,12 +241,15 @@ static int mctp_usb_stop(struct net_device *dev)
>  	flush_delayed_work(&mctp_usb->rx_retry_work);
>  
>  	usb_kill_urb(mctp_usb->rx_urb);
> -	/* we have stopped queues, the anchor's own lock will serialise
> -	 * access from the urb completion.
> +
> +	/* We have stopped queues, the anchor's own lock will serialise
> +	 * access from the urb completion. We are then guaranteed that no
> +	 * further completions can occur, so can clear tx_qmem without locking.
>  	 */
>  	usb_kill_anchored_urbs(&mctp_usb->tx_anchor);
>  
>  	mctp_usblib_tx_cancel(&mctp_usb->tx, dev);
> +	mctp_usb->tx_qmem = 0;

[Severity: High]
Is the assumption in the comment correct?

Since __usb_hcd_giveback_urb unanchors the URB before calling the completion
handler, usb_kill_anchored_urbs() doesn't guarantee that all completion
handlers have finished.

If tx_qmem is zeroed here before the completion handler finishes, the
subsequent subtraction in mctp_usb_out_complete() will underflow tx_qmem to a
huge unsigned value, causing the TX queue to be permanently stalled on the
next device open.

>  
>  	return 0;
>  }
-- 
This is an AI-generated review.