[PATCH net 0/2] net: ntb_netdev: Fix RX statistics accounting

[PATCH net 0/2] net: ntb_netdev: Fix RX statistics accounting

[Koichiro Den]

Hi,

This series addresses Jakub's comment on ntb_netdev RX statistics
accounting:
https://lore.kernel.org/r/20260818092938.4121c220@kernel.org/

It fixes the double counting and also the related packet/byte accounting
on RX refill failure.

Notes:
- No ordering dependency on the TX completion/error handling series,
  which is currently awaiting review. Either series can go first.
  https://patchwork.kernel.org/project/netdevbpf/cover/20260817053519.4135287-1-den@valinux.co.jp/
- Once both series land, I plan to post one follow-up patch to net for
  the multi-queue stats race introduced by commit 24d9e73c7e00 ("net:
  ntb_netdev: Support ethtool channels for multi-queue"). My plan is to
  move packet/byte/drop counters to core-managed per-CPU stats and use
  DEV_STATS_INC() for the remaining slow-path counters. IMO this is
  lower severity and better done after the TX/RX changes settle.

Best regards,
Koichiro


Koichiro Den (2):
  net: ntb_netdev: Avoid double-accounting netif_rx() drops
  net: ntb_netdev: Count packets dropped on RX refill failure

 drivers/net/ntb_netdev.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

base-commit: 564973a259ec76f2dad0853420e7034cc43994c4
-- 
2.51.0

[PATCH net 1/2] net: ntb_netdev: Avoid double-accounting netif_rx() drops

[Koichiro Den]

netif_rx() already accounts packets it drops in the core rx_dropped
counter. ntb_netdev counts them again as both errors and drops.

Leave netif_rx() drops to the core. Count the packet and bytes
unconditionally since it was received successfully by the driver.

Fixes: 548c237c0a99 ("net: Add support for NTB virtual ethernet device")
Cc: stable@vger.kernel.org
Suggested-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Koichiro Den <den@valinux.co.jp>
---
 drivers/net/ntb_netdev.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ntb_netdev.c b/drivers/net/ntb_netdev.c
index 029a4a532a10..4e53b00f016b 100644
--- a/drivers/net/ntb_netdev.c
+++ b/drivers/net/ntb_netdev.c
@@ -155,13 +155,9 @@ static void ntb_netdev_rx_handler(struct ntb_transport_qp *qp, void *qp_data,
 	skb->ip_summed = CHECKSUM_NONE;
 	skb_record_rx_queue(skb, q->qid);
 
-	if (netif_rx(skb) == NET_RX_DROP) {
-		ndev->stats.rx_errors++;
-		ndev->stats.rx_dropped++;
-	} else {
-		ndev->stats.rx_packets++;
-		ndev->stats.rx_bytes += len;
-	}
+	netif_rx(skb);
+	ndev->stats.rx_packets++;
+	ndev->stats.rx_bytes += len;
 
 	skb = new_skb;
 
-- 
2.51.0

[PATCH net 2/2] net: ntb_netdev: Count packets dropped on RX refill failure

[Koichiro Den]

When replacement skb allocation fails, ntb_netdev drops a packet that
was received successfully and requeues the original buffer. The drop is
counted, but rx_packets and rx_bytes are not.

Count every good packet before allocating its replacement.

Fixes: d2121faf133a ("NTB: ntb_netdev: Preserve RX queue depth on allocation failure")
Cc: stable@vger.kernel.org
Signed-off-by: Koichiro Den <den@valinux.co.jp>
---
 drivers/net/ntb_netdev.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ntb_netdev.c b/drivers/net/ntb_netdev.c
index 4e53b00f016b..5bb0c8be00f9 100644
--- a/drivers/net/ntb_netdev.c
+++ b/drivers/net/ntb_netdev.c
@@ -144,6 +144,9 @@ static void ntb_netdev_rx_handler(struct ntb_transport_qp *qp, void *qp_data,
 		goto enqueue_again;
 	}
 
+	ndev->stats.rx_packets++;
+	ndev->stats.rx_bytes += len;
+
 	new_skb = netdev_alloc_skb(ndev, ndev->mtu + ETH_HLEN);
 	if (!new_skb) {
 		ndev->stats.rx_dropped++;
@@ -156,8 +159,6 @@ static void ntb_netdev_rx_handler(struct ntb_transport_qp *qp, void *qp_data,
 	skb_record_rx_queue(skb, q->qid);
 
 	netif_rx(skb);
-	ndev->stats.rx_packets++;
-	ndev->stats.rx_bytes += len;
 
 	skb = new_skb;
 
-- 
2.51.0

Re: [PATCH net 1/2] net: ntb_netdev: Avoid double-accounting netif_rx() drops

[sashiko-bot]

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

Pre-existing issues:
- [Critical] Time-of-Check to Time-of-Use (TOCTOU) vulnerability in NTB receive processing allows a malicious remote peer to cause a massive kernel heap buffer overflow.
--

--- Patch [1]: [PATCH net 1/2] net: ntb_netdev: Avoid double-accounting netif_rx() drops ---
commit 002045303db9fc237bf287acbb8f01a9873c837d
Author: Koichiro Den <den@valinux.co.jp>

net: ntb_netdev: Avoid double-accounting netif_rx() drops

netif_rx() already accounts packets it drops in the core rx_dropped
counter. ntb_netdev counts them again as both errors and drops.
Leave netif_rx() drops to the core. Count the packet and bytes
unconditionally since it was received successfully by the driver.

> diff --git a/drivers/net/ntb_netdev.c b/drivers/net/ntb_netdev.c
> index 029a4a532a104..4e53b00f016b0 100644
> --- a/drivers/net/ntb_netdev.c
> +++ b/drivers/net/ntb_netdev.c
> @@ -155,13 +155,9 @@ static void ntb_netdev_rx_handler(struct ntb_transport_qp *qp, void *qp_data,
>  	skb->ip_summed = CHECKSUM_NONE;
>  	skb_record_rx_queue(skb, q->qid);
>  
> -	if (netif_rx(skb) == NET_RX_DROP) {
> -		ndev->stats.rx_errors++;
> -		ndev->stats.rx_dropped++;
> -	} else {
> -		ndev->stats.rx_packets++;
> -		ndev->stats.rx_bytes += len;
> -	}
> +	netif_rx(skb);
> +	ndev->stats.rx_packets++;
> +	ndev->stats.rx_bytes += len;

[Severity: Critical]
This is a pre-existing issue, but does this expose a time-of-check to
time-of-use issue during ntb receive processing?

Looking at ntb_process_rxc() in drivers/ntb/ntb_transport.c, hdr directly
accesses the shared memory window which the remote peer can write to via
pcie. A bounds check is performed, but then hdr->len is re-read directly
from the shared memory:

drivers/ntb/ntb_transport.c:ntb_process_rxc() {
	if (hdr->len > entry->len) {
		...
	} else {
		...
		entry->len = hdr->len;

		ntb_async_rx(entry, offset);
	}
}

Can the remote peer modify hdr->len to a massive value after the bounds
check passes but before the second read?

If so, this bypassed length is then passed to ntb_async_rx() and
subsequently to ntb_memcpy_rx():

drivers/ntb/ntb_transport.c:ntb_memcpy_rx() {
	void *buf = entry->buf;
	size_t len = entry->len;

	memcpy(buf, offset, len);
}

Could this cause memcpy() to overflow the skb data buffer? Should the
payload header values be copied to local variables using READ_ONCE() before
validation to prevent this?

>  
>  	skb = new_skb;
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260819172539.1450821-1-den@valinux.co.jp?part=1