[PATCH net v2] net: usb: ipheth: stop RX URB on ndo_stop

[PATCH net v2] net: usb: ipheth: stop RX URB on ndo_stop

[raoxu]

From: Xu Rao <raoxu@uniontech.com>

ipheth_open() submits the RX URB, and a successful RX completion
resubmits it from ipheth_rcvbulk_callback(). ipheth_close() stops the
netdev queue and disables the carrier work, but leaves the RX URB
running.

After an administrative link down, an already submitted RX URB can
therefore complete, pass received packets to the networking stack, and
submit itself again even though the interface has been stopped.

Stop the RX URB from ndo_stop. Do not kill the TX URB: packets in a
successfully submitted TX URB have already been accounted as transmitted,
so let an in-flight TX complete normally after the queue has been
stopped.

usb_kill_urb() waits for a pending RX completion to finish and prevents
the completion handler from successfully resubmitting the URB.

Fixes: a19259c3d589 ("drivers/net/usb: Add new driver ipheth")
Cc: stable@vger.kernel.org
Signed-off-by: Xu Rao <raoxu@uniontech.com>
---
v2:
- Kill only the RX URB and leave an in-flight TX URB to complete
  normally, as pointed out by Oliver Neukum.

 drivers/net/usb/ipheth.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/usb/ipheth.c b/drivers/net/usb/ipheth.c
index 2b490114d232..6b90fe1e0e38 100644
--- a/drivers/net/usb/ipheth.c
+++ b/drivers/net/usb/ipheth.c
@@ -505,6 +505,7 @@ static int ipheth_close(struct net_device *net)
 	 * it, so that such a schedule_delayed_work() is a no-op.
 	 */
 	disable_delayed_work_sync(&dev->carrier_work);
+	usb_kill_urb(dev->rx_urb);
 	return 0;
 }
 
-- 
2.50.1

Re: [PATCH net v2] net: usb: ipheth: stop RX URB on ndo_stop

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: usb: ipheth: stop RX URB on ndo_stop

ipheth_close() previously stopped the netdev queue and disabled the carrier
work but left the RX URB submitted, so an in-flight RX could still deliver
packets and re-arm itself after an administrative link down.  This commit
adds a usb_kill_urb() on the RX URB in ndo_stop, deliberately leaving an
in-flight TX URB to complete on its own.

> diff --git a/drivers/net/usb/ipheth.c b/drivers/net/usb/ipheth.c
> index 2b490114d2327..6b90fe1e0e382 100644
> --- a/drivers/net/usb/ipheth.c
> +++ b/drivers/net/usb/ipheth.c
> @@ -505,6 +505,7 @@ static int ipheth_close(struct net_device *net)
>  	 * it, so that such a schedule_delayed_work() is a no-op.
>  	 */
>  	disable_delayed_work_sync(&dev->carrier_work);
> +	usb_kill_urb(dev->rx_urb);
>  	return 0;
>  }

[Severity: Low]
Can this make a plain "ip link set ethN down" print a KERN_ERR line while
RX traffic is flowing?

usb_kill_urb() does atomic_inc(&urb->reject) before it unlinks and waits,
so a giveback that is already in progress can run ipheth_rcvbulk_callback()
with status 0 and fall through to the resubmit:

drivers/net/usb/ipheth.c:ipheth_rcvbulk_callback() {
	...
rx_submit:
	dev->confirmed_pairing = true;
	ipheth_rx_submit(dev, GFP_ATOMIC);
}

That resubmit then hits the reject check in usb_hcd_submit_urb():

drivers/usb/core/hcd.c:usb_hcd_submit_urb() {
	...
	/* Check that the URB isn't being killed */
	if (unlikely(atomic_read(&urb->reject))) {
		rc = -EPERM;
		goto done;
	}

and ipheth_rx_submit() logs that unconditionally:

drivers/net/usb/ipheth.c:ipheth_rx_submit() {
	...
	retval = usb_submit_urb(dev->rx_urb, mem_flags);
	if (retval)
		dev_err(&dev->intf->dev, "%s: usb_submit_urb: %d\n",
			__func__, retval);

Before this change urb->reject was never raised on the close path, so this
-EPERM branch was not reachable from ndo_stop.

The two sites also seem to disagree about what -EPERM means: ipheth_close()
treats the blocked resubmit as the intended outcome, while
ipheth_rx_submit() reports it as a device error.  Would it be worth teaching
ipheth_rx_submit() (or the rx_submit path in ipheth_rcvbulk_callback()) to
skip the dev_err() for -EPERM?

Related to that, ipheth_rcvbulk_callback() discards the return value of
ipheth_rx_submit(), so the now-expected -EPERM and a genuine failure such
as -ENOMEM, which leaves RX dead until the next ipheth_open(), are treated
identically.  Should the callback distinguish them?

Should the commit message also mention the new log output on the close path?
-- 
pw-bot: cr