[PATCH net-next 0/2] net: atlantic: convert RX path to page_pool

[PATCH net-next 0/2] net: atlantic: convert RX path to page_pool

[Yangyu Chen]

On systems where the NIC sits behind an IOMMU, the atlantic RX path
cannot reach line rate: every RX buffer is allocated with
dev_alloc_pages() and mapped with dma_map_page(), then unmapped and
freed once the stack has consumed the packet. Every map/unmap is an
IOTLB/pagetable operation, and at 10G rates this dominates the RX
path. On an AMD Strix Halo system with a Thunderbolt-attached QNAP
QNA-T310G1S (MTU 1500, TCP over IPv6, iperf3 -R), RX tops out at
about 2.2 Gbit/s.

An earlier patch [1] worked around this by making the RX page order
tunable via a module parameter, amortizing one map/unmap over eight
pages worth of frames. The review feedback was to convert the driver
to the page_pool API instead of adding a knob. This series does that
conversion.

Patch 1 fixes a long-standing teardown leak first: TX buffers beyond
the budgeted single aq_ring_tx_clean() pass, or still posted past the
frozen hw_head, are stranded when the interface goes down. Today that
is a silent leak; after the conversion a stranded XDP_TX frame would
hold a page_pool fragment reference and turn every such ifdown into a
permanently stalled pool shutdown, so it needs fixing before the
conversion lands.

Patch 2 converts the RX path to page_pool with the fragment API.
Pages are DMA-mapped once when they enter the per-ring pool
(PP_FLAG_DMA_MAP) and stay mapped while they recycle between the
driver and the stack, so steady-state RX performs no IOMMU work.
page_pool_dev_alloc_frag() takes over the sub-page splitting the
driver's hand-rolled "page flip" scheme did based on page_ref_count(),
buffer ownership becomes transfer-based, and the XDP memory model
switches to MEM_TYPE_PAGE_POOL.

Performance, QNA-T310G1S (AQC100) behind Thunderbolt/IOMMU, MTU 1500,
TCP over IPv6, iperf3 -R:

  before:  2.24 Gbit/s
  after:   9.14 Gbit/s

matching what previously required the rxpageorder=3 workaround, but
with order-0 pages and no tunable.

Tested on the same setup: line rate with plain RX and with XDP_PASS;
XDP_TX with a MAC-swap reflector under sustained traffic; repeated
ifdown/ifup and module unload cycles under both plain RX and XDP_TX
load complete without "stalled pool shutdown" warnings or other splats.

[1] https://lore.kernel.org/lkml/tencent_E71C2F71D9631843941A5DF87204D1B5B509@qq.com/

Yangyu Chen (2):
  net: atlantic: free stranded TX buffers on ring deinit
  net: atlantic: convert RX path to page_pool

 drivers/net/ethernet/aquantia/Kconfig         |   1 +
 .../ethernet/aquantia/atlantic/aq_ethtool.c   |   3 -
 .../net/ethernet/aquantia/atlantic/aq_ptp.c   |  20 +-
 .../net/ethernet/aquantia/atlantic/aq_ring.c  | 268 +++++++++---------
 .../net/ethernet/aquantia/atlantic/aq_ring.h  |   7 +-
 .../net/ethernet/aquantia/atlantic/aq_vec.c   |  17 +-
 6 files changed, 172 insertions(+), 144 deletions(-)

-- 
2.47.3

[PATCH net-next 1/2] net: atlantic: free stranded TX buffers on ring deinit

[Yangyu Chen]

aq_vec_deinit() drains the TX rings with a single aq_ring_tx_clean()
call, which frees at most AQ_CFG_TX_CLEAN_BUDGET (256) descriptors and
stops at hw_head, which no longer moves once aq_vec_stop() has stopped
the hardware and NAPI. Completed descriptors beyond the budget and
everything still posted in [hw_head, sw_tail) keep their skb or
xdp_frame when the interface goes down: aq_vec_ring_free() then frees
the buffer ring and the references are lost for good.

Today this is a silent memory leak on every interface down under
TX/XDP_TX load. With the following conversion of the RX path to
page_pool it becomes much more visible: XDP_TX frames carry fragment
references on the RX ring's page_pool, so a single stranded frame
keeps the pool's inflight count above zero forever. page_pool_destroy()
then never completes, the pool is leaked together with its pages, and
"page_pool_release_retry() stalled pool shutdown" is warned every 60
seconds from that point on, on every ifdown, XDP detach or ring resize
under XDP_TX load.

Bring back aq_ring_tx_deinit() as it was before the removal and use it
for teardown again, with one extension: TX rings can hold xdp_frames
nowadays, so release those too. They are returned with
xdp_return_frame() since this runs in process context.

Fixes: eb36bedf28be ("net: aquantia: remove function aq_ring_tx_deinit")
Assisted-by: Claude:claude-fable-5
Signed-off-by: Yangyu Chen <cyy@cyyself.name>
---

Notes:
    Without this fix, converting the RX path to page_pool (next patch)
    turns the stranded XDP_TX frames into leaked page_pool fragments, so
    page_pool_destroy() can never drain and the shutdown stalls forever.
    
    Reproduced on an AQC100 with this patch dropped from the series (i.e.
    page_pool applied without the tx-deinit fix):
    
      # reflect received frames back out through XDP_TX
      xdp-bench tx enp99s0                    # or any trivial XDP_TX prog
      # from a peer on the same link, flood RX so frames are in flight, then
      ip link set enp99s0 down
    
    The pool is destroyed with frames still stranded on the TX ring, and
    page_pool_release_retry() warns every 60s with the same id and inflight
    count and a growing age, indefinitely:
    
      [161110.753385] page_pool_release_retry() stalled pool shutdown: id 361, 12 inflight 60 sec
      [161171.170756] page_pool_release_retry() stalled pool shutdown: id 361, 12 inflight 120 sec
      [161231.588685] page_pool_release_retry() stalled pool shutdown: id 361, 12 inflight 181 sec
      [161292.005886] page_pool_release_retry() stalled pool shutdown: id 361, 12 inflight 241 sec
    
    With this patch the stranded buffers are freed at deinit, inflight
    drops to zero and the pool drains cleanly.

 .../net/ethernet/aquantia/atlantic/aq_ring.c  | 30 +++++++++++++++++++
 .../net/ethernet/aquantia/atlantic/aq_ring.h  |  1 +
 .../net/ethernet/aquantia/atlantic/aq_vec.c   |  2 +-
 3 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
index 8ff07de2bd52..c59b6c0b37f1 100644
--- a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
@@ -360,6 +360,36 @@ bool aq_ring_tx_clean(struct aq_ring_s *self)
 	return !!budget;
 }
 
+void aq_ring_tx_deinit(struct aq_ring_s *self)
+{
+	if (!self)
+		goto err_exit;
+
+	for (; self->sw_head != self->sw_tail;
+		self->sw_head = aq_ring_next_dx(self, self->sw_head)) {
+		struct aq_ring_buff_s *buff = &self->buff_ring[self->sw_head];
+		struct device *ndev = aq_nic_get_dev(self->aq_nic);
+
+		if (likely(buff->is_mapped)) {
+			if (unlikely(buff->is_sop)) {
+				dma_unmap_single(ndev, buff->pa, buff->len,
+						 DMA_TO_DEVICE);
+			} else {
+				dma_unmap_page(ndev, buff->pa, buff->len,
+					       DMA_TO_DEVICE);
+			}
+		}
+
+		if (unlikely(buff->is_eop)) {
+			if (buff->skb)
+				dev_kfree_skb_any(buff->skb);
+			else if (buff->xdpf)
+				xdp_return_frame(buff->xdpf);
+		}
+	}
+err_exit:;
+}
+
 static void aq_rx_checksum(struct aq_ring_s *self,
 			   struct aq_ring_buff_s *buff,
 			   struct sk_buff *skb)
diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_ring.h b/drivers/net/ethernet/aquantia/atlantic/aq_ring.h
index a70b880ada67..6431cc62962f 100644
--- a/drivers/net/ethernet/aquantia/atlantic/aq_ring.h
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_ring.h
@@ -202,6 +202,7 @@ void aq_ring_update_queue_state(struct aq_ring_s *ring);
 void aq_ring_queue_wake(struct aq_ring_s *ring);
 void aq_ring_queue_stop(struct aq_ring_s *ring);
 bool aq_ring_tx_clean(struct aq_ring_s *self);
+void aq_ring_tx_deinit(struct aq_ring_s *self);
 int aq_xdp_xmit(struct net_device *dev, int num_frames,
 		struct xdp_frame **frames, u32 flags);
 int aq_ring_rx_clean(struct aq_ring_s *self,
diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_vec.c b/drivers/net/ethernet/aquantia/atlantic/aq_vec.c
index 2f9033ceed8c..05814fea0f5f 100644
--- a/drivers/net/ethernet/aquantia/atlantic/aq_vec.c
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_vec.c
@@ -275,7 +275,7 @@ void aq_vec_deinit(struct aq_vec_s *self)
 
 	for (i = 0U; self->tx_rings > i; ++i) {
 		ring = self->ring[i];
-		aq_ring_tx_clean(&ring[AQ_VEC_TX_ID]);
+		aq_ring_tx_deinit(&ring[AQ_VEC_TX_ID]);
 		aq_ring_rx_deinit(&ring[AQ_VEC_RX_ID]);
 	}
 
-- 
2.47.3

[PATCH net-next 2/2] net: atlantic: convert RX path to page_pool

[Yangyu Chen]

The driver currently allocates RX buffers with dev_alloc_pages(), maps
them with dma_map_page(), and uses a hand-rolled page-flip scheme to
subdivide high-order pages. Behind an IOMMU, the map/unmap churn is a
major RX cost: on a Thunderbolt-attached QNAP QNA-T310G1S, iperf3 -R
over IPv6 tops out at about 2.2 Gbit/s over MTU 1500.

Convert RX buffers to page_pool fragments. Pages are DMA-mapped once
when entering the pool and recycled through the stack or XDP via the
MEM_TYPE_PAGE_POOL memory model. This removes the custom page-flip
accounting, lets page_pool handle fragment reuse, and ensures every RX
path either keeps the ring's fragment reference for reposting or hands
it to the skb/xdp_buff for later recycling.

Register the PTP RX ring's xdp_rxq as well, since it shares the RX
clean paths. Walk the full RX ring during deinit so consumed but not yet
refilled slots cannot leak page-pool references. Drop the ethtool
PageFlips/PageReuses/PageFrees counters which only described the old
scheme.

On the QNA-T310G1S, MTU 1500, TCP over IPv6, iperf3 -R improves from
2.24 Gbit/s to 9.14 Gbit/s. The module was also smoke-tested with native
XDP PASS, DROP, and ABORTED actions; carrier recovered after each
attach/detach cycle and dmesg showed no page_pool/DMA warnings.

Assisted-by: Claude:claude-fable-5
Signed-off-by: Yangyu Chen <cyy@cyyself.name>
---
 drivers/net/ethernet/aquantia/Kconfig         |   1 +
 .../ethernet/aquantia/atlantic/aq_ethtool.c   |   3 -
 .../net/ethernet/aquantia/atlantic/aq_ptp.c   |  20 +-
 .../net/ethernet/aquantia/atlantic/aq_ring.c  | 238 ++++++++----------
 .../net/ethernet/aquantia/atlantic/aq_ring.h  |   6 +-
 .../net/ethernet/aquantia/atlantic/aq_vec.c   |  15 +-
 6 files changed, 140 insertions(+), 143 deletions(-)

diff --git a/drivers/net/ethernet/aquantia/Kconfig b/drivers/net/ethernet/aquantia/Kconfig
index cec2018c84a9..c8fb7b33e5b7 100644
--- a/drivers/net/ethernet/aquantia/Kconfig
+++ b/drivers/net/ethernet/aquantia/Kconfig
@@ -20,6 +20,7 @@ config AQTION
 	tristate "aQuantia AQtion(tm) Support"
 	depends on PCI
 	depends on MACSEC || MACSEC=n
+	select PAGE_POOL
 	help
 	  This enables the support for the aQuantia AQtion(tm) Ethernet card.
 
diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_ethtool.c b/drivers/net/ethernet/aquantia/atlantic/aq_ethtool.c
index 420af958d486..0f5125bd2315 100644
--- a/drivers/net/ethernet/aquantia/atlantic/aq_ethtool.c
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_ethtool.c
@@ -100,9 +100,6 @@ static const char * const aq_ethtool_queue_rx_stat_names[] = {
 	"%sQueue[%d] AllocFails",
 	"%sQueue[%d] SkbAllocFails",
 	"%sQueue[%d] Polls",
-	"%sQueue[%d] PageFlips",
-	"%sQueue[%d] PageReuses",
-	"%sQueue[%d] PageFrees",
 	"%sQueue[%d] XdpAbort",
 	"%sQueue[%d] XdpDrop",
 	"%sQueue[%d] XdpPass",
diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_ptp.c b/drivers/net/ethernet/aquantia/atlantic/aq_ptp.c
index 558ac9237f75..5b8a4d0d1cff 100644
--- a/drivers/net/ethernet/aquantia/atlantic/aq_ptp.c
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_ptp.c
@@ -13,6 +13,7 @@
 #include <linux/ptp_classify.h>
 #include <linux/interrupt.h>
 #include <linux/clocksource.h>
+#include <net/xdp.h>
 
 #include "aq_nic.h"
 #include "aq_ptp.h"
@@ -1192,12 +1193,25 @@ int aq_ptp_ring_alloc(struct aq_nic_s *aq_nic)
 	if (err)
 		goto err_exit_ptp_tx;
 
+	err = xdp_rxq_info_reg(&aq_ptp->ptp_rx.xdp_rxq, aq_nic->ndev,
+			       rx_ring_idx, aq_ptp->napi.napi_id);
+	if (err < 0)
+		goto err_exit_ptp_rx;
+
+	err = xdp_rxq_info_reg_mem_model(&aq_ptp->ptp_rx.xdp_rxq,
+					 MEM_TYPE_PAGE_POOL,
+					 aq_ptp->ptp_rx.pg_pool);
+	if (err < 0) {
+		xdp_rxq_info_unreg(&aq_ptp->ptp_rx.xdp_rxq);
+		goto err_exit_ptp_rx;
+	}
+
 	if (aq_ptp->a1_ptp) {
 		err = aq_ring_hwts_rx_alloc(&aq_ptp->hwts_rx, aq_nic, PTP_HWST_RING_IDX,
 					    aq_nic->aq_nic_cfg.rxds,
 					    aq_nic->aq_nic_cfg.aq_hw_caps->rxd_size);
 		if (err)
-			goto err_exit_ptp_rx;
+			goto err_exit_xdp_rxq;
 	}
 
 	err = aq_ptp_skb_ring_init(&aq_ptp->skb_ring, aq_nic->aq_nic_cfg.rxds);
@@ -1217,6 +1231,8 @@ int aq_ptp_ring_alloc(struct aq_nic_s *aq_nic)
 err_exit_hwts_rx:
 	if (aq_ptp->a1_ptp)
 		aq_ring_hwts_rx_free(&aq_ptp->hwts_rx);
+err_exit_xdp_rxq:
+	xdp_rxq_info_unreg(&aq_ptp->ptp_rx.xdp_rxq);
 err_exit_ptp_rx:
 	aq_ring_free(&aq_ptp->ptp_rx);
 err_exit_ptp_tx:
@@ -1233,6 +1249,8 @@ void aq_ptp_ring_free(struct aq_nic_s *aq_nic)
 		return;
 
 	aq_ring_free(&aq_ptp->ptp_tx);
+	if (xdp_rxq_info_is_reg(&aq_ptp->ptp_rx.xdp_rxq))
+		xdp_rxq_info_unreg(&aq_ptp->ptp_rx.xdp_rxq);
 	aq_ring_free(&aq_ptp->ptp_rx);
 	if (aq_ptp->a1_ptp)
 		aq_ring_hwts_rx_free(&aq_ptp->hwts_rx);
diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
index c59b6c0b37f1..e06c502b59f9 100644
--- a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
@@ -14,120 +14,37 @@
 #include "aq_vec.h"
 #include "aq_main.h"
 
+#include <net/page_pool/helpers.h>
 #include <net/xdp.h>
 #include <linux/filter.h>
 #include <linux/bpf_trace.h>
 #include <linux/netdevice.h>
 #include <linux/etherdevice.h>
 
-static void aq_get_rxpages_xdp(struct aq_ring_buff_s *buff,
-			       struct xdp_buff *xdp)
-{
-	struct skb_shared_info *sinfo;
-	int i;
-
-	if (xdp_buff_has_frags(xdp)) {
-		sinfo = xdp_get_shared_info_from_buff(xdp);
-
-		for (i = 0; i < sinfo->nr_frags; i++) {
-			skb_frag_t *frag = &sinfo->frags[i];
-
-			page_ref_inc(skb_frag_page(frag));
-		}
-	}
-	page_ref_inc(buff->rxdata.page);
-}
-
-static inline void aq_free_rxpage(struct aq_rxpage *rxpage, struct device *dev)
-{
-	unsigned int len = PAGE_SIZE << rxpage->order;
-
-	dma_unmap_page(dev, rxpage->daddr, len, DMA_FROM_DEVICE);
-
-	/* Drop the ref for being in the ring. */
-	__free_pages(rxpage->page, rxpage->order);
-	rxpage->page = NULL;
-}
-
-static int aq_alloc_rxpages(struct aq_rxpage *rxpage, struct aq_ring_s *rx_ring)
+static int aq_get_rxpages(struct aq_ring_s *self, struct aq_ring_buff_s *rxbuf)
 {
-	struct device *dev = aq_nic_get_dev(rx_ring->aq_nic);
-	unsigned int order = rx_ring->page_order;
+	unsigned int size = self->page_offset + self->frame_max +
+			    self->tail_size;
+	unsigned int pg_off;
 	struct page *page;
-	int ret = -ENOMEM;
-	dma_addr_t daddr;
-
-	page = dev_alloc_pages(order);
-	if (unlikely(!page))
-		goto err_exit;
 
-	daddr = dma_map_page(dev, page, 0, PAGE_SIZE << order,
-			     DMA_FROM_DEVICE);
+	/* Buffers whose page was not passed up the stack are reposted
+	 * with the data they already carry discarded.
+	 */
+	if (rxbuf->rxdata.page)
+		return 0;
 
-	if (unlikely(dma_mapping_error(dev, daddr)))
-		goto free_page;
-
-	rxpage->page = page;
-	rxpage->daddr = daddr;
-	rxpage->order = order;
-	rxpage->pg_off = rx_ring->page_offset;
-
-	return 0;
-
-free_page:
-	__free_pages(page, order);
-
-err_exit:
-	return ret;
-}
-
-static int aq_get_rxpages(struct aq_ring_s *self, struct aq_ring_buff_s *rxbuf)
-{
-	unsigned int order = self->page_order;
-	u16 page_offset = self->page_offset;
-	u16 frame_max = self->frame_max;
-	u16 tail_size = self->tail_size;
-	int ret;
-
-	if (rxbuf->rxdata.page) {
-		/* One means ring is the only user and can reuse */
-		if (page_ref_count(rxbuf->rxdata.page) > 1) {
-			/* Try reuse buffer */
-			rxbuf->rxdata.pg_off += frame_max + page_offset +
-						tail_size;
-			if (rxbuf->rxdata.pg_off + frame_max + tail_size <=
-			    (PAGE_SIZE << order)) {
-				u64_stats_update_begin(&self->stats.rx.syncp);
-				self->stats.rx.pg_flips++;
-				u64_stats_update_end(&self->stats.rx.syncp);
-
-			} else {
-				/* Buffer exhausted. We have other users and
-				 * should release this page and realloc
-				 */
-				aq_free_rxpage(&rxbuf->rxdata,
-					       aq_nic_get_dev(self->aq_nic));
-				u64_stats_update_begin(&self->stats.rx.syncp);
-				self->stats.rx.pg_losts++;
-				u64_stats_update_end(&self->stats.rx.syncp);
-			}
-		} else {
-			rxbuf->rxdata.pg_off = page_offset;
-			u64_stats_update_begin(&self->stats.rx.syncp);
-			self->stats.rx.pg_reuses++;
-			u64_stats_update_end(&self->stats.rx.syncp);
-		}
+	page = page_pool_dev_alloc_frag(self->pg_pool, &pg_off, size);
+	if (unlikely(!page)) {
+		u64_stats_update_begin(&self->stats.rx.syncp);
+		self->stats.rx.alloc_fails++;
+		u64_stats_update_end(&self->stats.rx.syncp);
+		return -ENOMEM;
 	}
 
-	if (!rxbuf->rxdata.page) {
-		ret = aq_alloc_rxpages(&rxbuf->rxdata, self);
-		if (ret) {
-			u64_stats_update_begin(&self->stats.rx.syncp);
-			self->stats.rx.alloc_fails++;
-			u64_stats_update_end(&self->stats.rx.syncp);
-		}
-		return ret;
-	}
+	rxbuf->rxdata.page = page;
+	rxbuf->rxdata.daddr = page_pool_get_dma_addr(page);
+	rxbuf->rxdata.pg_off = pg_off + self->page_offset;
 
 	return 0;
 }
@@ -179,6 +96,15 @@ int aq_ring_rx_alloc(struct aq_ring_s *self,
 		     unsigned int idx,
 		     struct aq_nic_cfg_s *aq_nic_cfg)
 {
+	struct page_pool_params pp_params = {
+		.flags = PP_FLAG_DMA_MAP | PP_FLAG_DMA_SYNC_DEV,
+		.pool_size = aq_nic_cfg->rxds,
+		.nid = NUMA_NO_NODE,
+		.dev = aq_nic_get_dev(aq_nic),
+		.dma_dir = DMA_FROM_DEVICE,
+	};
+	struct page_pool *pool;
+
 	self->aq_nic = aq_nic;
 	self->idx = idx;
 	self->size = aq_nic_cfg->rxds;
@@ -200,6 +126,18 @@ int aq_ring_rx_alloc(struct aq_ring_s *self,
 		self->tail_size = 0;
 	}
 
+	pp_params.order = self->page_order;
+	pp_params.max_len = PAGE_SIZE << self->page_order;
+
+	pool = page_pool_create(&pp_params);
+	if (IS_ERR(pool))
+		return PTR_ERR(pool);
+
+	self->pg_pool = pool;
+
+	/* On failure aq_ring_alloc() calls aq_ring_free(), which also
+	 * destroys the page pool.
+	 */
 	return aq_ring_alloc(self, aq_nic);
 }
 
@@ -346,7 +284,11 @@ bool aq_ring_tx_clean(struct aq_ring_s *self)
 			++self->stats.tx.packets;
 			self->stats.tx.bytes += xdp_get_frame_len(buff->xdpf);
 			u64_stats_update_end(&self->stats.tx.syncp);
-			xdp_return_frame_rx_napi(buff->xdpf);
+			/* Frames queued via ndo_xdp_xmit() may come from a
+			 * page pool owned by another NAPI context: no direct
+			 * recycling.
+			 */
+			xdp_return_frame(buff->xdpf);
 		}
 
 out:
@@ -438,22 +380,15 @@ int aq_xdp_xmit(struct net_device *dev, int num_frames,
 }
 
 static struct sk_buff *aq_xdp_build_skb(struct xdp_buff *xdp,
-					struct net_device *dev,
-					struct aq_ring_buff_s *buff)
+					struct net_device *dev)
 {
 	struct xdp_frame *xdpf;
-	struct sk_buff *skb;
 
 	xdpf = xdp_convert_buff_to_frame(xdp);
 	if (unlikely(!xdpf))
 		return NULL;
 
-	skb = xdp_build_skb_from_frame(xdpf, dev);
-	if (!skb)
-		return NULL;
-
-	aq_get_rxpages_xdp(buff, xdp);
-	return skb;
+	return xdp_build_skb_from_frame(xdpf, dev);
 }
 
 static struct sk_buff *aq_xdp_run_prog(struct aq_nic_s *aq_nic,
@@ -474,8 +409,16 @@ static struct sk_buff *aq_xdp_run_prog(struct aq_nic_s *aq_nic,
 	u64_stats_update_end(&rx_ring->stats.rx.syncp);
 
 	prog = READ_ONCE(rx_ring->xdp_prog);
-	if (!prog)
-		return aq_xdp_build_skb(xdp, aq_nic->ndev, buff);
+	if (!prog) {
+		skb = aq_xdp_build_skb(xdp, aq_nic->ndev);
+		/* The ring has already handed its page pool reference to the
+		 * xdp_buff, so if the skb could not be built the buffer must
+		 * be returned to the pool here or its fragments would leak.
+		 */
+		if (!skb)
+			xdp_return_buff(xdp);
+		return skb;
+	}
 
 	prefetchw(xdp->data_hard_start); /* xdp_frame write */
 
@@ -486,7 +429,7 @@ static struct sk_buff *aq_xdp_run_prog(struct aq_nic_s *aq_nic,
 	act = bpf_prog_run_xdp(prog, xdp);
 	switch (act) {
 	case XDP_PASS:
-		skb = aq_xdp_build_skb(xdp, aq_nic->ndev, buff);
+		skb = aq_xdp_build_skb(xdp, aq_nic->ndev);
 		if (!skb)
 			goto out_aborted;
 		u64_stats_update_begin(&rx_ring->stats.rx.syncp);
@@ -504,7 +447,6 @@ static struct sk_buff *aq_xdp_run_prog(struct aq_nic_s *aq_nic,
 		u64_stats_update_begin(&rx_ring->stats.rx.syncp);
 		++rx_ring->stats.rx.xdp_tx;
 		u64_stats_update_end(&rx_ring->stats.rx.syncp);
-		aq_get_rxpages_xdp(buff, xdp);
 		break;
 	case XDP_REDIRECT:
 		if (xdp_do_redirect(aq_nic->ndev, xdp, prog) < 0)
@@ -513,7 +455,6 @@ static struct sk_buff *aq_xdp_run_prog(struct aq_nic_s *aq_nic,
 		u64_stats_update_begin(&rx_ring->stats.rx.syncp);
 		++rx_ring->stats.rx.xdp_redirect;
 		u64_stats_update_end(&rx_ring->stats.rx.syncp);
-		aq_get_rxpages_xdp(buff, xdp);
 		break;
 	default:
 		fallthrough;
@@ -524,11 +465,13 @@ static struct sk_buff *aq_xdp_run_prog(struct aq_nic_s *aq_nic,
 		u64_stats_update_end(&rx_ring->stats.rx.syncp);
 		trace_xdp_exception(aq_nic->ndev, prog, act);
 		bpf_warn_invalid_xdp_action(aq_nic->ndev, prog, act);
+		xdp_return_buff(xdp);
 		break;
 	case XDP_DROP:
 		u64_stats_update_begin(&rx_ring->stats.rx.syncp);
 		++rx_ring->stats.rx.xdp_drop;
 		u64_stats_update_end(&rx_ring->stats.rx.syncp);
+		xdp_return_buff(xdp);
 		break;
 	}
 
@@ -547,8 +490,11 @@ static bool aq_add_rx_fragment(struct device *dev,
 	do {
 		skb_frag_t *frag;
 
-		if (unlikely(sinfo->nr_frags >= MAX_SKB_FRAGS))
+		if (unlikely(sinfo->nr_frags >= MAX_SKB_FRAGS)) {
+			/* Attached frags must reach xdp_return_buff() */
+			xdp_buff_set_frags_flag(xdp);
 			return true;
+		}
 
 		frag = &sinfo->frags[sinfo->nr_frags++];
 		buff_ = &ring->buff_ring[buff_->next];
@@ -572,6 +518,11 @@ static bool aq_add_rx_fragment(struct device *dev,
 		if (page_is_pfmemalloc(buff_->rxdata.page))
 			xdp_buff_set_frag_pfmemalloc(xdp);
 
+		/* The frag's page pool reference is owned by the xdp_buff
+		 * from now on.
+		 */
+		buff_->rxdata.page = NULL;
+
 	} while (!buff_->is_eop);
 
 	xdp_buff_set_frags_flag(xdp);
@@ -675,6 +626,7 @@ static int __aq_ring_rx_clean(struct aq_ring_s *self, struct napi_struct *napi,
 			err = -ENOMEM;
 			goto err_exit;
 		}
+		skb_mark_for_recycle(skb);
 		if (is_ptp_ring)
 			buff->len -=
 				aq_ptp_extract_ts(self->aq_nic, skb_hwtstamps(skb),
@@ -695,7 +647,7 @@ static int __aq_ring_rx_clean(struct aq_ring_s *self, struct napi_struct *napi,
 					buff->rxdata.pg_off + hdr_len,
 					buff->len - hdr_len,
 					self->frame_max);
-			page_ref_inc(buff->rxdata.page);
+			buff->rxdata.page = NULL;
 		}
 
 		if (!buff->is_eop) {
@@ -714,7 +666,7 @@ static int __aq_ring_rx_clean(struct aq_ring_s *self, struct napi_struct *napi,
 						buff_->rxdata.pg_off,
 						buff_->len,
 						self->frame_max);
-				page_ref_inc(buff_->rxdata.page);
+				buff_->rxdata.page = NULL;
 				buff_->is_cleaned = 1;
 
 				buff->is_ip_cso &= buff_->is_ip_cso;
@@ -852,6 +804,11 @@ static int __aq_ring_xdp_clean(struct aq_ring_s *rx_ring,
 		xdp_init_buff(&xdp, frame_sz, &rx_ring->xdp_rxq);
 		xdp_prepare_buff(&xdp, hard_start, rx_ring->page_offset,
 				 buff->len, false);
+		/* The xdp_buff owns the buffer's page pool reference from
+		 * here on; it comes back through the MEM_TYPE_PAGE_POOL
+		 * memory model on every XDP verdict.
+		 */
+		buff->rxdata.page = NULL;
 		if (!buff->is_eop) {
 			if (aq_add_rx_fragment(dev, rx_ring, buff, &xdp)) {
 				u64_stats_update_begin(&rx_ring->stats.rx.syncp);
@@ -859,6 +816,7 @@ static int __aq_ring_xdp_clean(struct aq_ring_s *rx_ring,
 				rx_ring->stats.rx.bytes += xdp_get_buff_len(&xdp);
 				++rx_ring->stats.rx.xdp_aborted;
 				u64_stats_update_end(&rx_ring->stats.rx.syncp);
+				xdp_return_buff(&xdp);
 				continue;
 			}
 		}
@@ -951,15 +909,37 @@ int aq_ring_rx_fill(struct aq_ring_s *self)
 
 void aq_ring_rx_deinit(struct aq_ring_s *self)
 {
-	if (!self)
+	unsigned int i;
+
+	/* The ring may already be gone: on a partial aq_ptp_ring_alloc()
+	 * failure the unwind frees it but leaves aq_nic set, so the
+	 * deinit paths still get here.
+	 */
+	if (!self || !self->buff_ring)
 		return;
 
-	for (; self->sw_head != self->sw_tail;
-		self->sw_head = aq_ring_next_dx(self, self->sw_head)) {
-		struct aq_ring_buff_s *buff = &self->buff_ring[self->sw_head];
+	/* Release every fragment still owned by the ring, or
+	 * page_pool_destroy() will stall on the outstanding references.
+	 *
+	 * Walking [sw_head, sw_tail) is not enough: refill is batched
+	 * (aq_ring_rx_fill() waits for AQ_CFG_RX_REFILL_THRES free slots),
+	 * so slots that were cleaned but not yet reposted accumulate in the
+	 * [sw_tail, sw_head) gap. Frames kept for in-place repost (RX
+	 * errors, XDP_DROP and header-only packets) still hold a fragment
+	 * there, so walk the whole ring and release whatever is left.
+	 */
+	for (i = 0; i < self->size; i++) {
+		struct aq_ring_buff_s *buff = &self->buff_ring[i];
+
+		if (!buff->rxdata.page)
+			continue;
 
-		aq_free_rxpage(&buff->rxdata, aq_nic_get_dev(self->aq_nic));
+		page_pool_put_full_page(self->pg_pool, buff->rxdata.page,
+					false);
+		buff->rxdata.page = NULL;
 	}
+
+	self->sw_head = self->sw_tail;
 }
 
 void aq_ring_free(struct aq_ring_s *self)
@@ -970,6 +950,11 @@ void aq_ring_free(struct aq_ring_s *self)
 	kfree(self->buff_ring);
 	self->buff_ring = NULL;
 
+	if (self->pg_pool) {
+		page_pool_destroy(self->pg_pool);
+		self->pg_pool = NULL;
+	}
+
 	if (self->dx_ring) {
 		dma_free_coherent(aq_nic_get_dev(self->aq_nic),
 				  self->size * self->dx_size, self->dx_ring,
@@ -1008,9 +993,6 @@ unsigned int aq_ring_fill_stats_data(struct aq_ring_s *self, u64 *data)
 			data[++count] = self->stats.rx.alloc_fails;
 			data[++count] = self->stats.rx.skb_alloc_fails;
 			data[++count] = self->stats.rx.polls;
-			data[++count] = self->stats.rx.pg_flips;
-			data[++count] = self->stats.rx.pg_reuses;
-			data[++count] = self->stats.rx.pg_losts;
 			data[++count] = self->stats.rx.xdp_aborted;
 			data[++count] = self->stats.rx.xdp_drop;
 			data[++count] = self->stats.rx.xdp_pass;
diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_ring.h b/drivers/net/ethernet/aquantia/atlantic/aq_ring.h
index 6431cc62962f..58bcadb3e3cc 100644
--- a/drivers/net/ethernet/aquantia/atlantic/aq_ring.h
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_ring.h
@@ -17,12 +17,12 @@
 #define AQ_XDP_TAILROOM		SKB_DATA_ALIGN(sizeof(struct skb_shared_info))
 
 struct page;
+struct page_pool;
 struct aq_nic_cfg_s;
 
 struct aq_rxpage {
 	struct page *page;
 	dma_addr_t daddr;
-	unsigned int order;
 	unsigned int pg_off;
 };
 
@@ -105,9 +105,6 @@ struct aq_ring_stats_rx_s {
 	u64 alloc_fails;
 	u64 skb_alloc_fails;
 	u64 polls;
-	u64 pg_losts;
-	u64 pg_flips;
-	u64 pg_reuses;
 	u64 xdp_aborted;
 	u64 xdp_drop;
 	u64 xdp_pass;
@@ -151,6 +148,7 @@ struct aq_ring_s {
 	u16 tail_size;
 	union aq_ring_stats_s stats;
 	dma_addr_t dx_ring_pa;
+	struct page_pool *pg_pool;
 	struct bpf_prog *xdp_prog;
 	enum atl_ring_type ring_type;
 	struct xdp_rxq_info xdp_rxq;
diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_vec.c b/drivers/net/ethernet/aquantia/atlantic/aq_vec.c
index 05814fea0f5f..023a80f4614e 100644
--- a/drivers/net/ethernet/aquantia/atlantic/aq_vec.c
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_vec.c
@@ -146,25 +146,26 @@ int aq_vec_ring_alloc(struct aq_vec_s *self, struct aq_nic_s *aq_nic,
 		aq_nic_set_tx_ring(aq_nic, idx_ring, ring);
 
 		ring = &self->ring[i][AQ_VEC_RX_ID];
+		err = aq_ring_rx_alloc(ring, aq_nic, idx_ring, aq_nic_cfg);
+		if (err)
+			goto err_exit;
+
 		if (xdp_rxq_info_reg(&ring->xdp_rxq,
 				     aq_nic->ndev, idx,
 				     self->napi.napi_id) < 0) {
+			aq_ring_free(ring);
 			err = -ENOMEM;
 			goto err_exit;
 		}
 		if (xdp_rxq_info_reg_mem_model(&ring->xdp_rxq,
-					       MEM_TYPE_PAGE_SHARED, NULL) < 0) {
+					       MEM_TYPE_PAGE_POOL,
+					       ring->pg_pool) < 0) {
 			xdp_rxq_info_unreg(&ring->xdp_rxq);
+			aq_ring_free(ring);
 			err = -ENOMEM;
 			goto err_exit;
 		}
 
-		err = aq_ring_rx_alloc(ring, aq_nic, idx_ring, aq_nic_cfg);
-		if (err) {
-			xdp_rxq_info_unreg(&ring->xdp_rxq);
-			goto err_exit;
-		}
-
 		++self->rx_rings;
 	}
 
-- 
2.47.3

Re: [PATCH net-next 1/2] net: atlantic: free stranded TX buffers on ring deinit

[sashiko-bot]

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

Pre-existing issues:
- [High] The patch fixes a resource leak of stranded TX buffers on interface teardown by replacing `aq_ring_tx_clean()` with the restored `aq_ring_tx_deinit()` in `aq_vec_deinit()`, but misses the identical leak pattern in `aq_ptp_ring_deinit()`.
--

--- Patch [1]: [PATCH net-next 1/2] net: atlantic: free stranded TX buffers on ring deinit ---
commit 0bceee292a00058408f6f3baf2ae8103a4e90f9c
Author: Yangyu Chen <cyy@cyyself.name>

net: atlantic: free stranded TX buffers on ring deinit

This commit brings back aq_ring_tx_deinit() to properly drain TX rings
during teardown. This prevents silent memory leaks of stranded buffers and
stalls during page pool destruction when the interface goes down under
TX or XDP_TX load.

> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_vec.c b/drivers/net/ethernet/aquantia/atlantic/aq_vec.c
> index 2f9033ceed8ce..05814fea0f5f7 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/aq_vec.c
> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_vec.c
[ ... ]
> @@ -275,7 +275,7 @@ void aq_vec_deinit(struct aq_vec_s *self)
>  
>  	for (i = 0U; self->tx_rings > i; ++i) {
>  		ring = self->ring[i];
> -		aq_ring_tx_clean(&ring[AQ_VEC_TX_ID]);
> +		aq_ring_tx_deinit(&ring[AQ_VEC_TX_ID]);

[Severity: High]
This is a pre-existing issue, but should aq_ptp_ring_deinit() also be
updated to use aq_ring_tx_deinit() instead of aq_ring_tx_clean()?

In drivers/net/ethernet/aquantia/atlantic/aq_ptp.c:aq_ptp_ring_deinit():

	aq_ring_tx_clean(&aq_ptp->ptp_tx);
	aq_ring_rx_deinit(&aq_ptp->ptp_rx);

Since aq_ring_tx_clean() stops at hw_head and applies a budget, any
in-flight SKBs and their DMA mappings on the PTP TX ring might still be
permanently leaked when the hardware is stopped and the interface is
brought down.

Does the PTP ring teardown need the same fix applied here?

>  		aq_ring_rx_deinit(&ring[AQ_VEC_RX_ID]);
>  	}
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/tencent_7DB01BE7F8FA056BB5F11D3570CF636C4309@qq.com?part=1

Re: [PATCH net-next 2/2] net: atlantic: convert RX path to page_pool

[sashiko-bot]

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

New issues:
- [Critical] Uninitialized physical addresses cause severe DMA memory corruption during RX buffer refill.

Pre-existing issues:
- [High] The ndo_xdp_xmit callback violates API contract by not repacking arrays on failure, causing UAF and memory leaks.
- [High] Malformed RX packets exceeding MAX_SKB_FRAGS cause a permanent infinite NAPI polling loop.
- [High] Aborting XDP assembly on oversized packets leaves trailing fragments uncleaned, leading to packet injection.
- [High] Updating u64_stats without disabling preemption causes a system livelock on 32-bit architectures.
--

--- Patch [2]: [PATCH net-next 2/2] net: atlantic: convert RX path to page_pool ---
commit d3fa6dc4118e01fdb55de72ea89c2b574594ac08
Author: Yangyu Chen <cyy@cyyself.name>

net: atlantic: convert RX path to page_pool

This commit converts the atlantic driver's RX path to use page_pool for
fragment management to reduce DMA map/unmap overhead. It also registers the
PTP RX ring's xdp_rxq and cleans up deprecated ethtool counters.

> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
> index c59b6c0b37f1e..e06c502b59f95 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c

[ ... ]

> -static int aq_get_rxpages(struct aq_ring_s *self, struct aq_ring_buff_s *rxbuf)
> +static int aq_get_rxpages(struct aq_ring_s *self, struct aq_ring_buff_s *rxbuf)
>  {
> -	unsigned int order = self->page_order;
> -	u16 page_offset = self->page_offset;
> -	u16 frame_max = self->frame_max;
> -	u16 tail_size = self->tail_size;
> -	int ret;
> -
> -	if (rxbuf->rxdata.page) {
> -		/* One means ring is the only user and can reuse */
> -		if (page_ref_count(rxbuf->rxdata.page) > 1) {
> -			/* Try reuse buffer */
> -			rxbuf->rxdata.pg_off += frame_max + page_offset +
> -						tail_size;
> -			if (rxbuf->rxdata.pg_off + frame_max + tail_size <=
> -			    (PAGE_SIZE << order)) {
> -				u64_stats_update_begin(&self->stats.rx.syncp);
> -				self->stats.rx.pg_flips++;
> -				u64_stats_update_end(&self->stats.rx.syncp);
> -
> -			} else {
> -				/* Buffer exhausted. We have other users and
> -				 * should release this page and realloc
> -				 */
> -				aq_free_rxpage(&rxbuf->rxdata,
> -					       aq_nic_get_dev(self->aq_nic));
> -				u64_stats_update_begin(&self->stats.rx.syncp);
> -				self->stats.rx.pg_losts++;
> -				u64_stats_update_end(&self->stats.rx.syncp);
> -			}
> -		} else {
> -			rxbuf->rxdata.pg_off = page_offset;
> -			u64_stats_update_begin(&self->stats.rx.syncp);
> -			self->stats.rx.pg_reuses++;
> -			u64_stats_update_end(&self->stats.rx.syncp);
> -		}
> +	unsigned int size = self->page_offset + self->frame_max +
> +			    self->tail_size;
> +	unsigned int pg_off;
>  	struct page *page;
> -	int ret = -ENOMEM;
> -	dma_addr_t daddr;
> -
> -	page = dev_alloc_pages(order);
> -	if (unlikely(!page))
> -		goto err_exit;
>  
> -	daddr = dma_map_page(dev, page, 0, PAGE_SIZE << order,
> -			     DMA_FROM_DEVICE);
> +	/* Buffers whose page was not passed up the stack are reposted
> +	 * with the data they already carry discarded.
> +	 */
> +	if (rxbuf->rxdata.page)
> +		return 0;
>  
> -	if (unlikely(dma_mapping_error(dev, daddr)))
> -		goto free_page;
> -
> -	rxpage->page = page;
> -	rxpage->daddr = daddr;
> -	rxpage->order = order;
> -	rxpage->pg_off = rx_ring->page_offset;
> -
> -	return 0;
> -
> -free_page:
> -	__free_pages(page, order);
> -
> -err_exit:
> -	return ret;
> -}
> -
> -static int aq_get_rxpages(struct aq_ring_s *self, struct aq_ring_buff_s *rxbuf)
> -{
> -	unsigned int order = self->page_order;
> -	u16 page_offset = self->page_offset;
> -	u16 frame_max = self->frame_max;
> -	u16 tail_size = self->tail_size;
> -	int ret;
> -
> -	if (rxbuf->rxdata.page) {
> -		/* One means ring is the only user and can reuse */
> -		if (page_ref_count(rxbuf->rxdata.page) > 1) {
> -			/* Try reuse buffer */
> -			rxbuf->rxdata.pg_off += frame_max + page_offset +
> -						tail_size;
> -			if (rxbuf->rxdata.pg_off + frame_max + tail_size <=
> -			    (PAGE_SIZE << order)) {
> -				u64_stats_update_begin(&self->stats.rx.syncp);
> -				self->stats.rx.pg_flips++;
> -				u64_stats_update_end(&self->stats.rx.syncp);
> -
> -			} else {
> -				/* Buffer exhausted. We have other users and
> -				 * should release this page and realloc
> -				 */
> -				aq_free_rxpage(&rxbuf->rxdata,
> -					       aq_nic_get_dev(self->aq_nic));
> -				u64_stats_update_begin(&self->stats.rx.syncp);
> -				self->stats.rx.pg_losts++;
> -				u64_stats_update_end(&self->stats.rx.syncp);
> -			}
> -		} else {
> -			rxbuf->rxdata.pg_off = page_offset;
> -			u64_stats_update_begin(&self->stats.rx.syncp);
> -			self->stats.rx.pg_reuses++;
> -			u64_stats_update_end(&self->stats.rx.syncp);
> -		}
> +	page = page_pool_dev_alloc_frag(self->pg_pool, &pg_off, size);
> +	if (unlikely(!page)) {
> +		u64_stats_update_begin(&self->stats.rx.syncp);
> +		self->stats.rx.alloc_fails++;
> +		u64_stats_update_end(&self->stats.rx.syncp);

[Severity: High]
This is a pre-existing issue, but can this cause a system livelock on 32-bit
architectures?

The u64_stats_update_begin() function is called here in process context
(e.g., during aq_ring_rx_fill() via aq_vec_start()) without disabling
preemption.

If a writer is preempted by a reader (such as ndo_get_stats64() invoked from
a different CPU), the reader will spin forever in the
u64_stats_fetch_retry() loop on 32-bit systems because the sequence counter
will never become even.

> +		return -ENOMEM;
> +	}
>  
> -	if (!rxbuf->rxdata.page) {
> -		ret = aq_alloc_rxpages(&rxbuf->rxdata, self);
> -		if (ret) {
> -			u64_stats_update_begin(&self->stats.rx.syncp);
> -			self->stats.rx.alloc_fails++;
> -			u64_stats_update_end(&self->stats.rx.syncp);
> -		}
> -		return ret;
> -	}
> +	rxbuf->rxdata.page = page;
> +	rxbuf->rxdata.daddr = page_pool_get_dma_addr(page);
> +	rxbuf->rxdata.pg_off = pg_off + self->page_offset;

[Severity: Critical]
Can this cause severe DMA memory corruption during RX buffer refill?

The initialization of buff->pa appears to be missing or was removed here,
but the hardware callback still executes immediately after and
unconditionally overwrites the descriptor's physical address:

hw_atl_b0_hw_ring_rx_fill() {
    ...
    rxd->buf_addr = buff->pa;
    rxd->hdr_addr = 0U;
    ...
}

Because buff->pa is no longer initialized for RX, it contains garbage from
the previous packet (specifically rss_hash and next, due to the union in
the aq_ring_buff_s struct).

This programs the NIC with arbitrary physical addresses, causing it to DMA
incoming network packets into random physical memory.

[ ... ]

> @@ -438,22 +380,15 @@ int aq_xdp_xmit(struct net_device *dev, int num_frames,
>  }

[Severity: High]
This is a pre-existing issue, but does the error handling in aq_xdp_xmit()
violate the ndo_xdp_xmit API contract?

When transmitting a batch of XDP frames, if a frame in the middle fails
(returning NETDEV_TX_BUSY), the function increments a drop counter and
returns num_frames - drop:

aq_xdp_xmit() {
    ...
    if (aq_nic_xmit_xdpf(aq_nic, ring, xdpf) == NETDEV_TX_BUSY)
        drop++;
    ...
    return num_frames - drop;
}

The loop drops frames but does not repack the frames array. The network
stack (e.g., devmap) assumes that a partial success count N means the first
N frames succeeded and the rest failed.

It will free the frames at the end of the array, some of which were actually
successfully queued for transmission by the driver. This leads to freeing
memory that the NIC is actively trying to DMA, while leaking the memory of
the frame that actually failed.

[ ... ]

> @@ -547,8 +490,11 @@ static bool aq_add_rx_fragment(struct device *dev,
>  	do {
>  		skb_frag_t *frag;
>  
> -		if (unlikely(sinfo->nr_frags >= MAX_SKB_FRAGS))
> +		if (unlikely(sinfo->nr_frags >= MAX_SKB_FRAGS)) {
> +			/* Attached frags must reach xdp_return_buff() */
> +			xdp_buff_set_frags_flag(xdp);
>  			return true;

[Severity: High]
This is a pre-existing issue, but does aborting XDP assembly on oversized
packets leave trailing fragments uncleaned?

When an RX packet exceeds MAX_SKB_FRAGS during XDP processing, this early
return aborts but fails to set buff_->is_cleaned = 1 for the remaining
descriptors of the malformed packet.

The outer loop in __aq_ring_xdp_clean() handles the abort by advancing
sw_head by 1. Because the trailing fragments were never marked as cleaned,
subsequent iterations of the loop will encounter them and incorrectly parse
them as the Start-Of-Packet (SOP) for new frames.

This forces the driver to interpret raw middle-of-packet payload data as
Ethernet/IP headers, leading to arbitrary packet injection.

> +		}
>  
>  		frag = &sinfo->frags[sinfo->nr_frags++];
>  		buff_ = &ring->buff_ring[buff_->next];

[ ... ]

> @@ -675,6 +626,7 @@ static int __aq_ring_rx_clean(struct aq_ring_s *self, struct napi_struct *napi,
>  			err = -ENOMEM;
>  			goto err_exit;
>  		}
> +		skb_mark_for_recycle(skb);

[Severity: High]
This is a pre-existing issue, but does the bailout logic in
__aq_ring_rx_clean() create an unrecoverable infinite loop?

If a remote attacker sends a highly fragmented or malformed packet that
spans more than MAX_SKB_FRAGS descriptors, the driver detects this and
jumps to err_exit:

__aq_ring_rx_clean() {
    ...
    if (unlikely(!is_rsc_completed) || frag_cnt > MAX_SKB_FRAGS) {
        err = 0;
        goto err_exit;
    }
    ...
}

Because it bails out without advancing the sw_head ring pointer or dropping
the packet, the driver will process the exact same malformed packet on the
next NAPI poll, hit the exact same condition, and bail out again.

This creates a permanent infinite loop in softirq context, leading to a
complete denial of service for the CPU core processing RX queues.

>  		if (is_ptp_ring)
>  			buff->len -=
>  				aq_ptp_extract_ts(self->aq_nic, skb_hwtstamps(skb),

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/tencent_7DB01BE7F8FA056BB5F11D3570CF636C4309@qq.com?part=2