Re: [PATCH net-next v2 4/4] net: ntb_netdev: Preserve CHECKSUM_PARTIAL across NTB

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Pre-existing memory leak of `skb` when transmitting while the NTB link is down.
--

--- Patch [4]: [PATCH net-next v2 4/4] net: ntb_netdev: Preserve CHECKSUM_PARTIAL across NTB ---
commit 52574632d1af60a032a3fbef27bc73eb5ad8f90c
Author: Koichiro Den <[email protected]>

net: ntb_netdev: Preserve CHECKSUM_PARTIAL across NTB

This commit carries CHECKSUM_PARTIAL with csum_start and csum_offset across
the NTB link to offload checksum calculations to the peer. It advertises
support in every frame and falls back to software until the peer capability
is seen.

> diff --git a/drivers/net/ntb_netdev.c b/drivers/net/ntb_netdev.c
> index 5c7fe6883cb93..9cfb463e472c3 100644
> --- a/drivers/net/ntb_netdev.c
> +++ b/drivers/net/ntb_netdev.c

[ ... ]

> @@ -249,6 +288,7 @@ static const struct ntb_queue_handlers ntb_netdev_handlers = {
>  static netdev_tx_t ntb_netdev_start_xmit(struct sk_buff *skb,
>  					 struct net_device *ndev)
>  {
> +	unsigned int meta = NTB_NETDEV_META_CAP_CSUM;

[ ... ]

> -	rc = ntb_transport_tx_enqueue(q->qp, skb, skb->data, skb->len, 0);
> +	if (skb->ip_summed == CHECKSUM_PARTIAL) {
> +		if (READ_ONCE(dev->peer_csum))
> +			meta |= NTB_NETDEV_META_CSUM |
> +				(skb_checksum_start_offset(skb) <<
> +				 NTB_NETDEV_META_CSUM_START_SHIFT) |
> +				skb->csum_offset;
> +		else if (skb_checksum_help(skb))
> +			goto drop;
> +	}
> +
> +	rc = ntb_transport_tx_enqueue(q->qp, skb, skb->data, skb->len, meta);

[Severity: High]
This is a pre-existing issue, but does this code leak the skb when the NTB
link is down?

Looking at ntb_transport_tx_enqueue() in drivers/ntb/ntb_transport.c, if the
link is down, it returns 0 without taking ownership of the buffer:

drivers/ntb/ntb_transport.c:ntb_transport_tx_enqueue() {
    ...
    /* If the qp link is down already, just ignore. */
    if (!qp->link_is_up)
        return 0;
    ...
}

Since ntb_netdev_start_xmit() sees a return code of 0, it assumes successful
queuing and returns NETDEV_TX_OK without freeing the skb. Because the driver
accepts packets from the networking stack regardless of the physical link
state, sending packets while the link is disconnected appears to cause an
unbounded memory leak.

>  	if (rc)
>  		goto err;
>  
> @@ -267,12 +317,29 @@ static netdev_tx_t ntb_netdev_start_xmit(struct sk_buff *skb,
>  
>  	return NETDEV_TX_OK;
>  
> +drop:
> +	dev_kfree_skb_any(skb);
> +	ndev->stats.tx_dropped++;
> +	ndev->stats.tx_errors++;
> +	return NETDEV_TX_OK;
> +
>  err:
>  	ndev->stats.tx_dropped++;
>  	ndev->stats.tx_errors++;
>  	return NETDEV_TX_BUSY;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4