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

Hi,

Thank you for the review.

> > 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?

Yes, this is a real pre-existing race. The TX completion path removes the
echo skb from `can.echo_skb[]` and queues it to the RX offload path, while
`rkcanfd_tx_tail_is_eff()` can concurrently retain the old pointer and
dereference `skb->data`. It can therefore observe inconsistent TX state or
access an skb after it has been consumed by the completion path.

This is addressed by PATCH 3/3. It adds `tx_lock` and uses
`spin_lock_irqsave()` to protect `tx_head`, `tx_tail`, and the echo skb ring as
one state. Both the lookup and dereference in
`rkcanfd_tx_tail_is_eff()` and the echo removal plus `tx_tail` update in the
completion path are performed while holding this lock.

I will make this relationship clearer in the v2 cover letter and PATCH 3/3
description.

Regards,
Cunhao