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

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

[Yogesh Gaur]

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, 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@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=937a3a1fbfbc99d6fcaf
Assisted-by: LLM
Signed-off-by: Yogesh Gaur <yogeshgaur.83@gmail.com>
---
 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 = {
-- 
2.55.0.windows.5

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

[Marc Kleine-Budde]

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

On 07.10.2026 17:06:11, 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, 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@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=937a3a1fbfbc99d6fcaf
> Assisted-by: LLM
> Signed-off-by: Yogesh Gaur <yogeshgaur.83@gmail.com>

So far all drivers use the classic stats, and the CAN dev helper code
(in drivers/net/can/dev) also modifies these fields. A quick search
shows these places:

drivers/net/can/dev/rx-offload.c:48:     struct net_device_stats *stats = &dev->stats;
drivers/net/can/dev/rx-offload.c:162:            offload->dev->stats.rx_dropped++;
drivers/net/can/dev/rx-offload.c:163:            offload->dev->stats.rx_fifo_errors++;
drivers/net/can/dev/rx-offload.c:249:    struct net_device_stats *stats = &dev->stats;
drivers/net/can/dev/rx-offload.c:289:    struct net_device_stats *stats = &dev->stats;
drivers/net/can/dev/skb.c:29:    struct net_device_stats *stats = &dev->stats;
drivers/net/can/dev/skb.c:402:   dev->stats.tx_dropped++;

Can you modify the code to update the dstats if they are active?

regards,
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 --]

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

[Yogesh Gaur]

On Wed, Oct 7, 2026 at 6:50 PM Marc Kleine-Budde <mkl@pengutronix.de> wrote:
>
> On 07.10.2026 17:06:11, 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, 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@syzkaller.appspotmail.com
> > Closes: https://syzkaller.appspot.com/bug?extid=937a3a1fbfbc99d6fcaf
> > Assisted-by: LLM
> > Signed-off-by: Yogesh Gaur <yogeshgaur.83@gmail.com>
>
> So far all drivers use the classic stats, and the CAN dev helper code
> (in drivers/net/can/dev) also modifies these fields. A quick search
> shows these places:
>
> drivers/net/can/dev/rx-offload.c:48:     struct net_device_stats *stats = &dev->stats;
> drivers/net/can/dev/rx-offload.c:162:            offload->dev->stats.rx_dropped++;
> drivers/net/can/dev/rx-offload.c:163:            offload->dev->stats.rx_fifo_errors++;
> drivers/net/can/dev/rx-offload.c:249:    struct net_device_stats *stats = &dev->stats;
> drivers/net/can/dev/rx-offload.c:289:    struct net_device_stats *stats = &dev->stats;
> drivers/net/can/dev/skb.c:29:    struct net_device_stats *stats = &dev->stats;
> drivers/net/can/dev/skb.c:402:   dev->stats.tx_dropped++;
>
> Can you modify the code to update the dstats if they are active?

Yes and as per Oliver's suggestion send race fix along with dstats
work on top for can-next.

Yogesh
>
> regards,
> 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   |

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

[Oliver Hartkopp]

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+.

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?

Best regards,
Oliver

ps. vxcan should have a similar problem.

, 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@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=937a3a1fbfbc99d6fcaf
> Assisted-by: LLM
> Signed-off-by: Yogesh Gaur <yogeshgaur.83@gmail.com>
> ---
>   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 = {

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

[Yogesh Gaur]

On Wed, Oct 7, 2026 at 10:41 PM Oliver Hartkopp <socketcan@hartkopp.net> 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.
>
> 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.

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@syzkaller.appspotmail.com
> > Closes: https://syzkaller.appspot.com/bug?extid=937a3a1fbfbc99d6fcaf
> > Assisted-by: LLM
> > Signed-off-by: Yogesh Gaur <yogeshgaur.83@gmail.com>
> > ---
> >   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 = {
>

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@hartkopp.net> 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@syzkaller.appspotmail.com
>>> Closes: https://syzkaller.appspot.com/bug?extid=937a3a1fbfbc99d6fcaf
>>> Assisted-by: LLM
>>> Signed-off-by: Yogesh Gaur <yogeshgaur.83@gmail.com>
>>> ---
>>>    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 = {
>>

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@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=937a3a1fbfbc99d6fcaf
> Assisted-by: LLM
> Signed-off-by: Yogesh Gaur <yogeshgaur.83@gmail.com>

[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