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

[Jakub Kicinski <kuba@kernel.org>]

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.

> 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.
-- 
pw-bot: cr