Re: [PATCH 1/3] can: rockchip_canfd: prevent TX stall on echo skb failure

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

Pre-existing issues:
- [High] Concurrent access to `echo_skb` in `rkcanfd_tx_tail_is_eff()` without locking leads to a Use-After-Free when the IRQ handler frees the SKB.
--

--- Patch [1]: [PATCH 1/3] can: rockchip_canfd: prevent TX stall on echo skb failure ---
commit dae5727dde941410ecb97f7ed728561d5c939a22
Author: Cunhao Lu <[email protected]>

can: rockchip_canfd: prevent TX stall on echo skb failure

rkcanfd_start_xmit() advances tx_head and requests transmission even when
can_put_echo_skb() fails. This creates a pending TX entry without the echo
skb that the RXSTX completion path needs to match the self-received frame.
The entry cannot be completed, and the netdev TX queue can remain stopped
after the two-entry software FIFO fills.

Do not advance tx_head or request transmission if the echo skb cannot be
installed. Account the frame as dropped and report the error. The
-ENOMEM and -EBUSY paths have already consumed the skb; explicitly free
it on -EINVAL, where ownership remains with the caller.

> diff --git a/drivers/net/can/rockchip/rockchip_canfd-tx.c b/drivers/net/can/rockchip/rockchip_canfd-tx.c
> index 12200dcfd3389..d2cbe25c46d6c 100644
> --- a/drivers/net/can/rockchip/rockchip_canfd-tx.c
> +++ b/drivers/net/can/rockchip/rockchip_canfd-tx.c
> @@ -125,8 +125,18 @@ netdev_tx_t rkcanfd_start_xmit(struct sk_buff *skb, struct net_device *ndev)
>  
>  	frame_len = can_skb_get_frame_len(skb);
>  	err = can_put_echo_skb(skb, ndev, tx_head, frame_len);
> -	if (!err)
> -		netdev_sent_queue(priv->ndev, frame_len);
> +	if (err) {
> +		if (err == -EINVAL)
> +			dev_kfree_skb_any(skb);
> +
> +		ndev->stats.tx_dropped++;
> +		if (net_ratelimit())
> +			netdev_err(ndev, "%s: failed to put echo skb: %pe\n",
> +				   __func__, ERR_PTR(err));
> +
> +		return NETDEV_TX_OK;
> +	}
> +	netdev_sent_queue(priv->ndev, frame_len);

[Severity: High]
This is a pre-existing issue, but I noticed a potential use-after-free
related to echo_skb accesses in rkcanfd_tx_tail_is_eff() when called from
rkcanfd_start_xmit() via rkcanfd_get_effective_tx_free().

Does rkcanfd_tx_tail_is_eff() need locking to safely access echo_skb?

drivers/net/can/rockchip/rockchip_canfd-tx.c:rkcanfd_tx_tail_is_eff() {
    ...
	tx_tail = rkcanfd_get_tx_tail(priv);
	skb = priv->can.echo_skb[tx_tail];
	if (!skb) {
        ...
	}

	cfd = (struct canfd_frame *)skb->data;
    ...
}

If a concurrent TX completion interrupt on another CPU executes
rkcanfd_rxstx_filter() and rkcanfd_handle_tx_done_one(), wouldn't it
consume this same SKB and set echo_skb[tx_tail] to NULL?

If the SKB is then queued to the NAPI offload subsystem and immediately
processed and freed on the remote CPU, could the original thread continue
and dereference the now-freed SKB when accessing skb->data?

>  
>  	WRITE_ONCE(priv->tx_head, priv->tx_head + 1);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1