In aligned mode, a fill ring address maps straight to its pool->heads[] entry. If user space posts the same address twice, a driver can pass xsk_buff_add_frag() a buffer that is the head of the frame or is already on pool->xskb_list or pool->free_list. Re-adding a linked node corrupts the list. With buffers A, B, C, B, xsk_rcv_zc() emits two descriptors instead of four and leaves the list head pointing at a self-linked B. When the Rx ring later has no room for a frame, the xsk_buff_free() cleanup follows B into pool->free_list and passes the list head to xp_free(): BUG: kernel NULL pointer dereference, address: 00000000000000c0 RIP: 0010:xp_free+0x1a/0x50 Call Trace: xsk_rcv_zc+0xca/0x200 The head is not on any list and needs a separate check. Adding it as its own frag lets bpf_xdp_adjust_tail() free the in-use head and wrap sinfo->nr_frags to 255. Reject such buffers before adding them to the frags array, as commit e37b2abca804 ("xsk: fix NULL pointer dereference in __xsk_rcv()") does in the copy path. Callers already drop the frame on failure. Unaligned mode is not affected, as each fill ring entry gets its own xskb from pool->free_heads[]. Reproduced with a KUnit test feeding synthetic buffers to xsk_rcv_zc(), by Nagato Shiratana. Fixes: 24ea50127ecf ("xsk: support mbuf on ZC RX") Reported-by: Nagato Shiratana Signed-off-by: Maciej Fijalkowski --- include/net/xdp_sock_drv.h | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/include/net/xdp_sock_drv.h b/include/net/xdp_sock_drv.h index d94aeb506379..29965e369598 100644 --- a/include/net/xdp_sock_drv.h +++ b/include/net/xdp_sock_drv.h @@ -160,15 +160,18 @@ static inline void xsk_buff_free(struct xdp_buff *xdp) static inline bool xsk_buff_add_frag(struct xdp_buff *head, struct xdp_buff *xdp) { + struct xdp_buff_xsk *frag = container_of(xdp, struct xdp_buff_xsk, xdp); const void *data = xdp->data; - struct xdp_buff_xsk *frag; + + /* Duplicate fill ring entries map to the same xskb in aligned mode */ + if (unlikely(xdp == head || !list_empty(&frag->list_node))) + return false; if (!__xdp_buff_add_frag(head, virt_to_netmem(data), offset_in_page(data), xdp->data_end - data, xdp->frame_sz, false)) return false; - frag = container_of(xdp, struct xdp_buff_xsk, xdp); list_add_tail(&frag->list_node, &frag->pool->xskb_list); return true; -- 2.43.0 When xsk_buff_add_frag() fails, i40e_clean_rx_irq_zc() frees the frame but stays on the current descriptor, so the next poll restarts from the freed head. If the frags array was full, the retry passes on a frame missing the freed frags, with its head already on the free list. If the buffer is a duplicate, every poll fails the same way and the queue stalls. Free the current buffer along with the frame and move on to the next descriptor, as ice does. The rest of the dropped frame is received as a new frame. xp_free() skips a duplicate already freed with the frame. Fixes: 1c9ba9c14658 ("i40e: xsk: add RX multi-buffer support") Signed-off-by: Maciej Fijalkowski --- drivers/net/ethernet/intel/i40e/i40e_xsk.c | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/drivers/net/ethernet/intel/i40e/i40e_xsk.c b/drivers/net/ethernet/intel/i40e/i40e_xsk.c index 51a05ce4c7ce..e6ae4c7a5cb3 100644 --- a/drivers/net/ethernet/intel/i40e/i40e_xsk.c +++ b/drivers/net/ethernet/intel/i40e/i40e_xsk.c @@ -469,16 +469,24 @@ int i40e_clean_rx_irq_zc(struct i40e_ring *rx_ring, int budget) xsk_buff_set_size(bi, size); xsk_buff_dma_sync_for_cpu(bi); - if (!first) + if (++next_to_process == count) + next_to_process = 0; + + if (!first) { first = bi; - else if (!xsk_buff_add_frag(first, bi)) { + } else if (!xsk_buff_add_frag(first, bi)) { + /* Drop what has been gathered so far. xp_free() skips + * @bi if it duplicates a buffer that is already on a + * pool list. Remaining buffers of this packet, if any, + * will be processed as a new packet. + */ xsk_buff_free(first); - break; + xsk_buff_free(bi); + next_to_clean = next_to_process; + first = NULL; + continue; } - if (++next_to_process == count) - next_to_process = 0; - if (i40e_is_non_eop(rx_ring, rx_desc)) continue; -- 2.43.0 ice_clean_rx_irq_zc() consumes a descriptor without freeing its buffer when a frag has zero length or when xsk_buff_add_frag() fails, which happens if the frags array is full or the fill ring has duplicates. Such buffers are lost to user space until the socket is closed. Free the buffer in both cases. xp_free() skips a duplicate already freed with the frame. Fixes: 1bbc04de607b ("ice: xsk: add RX multi-buffer support") Fixes: 93f53db9f9dc ("ice: switch to Page Pool") Signed-off-by: Maciej Fijalkowski --- drivers/net/ethernet/intel/ice/ice_xsk.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/drivers/net/ethernet/intel/ice/ice_xsk.c b/drivers/net/ethernet/intel/ice/ice_xsk.c index 0643017541c3..ea8ac0fcdf6e 100644 --- a/drivers/net/ethernet/intel/ice/ice_xsk.c +++ b/drivers/net/ethernet/intel/ice/ice_xsk.c @@ -618,8 +618,11 @@ int ice_clean_rx_irq_zc(struct ice_rx_ring *rx_ring, if (!first) { first = xdp; - } else if (likely(size) && !xsk_buff_add_frag(first, xdp)) { + } else if (unlikely(!size)) { + xsk_buff_free(xdp); + } else if (!xsk_buff_add_frag(first, xdp)) { xsk_buff_free(first); + xsk_buff_free(xdp); first = NULL; } -- 2.43.0 ice_xmit_xdp_tx_zc() takes the frags of a multi-buffer frame off pool->xskb_list but leaves XDP_FLAGS_HAS_FRAGS set on the head. When the head completes, xsk_buff_free() frees whatever is on the list at that point, which are the frags of a frame still being received. If ice_xmit_xdp_tx_zc() completes such a head while cleaning the ring before sending another frame, xsk_buff_get_frag() returns NULL and xsk_buff_xdp_get_dma() dereferences it. Clear the flag on the head once its frags are on the ring. As xdp_get_buff_len() then covers only the head, count Rx bytes before running the XDP program, like libeth does. Fixes: eeb2b5381038 ("ice: xsk: Tx multi-buffer support") Signed-off-by: Maciej Fijalkowski --- drivers/net/ethernet/intel/ice/ice_xsk.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/drivers/net/ethernet/intel/ice/ice_xsk.c b/drivers/net/ethernet/intel/ice/ice_xsk.c index ea8ac0fcdf6e..bf14ed8f87a3 100644 --- a/drivers/net/ethernet/intel/ice/ice_xsk.c +++ b/drivers/net/ethernet/intel/ice/ice_xsk.c @@ -493,6 +493,11 @@ static int ice_xmit_xdp_tx_zc(struct xdp_buff *xdp, tx_desc->cmd_type_offset_bsz |= cpu_to_le64(ICE_TX_DESC_CMD_EOP << ICE_TXD_QW1_CMD_S); + /* Each frag is now freed via its own Tx buffer. Completing the head + * must not release what is on the pool's frag list at that time. + */ + xdp_buff_clear_frags_flag(head); + return ICE_XDP_TX; busy: @@ -592,6 +597,7 @@ int ice_clean_rx_irq_zc(struct ice_rx_ring *rx_ring, struct sk_buff *skb; u16 stat_err_bits; u16 vlan_tci; + u32 xdp_len; rx_desc = ICE_RX_DESC(rx_ring, ntc); @@ -634,6 +640,7 @@ int ice_clean_rx_irq_zc(struct ice_rx_ring *rx_ring, ((struct libeth_xdp_buff *)first)->desc = rx_desc; + xdp_len = xdp_get_buff_len(first); xdp_res = ice_run_xdp_zc(rx_ring, first, xdp_prog, xdp_ring, xsk_pool); if (likely(xdp_res & (ICE_XDP_TX | ICE_XDP_REDIR))) { @@ -648,7 +655,7 @@ int ice_clean_rx_irq_zc(struct ice_rx_ring *rx_ring, goto construct_skb; } - total_rx_bytes += xdp_get_buff_len(first); + total_rx_bytes += xdp_len; total_rx_packets++; first = NULL; -- 2.43.0 libeth_xsk_tx_queue_bulk() takes the frags of a multi-buffer frame off pool->xskb_list but leaves XDP_FLAGS_HAS_FRAGS set on the head. When the head completes, libeth_xsk_buff_free_slow() frees whatever is on the list at that point. The XDPSQ can be completed by a bulk flush while the frags of a frame are being queued, or by its cleanup timer while a partial Rx frame is stashed. Those frags are then freed while in use, and the frame being queued is sent truncated. Clear the flag on the head once all of its frags are queued. Fixes: b3ad8450b4dc ("libeth: xsk: add XSk XDP_TX sending helpers") Signed-off-by: Maciej Fijalkowski --- include/net/libeth/xsk.h | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/include/net/libeth/xsk.h b/include/net/libeth/xsk.h index a452b7828ce4..249b1c740788 100644 --- a/include/net/libeth/xsk.h +++ b/include/net/libeth/xsk.h @@ -77,11 +77,17 @@ libeth_xsk_tx_queue_bulk(struct libeth_xdp_tx_bulk *bq, if (!libeth_xsk_tx_queue_head(bq, xdp)) goto out; - for (const struct libeth_xdp_buff *head = xdp; ; ) { + for (struct libeth_xdp_buff *head = xdp; ; ) { xdp = container_of(xsk_buff_get_frag(&head->base), typeof(*xdp), base); - if (!xdp) + if (!xdp) { + /* Each frag is now freed via its own SQE. Freeing + * the head must not release what is on the pool's + * frag list at that time. + */ + xdp_buff_clear_frags_flag(&head->base); break; + } if (unlikely(bq->count == LIBETH_XDP_TX_BULK) && unlikely(!flush_bulk(bq, LIBETH_XDP_TX_XSK))) { -- 2.43.0 xp_alloc() and xsk_buff_set_size() reset xdp_buff::flags of every buffer they hand out. In aligned mode, duplicate fill ring addresses map to the same xskb, so the head of a frame still being gathered can be handed out again and lose XDP_FLAGS_HAS_FRAGS. xsk_buff_add_frag() rejects the duplicate and the frame is dropped, but xsk_buff_free() leaves its frags on pool->xskb_list, where they are taken as frags of the next frame. Keep XDP_FLAGS_HAS_FRAGS on hand-out and clear it when the frags are released, in xsk_buff_free() and xsk_rcv_zc(). The driver XDP_TX paths, bpf_xdp_adjust_tail() and bpf_xdp_pull_data() already clear it, and pool->heads starts zeroed. Other flags are still reset on hand-out, as nothing clears them on release. Since xsk_rcv_zc() now clears the flag, i40e must count Rx bytes before running the XDP program, like ice and libeth. Fixes: f7f6aa8e2438 ("xsk: make xsk_buff_pool responsible for clearing xdp_buff::flags") Signed-off-by: Maciej Fijalkowski --- drivers/net/ethernet/intel/i40e/i40e_xsk.c | 2 +- include/net/xdp_sock_drv.h | 4 +++- net/xdp/xsk.c | 1 + net/xdp/xsk_buff_pool.c | 2 +- 4 files changed, 6 insertions(+), 3 deletions(-) diff --git a/drivers/net/ethernet/intel/i40e/i40e_xsk.c b/drivers/net/ethernet/intel/i40e/i40e_xsk.c index e6ae4c7a5cb3..57b0f520fbb2 100644 --- a/drivers/net/ethernet/intel/i40e/i40e_xsk.c +++ b/drivers/net/ethernet/intel/i40e/i40e_xsk.c @@ -354,7 +354,6 @@ static void i40e_handle_xdp_result_zc(struct i40e_ring *rx_ring, struct sk_buff *skb; *rx_packets = 1; - *rx_bytes = xdp_get_buff_len(xdp_buff); if (likely(xdp_res == I40E_XDP_REDIR) || xdp_res == I40E_XDP_TX) return; @@ -492,6 +491,7 @@ int i40e_clean_rx_irq_zc(struct i40e_ring *rx_ring, int budget) xsk_buff_to_i40e_ctx(first)->desc = rx_desc; + rx_bytes = xdp_get_buff_len(first); xdp_res = i40e_run_xdp_zc(rx_ring, first, xdp_prog); i40e_handle_xdp_result_zc(rx_ring, first, rx_desc, &rx_packets, &rx_bytes, xdp_res, &failure); diff --git a/include/net/xdp_sock_drv.h b/include/net/xdp_sock_drv.h index 29965e369598..a40ad3f90f13 100644 --- a/include/net/xdp_sock_drv.h +++ b/include/net/xdp_sock_drv.h @@ -153,6 +153,7 @@ static inline void xsk_buff_free(struct xdp_buff *xdp) } xdp_get_shared_info_from_buff(xdp)->nr_frags = 0; + xdp_buff_clear_frags_flag(xdp); out: xp_free(xskb); } @@ -225,7 +226,8 @@ static inline void xsk_buff_set_size(struct xdp_buff *xdp, u32 size) xdp->data = xdp->data_hard_start + XDP_PACKET_HEADROOM; xdp->data_meta = xdp->data; xdp->data_end = xdp->data + size; - xdp->flags = 0; + /* Frags flag is cleared when the frags of a head are released */ + xdp->flags &= XDP_FLAGS_HAS_FRAGS; } static inline dma_addr_t xsk_buff_raw_get_dma(struct xsk_buff_pool *pool, diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c index 33475b180ea6..d22be82533ae 100644 --- a/net/xdp/xsk.c +++ b/net/xdp/xsk.c @@ -241,6 +241,7 @@ static int xsk_rcv_zc(struct xdp_sock *xs, struct xdp_buff *xdp, u32 len) goto err; } + xdp_buff_clear_frags_flag(xdp); __xsk_rcv_zc_safe(xs, xskb, len, contd); xskb_list = &xskb->pool->xskb_list; list_for_each_entry_safe(pos, tmp, xskb_list, list_node) { diff --git a/net/xdp/xsk_buff_pool.c b/net/xdp/xsk_buff_pool.c index 9d2d94f1fb75..4318ec98199e 100644 --- a/net/xdp/xsk_buff_pool.c +++ b/net/xdp/xsk_buff_pool.c @@ -597,7 +597,7 @@ struct xdp_buff *xp_alloc(struct xsk_buff_pool *pool) xskb->xdp.data = xskb->xdp.data_hard_start + XDP_PACKET_HEADROOM; xskb->xdp.data_meta = xskb->xdp.data; - xskb->xdp.flags = 0; + xskb->xdp.flags &= XDP_FLAGS_HAS_FRAGS; if (pool->dev) xp_dma_sync_for_device(pool, xskb->dma, pool->frame_len); -- 2.43.0 From: Chenguang Zhao When AF_XDP ZC receives a multi-buffer frame and the XDP program returns XDP_PASS, i40e_construct_skb_zc() copies frags into a new skb. The copy used skb_frag_page() as the memcpy source (page metadata instead of packet data) and passed a virtual address to __skb_fill_page_desc_noacc(), which expects a struct page *. Use skb_frag_address() for the copy, attach frags with skb_add_rx_frag() so len/data_len/truesize are updated, and on dev_alloc_page() failure free the skb via the shared out path so xsk_buff_free() still runs and previously attached pages are released by kfree_skb. Fixes: 1c9ba9c14658 ("i40e: xsk: add RX multi-buffer support") Signed-off-by: Chenguang Zhao Reviewed-by: Aleksandr Loktionov Reviewed-by: Jason Xing Acked-by: Maciej Fijalkowski Tested-by: Patryk Holda Signed-off-by: Tony Nguyen --- drivers/net/ethernet/intel/i40e/i40e_xsk.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/drivers/net/ethernet/intel/i40e/i40e_xsk.c b/drivers/net/ethernet/intel/i40e/i40e_xsk.c index 57b0f520fbb2..5f5f33b2a097 100644 --- a/drivers/net/ethernet/intel/i40e/i40e_xsk.c +++ b/drivers/net/ethernet/intel/i40e/i40e_xsk.c @@ -320,22 +320,19 @@ static struct sk_buff *i40e_construct_skb_zc(struct i40e_ring *rx_ring, goto out; for (int i = 0; i < nr_frags; i++) { - struct skb_shared_info *skinfo = skb_shinfo(skb); skb_frag_t *frag = &sinfo->frags[i]; + unsigned int frag_size = skb_frag_size(frag); struct page *page; - void *addr; page = dev_alloc_page(); if (!page) { dev_kfree_skb(skb); - return NULL; + skb = NULL; + goto out; } - addr = page_to_virt(page); - memcpy(addr, skb_frag_page(frag), skb_frag_size(frag)); - - __skb_fill_page_desc_noacc(skinfo, skinfo->nr_frags++, - addr, 0, skb_frag_size(frag)); + memcpy(page_to_virt(page), skb_frag_address(frag), frag_size); + skb_add_rx_frag(skb, i, page, 0, frag_size, PAGE_SIZE); } out: -- 2.43.0 ice_clean_rx_irq_zc() stashes a partially received multi-buffer frame in rx_ring->xsk, with its frags on pool->xskb_list. next_to_clean is already past its buffers, so ice_xsk_clean_rx_ring(), which walks next_to_clean..next_to_use, misses the frame, and ice_vsi_cfg_rxq() later clears the pointer via ring->xdp.data. The buffers are lost to user space and, as the pool outlives the ring across ifdown/ifup or a reset, the stale frags are taken as frags of the next frame. Before commit 93f53db9f9dc ("ice: switch to Page Pool"), the frame was tracked by rx_ring->first_desc and leaked the same way. Free the stashed frame when cleaning the ring. Fixes: 1bbc04de607b ("ice: xsk: add RX multi-buffer support") Signed-off-by: Maciej Fijalkowski --- drivers/net/ethernet/intel/ice/ice_xsk.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/drivers/net/ethernet/intel/ice/ice_xsk.c b/drivers/net/ethernet/intel/ice/ice_xsk.c index bf14ed8f87a3..0c1ce3e86a45 100644 --- a/drivers/net/ethernet/intel/ice/ice_xsk.c +++ b/drivers/net/ethernet/intel/ice/ice_xsk.c @@ -912,6 +912,11 @@ void ice_xsk_clean_rx_ring(struct ice_rx_ring *rx_ring) if (xdp_rxq_info_is_reg(&rx_ring->xdp_rxq)) xdp_rxq_info_unreg(&rx_ring->xdp_rxq); + if (rx_ring->xsk) { + xsk_buff_free(&rx_ring->xsk->base); + rx_ring->xsk = NULL; + } + while (ntc != ntu) { struct xdp_buff *xdp = *ice_xdp_buf(rx_ring, ntc); -- 2.43.0 idpf_xskrq_poll() stashes a partially received multi-buffer frame in rxq->xsk, with its frags on pool->xskb_list. Nothing frees or clears rxq->xsk on queue release, as idpf_rx_desc_rel() only returns the page stash sharing its storage for non-XSk queues. On ifdown, the buffers are lost to user space and, as the pool outlives the queue, the stale frags are taken as frags of the next frame. If the pool is disabled on a running interface, the queue restarts in page mode with rxq->xdp.data still pointing to an xskb of the pool, which is freed later. The page path then takes it for a partial frame, writes frag info through it and returns it to a page pool. Free the stashed frame and clear the pointer when the XSk state of the queue is cleared, while the pool is still attached. Fixes: 9705d6552f58 ("idpf: implement Rx path for AF_XDP") Signed-off-by: Maciej Fijalkowski --- drivers/net/ethernet/intel/idpf/xsk.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/drivers/net/ethernet/intel/idpf/xsk.c b/drivers/net/ethernet/intel/idpf/xsk.c index d95d3efdfd36..5a0a1d8a7a86 100644 --- a/drivers/net/ethernet/intel/idpf/xsk.c +++ b/drivers/net/ethernet/intel/idpf/xsk.c @@ -134,6 +134,11 @@ void idpf_xsk_clear_queue(void *q, enum virtchnl2_queue_type type) if (!idpf_queue_has_clear(XSK, rxq)) return; + if (rxq->xsk) { + libeth_xsk_buff_free_slow(rxq->xsk); + rxq->xsk = NULL; + } + rxq->pool = NULL; break; case VIRTCHNL2_QUEUE_TYPE_RX_BUFFER: -- 2.43.0