[PATCH net 0/4] net: ntb_netdev: Fix TX completion and error handling

[PATCH net 0/4] net: ntb_netdev: Fix TX completion and error handling

[Koichiro Den]

Hi,

This small series fixes several TX buffer ownership and queue handling
bugs in ntb_netdev and ntb_transport.

Patch 4 first appeared in my "NTB: Add direct TX/RX using PCI endpoint
DMA" series. Sashiko later reported the same pre-existing leak while
reviewing another series, so I moved the fix here. See:
https://lore.kernel.org/r/20260815032932.151F11F000E9@smtp.kernel.org/

The ntb_transport fixes affect buffer ownership and queue handling in
ntb_netdev, the only in-tree ntb_transport_client implementation, so the
patches need to go in together. I am targeting the net tree for the
series.

Dave and other NTB maintainers, I would appreciate your review of the
ntb_transport-specific changes.

Best regards,
Koichiro


Koichiro Den (4):
  NTB: ntb_transport: Recycle TX entries before client callbacks
  net: ntb_netdev: Fix TX busy and drop handling
  NTB: ntb_transport: Fail TX enqueue when the QP link is down
  NTB: ntb_transport: Reject oversized TX buffers

 drivers/net/ntb_netdev.c    | 27 ++++++++++++++++++--------
 drivers/ntb/ntb_transport.c | 38 ++++++++++++++++++-------------------
 2 files changed, 37 insertions(+), 28 deletions(-)

base-commit: 24ef02f934eeb48830cff6b739abc3c62b1d107b
-- 
2.51.0

[PATCH net 1/4] NTB: ntb_transport: Recycle TX entries before client callbacks

[Koichiro Den]

ntb_tx_copy_callback() invokes the client callback before returning the
entry to tx_free_q. The callback may wake a stopped client queue, only
for the next enqueue to find no local entry and return -EBUSY. The window
is narrow, but the retry is unnecessary.

Save the callback data and length, then return the entry to tx_free_q
before invoking the client. A completion callback then means both the
client buffer and transport entry are ready for reuse.

Fixes: fce8a7bb5b4b ("PCI-Express Non-Transparent Bridge Support")
Cc: stable@vger.kernel.org
Signed-off-by: Koichiro Den <den@valinux.co.jp>
---
 drivers/ntb/ntb_transport.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
index f59f926d4bfa..e92b96f4afad 100644
--- a/drivers/ntb/ntb_transport.c
+++ b/drivers/ntb/ntb_transport.c
@@ -1719,9 +1719,16 @@ static void ntb_transport_rxc_db(unsigned long data)
 static void ntb_tx_copy_callback(void *data,
 				 const struct dmaengine_result *res)
 {
+	struct ntb_payload_header __iomem *hdr;
 	struct ntb_queue_entry *entry = data;
-	struct ntb_transport_qp *qp = entry->qp;
-	struct ntb_payload_header __iomem *hdr = entry->tx_hdr;
+	struct ntb_transport_qp *qp;
+	unsigned int len;
+	void *cb_data;
+
+	qp = entry->qp;
+	hdr = entry->tx_hdr;
+	cb_data = entry->cb_data;
+	len = entry->len;
 
 	/* we need to check DMA results if we are using DMA */
 	if (res) {
@@ -1768,15 +1775,13 @@ static void ntb_tx_copy_callback(void *data,
 	 * "link down" or similar.  Since no payload is being sent in these
 	 * cases, there is nothing to add to the completion queue.
 	 */
-	if (entry->len > 0) {
-		qp->tx_bytes += entry->len;
-
-		if (qp->tx_handler)
-			qp->tx_handler(qp, qp->cb_data, entry->cb_data,
-				       entry->len);
-	}
+	if (len > 0)
+		qp->tx_bytes += len;
 
 	ntb_list_add(&qp->ntb_tx_free_q_lock, &entry->entry, &qp->tx_free_q);
+
+	if (len > 0 && qp->tx_handler)
+		qp->tx_handler(qp, qp->cb_data, cb_data, len);
 }
 
 static void ntb_memcpy_tx_on_stack(struct ntb_queue_entry *entry, void __iomem *offset)
-- 
2.51.0

[PATCH net 2/4] net: ntb_netdev: Fix TX busy and drop handling

[Koichiro Den]

Currently, ntb_netdev returns NETDEV_TX_BUSY for every enqueue error. It
also increments the drop and error counters while leaving the skb owned
by the qdisc, and may return BUSY with the subqueue still awake.
Retrying a permanent error cannot succeed either.

The unconditional BUSY return and premature accounting date back to the
initial driver. The error-path queue stop was later removed without
changing that return value. The current flow-control code includes a
resource check, but ntb_netdev does not honor its result before enqueue.

Honor the resource check before enqueue. For -EAGAIN and -EBUSY, stop
the subqueue, arm the existing reaper timer, and return BUSY without
touching the skb. For other errors, free the skb, increment tx_dropped,
and return NETDEV_TX_OK.

Fixes: 548c237c0a99 ("net: Add support for NTB virtual ethernet device")
Fixes: d723485cb4ca ("ntb_netdev: remove tx timeout")
Fixes: e74bfeedad08 ("NTB: Add flow control to the ntb_netdev")
Cc: stable@vger.kernel.org
Signed-off-by: Koichiro Den <den@valinux.co.jp>
---
 drivers/net/ntb_netdev.c | 27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ntb_netdev.c b/drivers/net/ntb_netdev.c
index 029a4a532a10..02b35cf53a62 100644
--- a/drivers/net/ntb_netdev.c
+++ b/drivers/net/ntb_netdev.c
@@ -199,8 +199,10 @@ static int __ntb_netdev_maybe_stop_tx(struct net_device *netdev,
 static int ntb_netdev_maybe_stop_tx(struct net_device *ndev,
 				    struct ntb_netdev_queue *q, int size)
 {
-	if (__netif_subqueue_stopped(ndev, q->qid) ||
-	    (ntb_transport_tx_free_entry(q->qp) >= size))
+	if (__netif_subqueue_stopped(ndev, q->qid))
+		return -EBUSY;
+
+	if (ntb_transport_tx_free_entry(q->qp) >= size)
 		return 0;
 
 	return __ntb_netdev_maybe_stop_tx(ndev, q, size);
@@ -256,21 +258,30 @@ static netdev_tx_t ntb_netdev_start_xmit(struct sk_buff *skb,
 
 	q = &dev->queues[qid];
 
-	ntb_netdev_maybe_stop_tx(ndev, q, tx_stop);
+	if (unlikely(ntb_netdev_maybe_stop_tx(ndev, q, tx_stop)))
+		return NETDEV_TX_BUSY;
 
 	rc = ntb_transport_tx_enqueue(q->qp, skb, skb->data, skb->len);
-	if (rc)
-		goto err;
+	if (rc) {
+		if (rc == -EAGAIN || rc == -EBUSY) {
+			netif_stop_subqueue(ndev, q->qid);
+			mod_timer(&q->tx_timer,
+				  jiffies + usecs_to_jiffies(tx_time));
+			return NETDEV_TX_BUSY;
+		}
+
+		goto drop;
+	}
 
 	/* check for next submit */
 	ntb_netdev_maybe_stop_tx(ndev, q, tx_stop);
 
 	return NETDEV_TX_OK;
 
-err:
+drop:
+	dev_kfree_skb_any(skb);
 	ndev->stats.tx_dropped++;
-	ndev->stats.tx_errors++;
-	return NETDEV_TX_BUSY;
+	return NETDEV_TX_OK;
 }
 
 static void ntb_netdev_tx_timer(struct timer_list *t)
-- 
2.51.0

[PATCH net 3/4] NTB: ntb_transport: Fail TX enqueue when the QP link is down

[Koichiro Den]

Commit f195a1a6fe41 ("ntb: Drop packets when qp link is down") meant to
make ntb_transport_tx_enqueue() drop packets submitted while the QP link
is down, but it only returns 0 without consuming the packet. Zero means
success by this function's contract, so ntb_netdev reports NETDEV_TX_OK
and forgets the skb: nothing queued it, nothing frees it, and it leaks,
one skb for every transmit racing a link-down.

Return -ENOLINK instead, restoring the contract that a non-zero return
leaves the buffer owned by the caller. With the preceding patch,
ntb_netdev frees the skb on non-retryable enqueue failures and returns
NETDEV_TX_OK, so a packet racing with link-down is dropped without leaking
or entering a busy retry loop.

Fixes: f195a1a6fe41 ("ntb: Drop packets when qp link is down")
Cc: stable@vger.kernel.org
Signed-off-by: Koichiro Den <den@valinux.co.jp>
---
 drivers/ntb/ntb_transport.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
index e92b96f4afad..4bdd81edcb87 100644
--- a/drivers/ntb/ntb_transport.c
+++ b/drivers/ntb/ntb_transport.c
@@ -2353,9 +2353,8 @@ int ntb_transport_tx_enqueue(struct ntb_transport_qp *qp, void *cb, void *data,
 	if (!qp || !len)
 		return -EINVAL;
 
-	/* If the qp link is down already, just ignore. */
 	if (!qp->link_is_up)
-		return 0;
+		return -ENOLINK;
 
 	entry = ntb_list_rm(&qp->ntb_tx_free_q_lock, &qp->tx_free_q);
 	if (!entry) {
-- 
2.51.0

[PATCH net 4/4] NTB: ntb_transport: Reject oversized TX buffers

[Koichiro Den]

ntb_process_tx() handles an oversized buffer by calling tx_handler()
with a NULL data pointer and returning success. ntb_netdev therefore
neither frees the skb in its completion callback nor takes its enqueue
error path, leaking it.

Reject oversized buffers in ntb_transport_tx_enqueue() before acquiring
a queue entry and return -EMSGSIZE. The caller retains ownership of the
buffer, and the preceding netdev patch frees the skb when enqueue
returns this permanent error.

Fixes: fce8a7bb5b4b ("PCI-Express Non-Transparent Bridge Support")
Cc: stable@vger.kernel.org
Signed-off-by: Koichiro Den <den@valinux.co.jp>
---
Previously posted at:
https://lore.kernel.org/r/20260810165136.2292436-3-den@valinux.co.jp/

I moved the fix here because it addresses a pre-existing issue
independently of that feature series. I also moved the size check before
the tx_free_q lookup so -EBUSY cannot mask -EMSGSIZE.
---
 drivers/ntb/ntb_transport.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
index 4bdd81edcb87..f9caa1a653c5 100644
--- a/drivers/ntb/ntb_transport.c
+++ b/drivers/ntb/ntb_transport.c
@@ -1955,15 +1955,6 @@ static int ntb_process_tx(struct ntb_transport_qp *qp,
 		return -EAGAIN;
 	}
 
-	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;
-	}
-
 	ntb_async_tx(qp, entry);
 
 	qp->tx_pkts++;
@@ -2356,6 +2347,9 @@ int ntb_transport_tx_enqueue(struct ntb_transport_qp *qp, void *cb, void *data,
 	if (!qp->link_is_up)
 		return -ENOLINK;
 
+	if (len > qp->tx_max_frame - sizeof(struct ntb_payload_header))
+		return -EMSGSIZE;
+
 	entry = ntb_list_rm(&qp->ntb_tx_free_q_lock, &qp->tx_free_q);
 	if (!entry) {
 		qp->tx_err_no_buf++;
-- 
2.51.0

Re: [PATCH net 1/4] NTB: ntb_transport: Recycle TX entries before client callbacks

[Dave Jiang]

On 8/16/26 10:35 PM, Koichiro Den wrote:
> ntb_tx_copy_callback() invokes the client callback before returning the
> entry to tx_free_q. The callback may wake a stopped client queue, only
> for the next enqueue to find no local entry and return -EBUSY. The window
> is narrow, but the retry is unnecessary.
> 
> Save the callback data and length, then return the entry to tx_free_q
> before invoking the client. A completion callback then means both the
> client buffer and transport entry are ready for reuse.
> 
> Fixes: fce8a7bb5b4b ("PCI-Express Non-Transparent Bridge Support")
> Cc: stable@vger.kernel.org
> Signed-off-by: Koichiro Den <den@valinux.co.jp>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>

> ---
>  drivers/ntb/ntb_transport.c | 23 ++++++++++++++---------
>  1 file changed, 14 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
> index f59f926d4bfa..e92b96f4afad 100644
> --- a/drivers/ntb/ntb_transport.c
> +++ b/drivers/ntb/ntb_transport.c
> @@ -1719,9 +1719,16 @@ static void ntb_transport_rxc_db(unsigned long data)
>  static void ntb_tx_copy_callback(void *data,
>  				 const struct dmaengine_result *res)
>  {
> +	struct ntb_payload_header __iomem *hdr;
>  	struct ntb_queue_entry *entry = data;
> -	struct ntb_transport_qp *qp = entry->qp;
> -	struct ntb_payload_header __iomem *hdr = entry->tx_hdr;
> +	struct ntb_transport_qp *qp;
> +	unsigned int len;
> +	void *cb_data;
> +
> +	qp = entry->qp;
> +	hdr = entry->tx_hdr;
> +	cb_data = entry->cb_data;
> +	len = entry->len;
>  
>  	/* we need to check DMA results if we are using DMA */
>  	if (res) {
> @@ -1768,15 +1775,13 @@ static void ntb_tx_copy_callback(void *data,
>  	 * "link down" or similar.  Since no payload is being sent in these
>  	 * cases, there is nothing to add to the completion queue.
>  	 */
> -	if (entry->len > 0) {
> -		qp->tx_bytes += entry->len;
> -
> -		if (qp->tx_handler)
> -			qp->tx_handler(qp, qp->cb_data, entry->cb_data,
> -				       entry->len);
> -	}
> +	if (len > 0)
> +		qp->tx_bytes += len;
>  
>  	ntb_list_add(&qp->ntb_tx_free_q_lock, &entry->entry, &qp->tx_free_q);
> +
> +	if (len > 0 && qp->tx_handler)
> +		qp->tx_handler(qp, qp->cb_data, cb_data, len);
>  }
>  
>  static void ntb_memcpy_tx_on_stack(struct ntb_queue_entry *entry, void __iomem *offset)

Re: [PATCH net 2/4] net: ntb_netdev: Fix TX busy and drop handling

[Dave Jiang]

On 8/16/26 10:35 PM, Koichiro Den wrote:
> Currently, ntb_netdev returns NETDEV_TX_BUSY for every enqueue error. It
> also increments the drop and error counters while leaving the skb owned
> by the qdisc, and may return BUSY with the subqueue still awake.
> Retrying a permanent error cannot succeed either.
> 
> The unconditional BUSY return and premature accounting date back to the
> initial driver. The error-path queue stop was later removed without
> changing that return value. The current flow-control code includes a
> resource check, but ntb_netdev does not honor its result before enqueue.
> 
> Honor the resource check before enqueue. For -EAGAIN and -EBUSY, stop
> the subqueue, arm the existing reaper timer, and return BUSY without
> touching the skb. For other errors, free the skb, increment tx_dropped,
> and return NETDEV_TX_OK.
> 
> Fixes: 548c237c0a99 ("net: Add support for NTB virtual ethernet device")
> Fixes: d723485cb4ca ("ntb_netdev: remove tx timeout")

This is probably the only Fixes tag we need.

> Fixes: e74bfeedad08 ("NTB: Add flow control to the ntb_netdev")
> Cc: stable@vger.kernel.org> Signed-off-by: Koichiro Den <den@valinux.co.jp>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>

> ---
>  drivers/net/ntb_netdev.c | 27 +++++++++++++++++++--------
>  1 file changed, 19 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/ntb_netdev.c b/drivers/net/ntb_netdev.c
> index 029a4a532a10..02b35cf53a62 100644
> --- a/drivers/net/ntb_netdev.c
> +++ b/drivers/net/ntb_netdev.c
> @@ -199,8 +199,10 @@ static int __ntb_netdev_maybe_stop_tx(struct net_device *netdev,
>  static int ntb_netdev_maybe_stop_tx(struct net_device *ndev,
>  				    struct ntb_netdev_queue *q, int size)
>  {
> -	if (__netif_subqueue_stopped(ndev, q->qid) ||
> -	    (ntb_transport_tx_free_entry(q->qp) >= size))
> +	if (__netif_subqueue_stopped(ndev, q->qid))
> +		return -EBUSY;
> +
> +	if (ntb_transport_tx_free_entry(q->qp) >= size)
>  		return 0;
>  
>  	return __ntb_netdev_maybe_stop_tx(ndev, q, size);
> @@ -256,21 +258,30 @@ static netdev_tx_t ntb_netdev_start_xmit(struct sk_buff *skb,
>  
>  	q = &dev->queues[qid];
>  
> -	ntb_netdev_maybe_stop_tx(ndev, q, tx_stop);
> +	if (unlikely(ntb_netdev_maybe_stop_tx(ndev, q, tx_stop)))
> +		return NETDEV_TX_BUSY;
>  
>  	rc = ntb_transport_tx_enqueue(q->qp, skb, skb->data, skb->len);
> -	if (rc)
> -		goto err;
> +	if (rc) {
> +		if (rc == -EAGAIN || rc == -EBUSY) {
> +			netif_stop_subqueue(ndev, q->qid);
> +			mod_timer(&q->tx_timer,
> +				  jiffies + usecs_to_jiffies(tx_time));
> +			return NETDEV_TX_BUSY;
> +		}
> +
> +		goto drop;
> +	}
>  
>  	/* check for next submit */
>  	ntb_netdev_maybe_stop_tx(ndev, q, tx_stop);
>  
>  	return NETDEV_TX_OK;
>  
> -err:
> +drop:
> +	dev_kfree_skb_any(skb);
>  	ndev->stats.tx_dropped++;
> -	ndev->stats.tx_errors++;
> -	return NETDEV_TX_BUSY;
> +	return NETDEV_TX_OK;
>  }
>  
>  static void ntb_netdev_tx_timer(struct timer_list *t)

Re: [PATCH net 3/4] NTB: ntb_transport: Fail TX enqueue when the QP link is down

[Dave Jiang]

On 8/16/26 10:35 PM, Koichiro Den wrote:
> Commit f195a1a6fe41 ("ntb: Drop packets when qp link is down") meant to
> make ntb_transport_tx_enqueue() drop packets submitted while the QP link
> is down, but it only returns 0 without consuming the packet. Zero means
> success by this function's contract, so ntb_netdev reports NETDEV_TX_OK
> and forgets the skb: nothing queued it, nothing frees it, and it leaks,
> one skb for every transmit racing a link-down.
> 
> Return -ENOLINK instead, restoring the contract that a non-zero return
> leaves the buffer owned by the caller. With the preceding patch,
> ntb_netdev frees the skb on non-retryable enqueue failures and returns
> NETDEV_TX_OK, so a packet racing with link-down is dropped without leaking
> or entering a busy retry loop.
> 
> Fixes: f195a1a6fe41 ("ntb: Drop packets when qp link is down")
> Cc: stable@vger.kernel.org
> Signed-off-by: Koichiro Den <den@valinux.co.jp>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>

> ---
>  drivers/ntb/ntb_transport.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
> index e92b96f4afad..4bdd81edcb87 100644
> --- a/drivers/ntb/ntb_transport.c
> +++ b/drivers/ntb/ntb_transport.c
> @@ -2353,9 +2353,8 @@ int ntb_transport_tx_enqueue(struct ntb_transport_qp *qp, void *cb, void *data,
>  	if (!qp || !len)
>  		return -EINVAL;
>  
> -	/* If the qp link is down already, just ignore. */
>  	if (!qp->link_is_up)
> -		return 0;
> +		return -ENOLINK;
>  
>  	entry = ntb_list_rm(&qp->ntb_tx_free_q_lock, &qp->tx_free_q);
>  	if (!entry) {

Re: [PATCH net 4/4] NTB: ntb_transport: Reject oversized TX buffers

[Dave Jiang]

On 8/16/26 10:35 PM, Koichiro Den wrote:
> ntb_process_tx() handles an oversized buffer by calling tx_handler()
> with a NULL data pointer and returning success. ntb_netdev therefore
> neither frees the skb in its completion callback nor takes its enqueue
> error path, leaking it.
> 
> Reject oversized buffers in ntb_transport_tx_enqueue() before acquiring
> a queue entry and return -EMSGSIZE. The caller retains ownership of the
> buffer, and the preceding netdev patch frees the skb when enqueue
> returns this permanent error.
> 
> Fixes: fce8a7bb5b4b ("PCI-Express Non-Transparent Bridge Support")
> Cc: stable@vger.kernel.org
> Signed-off-by: Koichiro Den <den@valinux.co.jp>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>

> ---
> Previously posted at:
> https://lore.kernel.org/r/20260810165136.2292436-3-den@valinux.co.jp/
> 
> I moved the fix here because it addresses a pre-existing issue
> independently of that feature series. I also moved the size check before
> the tx_free_q lookup so -EBUSY cannot mask -EMSGSIZE.
> ---
>  drivers/ntb/ntb_transport.c | 12 +++---------
>  1 file changed, 3 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
> index 4bdd81edcb87..f9caa1a653c5 100644
> --- a/drivers/ntb/ntb_transport.c
> +++ b/drivers/ntb/ntb_transport.c
> @@ -1955,15 +1955,6 @@ static int ntb_process_tx(struct ntb_transport_qp *qp,
>  		return -EAGAIN;
>  	}
>  
> -	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;
> -	}
> -
>  	ntb_async_tx(qp, entry);
>  
>  	qp->tx_pkts++;
> @@ -2356,6 +2347,9 @@ int ntb_transport_tx_enqueue(struct ntb_transport_qp *qp, void *cb, void *data,
>  	if (!qp->link_is_up)
>  		return -ENOLINK;
>  
> +	if (len > qp->tx_max_frame - sizeof(struct ntb_payload_header))
> +		return -EMSGSIZE;
> +
>  	entry = ntb_list_rm(&qp->ntb_tx_free_q_lock, &qp->tx_free_q);
>  	if (!entry) {
>  		qp->tx_err_no_buf++;