Re: [PATCH net-next v2 3/4] 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] Memory leak of SKB when NTB queue link is down during transmit.
- [High] Memory leak of the transmitted SKB when the payload exceeds the maximum frame size.
- [High] Memory leak of receive buffers (SKBs) if completed when the client is not ready.
--

--- Patch [3]: [PATCH net-next v2 3/4] NTB: ntb_transport: Add per-payload client metadata ---
commit 21a42de29f1891a17d65f9eb283c9a3b65134a03
Author: Koichiro Den <[email protected]>

NTB: ntb_transport: Add per-payload client metadata

ntb_transport currently carries only payload bytes, with no way for clients
to associate metadata with an individual payload. This patch adapts the
interface to pass metadata in the payload header.

> diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
> index 967a5ad381646..70be06a42201f 100644
> --- a/drivers/ntb/ntb_transport.c
> +++ b/drivers/ntb/ntb_transport.c

[ ... ]

> @@ -1436,6 +1440,7 @@ static void ntb_complete_rxc(struct ntb_transport_qp *qp)
>  	struct ntb_queue_entry *entry;
>  	void *cb_data;
>  	unsigned int len;
> +	unsigned int meta;
>  	unsigned long irqflags;
>  
>  	spin_lock_irqsave(&qp->ntb_rx_q_lock, irqflags);
> @@ -1451,13 +1456,14 @@ static void ntb_complete_rxc(struct ntb_transport_qp *qp)
>  
>  		cb_data = entry->cb_data;
>  		len = entry->len;
> +		meta = entry->flags >> DESC_META_SHIFT;
>  
>  		list_move_tail(&entry->entry, &qp->rx_free_q);
>  
>  		spin_unlock_irqrestore(&qp->ntb_rx_q_lock, irqflags);
>  
>  		if (qp->rx_handler && qp->client_ready)
> -			qp->rx_handler(qp, qp->cb_data, cb_data, len);
> +			qp->rx_handler(qp, qp->cb_data, cb_data, len, meta);

[Severity: High]
This is a pre-existing issue, but does this lead to a memory leak of receive
buffers (SKBs) if an RX packet completes while the client is not ready?

If ntb_complete_rxc() runs during a link down event (!qp->client_ready), the
function skips invoking the client's rx_handler. However, it moves the entry
to qp->rx_free_q without releasing the client-provided buffer stored in
cb_data.

During teardown, the client uses ntb_transport_rx_remove() to recover unused
buffers, but that function only checks qp->rx_pend_q. Are the buffers left
stranded in rx_free_q permanently lost?

>  
>  		spin_lock_irqsave(&qp->ntb_rx_q_lock, irqflags);
>  	}

[ ... ]

> @@ -2349,12 +2357,12 @@ EXPORT_SYMBOL_GPL(ntb_transport_rx_enqueue);
>   * RETURNS: An appropriate -ERRNO error value on error, or zero for success.
>   */
>  int ntb_transport_tx_enqueue(struct ntb_transport_qp *qp, void *cb, void *data,
> -			     unsigned int len)
> +			     unsigned int len, unsigned int meta)
>  {
>  	struct ntb_queue_entry *entry;
>  	int rc;
>  
> -	if (!qp || !len)
> +	if (!qp || !len || meta > NTB_TRANSPORT_MAX_META)
>  		return -EINVAL;
>  
>  	/* If the qp link is down already, just ignore. */
>  	if (!qp->link_is_up)
>  		return 0;

[Severity: High]
This is a pre-existing issue, but does returning 0 instead of an error code
cause a memory leak of the SKB when the link is down?

If ntb_transport_tx_enqueue() returns 0, the caller (such as
ntb_netdev_start_xmit()) receives a success code and assumes the buffer
was successfully enqueued, returning NETDEV_TX_OK without calling
dev_kfree_skb_any().

Should this return an error like -ENETDOWN so the caller knows to drop the
packet?

>  
>  	entry = ntb_list_rm(&qp->ntb_tx_free_q_lock, &qp->tx_free_q);
>  	if (!entry) {
> @@ -2370,7 +2378,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;
>  
>  	rc = ntb_process_tx(qp, entry);

[Severity: High]
This is a pre-existing issue, but can this lead to an SKB memory leak when
the payload exceeds the maximum frame size?

When a packet is larger than the NTB transport MTU, ntb_process_tx() handles
the overflow like this:

drivers/ntb/ntb_transport.c:ntb_process_tx() {
	if (entry->len > qp->tx_max_frame - sizeof(struct ntb_payload_header)) {
		if (qp->tx_handler)
			qp->tx_handler(qp, qp->cb_data, NULL, -EIO);

		ntb_list_add(&qp->ntb_tx_free_q_lock, &entry->entry,
			     &qp->tx_free_q);
		return 0;
	}
	...
}

Because ntb_process_tx() passes NULL instead of entry->cb_data to the 
tx_handler, the client (ntb_netdev_tx_handler()) cannot recover and free 
the SKB. ntb_process_tx() then returns 0, so the original caller also thinks 
the packet was enqueued and does not free it.

>  	if (rc)
>  		ntb_list_add(&qp->ntb_tx_free_q_lock, &entry->entry,
>  			     &qp->tx_free_q);
>  
>  	return rc;
>  }

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