Skip to contentSkip to conversation

net: mvpp2: fix XDP RX buffer handling

PATCH Applied Til Kaiser 3 participants 7 messages Jun 7 – Jun 11

Subsystems (4)
Networking Network drivers BPF Marvell MVPP2 Ethernet driver
lore
Share & tools
lore

Til Kaiser

author
#0 lore raw

v5 cover net: mvpp2: fix XDP RX buffer handling

This is v5 of the earlier XDP_PASS fix. The XDP_PASS change is retained, and the series also fixes related RX/XDP buffer handling issues found during review.

Tested with tools/testing/selftests/drivers/net/xdp.py on mvpp2 hardware.

---
v1: https://lore.kernel.org/all/[email protected]/
v2: https://lore.kernel.org/all/[email protected]/
v3: https://lore.kernel.org/all/[email protected]/
v4: https://lore.kernel.org/all/[email protected]/

Changes in v5:
- Kept a separate page-pool recycle sync length for skb allocation failure, which must stay tied to the received buffer range.

Changes in v4:
- Reposted as a new independent thread, no code changes.

Changes in v3:
- Reworked RX refill failure handling to preserve BM pool depth by refilling before the current buffer is consumed by XDP or an skb.

Changes in v2:
- Fixed RX DMA sync range to account for RX headroom.
- Limited XDP frame size to the actual RX buffer size.
- Fixed refill error paths so retired buffers are not returned to BM.

Til Kaiser (4):
net: mvpp2: sync RX data at the hardware packet offset
net: mvpp2: limit XDP frame size to the RX buffer
net: mvpp2: refill RX buffers before XDP or skb use
net: mvpp2: build skb from XDP-adjusted data on XDP_PASS

 .../net/ethernet/marvell/mvpp2/mvpp2_main.c   | 67 ++++++++++++-------
 1 file changed, 41 insertions(+), 26 deletions(-)
Signature · 1 line
2.54.0
author
#1 lore raw

PATCH v5 1/4 net: mvpp2: sync RX data at the hardware packet offset

mvpp2 programs the RX queue packet offset, so hardware writes received data at dma_addr + MVPP2_SKB_HEADROOM. The current CPU sync starts at dma_addr and only covers rx_bytes + MVPP2_MH_SIZE bytes, which syncs the unused headroom and misses the same number of bytes at the packet tail.

On non-coherent DMA systems this can leave the CPU reading stale cache contents for the end of the received frame.

Use dma_sync_single_range_for_cpu() with MVPP2_SKB_HEADROOM as the range offset so the sync covers the Marvell header and packet data actually written by hardware.

Fixes e1921168bbd4 ("mvpp2: sync only the received frame")
Signed-off-by Til Kaiser <[email protected]>
---
 drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)
drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c +4 -3
index f442b874bb59..92a701f4fe3f 100644
@@ -3946,9 +3946,10 @@ static int mvpp2_rx(struct mvpp2_port *port, struct napi_struct *napi,
 			dma_dir = DMA_FROM_DEVICE;
 		}
 
-		dma_sync_single_for_cpu(dev->dev.parent, dma_addr,
-					rx_bytes + MVPP2_MH_SIZE,
-					dma_dir);
+		dma_sync_single_range_for_cpu(dev->dev.parent, dma_addr,
+					      MVPP2_SKB_HEADROOM,
+					      rx_bytes + MVPP2_MH_SIZE,
+					      dma_dir);
 
 		/* Buffer header not supported */
 		if (rx_status & MVPP2_RXD_BUF_HDR)
Signature · 1 line
2.54.0
author
#2 lore raw

PATCH v5 2/4 net: mvpp2: limit XDP frame size to the RX buffer

In reply to Til Kaiser

mvpp2 has short and long BM pools, and short pool buffers can be smaller than PAGE_SIZE. The XDP path nevertheless initializes every xdp_buff with PAGE_SIZE as frame size.

XDP helpers use frame_sz to validate tail growth and to derive the hard end of the data area. Advertising PAGE_SIZE for short buffers can let bpf_xdp_adjust_tail() grow a packet past the real allocation, corrupting memory or later tripping skb tailroom checks.

Initialize the XDP buffer with bm_pool->frag_size so XDP tailroom matches the actual buffer backing the packet.

Fixes 07dd0a7aae7f ("mvpp2: add basic XDP support")
Signed-off-by Til Kaiser <[email protected]>
---
 drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c +1 -1
index 92a701f4fe3f..3372ed27cc8d 100644
@@ -3979,7 +3979,7 @@ static int mvpp2_rx(struct mvpp2_port *port, struct napi_struct *napi,
 			else
 				xdp_rxq = &rxq->xdp_rxq_long;
 
-			xdp_init_buff(&xdp, PAGE_SIZE, xdp_rxq);
+			xdp_init_buff(&xdp, bm_pool->frag_size, xdp_rxq);
 			xdp_prepare_buff(&xdp, data,
 					 MVPP2_MH_SIZE + MVPP2_SKB_HEADROOM,
 					 rx_bytes, true);
Signature · 1 line
2.54.0
AI review
#3 lore raw

Re: [PATCH net v5 2/4] net: mvpp2: limit XDP frame size to the RX buffer

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

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

Pre-existing issues:
- [High] Changing MTU to a value > 1664 silently disables per-CPU pools while leaving XDP programs attached, leading to a NULL pointer dereference when XDP drops or redirects packets. --

commit de2e76bdbee6c2ad0c3dc5a24f4854767f944f85
Author: Til Kaiser <[email protected]>

net: mvpp2: limit XDP frame size to the RX buffer

mvpp2 has short and long BM pools, and short pool buffers can be smaller than PAGE_SIZE. The XDP path nevertheless initializes every xdp_buff with PAGE_SIZE as frame size.

drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c static int mvpp2_rx(struct mvpp2_port *port, struct napi_struct *napi, · line 3985

Quoted 13 lines
drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c +1 -1
index 92a701f4fe3f5..3372ed27cc8d6 100644
@@ -3979,7 +3979,7 @@ static int mvpp2_rx(struct mvpp2_port *port, struct napi_struct *napi,
 			else
 				xdp_rxq = &rxq->xdp_rxq_long;
 
-			xdp_init_buff(&xdp, PAGE_SIZE, xdp_rxq);
+			xdp_init_buff(&xdp, bm_pool->frag_size, xdp_rxq);
 			xdp_prepare_buff(&xdp, data,
 					 MVPP2_MH_SIZE + MVPP2_SKB_HEADROOM,
 					 rx_bytes, true);

[Severity: High]
This isn't a bug introduced by this patch, but could this lead to a NULL pointer dereference in the XDP path?

If the MTU is changed to a value > 1664 on another port, mvpp2_change_mtu() globally disables per-CPU pools:

drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c:mvpp2_change_mtu() {
    ...
	if (priv->percpu_pools) {
		netdev_warn(dev, "mtu %d too high, switching to shared buffers", mtu);
		mvpp2_bm_switch_buffers(priv, false);
	}
    ...
}

This leaves other ports with their XDP programs still attached but without per-CPU pools. When a packet arrives, mvpp2_rx() leaves pp as NULL:

drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c:mvpp2_rx() {
    ...
		if (port->priv->percpu_pools) {
			pp = port->priv->page_pool[pool];
			dma_dir = page_pool_get_dma_dir(pp);
		} else {
			dma_dir = DMA_FROM_DEVICE;
		}
    ...
}

Shortly after xdp_prepare_buff() in mvpp2_rx(), mvpp2_run_xdp() is called with the NULL pp. If the XDP program drops or redirects the packet, does mvpp2_run_xdp() pass the NULL pp to page_pool_put_page(), causing a panic?

drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c:mvpp2_run_xdp() {
    ...
	case XDP_DROP:
		ret = MVPP2_XDP_DROPPED;
		page = virt_to_head_page(xdp->data);
		page_pool_put_page(pp, page, sync, true);
    ...
}
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
author
#4 lore raw

PATCH v5 3/4 net: mvpp2: refill RX buffers before XDP or skb use

In reply to Til Kaiser

The RX error path returns the current descriptor buffer to the hardware BM pool. That is only valid while the driver still owns the buffer.

mvpp2_rx_refill() can fail after the current buffer has been handed to XDP or attached to an skb. In those cases mvpp2_run_xdp() may have recycled, redirected, or queued the page for XDP_TX, and an skb free also retires the data buffer. Returning such a buffer to BM lets hardware DMA into memory that is no longer owned by the RX ring.

Refill the BM pool before handing the current buffer to XDP or to the skb. If the allocation fails there, drop the packet and return the still-owned current buffer to BM, preserving the pool depth. Once the refill succeeds, later local drops retire/free the current buffer instead of returning it to BM.

Fixes 07dd0a7aae7f ("mvpp2: add basic XDP support")
Fixes d6526926de73 ("net: mvpp2: fix memory leak in mvpp2_rx")
Signed-off-by Til Kaiser <[email protected]>
---
 .../net/ethernet/marvell/mvpp2/mvpp2_main.c   | 43 +++++++++++--------
 1 file changed, 24 insertions(+), 19 deletions(-)
drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c +24 -19
index 3372ed27cc8d..481daafdc1cb 100644
@@ -3971,6 +3971,12 @@ static int mvpp2_rx(struct mvpp2_port *port, struct napi_struct *napi,
 		else
 			frag_size = bm_pool->frag_size;
 
+		err = mvpp2_rx_refill(port, bm_pool, pp, pool);
+		if (err) {
+			netdev_err(port->dev, "failed to refill BM pools\n");
+			goto err_drop_frame;
+		}
+
 		if (xdp_prog) {
 			struct xdp_rxq_info *xdp_rxq;
 

@@ -3988,12 +3994,6 @@ static int mvpp2_rx(struct mvpp2_port *port, struct napi_struct *napi,
 
 			if (ret) {
 				xdp_ret |= ret;
-				err = mvpp2_rx_refill(port, bm_pool, pp, pool);
-				if (err) {
-					netdev_err(port->dev, "failed to refill BM pools\n");
-					goto err_drop_frame;
-				}
-
 				ps.rx_packets++;
 				ps.rx_bytes += rx_bytes;
 				continue;

@@ -4008,8 +4008,21 @@ static int mvpp2_rx(struct mvpp2_port *port, struct napi_struct *napi,
 			skb = slab_build_skb(data);
 		if (!skb) {
 			netdev_warn(port->dev, "skb build failed\n");
-			goto err_drop_frame;
+			if (pp) {
+				page_pool_put_page(pp, virt_to_head_page(data),
+						   rx_bytes + MVPP2_MH_SIZE,
+						   true);
+			} else {
+				dma_unmap_single_attrs(dev->dev.parent, dma_addr,
+						       bm_pool->buf_size,
+						       DMA_FROM_DEVICE,
+						       DMA_ATTR_SKIP_CPU_SYNC);
+				mvpp2_frag_free(bm_pool, pp, data);
+			}
+			goto err_drop_frame_retired;
 		}
+		if (pp)
+			skb_mark_for_recycle(skb);
 
 		/* If we have RX hardware timestamping enabled, grab the
 		 * timestamp from the queue and convert.

@@ -4020,16 +4033,7 @@ static int mvpp2_rx(struct mvpp2_port *port, struct napi_struct *napi,
 					 skb_hwtstamps(skb));
 		}
 
-		err = mvpp2_rx_refill(port, bm_pool, pp, pool);
-		if (err) {
-			netdev_err(port->dev, "failed to refill BM pools\n");
-			dev_kfree_skb_any(skb);
-			goto err_drop_frame;
-		}
-
-		if (pp)
-			skb_mark_for_recycle(skb);
-		else
+		if (!pp)
 			dma_unmap_single_attrs(dev->dev.parent, dma_addr,
 					       bm_pool->buf_size, DMA_FROM_DEVICE,
 					       DMA_ATTR_SKIP_CPU_SYNC);

@@ -4048,13 +4052,14 @@ static int mvpp2_rx(struct mvpp2_port *port, struct napi_struct *napi,
 		continue;
 
 err_drop_frame:
-		dev->stats.rx_errors++;
-		mvpp2_rx_error(port, rx_desc);
 		/* Return the buffer to the pool */
 		if (rx_status & MVPP2_RXD_BUF_HDR)
 			mvpp2_buff_hdr_pool_put(port, rx_desc, pool, rx_status);
 		else
 			mvpp2_bm_pool_put(port, pool, dma_addr, phys_addr);
+err_drop_frame_retired:
+		dev->stats.rx_errors++;
+		mvpp2_rx_error(port, rx_desc);
 	}
 
 	if (xdp_ret & MVPP2_XDP_REDIR)
Signature · 1 line
2.54.0
author
#5 lore raw

PATCH v5 4/4 net: mvpp2: build skb from XDP-adjusted data on XDP_PASS

In reply to Til Kaiser

When an XDP program uses bpf_xdp_adjust_head() or bpf_xdp_adjust_tail() and then returns XDP_PASS, mvpp2 still builds the skb from fixed offsets derived from the original RX descriptor. Packet geometry changes made by the XDP program are therefore discarded before the skb reaches the stack.

Update rx_offset and rx_bytes from xdp.data and xdp.data_end for XDP_PASS. This makes skb_reserve() and skb_put() reflect the packet seen by XDP, and makes RX byte accounting for XDP_PASS follow the length of the skb passed to the network stack.

Keep a separate rx_sync_size for page-pool recycling on skb allocation failure, which must stay tied to the received buffer range.

Non-PASS verdicts continue to account the descriptor length because no skb is passed up in those cases.

Fixes 07dd0a7aae7f ("mvpp2: add basic XDP support")
Signed-off-by Til Kaiser <[email protected]>
---
 .../net/ethernet/marvell/mvpp2/mvpp2_main.c   | 21 +++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)
drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c +15 -6
index 481daafdc1cb..ccc24a1301f2 100644
@@ -3917,10 +3917,10 @@ static int mvpp2_rx(struct mvpp2_port *port, struct napi_struct *napi,
 		struct mvpp2_bm_pool *bm_pool;
 		struct page_pool *pp = NULL;
 		struct sk_buff *skb;
-		unsigned int frag_size;
+		unsigned int frag_size, rx_sync_size;
 		dma_addr_t dma_addr;
 		phys_addr_t phys_addr;
-		int pool, rx_bytes, err, ret;
+		int pool, rx_bytes, rx_offset, err, ret;
 		struct page *page;
 		void *data;
 

@@ -3933,6 +3933,8 @@ static int mvpp2_rx(struct mvpp2_port *port, struct napi_struct *napi,
 		rx_status = mvpp2_rxdesc_status_get(port, rx_desc);
 		rx_bytes = mvpp2_rxdesc_size_get(port, rx_desc);
 		rx_bytes -= MVPP2_MH_SIZE;
+		rx_sync_size = rx_bytes + MVPP2_MH_SIZE;
+		rx_offset = MVPP2_MH_SIZE + MVPP2_SKB_HEADROOM;
 		dma_addr = mvpp2_rxdesc_dma_addr_get(port, rx_desc);
 
 		pool = (rx_status & MVPP2_RXD_BM_POOL_ID_MASK) >>

@@ -3948,7 +3950,7 @@ static int mvpp2_rx(struct mvpp2_port *port, struct napi_struct *napi,
 
 		dma_sync_single_range_for_cpu(dev->dev.parent, dma_addr,
 					      MVPP2_SKB_HEADROOM,
-					      rx_bytes + MVPP2_MH_SIZE,
+					      rx_sync_size,
 					      dma_dir);
 
 		/* Buffer header not supported */

@@ -3999,6 +4001,14 @@ static int mvpp2_rx(struct mvpp2_port *port, struct napi_struct *napi,
 				continue;
 			}
 
+			rx_sync_size = max_t(unsigned int, rx_sync_size,
+					     xdp.data_end - xdp.data_hard_start -
+					     MVPP2_SKB_HEADROOM);
+
+			/* Update offset and length to reflect any XDP adjustments. */
+			rx_offset = xdp.data     - data;
+			rx_bytes  = xdp.data_end - xdp.data;
+
 			metasize = xdp.data - xdp.data_meta;
 		}
 

@@ -4010,8 +4020,7 @@ static int mvpp2_rx(struct mvpp2_port *port, struct napi_struct *napi,
 			netdev_warn(port->dev, "skb build failed\n");
 			if (pp) {
 				page_pool_put_page(pp, virt_to_head_page(data),
-						   rx_bytes + MVPP2_MH_SIZE,
-						   true);
+						   rx_sync_size, true);
 			} else {
 				dma_unmap_single_attrs(dev->dev.parent, dma_addr,
 						       bm_pool->buf_size,

@@ -4041,7 +4050,7 @@ static int mvpp2_rx(struct mvpp2_port *port, struct napi_struct *napi,
 		ps.rx_packets++;
 		ps.rx_bytes += rx_bytes;
 
-		skb_reserve(skb, MVPP2_MH_SIZE + MVPP2_SKB_HEADROOM);
+		skb_reserve(skb, rx_offset);
 		skb_put(skb, rx_bytes);
 		if (metasize)
 			skb_metadata_set(skb, metasize);
Signature · 1 line
2.54.0
CI Applied
#6 lore raw
In reply to Til Kaiser
This series was applied to netdev/net.git (main) by Paolo Abeni <[email protected]>:

Hello:

This series was applied to netdev/net.git (main)
by Paolo Abeni <[email protected]>:

Quoted 8 lines · On Sun, 7 Jun 2026 15:49:39 +0200 you wrote:

This is v5 of the earlier XDP_PASS fix. The XDP_PASS change is retained, and the series also fixes related RX/XDP buffer handling issues found during review.

Tested with tools/testing/selftests/drivers/net/xdp.py on mvpp2 hardware.

[...]

Here is the summary with links:
- [net,v5,1/4] net: mvpp2: sync RX data at the hardware packet offset

    https://git.kernel.org/netdev/net/c/180235600934
  - [net,v5,2/4] net: mvpp2: limit XDP frame size to the RX buffer
    https://git.kernel.org/netdev/net/c/f3c6aa078927
  - [net,v5,3/4] net: mvpp2: refill RX buffers before XDP or skb use
    https://git.kernel.org/netdev/net/c/5e8e2a9624df
  - [net,v5,4/4] net: mvpp2: build skb from XDP-adjusted data on XDP_PASS
    https://git.kernel.org/netdev/net/c/77a6b90ce56b

You are awesome, thank you!

Signature · 2 lines
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html