mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* [PATCH] can: vcan: use per-CPU device stats
@ 2026-10-07 11:36 Yogesh Gaur
  2026-10-07 13:20 ` Marc Kleine-Budde
  2026-10-07 17:10 ` Oliver Hartkopp
  0 siblings, 2 replies; 6+ messages in thread
From: Yogesh Gaur @ 2026-10-07 11:36 UTC (permalink / raw)
  To: Marc Kleine-Budde, Vincent Mailhol
  Cc: linux-can, linux-kernel, Yogesh Gaur, syzbot+937a3a1fbfbc99d6fcaf

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] can: vcan: use per-CPU device stats
  2026-10-07 11:36 [PATCH] can: vcan: use per-CPU device stats Yogesh Gaur
@ 2026-10-07 13:20 ` Marc Kleine-Budde
  2026-10-08  4:15   ` Yogesh Gaur
  2026-10-07 17:10 ` Oliver Hartkopp
  1 sibling, 1 reply; 6+ messages in thread
From: Marc Kleine-Budde @ 2026-10-07 13:20 UTC (permalink / raw)
  To: Yogesh Gaur
  Cc: Vincent Mailhol, linux-can, linux-kernel, syzbot+937a3a1fbfbc99d6fcaf

[-- 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 --]

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] can: vcan: use per-CPU device stats
  2026-10-07 11:36 [PATCH] can: vcan: use per-CPU device stats Yogesh Gaur
  2026-10-07 13:20 ` Marc Kleine-Budde
@ 2026-10-07 17:10 ` Oliver Hartkopp
  2026-10-08  4:13   ` Yogesh Gaur
  1 sibling, 1 reply; 6+ messages in thread
From: Oliver Hartkopp @ 2026-10-07 17:10 UTC (permalink / raw)
  To: Yogesh Gaur, Marc Kleine-Budde, Vincent Mailhol
  Cc: linux-can, linux-kernel, syzbot+937a3a1fbfbc99d6fcaf



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 = {


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] can: vcan: use per-CPU device stats
  2026-10-07 17:10 ` Oliver Hartkopp
@ 2026-10-08  4:13   ` Yogesh Gaur
  2026-10-08  6:12     ` Oliver Hartkopp
  0 siblings, 1 reply; 6+ messages in thread
From: Yogesh Gaur @ 2026-10-08  4:13 UTC (permalink / raw)
  To: socketcan
  Cc: Marc Kleine-Budde, Vincent Mailhol, linux-can, linux-kernel,
	syzbot+937a3a1fbfbc99d6fcaf

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 = {
>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] can: vcan: use per-CPU device stats
  2026-10-07 13:20 ` Marc Kleine-Budde
@ 2026-10-08  4:15   ` Yogesh Gaur
  0 siblings, 0 replies; 6+ messages in thread
From: Yogesh Gaur @ 2026-10-08  4:15 UTC (permalink / raw)
  To: Marc Kleine-Budde
  Cc: Vincent Mailhol, linux-can, linux-kernel, syzbot+937a3a1fbfbc99d6fcaf

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   |

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] can: vcan: use per-CPU device stats
  2026-10-08  4:13   ` Yogesh Gaur
@ 2026-10-08  6:12     ` Oliver Hartkopp
  0 siblings, 0 replies; 6+ messages in thread
From: Oliver Hartkopp @ 2026-10-08  6:12 UTC (permalink / raw)
  To: Yogesh Gaur
  Cc: Marc Kleine-Budde, Vincent Mailhol, linux-can, linux-kernel,
	syzbot+937a3a1fbfbc99d6fcaf



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 = {
>>


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2026-10-08  6:15 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-10-07 11:36 [PATCH] can: vcan: use per-CPU device stats Yogesh Gaur
2026-10-07 13:20 ` Marc Kleine-Budde
2026-10-08  4:15   ` Yogesh Gaur
2026-10-07 17:10 ` Oliver Hartkopp
2026-10-08  4:13   ` Yogesh Gaur
2026-10-08  6:12     ` Oliver Hartkopp

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®