Re: [net-next,v7,2/6] can: add CAN skb extension infrastructure

[Oliver Hartkopp]

On 31.01.26 19:00, Jakub Kicinski wrote:
> 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? :(

Fortunately there have not been uninitialized ret values before.
E.g. the sock_alloc_send_skb( ..., &err) calls set the ret val before 
going to the out/err labels.

In the other goto/return cases the existing code sets the values explicitly.

Therefore setting the missing err values within the newly added code 
completely addresses the uninitialized ret value problems.

Additionally the kernel test robot discovered an indention issue in the 
original j1939 code from 2018 which is no candidate for net/fixes either 
and can be silently corrected.

I'll send a v8 patch set when the 24h grace period is over 8-)

Best regards,
Oliver

> 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.
>
> > 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
>
> [ ... ]
>
> > @@ -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;
> > +	}
>
> [ ... ]
>
> > 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
>
> [ ... ]
>
> > @@ -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?
>
> > 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
>
> [ ... ]
>
> > @@ -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?
>
> > 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
>
> [ ... ]
>
> > @@ -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.