netdev - Re: [net,2/7] can: gs_usb: gs_usb_receive_bulk_callback(): unanchor URL on usb_submit

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20260119181904.1209979-1-kuba@kernel.org>
Date: Mon, 19 Jan 2026 10:19:03 -0800
From: Jakub Kicinski <kuba@...nel.org>
To: mkl@...gutronix.de
Cc: Jakub Kicinski <kuba@...nel.org>,
	stable@...r.kernel.org,
	kernel@...gutronix.de,
	socketcan@...tkopp.net,
	davem@...emloft.net,
	linux-can@...r.kernel.org,
	netdev@...r.kernel.org
Subject: Re: [net,2/7] can: gs_usb: gs_usb_receive_bulk_callback(): unanchor URL on usb_submit_urb() error

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
can: gs_usb: gs_usb_receive_bulk_callback(): unanchor URL on usb_submit_urb() error

This patch fixes an infinite loop in gs_can_close() by unanchoring the URB
when usb_submit_urb() fails after re-anchoring. It also adds an error
message for non-ENODEV/ESHUTDOWN failures.

Link: https://patch.msgid.link/20260116-can_usb-fix-reanchor-v1-1-9d74e7289225@pengutronix.de

> diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
> index d093babbc320..192338b481f2 100644
> --- a/drivers/net/can/usb/gs_usb.c
> +++ b/drivers/net/can/usb/gs_usb.c
> @@ -754,6 +754,10 @@ static void gs_usb_receive_bulk_callback(struct urb *urb)
>  	usb_anchor_urb(urb, &parent->rx_submitted);
>
>  	rc = usb_submit_urb(urb, GFP_ATOMIC);
> +	if (!rc)
> +		return;
> +
> +	usb_unanchor_urb(urb);
>
>  	/* USB failure take down all interfaces */
>  	if (rc == -ENODEV) {

[ ... ]

> +	} else if (rc != -ESHUTDOWN && net_ratelimit()) {
> +		netdev_info(netdev, "failed to re-submit IN URB: %pe\n",
> +			    ERR_PTR(urb->status));

Two issues with this error message:

1. The message prints urb->status, but shouldn't it print rc instead? The
   variable rc holds the actual error from usb_submit_urb(), while urb->status
   contains the completion status from the previous callback invocation (which
   would typically be 0 for success since we only reach resubmit_urb on
   successful URB completion). Other CAN USB drivers like usb_8dev print the
   return value from usb_submit_urb() in similar error paths.

2. The local variable netdev is uninitialized when gs_usb_receive_bulk_callback()
   reaches this code path via the short read case. The function declares netdev
   at the top without initialization, and the short read check at the beginning
   of the function jumps directly to resubmit_urb without ever assigning netdev.
   If usb_submit_urb() then fails with an error other than -ENODEV or -ESHUTDOWN,
   this code will pass the uninitialized netdev pointer to netdev_info().

>  	}
>  }