Re: [net-next,v7,2/6] can: add CAN skb extension infrastructure
From: Jakub Kicinski <kuba@kernel.org>
Date: 2026-01-31 18:01:49
Also in:
linux-can, lkml
This is an AI-generated review of your patch. The human sending this email says: Not sure about the first issue, I think you mentioned we have an extra ref? The uninitialized errors should probably be fixed "in net", before this series? :( For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html --- can: add CAN skb extension infrastructure This patch introduces CAN skb extension infrastructure to store per-frame metadata (interface index, frame length, gateway hops, flags) using the standard skb_ext mechanism instead of the private CAN bus skb headroom.
quoted hunk ↗ jump to hunk
diff --git a/drivers/net/can/vxcan.c b/drivers/net/can/vxcan.c index f14c6f02b662..53d7d9046f85 100644 --- a/drivers/net/can/vxcan.c +++ b/drivers/net/can/vxcan.c
[ ... ]
quoted hunk ↗ jump to hunk
@@ -39,6 +40,7 @@ static netdev_tx_t vxcan_xmit(struct sk_buff *oskb, struct net_device *dev) struct vxcan_priv *priv = netdev_priv(dev); struct net_device *peer; struct net_device_stats *peerstats, *srcstats = &dev->stats; + struct can_skb_ext *csx; struct sk_buff *skb; unsigned int len;@@ -63,6 +65,18 @@ static netdev_tx_t vxcan_xmit(struct sk_buff *oskb, struct net_device *dev) goto out_unlock; } + /* the cloned skb points to the skb extension of the original oskb + * with an increased refcount. skb_ext_add() creates a copy to + * separate the skb extension data which is needed to start with a + * fresh can_gw_hops counter in the other namespace. + */ + csx = skb_ext_add(skb, SKB_EXT_CAN); + if (!csx) { + kfree_skb(skb); + kfree_skb(oskb);
^^^^^^^^^^^^^^^^
Is this a double-free of oskb? Looking at the code flow:
skb = skb_clone(oskb, GFP_ATOMIC); /* line 60 */
if (unlikely(!skb)) { ... }
consume_skb(oskb); /* line 62 - frees oskb */
At this point oskb has been freed by consume_skb(). If skb_ext_add() then
fails, the error handler calls kfree_skb(oskb) on the already-freed pointer.
This can occur under memory pressure when skb_clone() succeeds but the
subsequent skb_ext_add() allocation fails. The kfree_skb(oskb) line should
be removed from this error path.
+ goto out_unlock; + }
[ ... ]
quoted hunk ↗ jump to hunk
diff --git a/net/can/isotp.c b/net/can/isotp.c index 4bb60b8f9b96..94103fe654ff 100644 --- a/net/can/isotp.c +++ b/net/can/isotp.c
[ ... ]
quoted hunk ↗ jump to hunk
@@ -1005,6 +1022,13 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size) goto err_out_drop; } + csx = can_skb_ext_add(skb); + if (!csx) { + kfree_skb(skb); + netdev_put(dev, NULL); + goto err_out_drop; + }
Does this error path return an uninitialized value to userspace? The variable err is declared without initialization at line 959 and sock_alloc_send_skb() only sets it on failure. When can_skb_ext_add() fails after a successful sock_alloc_send_skb(), err contains garbage. Other error paths in this function set err explicitly before jumping to err_out_drop. Should this add err = -ENOMEM before the goto?
quoted hunk ↗ jump to hunk
diff --git a/net/can/j1939/socket.c b/net/can/j1939/socket.c index 1589e8ca634e..fc28a7677369 100644 --- a/net/can/j1939/socket.c +++ b/net/can/j1939/socket.c
[ ... ]
quoted hunk ↗ jump to hunk
@@ -895,6 +897,12 @@ static struct sk_buff *j1939_sk_alloc_skb(struct net_device *ndev, if (!skb) goto failure; + csx = can_skb_ext_add(skb); + if (!csx) { + kfree_skb(skb); + goto failure; + }
Similar concern here: ret is declared without initialization at line 891. sock_alloc_send_skb() only writes to ret on failure. When can_skb_ext_add() fails after successful allocation, ret is uninitialized when returned via *errcode at the failure label. Should this set ret = -ENOMEM before goto failure?
quoted hunk ↗ jump to hunk
diff --git a/net/can/raw.c b/net/can/raw.c index fb4f9c854df0..03438e9bc535 100644 --- a/net/can/raw.c +++ b/net/can/raw.c
[ ... ]
quoted hunk ↗ jump to hunk
@@ -956,6 +958,12 @@ static int raw_sendmsg(struct socket *sock, struct msghdr *msg, size_t size) if (!skb) goto put_dev; + csx = can_skb_ext_add(skb); + if (!csx) { + kfree_skb(skb); + goto put_dev; + }
This returns -EINVAL (from line 923) when can_skb_ext_add() fails due to memory allocation failure. Should this be -ENOMEM instead? The sock_alloc_send_skb() above this code returns -ENOMEM for allocation failures. -- pw-bot: cr