[PATCH 0/4] gs_usb: fixes

[PATCH 0/4] gs_usb: fixes

[Marc Kleine-Budde]

Hello,

this series contains fixes for issues found by sashiko.

regards,
Marc

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
Marc Kleine-Budde (4):
      can: gs_usb: gs_can_start_xmit(): don't leak uninitialized memory over USB
      can: gs_usb: gs_usb_probe(): init netdev completely before register_candev()
      can: gs_usb: gs_usb_receive_bulk_callback(): check for overflow flag if SKB allocation fails
      can: gs_usb: gs_usb_receive_bulk_callback(): add missing call to can_rx_offload_irq_finish

 drivers/net/can/usb/gs_usb.c | 67 +++++++++++++++++++++++++++++---------------
 1 file changed, 45 insertions(+), 22 deletions(-)
---
base-commit: 734bef548a96e8774d6eef5dfa2026c6edc4e80b
base-tree: git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git main
change-id: 20261025-gs_usb-fixes-250f38833107

Best regards,
--  
Marc Kleine-Budde <mkl@pengutronix.de>

[PATCH 1/4] can: gs_usb: gs_can_start_xmit(): don't leak uninitialized memory over USB

[Marc Kleine-Budde]

The driver allocates the transmission buffer using kmalloc(), which means
the memory remains uninitialized. Then various headers are filled and, but
only the actual CAN frame data payload (of length cfd->len or cf->len) is
copied into the buffer, leaving the remaining buffer uninitialized.

Fix this information leak, allocate the buffer with kzalloc().

Closes: https://sashiko.dev/#/bug/linux-0a8c0e8a-c623-426a-8db3-f55111f6cd84
Fixes: 26949ac935e3 ("can: gs_usb: add CAN-FD support")
Cc: stable@vger.kernel.org
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/usb/gs_usb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
index 3b9b2f104d86..15f28833f2de 100644
--- a/drivers/net/can/usb/gs_usb.c
+++ b/drivers/net/can/usb/gs_usb.c
@@ -861,7 +861,7 @@ static netdev_tx_t gs_can_start_xmit(struct sk_buff *skb,
 	if (!urb)
 		goto nomem_urb;
 
-	hf = kmalloc(dev->hf_size_tx, GFP_ATOMIC);
+	hf = kzalloc(dev->hf_size_tx, GFP_ATOMIC);
 	if (!hf)
 		goto nomem_hf;
 

-- 
2.53.0

[PATCH 2/4] can: gs_usb: gs_usb_probe(): init netdev completely before register_candev()

[Marc Kleine-Budde]

In gs_usb_probe(), gs_make_candev() is called to allocate and set up the
CAN channel, as well as to call register_candev(). However, gs_usb_probe()
defers the assignment of dev->parent and parent->hf_size_rx until after
gs_make_candev() returns. This opens a race window for the user space to
bring up the CAN interface, while it's not completely setup.

To close the race window, initialize all CAN interfaces in a first loop and
then register them in a second loop.

Closes: https://sashiko.dev/#/bug/linux-be54de7d-0b75-408c-b3ee-dd90c9756857
Closes: https://sashiko.dev/#/bug/linux-71076d59-0843-4749-a5ab-fdd98a08582f
Fixes: d08e973a77d1 ("can: gs_usb: Added support for the GS_USB CAN devices")
Cc: stable@vger.kernel.org
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/usb/gs_usb.c | 45 ++++++++++++++++++++++++++++----------------
 1 file changed, 29 insertions(+), 16 deletions(-)

diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
index 15f28833f2de..985d6a9113d9 100644
--- a/drivers/net/can/usb/gs_usb.c
+++ b/drivers/net/can/usb/gs_usb.c
@@ -1490,18 +1490,8 @@ static struct gs_can *gs_make_candev(unsigned int channel,
 	can_rx_offload_add_manual(netdev, &dev->offload, GS_NAPI_WEIGHT);
 	SET_NETDEV_DEV(netdev, &intf->dev);
 
-	rc = register_candev(dev->netdev);
-	if (rc) {
-		dev_err(&intf->dev,
-			"Couldn't register candev for channel %d (%pe)\n",
-			channel, ERR_PTR(rc));
-		goto out_can_rx_offload_del;
-	}
-
 	return dev;
 
-out_can_rx_offload_del:
-	can_rx_offload_del(&dev->offload);
 out_free_candev:
 	free_candev(dev->netdev);
 	return ERR_PTR(rc);
@@ -1593,14 +1583,14 @@ static int gs_usb_probe(struct usb_interface *intf,
 			/* save error code to return later */
 			rc = PTR_ERR(parent->canch[i]);
 
-			/* on failure destroy previously created candevs */
+			/* on failure clean up previously created candevs */
 			icount = i;
-			for (i = 0; i < icount; i++)
-				gs_destroy_candev(parent->canch[i]);
+			for (i = 0; i < icount; i++) {
+				can_rx_offload_del(&parent->canch[i]->offload);
+				free_candev(parent->canch[i]->netdev);
+			}
 
-			usb_kill_anchored_urbs(&parent->rx_submitted);
-			kfree(parent);
-			return rc;
+			goto out_usb_kill_anchored_urbs;
 		}
 		parent->canch[i]->parent = parent;
 
@@ -1621,7 +1611,30 @@ static int gs_usb_probe(struct usb_interface *intf,
 		parent->hf_size_rx = max(parent->hf_size_rx, hf_size_rx);
 	}
 
+	for (i = 0; i < icount; i++) {
+		struct gs_can *dev = parent->canch[i];
+
+		rc = register_candev(dev->netdev);
+		if (rc) {
+			dev_err(&intf->dev,
+				"Couldn't register candev for channel %d (%pe)\n",
+				i, ERR_PTR(rc));
+
+			icount = i;
+			for (i = 0; i < icount; i++)
+				gs_destroy_candev(parent->canch[i]);
+
+			goto out_usb_kill_anchored_urbs;
+		}
+	}
+
 	return 0;
+
+out_usb_kill_anchored_urbs:
+	usb_kill_anchored_urbs(&parent->rx_submitted);
+	kfree(parent);
+
+	return rc;
 }
 
 static void gs_usb_disconnect(struct usb_interface *intf)

-- 
2.53.0

[PATCH 3/4] can: gs_usb: gs_usb_receive_bulk_callback(): check for overflow flag if SKB allocation fails

[Marc Kleine-Budde]

If the allocation of an SKB fails in gs_usb_receive_bulk_callback(), the
function neither updates the statistics nor checks whether an overflow flag
is set in the host frame.

Instead of directly resubmitting the URB, first update the dropped RX
statistics then check if an overflow flag is set.

Closes: https://sashiko.dev/#/bug/linux-7b3fe6a0-4628-4d0e-aec0-69f4bf8ad5d4
Fixes: 68c5724ecd15 ("can: gs_usb: gs_usb_receive_bulk_callback(): resubmit URB on skb allocation failure")
Cc: stable@vger.kernel.org
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/usb/gs_usb.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
index 985d6a9113d9..5687544af99c 100644
--- a/drivers/net/can/usb/gs_usb.c
+++ b/drivers/net/can/usb/gs_usb.c
@@ -673,8 +673,12 @@ static void gs_usb_receive_bulk_callback(struct urb *urb)
 	if (hf->echo_id == GS_HOST_FRAME_ECHO_ID_RX) { /* normal rx */
 		if (hf->flags & GS_CAN_FLAG_FD) {
 			skb = alloc_canfd_skb(netdev, &cfd);
-			if (!skb)
-				goto resubmit_urb;
+			if (!skb) {
+				netdev->stats.rx_dropped++;
+				netdev->stats.rx_errors++;
+
+				goto check_overflow;
+			}
 
 			cfd->can_id = le32_to_cpu(hf->can_id);
 			cfd->len = data_length;
@@ -686,8 +690,12 @@ static void gs_usb_receive_bulk_callback(struct urb *urb)
 			memcpy(cfd->data, hf->canfd->data, data_length);
 		} else {
 			skb = alloc_can_skb(netdev, &cf);
-			if (!skb)
-				goto resubmit_urb;
+			if (!skb) {
+				netdev->stats.rx_dropped++;
+				netdev->stats.rx_errors++;
+
+				goto check_overflow;
+			}
 
 			cf->can_id = le32_to_cpu(hf->can_id);
 			can_frame_set_cc_len(cf, hf->can_dlc, dev->can.ctrlmode);
@@ -728,6 +736,7 @@ static void gs_usb_receive_bulk_callback(struct urb *urb)
 		netif_wake_queue(netdev);
 	}
 
+check_overflow:
 	if (hf->flags & GS_CAN_FLAG_OVERFLOW) {
 		stats->rx_over_errors++;
 		stats->rx_errors++;

-- 
2.53.0

[PATCH 4/4] can: gs_usb: gs_usb_receive_bulk_callback(): add missing call to can_rx_offload_irq_finish

[Marc Kleine-Budde]

If the overflow flag is set for a host frame and the allocation of the
error SKB fails, the URB should not be resubmitted immediately.

Instead, call can_rx_offload_irq_finish(), since an SKB may have been added
to rx-offload.

Closes: https://sashiko.dev/#/bug/linux-441137e2-cd75-411f-9ff5-dc4d473c5ddb
Fixes: 24bc41b45583 ("can: gs_usb: convert to NAPI/rx-offload to avoid OoO reception")
Cc: stable@vger.kernel.org
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/usb/gs_usb.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
index 5687544af99c..4635bb60e14b 100644
--- a/drivers/net/can/usb/gs_usb.c
+++ b/drivers/net/can/usb/gs_usb.c
@@ -743,7 +743,7 @@ static void gs_usb_receive_bulk_callback(struct urb *urb)
 
 		skb = alloc_can_err_skb(netdev, &cf);
 		if (!skb)
-			goto resubmit_urb;
+			goto can_rx_offload_irq_finish;
 
 		cf->can_id |= CAN_ERR_CRTL;
 		cf->len = CAN_ERR_DLC;
@@ -752,6 +752,7 @@ static void gs_usb_receive_bulk_callback(struct urb *urb)
 		gs_usb_rx_offload(dev, skb, hf);
 	}
 
+can_rx_offload_irq_finish:
 	can_rx_offload_irq_finish(&dev->offload);
 
 resubmit_urb:

-- 
2.53.0

Re: [PATCH 2/4] can: gs_usb: gs_usb_probe(): init netdev completely before register_candev()

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 1 (1) · Medium: 0 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] This patch adds a memory leak to the gs_usb_probe() error path
  in drivers/net/can/usb/gs_usb.c.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the
  gs_usb_probe() register-failure path keeps an existing use-after-free.

--

can: gs_usb: gs_usb_probe(): init netdev completely before register_candev()

This patch moves register_candev() out of gs_make_candev() and into a
second loop in gs_usb_probe(). Every channel, including dev->parent and
parent->hf_size_rx, is now fully set up before any channel is registered.
This closes the window where userspace could bring up a partially
initialized CAN interface.

Closes: https://sashiko.dev/#/bug/linux-be54de7d-0b75-408c-b3ee-dd90c9756857
Closes: https://sashiko.dev/#/bug/linux-71076d59-0843-4749-a5ab-fdd98a08582f

> diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
> index 15f28833f2deb..985d6a9113d96 100644
> --- a/drivers/net/can/usb/gs_usb.c
> +++ b/drivers/net/can/usb/gs_usb.c
> @@ -1490,18 +1490,8 @@ static struct gs_can *gs_make_candev(unsigned int channel,
>  	can_rx_offload_add_manual(netdev, &dev->offload, GS_NAPI_WEIGHT);
>  	SET_NETDEV_DEV(netdev, &intf->dev);
>  
> -	rc = register_candev(dev->netdev);
> -	if (rc) {
> -		dev_err(&intf->dev,
> -			"Couldn't register candev for channel %d (%pe)\n",
> -			channel, ERR_PTR(rc));
> -		goto out_can_rx_offload_del;
> -	}
> -
>  	return dev;
>  
> -out_can_rx_offload_del:
> -	can_rx_offload_del(&dev->offload);
>  out_free_candev:
>  	free_candev(dev->netdev);
>  	return ERR_PTR(rc);
> @@ -1593,14 +1583,14 @@ static int gs_usb_probe(struct usb_interface *intf,
>  			/* save error code to return later */
>  			rc = PTR_ERR(parent->canch[i]);
>  
> -			/* on failure destroy previously created candevs */
> +			/* on failure clean up previously created candevs */
>  			icount = i;
> -			for (i = 0; i < icount; i++)
> -				gs_destroy_candev(parent->canch[i]);
> +			for (i = 0; i < icount; i++) {
> +				can_rx_offload_del(&parent->canch[i]->offload);
> +				free_candev(parent->canch[i]->netdev);
> +			}
>  
> -			usb_kill_anchored_urbs(&parent->rx_submitted);
> -			kfree(parent);
> -			return rc;
> +			goto out_usb_kill_anchored_urbs;
>  		}
>  		parent->canch[i]->parent = parent;
>  

[ ... ]

> @@ -1621,7 +1611,30 @@ static int gs_usb_probe(struct usb_interface *intf,
>  		parent->hf_size_rx = max(parent->hf_size_rx, hf_size_rx);
>  	}
>  
> +	for (i = 0; i < icount; i++) {
> +		struct gs_can *dev = parent->canch[i];
> +
> +		rc = register_candev(dev->netdev);
> +		if (rc) {
> +			dev_err(&intf->dev,
> +				"Couldn't register candev for channel %d (%pe)\n",
> +				i, ERR_PTR(rc));
> +
> +			icount = i;
> +			for (i = 0; i < icount; i++)
> +				gs_destroy_candev(parent->canch[i]);

[Severity: High]
Does this leak the candevs for channel i and every channel after it?

The second loop only runs after the first loop has called gs_make_candev()
for all channels. At that point every canch[] entry holds an allocated
netdev with rx-offload attached.

When register_candev() fails for channel i, only canch[0..i-1] go through
gs_destroy_candev(). Channels canch[i] through canch[channel_cnt - 1] never
get can_rx_offload_del() or free_candev().

The kfree(parent) at out_usb_kill_anchored_urbs then frees canch[], which
held the only pointers to those netdevs.

Before this patch, gs_make_candev() cleaned up its own channel when
register_candev() failed, and no later channels had been allocated yet.

The USB device can trigger this. register_candev() returns -EINVAL when
can_bittiming_const_valid() fails:

drivers/net/can/dev/dev.c:can_bittiming_const_valid() {
    ...
	if (!btc->sjw_max)
		return false;
    ...
}

gs_make_candev() copies sjw_max straight from the device's
GS_USB_BREQ_BT_CONST reply. A device that reports sjw_max == 0 for channel
0 would leak every channel on each probe.

Should this error path also run can_rx_offload_del() and free_candev() on
channels i through channel_cnt - 1, the same teardown the first loop's
error path uses?

> +
> +			goto out_usb_kill_anchored_urbs;
> +		}
> +	}
> +
>  	return 0;
> +
> +out_usb_kill_anchored_urbs:
> +	usb_kill_anchored_urbs(&parent->rx_submitted);
> +	kfree(parent);
> +
> +	return rc;
>  }

[Severity: High]
This isn't a bug introduced by this patch, but can this teardown order
cause a use-after-free in gs_usb_receive_bulk_callback()?

The old gs_make_candev() failure path had the same pattern. Registered
channels 0..i-1 are destroyed one at a time, and gs_destroy_candev() frees
each netdev right after unregistering it. The freed pointers stay in
parent->canch[].

The shared RX URBs are only killed when the last active channel closes:

gs_can_close() {
    ...
	parent->active_channels--;
	if (!parent->active_channels) {
		usb_kill_anchored_urbs(&parent->rx_submitted);
    ...
}

The only other place they are killed is out_usb_kill_anchored_urbs, which
runs after the whole destroy loop.

Suppose userspace has already brought up canch[0] and canch[1] when
register_candev() fails for a later channel. Unregistering canch[0] closes
it, but active_channels is still 1, so the RX URBs keep running after
canch[0] is freed.

If the device then sends a frame with hf->channel == 0, the callback
dereferences the freed entry:

gs_usb_receive_bulk_callback() {
    ...
	dev = parent->canch[hf->channel];

	netdev = dev->netdev;
	stats = &netdev->stats;

	if (!netif_device_present(netdev))
    ...
}

A frame with an out-of-range channel id goes to the device_detach label
instead. That code calls netif_device_detach() on every non-NULL canch[]
entry, which writes to the state field of the freed netdev.

The window is shorter now that no USB control messages run between
registrations, but the ordering is the same. gs_usb_disconnect() is not
affected, because usb_unbind_interface() disables the interface before it
calls disconnect.

Would it help to unregister all the registered channels first, so the last
close stops the shared RX URBs, and only then free them?

>  
>  static void gs_usb_disconnect(struct usb_interface *intf)

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261025-gs_usb-fixes-v1-0-f74ebbd4da36%40pengutronix.de

Re: [PATCH 3/4] can: gs_usb: gs_usb_receive_bulk_callback(): check for overflow flag if SKB allocation fails

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 (1) · Low: 0 (1)

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] In gs_usb_receive_bulk_callback()
  (drivers/net/can/usb/gs_usb.c), both new SKB-allocation-failure…

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but in
  gs_usb_receive_bulk_callback() (drivers/net/can/usb/gs_usb.c) the…
- [Low] At this commit, the check_overflow block in
  gs_usb_receive_bulk_callback() (drivers/net/can/usb/gs_usb.c:744-746)…

--

can: gs_usb: gs_usb_receive_bulk_callback(): check for overflow flag if SKB allocation fails

This commit changes the alloc_can_skb() and alloc_canfd_skb() failure
paths in gs_usb_receive_bulk_callback(). They now update the RX statistics
and jump to a new check_overflow label instead of resubmitting the URB
directly. As a result, GS_CAN_FLAG_OVERFLOW in the host frame is still
handled when the SKB allocation fails.

Closes: https://sashiko.dev/#/bug/linux-7b3fe6a0-4628-4d0e-aec0-69f4bf8ad5d4

> diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
> index 985d6a9113d96..5687544af99c8 100644
> --- a/drivers/net/can/usb/gs_usb.c
> +++ b/drivers/net/can/usb/gs_usb.c
> @@ -673,8 +673,12 @@ static void gs_usb_receive_bulk_callback(struct urb *urb)
>  	if (hf->echo_id == GS_HOST_FRAME_ECHO_ID_RX) { /* normal rx */
>  		if (hf->flags & GS_CAN_FLAG_FD) {
>  			skb = alloc_canfd_skb(netdev, &cfd);
> -			if (!skb)
> -				goto resubmit_urb;
> +			if (!skb) {
> +				netdev->stats.rx_dropped++;
> +				netdev->stats.rx_errors++;
> +
> +				goto check_overflow;
> +			}

[Severity: Medium]
The commit message says "first update the dropped RX statistics". However,
both new allocation failure branches (CAN FD here, and Classical CAN in the
next hunk) also increment rx_errors. Is rx_errors the right counter for a
host-side allocation failure?

The kernel-doc for struct rtnl_link_stats64 in include/uapi/linux/if_link.h
defines the two counters like this:

 * @rx_errors: Total number of bad packets received on this network device.
 ...
 * @rx_dropped: Number of packets received but not processed,
 *   e.g. due to lack of resources or unsupported protocol.

In this path the frame arrived intact and only the SKB allocation on the
host failed.

Other CAN drivers that handle an alloc_can_skb() or alloc_canfd_skb()
failure count only rx_dropped. Examples are m_can.c, c_can_main.c,
mcp251xfd-rx.c, kvaser_usb_hydra.c and es58x_core.c. The shared rx-offload
helper also leaves rx_errors alone:

drivers/net/can/dev/rx-offload.c:can_rx_offload_offload_one() {
    ...
	if (IS_ERR(skb)) {
		offload->dev->stats.rx_dropped++;
		offload->dev->stats.rx_fifo_errors++;
    ...
}

With this change, would memory pressure on the host show up as RX errors
on a gs_usb interface? Those would look like bus or controller faults, and
the change would reach stable through the Cc: stable tag.

A minor point: the new lines use netdev->stats directly, while the rest of
gs_usb_receive_bulk_callback() uses the local stats pointer.

>  
>  			cfd->can_id = le32_to_cpu(hf->can_id);
>  			cfd->len = data_length;
> @@ -686,8 +690,12 @@ static void gs_usb_receive_bulk_callback(struct urb *urb)
>  			memcpy(cfd->data, hf->canfd->data, data_length);
>  		} else {
>  			skb = alloc_can_skb(netdev, &cf);
> -			if (!skb)
> -				goto resubmit_urb;
> +			if (!skb) {
> +				netdev->stats.rx_dropped++;
> +				netdev->stats.rx_errors++;
> +
> +				goto check_overflow;
> +			}
>  
>  			cf->can_id = le32_to_cpu(hf->can_id);
>  			can_frame_set_cc_len(cf, hf->can_dlc, dev->can.ctrlmode);

[Severity: Medium]
This isn't a bug introduced by this patch, but the Classical CAN error
frame handling is still skipped when alloc_can_skb() fails. A little
further down, after the allocation succeeds:

	/* ERROR frames tell us information about the controller */
	if (le32_to_cpu(hf->can_id) & CAN_ERR_FLAG)
		gs_update_state(dev, cf);

gs_update_state() is the only place where controller state reported by the
device reaches dev->can.state and can_stats (restarts, bus_off,
error_warning, error_passive). It reads cf->can_id and cf->data[1], and the
same values are available in hf->can_id and hf->classic_can->data[1].

An error frame can carry a BUS_OFF, RESTARTED, warning or passive
transition. If one arrives while alloc_can_skb() fails, the goto
check_overflow skips the state update. Can dev->can.state then stay stale
until another state change frame arrives and its allocation succeeds?

Some peer drivers update controller state even when the error SKB
allocation fails. For example, esd_usb.c does:

	skb = alloc_can_err_skb(priv->netdev, &cf);
	if (!skb)
		stats->rx_dropped++;

	if (state != priv->old_state) {

The original return, and the later goto resubmit_urb from commit
68c5724ecd15, also skipped this update. The patch does not address it.

[ ... ]

> @@ -728,6 +736,7 @@ static void gs_usb_receive_bulk_callback(struct urb *urb)
>  		netif_wake_queue(netdev);
>  	}
>  
> +check_overflow:
>  	if (hf->flags & GS_CAN_FLAG_OVERFLOW) {
>  		stats->rx_over_errors++;
>  		stats->rx_errors++;

[Severity: Low]
This is a pre-existing issue, but at this commit the overflow block
continues like this:

		skb = alloc_can_err_skb(netdev, &cf);
		if (!skb)
			goto resubmit_urb;
		...
	}

	can_rx_offload_irq_finish(&dev->offload);

resubmit_urb:

If alloc_can_err_skb() fails, this path skips
can_rx_offload_irq_finish(). An RX SKB queued earlier in the same callback
by gs_usb_rx_offload(), or an echo SKB queued by gs_usb_get_echo_skb(),
then stays on offload->skb_irq_queue. NAPI is not scheduled until a later
URB completion calls irq_finish.

On the new allocation failure paths added here, nothing has been queued
before check_overflow, so this patch does not make the delay worse.

The next patch in this series, "can: gs_usb:
gs_usb_receive_bulk_callback(): add missing call to
can_rx_offload_irq_finish", changes this to goto can_rx_offload_irq_finish
and adds a matching label, which fixes the skipped irq_finish.

One part is not covered by that patch: when alloc_can_err_skb() fails,
rx_dropped is not incremented for the lost overflow error frame. The
overflow event itself is still counted in rx_over_errors and rx_errors.
Should rx_dropped be incremented here as well?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261025-gs_usb-fixes-v1-0-f74ebbd4da36%40pengutronix.de

Re: [PATCH 2/4] can: gs_usb: gs_usb_probe(): init netdev completely before register_candev()

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 977 bytes --]

On 27.10.2026 07:37:27, netdev-bot+sashiko@kernel.org wrote:
> A frame with an out-of-range channel id goes to the device_detach label
> instead. That code calls netif_device_detach() on every non-NULL canch[]
> entry, which writes to the state field of the freed netdev.
>
> The window is shorter now that no USB control messages run between
> registrations, but the ordering is the same. gs_usb_disconnect() is not
> affected, because usb_unbind_interface() disables the interface before it
> calls disconnect.
>
> Would it help to unregister all the registered channels first, so the last
> close stops the shared RX URBs, and only then free them?

Will be fixed in v5.

pw-bot: cr

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]