Re: [PATCH] can: vcan: use per-CPU device stats

[netdev-bot+sashiko]

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

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

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] The fix is incomplete.
- [Low] The commit message gets the cause wrong: 'vcan does not implement
  get_num_tx_queues(), so rtnl_create_link() honours IFLA_NUM_TX_QUEUES'.
- [Low] The patch fixes a syzbot-reported data race (it has Reported-by:
  syzbot+937a3a1fbfbc99d6fcaf and a Closes: link) that loses tx/rx…

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but the sibling
  driver vxcan (drivers/net/can/vxcan.c) has the same multi-queue stats…

--

can: vcan: use per-CPU device stats

The patch moves the vcan tx/rx packet and byte counters off dev->stats,
where they were plain increments, and onto the core per-CPU dstats. It
sets pcpu_stat_type to NETDEV_PCPU_STAT_DSTATS and uses
dev_dstats_rx_add() and dev_dstats_tx_add(). This addresses a KCSAN data
race that syzbot reported on vcan devices with several tx queues.

Closes: https://syzkaller.appspot.com/bug?extid=937a3a1fbfbc99d6fcaf

> single tx queue.  vcan does not implement get_num_tx_queues(), so
> rtnl_create_link() honours IFLA_NUM_TX_QUEUES and a vcan device can be
> created with several queues, each with its own xmit lock.

[Severity: Low]
Is this cause and effect accurate? rtnl_create_link() uses
IFLA_NUM_TX_QUEUES whenever the attribute is present. It only falls back
to ops->get_num_tx_queues() when the attribute is absent:

net/core/rtnetlink.c:rtnl_create_link() {
    ...
	if (tb[IFLA_NUM_TX_QUEUES])
		num_tx_queues = nla_get_u32(tb[IFLA_NUM_TX_QUEUES]);
	else if (ops->get_num_tx_queues)
		num_tx_queues = ops->get_num_tx_queues();
    ...
}

So adding get_num_tx_queues() to vcan would not prevent a multi-queue
device.

Could the wording be changed so it does not suggest that?

> Reported-by: syzbot+937a3a1fbfbc99d6fcaf@xxxxxxxxxxxxxxxxxxxxxxxxx
> Closes: https://syzkaller.appspot.com/bug?extid=937a3a1fbfbc99d6fcaf
> Assisted-by: LLM
> Signed-off-by: Yogesh Gaur <yogeshgaur.83@xxxxxxxxx>

[Severity: Low]
This isn't a bug, but this patch fixes a data race reported by syzbot.
Should it carry a Fixes: tag naming the commit that introduced the
problem? That would let stable and backport tooling tell which trees need
the fix.

> diff --git a/drivers/net/can/vcan.c b/drivers/net/can/vcan.c
> index 76e6b7b5c6a11..2ff90cb7eee9e 100644
> --- a/drivers/net/can/vcan.c
> +++ b/drivers/net/can/vcan.c

[ ... ]

> @@ -93,8 +89,7 @@ static netdev_tx_t vcan_tx(struct sk_buff *skb, struct net_device *dev)
>  		return NETDEV_TX_OK;
>  
>  	len = can_skb_get_data_len(skb);
> -	stats->tx_packets++;
> -	stats->tx_bytes += len;
> +	dev_dstats_tx_add(dev, len);

[Severity: High]
Does this fully close the race described in the commit message?

vcan_tx() still calls can_dropped_invalid_skb() first. That helper's drop
path does a plain increment on the shared dev->stats:

drivers/net/can/dev/skb.c:can_dropped_invalid_skb() {
    ...
inval_skb:
	kfree_skb(skb);
	dev->stats.tx_dropped++;
	return true;
}

Take a vcan created with IFLA_NUM_TX_QUEUES > 1. Two CPUs send invalid
frames on different tx queues, for example via AF_PACKET with a non-CAN
skb->protocol. Each CPU holds only its own queue's xmit lock:

__dev_direct_xmit()
    HARD_TX_LOCK(dev, txq, smp_processor_id());
    netdev_start_xmit()
        vcan_tx()
            can_dropped_invalid_skb()
                dev->stats.tx_dropped++;

Can these tx_dropped increments still be lost? Could KCSAN still report a
data-race in vcan_tx / vcan_tx on this path?

The commit message says tx_dropped "is still reported" because
dev_get_dstats64() folds in dev->stats. The field itself is still updated
without any shared lock, though.

Would it make sense to use DEV_STATS_INC(dev, tx_dropped) in
can_dropped_invalid_skb(), which would cover every CAN driver? Another
option is for vcan to count drops with dev_dstats_tx_dropped().

[ ... ]

> @@ -185,6 +179,7 @@ static void vcan_setup(struct net_device *dev)
>  	dev->netdev_ops		= &vcan_netdev_ops;
>  	dev->ethtool_ops	= &vcan_ethtool_ops;
>  	dev->needs_free_netdev	= true;
> +	dev->pcpu_stat_type	= NETDEV_PCPU_STAT_DSTATS;
>  }

[Severity: Medium]
This isn't a bug introduced by this patch, but the sibling driver vxcan
seems to have the same multi-queue stats race. vxcan_xmit() in
drivers/net/can/vxcan.c does plain increments on both the local and the
peer device stats:

drivers/net/can/vxcan.c:vxcan_xmit() {
    ...
		srcstats->tx_packets++;
		srcstats->tx_bytes += len;
		peerstats = &peer->stats;
		peerstats->rx_packets++;
		peerstats->rx_bytes += len;
    ...
}

It also does dev->stats.tx_dropped++ when the peer is gone.

rtnl_create_link() also honours IFLA_NUM_TX_QUEUES for a vxcan pair, and
vxcan does not set pcpu_stat_type. Concurrent xmits on different tx
queues can lose these increments, including the updates to the peer's rx
counters.

Should vxcan get the same conversion?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261007113611.2082-1-yogeshgaur.83%40gmail.com