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

[Oliver Hartkopp]

On 08.10.26 06:13, Yogesh Gaur wrote:
> On Wed, Oct 7, 2026 at 10:41 PM Oliver Hartkopp <socketcan@xxxxxxxxxxxx> wrote:
> > On 07.10.26 13:36, Yogesh Gaur wrote:
> > > vcan_tx() and vcan_rx() update dev->stats with plain increments and
> > > rely on the tx queue lock to serialise them.  That only holds for a
> > > 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
> >
> > dev_dstats_tx_add() has been introduced in Linux 6.14 so it is probably
> > some can-next material for Linux 7.4+.
>
> Agreed.
>
> > Btw. why don't we implement get_num_tx_queues() and return 1 as a
> > potential stable fix? This would add a correct locking via the existing
> > tx queue lock for all vcan statistics right?
>
> The locking argument holds: vcan is not LLTX, and every counter update
> (tx, the echo-mode rx in vcan_rx(), and tx_dropped from
> can_dropped_invalid_skb()) happens in vcan_tx() under the xmit locak.
>
> But get_num_tx_queues() alone does not stop the multi-queue case, as
> rtnl_create_link() only consults it when userspace did not ask.

Right, but there is no reason why userspace should set this value - 
except if you want to race in the stats now ;-)

> > Best regards,
> > Oliver
> >
> > ps. vxcan should have a similar problem.
>
> Yes - same pattern and the same single-queue argument works there.
>
> Plan for v2: limiting vcan and vxcan to one TX queue, then the dstats
> conversion for can-next.

Great! This might then also cover the dstats for "real" CAN devices as 
Marc already pointed out.

Many thanks!
Oliver

> Yogesh
> > , each with its own xmit lock.  Two CPUs
> > > transmitting on different queues then update the same counters
> > > concurrently and lose increments:
> > >
> > >     BUG: KCSAN: data-race in vcan_tx / vcan_tx
> > >     read-write to 0xffff88811aad8228 of 8 bytes by interrupt on cpu 1:
> > >      vcan_tx+0x325/0x5d0 drivers/net/can/vcan.c:110
> > >      dev_hard_start_xmit+0x10c/0x380 net/core/dev.c:3969
> > >      __dev_queue_xmit+0xbfd/0x1ec0 net/core/dev.c:4958
> > >      can_send+0x584/0x720 net/can/af_can.c:279
> > >      bcm_can_tx+0x3ba/0x5b0 net/can/bcm.c:367
> > >     read-write to 0xffff88811aad8228 of 8 bytes by interrupt on cpu 0:
> > >      vcan_tx+0x325/0x5d0 drivers/net/can/vcan.c:110
> > >     value changed: 0x00000000000025ef -> 0x00000000000025f0
> > >
> > > Switch the packet and byte counters to the core's per-CPU dstats.
> > > The core allocates them for NETDEV_PCPU_STAT_DSTATS and folds them
> > > together with dev->stats in dev_get_stats(), so tx_dropped from
> > > can_dropped_invalid_skb() is still reported.
> > >
> > > Reported-by: syzbot+937a3a1fbfbc99d6fcaf@xxxxxxxxxxxxxxxxxxxxxxxxx
> > > Closes: https://syzkaller.appspot.com/bug?extid=937a3a1fbfbc99d6fcaf
> > > Assisted-by: LLM
> > > Signed-off-by: Yogesh Gaur <yogeshgaur.83@xxxxxxxxx>
> > > ---
> > >    drivers/net/can/vcan.c | 13 ++++---------
> > >    1 file changed, 4 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/drivers/net/can/vcan.c b/drivers/net/can/vcan.c
> > > index 76e6b7b5c6a1..2ff90cb7eee9 100644
> > > --- a/drivers/net/can/vcan.c
> > > +++ b/drivers/net/can/vcan.c
> > > @@ -71,10 +71,7 @@ MODULE_PARM_DESC(echo, "Echo sent frames (for testing). Default: 0 (Off)");
> > >
> > >    static void vcan_rx(struct sk_buff *skb, struct net_device *dev)
> > >    {
> > > -     struct net_device_stats *stats = &dev->stats;
> > > -
> > > -     stats->rx_packets++;
> > > -     stats->rx_bytes += can_skb_get_data_len(skb);
> > > +     dev_dstats_rx_add(dev, can_skb_get_data_len(skb));
> > >
> > >        skb->pkt_type  = PACKET_BROADCAST;
> > >        skb->dev       = dev;
> > > @@ -85,7 +82,6 @@ static void vcan_rx(struct sk_buff *skb, struct net_device *dev)
> > >
> > >    static netdev_tx_t vcan_tx(struct sk_buff *skb, struct net_device *dev)
> > >    {
> > > -     struct net_device_stats *stats = &dev->stats;
> > >        unsigned int len;
> > >        int loop;
> > >
> > > @@ -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);
> > >
> > >        /* set flag whether this packet has to be looped back */
> > >        loop = skb->pkt_type == PACKET_LOOPBACK;
> > > @@ -107,8 +102,7 @@ static netdev_tx_t vcan_tx(struct sk_buff *skb, struct net_device *dev)
> > >                        /* only count the packets here, because the
> > >                         * CAN core already did the echo for us
> > >                         */
> > > -                     stats->rx_packets++;
> > > -                     stats->rx_bytes += len;
> > > +                     dev_dstats_rx_add(dev, len);
> > >                }
> > >                consume_skb(skb);
> > >                return NETDEV_TX_OK;
> > > @@ -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;
> > >    }
> > >
> > >    static struct rtnl_link_ops vcan_link_ops __read_mostly = {