Re: [PATCH net-next 1/2] NTB: ntb_transport: Add per-payload client metadata

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

Pre-existing issues:
- [High] Oversized packets trigger a memory leak by passing NULL to the tx_handler, failing to free the SKB.
- [High] The RX descriptor polling path lacks a dma_rmb() barrier between checking the completion flag and reading the descriptor data.
- [High] The driver writes hardware descriptor fields using little-endian I/O accessors but reads them as native integers, breaking functionality on big-endian architectures.
--

--- Patch [1]: [PATCH net-next 1/2] NTB: ntb_transport: Add per-payload client metadata ---
commit 581d6bed46f9b62bccae9a18ddae3275987b5c5b
Author: Koichiro Den <[email protected]>

NTB: ntb_transport: Add per-payload client metadata

This patch adds support for 24-bit per-payload client metadata in the NTB
transport payload header. It updates the transport enqueue and receive
handlers to pass this metadata without bumping the protocol version.

> diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
> index f59f926d4bfaa..ea97460c118ad 100644
> --- a/drivers/ntb/ntb_transport.c
> +++ b/drivers/ntb/ntb_transport.c

[ ... ]

> @@ -1647,6 +1653,7 @@ static int ntb_process_rxc(struct ntb_transport_qp *qp)
>  
>  	entry->rx_hdr = hdr;
>  	entry->rx_index = qp->rx_index;
> +	entry->flags = hdr->flags & DESC_META_MASK;
>  
>  	if (hdr->len > entry->len) {
>  		dev_dbg(&qp->ndev->pdev->dev,

[Severity: High]
This is a pre-existing issue, but I noticed there may be a missing memory
barrier in ntb_process_rxc() when reading the descriptor.

The hardware writes the payload and the descriptor to host memory. The CPU
polls hdr->flags for DESC_DONE_FLAG, and then immediately reads fields like
hdr->ver and hdr->len.

Without a dma_rmb() after checking the flag, could weakly-ordered CPUs
speculatively fetch the descriptor fields or payload before the DMA transfer
actually commits to memory?

[Severity: High]
This isn't a bug introduced by this patch, but there appears to be an
endianness mismatch when reading these descriptor fields.

On the TX side, descriptor fields seem to be written using iowrite32(),
which implicitly performs a little-endian byte swap on big-endian CPUs.

However, in ntb_process_rxc(), they are accessed directly as native
integers (e.g., hdr->len). Won't this evaluate the little-endian bytes
incorrectly on big-endian architectures, causing version mismatches and
incorrect lengths?

[ ... ]

> @@ -2361,7 +2369,7 @@ int ntb_transport_tx_enqueue(struct ntb_transport_qp *qp, void *cb, void *data,
>  	entry->cb_data = cb;
>  	entry->buf = data;
>  	entry->len = len;
> -	entry->flags = 0;
> +	entry->flags = meta << DESC_META_SHIFT;
>  	entry->errors = 0;
>  	entry->tx_index = 0;
>  

[Severity: High]
This is a pre-existing issue, but I noticed a potential memory leak during
TX when an oversized packet is enqueued.

If ntb_process_tx() detects a length overflow, it aborts transmission and
calls the transmit handler. However, it passes NULL instead of the original
buffer context to the handler:

ntb_process_tx()
    if (qp->tx_handler)
        qp->tx_handler(qp, qp->cb_data, NULL, -EIO);

Since ntb_netdev_tx_handler() explicitly checks if the skb is NULL and
returns immediately without freeing it, won't this leak the SKB if the
MTU check is bypassed or misconfigured?

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