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

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Race condition during link teardown leaves `peer_csum` flag incorrectly enabled for subsequent connections, leading to dropped packets.
--

--- Patch [2]: [PATCH net-next 2/2] net: ntb_netdev: Preserve CHECKSUM_PARTIAL across NTB ---
commit 361f3725205633c095fdcdcc524204c1da6ec87e
Author: Koichiro Den <[email protected]>
Subject: net: ntb_netdev: Preserve CHECKSUM_PARTIAL across NTB

This commit introduces CHECKSUM_PARTIAL metadata across the NTB link to
improve throughput and reduce CPU usage on trusted PCIe fabrics. It
advertises checksum support in every frame and falls back to software
checksumming until the peer capability is observed. TX and RX checksum
features remain disabled by default.

> diff --git a/drivers/net/ntb_netdev.c b/drivers/net/ntb_netdev.c
> index 5c7fe6883cb93..b9a78ff695c87 100644
> --- a/drivers/net/ntb_netdev.c
> +++ b/drivers/net/ntb_netdev.c
[ ... ]
> @@ -108,6 +125,8 @@ static void ntb_netdev_event_handler(void *data, int link_is_up)
>  	struct net_device *ndev;
>  
>  	ndev = dev->ndev;
> +	if (!link_is_up)
> +		WRITE_ONCE(dev->peer_csum, false);
>  
>  	netdev_dbg(ndev, "Event %x, Link %x, qp %u\n", link_is_up,
>  		   ntb_transport_link_query(q->qp), q->qid);
[ ... ]
> @@ -151,8 +170,21 @@ static void ntb_netdev_rx_handler(struct ntb_transport_qp *qp, void *qp_data,
>  	}
>  
>  	skb_put(skb, len);
> +	if (meta & NTB_NETDEV_META_CAP_CSUM)
> +		WRITE_ONCE(dev->peer_csum, true);

[Severity: High]
Could a race condition occur during link teardown that leaves peer_csum
incorrectly enabled?

If the NTB link goes down, ntb_netdev_event_handler() clears dev->peer_csum.
However, if the RX tasklet is concurrently processing a straggler packet
sent before the link went down, ntb_netdev_rx_handler() will execute and
unconditionally set dev->peer_csum to true when it sees the capability bit.

When a new connection is established, dev->peer_csum would not be reset. If
the new peer is an older driver that does not support checksum offload, the
local driver might incorrectly skip software checksumming and send packets
with CHECKSUM_PARTIAL. Would this cause the older peer's networking stack to
see invalid checksums and silently drop the packets?

> +
> +	if (meta & NTB_NETDEV_META_CSUM) {
> +		u16 csum_start = (meta >> NTB_NETDEV_META_CSUM_START_SHIFT) & U16_MAX;
> +		u16 csum_offset = meta & NTB_NETDEV_META_CSUM_OFFSET_MASK;
> +
> +		if (!skb_partial_csum_set(skb, csum_start, csum_offset))
> +			goto rx_drop;

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