[v2,2/2] net/af_xdp: Refactor af_xdp_tx_zc()

Message ID 20250116225151.188214-3-ariel.otilibili@6wind.com (mailing list archive)
State Changes Requested
Delegated to: Stephen Hemminger
Headers
Series Fix use after free, and refactor af_xdp_tx_zc() |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/loongarch-compilation success Compilation OK
ci/loongarch-unit-testing success Unit Testing PASS
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/github-robot: build success github build: passed
ci/iol-mellanox-Performance success Performance Testing PASS
ci/intel-Functional success Functional PASS
ci/iol-marvell-Functional success Functional Testing PASS
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-sample-apps-testing success Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-unit-amd64-testing success Testing PASS
ci/iol-abi-testing success Testing PASS
ci/iol-unit-arm64-testing success Testing PASS
ci/iol-compile-arm64-testing success Testing PASS
ci/iol-compile-amd64-testing success Testing PASS

Commit Message

Ariel Otilibili Jan. 16, 2025, 10:51 p.m. UTC
Both legs of the loop share the same logic: either to go out of the
function, or to fall through to the next instructions.

Depends-on: patch-1 ("net/af_xdp: fix use after free in af_xdp_tx_zc()")
Bugzilla ID: 1440
Signed-off-by: Ariel Otilibili <ariel.otilibili@6wind.com>
---
 drivers/net/af_xdp/rte_eth_af_xdp.c | 47 +++++++++++++----------------
 1 file changed, 21 insertions(+), 26 deletions(-)
  

Comments

Maryam Tahhan Jan. 20, 2025, 3:28 p.m. UTC | #1
On 16/01/2025 17:51, Ariel Otilibili wrote:
> Both legs of the loop share the same logic: either to go out of the
> function, or to fall through to the next instructions.
>
> Depends-on: patch-1 ("net/af_xdp: fix use after free in af_xdp_tx_zc()")
> Bugzilla ID: 1440
> Signed-off-by: Ariel Otilibili <ariel.otilibili@6wind.com>
> ---
>   drivers/net/af_xdp/rte_eth_af_xdp.c | 47 +++++++++++++----------------
>   1 file changed, 21 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/net/af_xdp/rte_eth_af_xdp.c b/drivers/net/af_xdp/rte_eth_af_xdp.c
> index 4326a29f7042..b1de47a41eb3 100644
> --- a/drivers/net/af_xdp/rte_eth_af_xdp.c
> +++ b/drivers/net/af_xdp/rte_eth_af_xdp.c
> @@ -551,6 +551,7 @@ af_xdp_tx_zc(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
>   	uint64_t addr, offset;
>   	struct xsk_ring_cons *cq = &txq->pair->cq;
>   	uint32_t free_thresh = cq->size >> 1;
> +	struct rte_mbuf *local_mbuf = NULL;
>   
>   	if (xsk_cons_nb_avail(cq, free_thresh) >= free_thresh)
>   		pull_umem_cq(umem, XSK_RING_CONS__DEFAULT_NUM_DESCS, cq);
> @@ -565,21 +566,8 @@ af_xdp_tx_zc(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
>   							    &idx_tx))
>   					goto out;
>   			}
> -			desc = xsk_ring_prod__tx_desc(&txq->tx, idx_tx);
> -			desc->len = mbuf->pkt_len;
> -			addr = (uint64_t)mbuf - (uint64_t)umem->buffer -
> -					umem->mb_pool->header_size;
> -			offset = rte_pktmbuf_mtod(mbuf, uint64_t) -
> -					(uint64_t)mbuf +
> -					umem->mb_pool->header_size;
> -			offset = offset << XSK_UNALIGNED_BUF_OFFSET_SHIFT;
> -			desc->addr = addr | offset;
> -			tx_bytes += mbuf->pkt_len;
> -			count++;
>   		} else {
> -			struct rte_mbuf *local_mbuf =
> -					rte_pktmbuf_alloc(umem->mb_pool);
> -			void *pkt;
> +			local_mbuf = rte_pktmbuf_alloc(umem->mb_pool);
>   
>   			if (local_mbuf == NULL)
>   				goto out;
> @@ -588,24 +576,31 @@ af_xdp_tx_zc(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
>   				rte_pktmbuf_free(local_mbuf);
>   				goto out;
>   			}
> +		}
>   
> -			desc = xsk_ring_prod__tx_desc(&txq->tx, idx_tx);
> -			desc->len = mbuf->pkt_len;
> +		struct rte_mbuf *tmp;
> +		void *pkt;
> +		tmp = mbuf->pool == umem->mb_pool ? mbuf : local_mbuf;
> +
> +		desc = xsk_ring_prod__tx_desc(&txq->tx, idx_tx);
> +		desc->len = mbuf->pkt_len;
>   
> -			addr = (uint64_t)local_mbuf - (uint64_t)umem->buffer -
> -					umem->mb_pool->header_size;
> -			offset = rte_pktmbuf_mtod(local_mbuf, uint64_t) -
> -					(uint64_t)local_mbuf +
> -					umem->mb_pool->header_size;
> +		addr = (uint64_t)tmp - (uint64_t)umem->buffer
> +			- umem->mb_pool->header_size;
> +		offset = rte_pktmbuf_mtod(tmp, uint64_t) - (uint64_t)tmp
> +			+ umem->mb_pool->header_size;
> +		offset = offset << XSK_UNALIGNED_BUF_OFFSET_SHIFT;
> +		desc->addr = addr | offset;
> +
> +		if (mbuf->pool == umem->mb_pool) {
> +			tx_bytes += mbuf->pkt_len;
> +		} else {
>   			pkt = xsk_umem__get_data(umem->buffer, addr + offset);
> -			offset = offset << XSK_UNALIGNED_BUF_OFFSET_SHIFT;
> -			desc->addr = addr | offset;
> -			rte_memcpy(pkt, rte_pktmbuf_mtod(mbuf, void *),
> -					desc->len);
> +			rte_memcpy(pkt, rte_pktmbuf_mtod(mbuf, void *), desc->len);
>   			tx_bytes += mbuf->pkt_len;
>   			rte_pktmbuf_free(mbuf);
> -			count++;
>   		}
> +		count++;
>   	}
>   
>   out:


This ends up duplicating the if condition `if (mbuf->pool == 
umem->mb_pool) {` twice in `af_xdp_tx_zc`. Which is messy to read tbh...

I think it would be better to create an inline function for the 
duplicate code that setting desc, addr and offset. These three things 
could be pointers passed to the new inline function and that way their 
values can be used in `af_xdp_tx_zc()` after they are set. I think that 
would cleanup the `af_xdp_tx_zc()` function in a neater way.
  
Ariel Otilibili Jan. 21, 2025, 8:57 a.m. UTC | #2
Hi Maryam,

On Mon, Jan 20, 2025 at 4:28 PM Maryam Tahhan <mtahhan@redhat.com> wrote:

>
> On 16/01/2025 17:51, Ariel Otilibili wrote:
>
This ends up duplicating the if condition `if (mbuf->pool ==
> umem->mb_pool) {` twice in `af_xdp_tx_zc`. Which is messy to read tbh...
>
> I think it would be better to create an inline function for the
> duplicate code that setting desc, addr and offset. These three things
> could be pointers passed to the new inline function and that way their
> values can be used in `af_xdp_tx_zc()` after they are set. I think that
> would cleanup the `af_xdp_tx_zc()` function in a neater way.
>

Thanks for having looked into this patch. I'll improve the series on your
feedback.
  

Patch

diff --git a/drivers/net/af_xdp/rte_eth_af_xdp.c b/drivers/net/af_xdp/rte_eth_af_xdp.c
index 4326a29f7042..b1de47a41eb3 100644
--- a/drivers/net/af_xdp/rte_eth_af_xdp.c
+++ b/drivers/net/af_xdp/rte_eth_af_xdp.c
@@ -551,6 +551,7 @@  af_xdp_tx_zc(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 	uint64_t addr, offset;
 	struct xsk_ring_cons *cq = &txq->pair->cq;
 	uint32_t free_thresh = cq->size >> 1;
+	struct rte_mbuf *local_mbuf = NULL;
 
 	if (xsk_cons_nb_avail(cq, free_thresh) >= free_thresh)
 		pull_umem_cq(umem, XSK_RING_CONS__DEFAULT_NUM_DESCS, cq);
@@ -565,21 +566,8 @@  af_xdp_tx_zc(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 							    &idx_tx))
 					goto out;
 			}
-			desc = xsk_ring_prod__tx_desc(&txq->tx, idx_tx);
-			desc->len = mbuf->pkt_len;
-			addr = (uint64_t)mbuf - (uint64_t)umem->buffer -
-					umem->mb_pool->header_size;
-			offset = rte_pktmbuf_mtod(mbuf, uint64_t) -
-					(uint64_t)mbuf +
-					umem->mb_pool->header_size;
-			offset = offset << XSK_UNALIGNED_BUF_OFFSET_SHIFT;
-			desc->addr = addr | offset;
-			tx_bytes += mbuf->pkt_len;
-			count++;
 		} else {
-			struct rte_mbuf *local_mbuf =
-					rte_pktmbuf_alloc(umem->mb_pool);
-			void *pkt;
+			local_mbuf = rte_pktmbuf_alloc(umem->mb_pool);
 
 			if (local_mbuf == NULL)
 				goto out;
@@ -588,24 +576,31 @@  af_xdp_tx_zc(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 				rte_pktmbuf_free(local_mbuf);
 				goto out;
 			}
+		}
 
-			desc = xsk_ring_prod__tx_desc(&txq->tx, idx_tx);
-			desc->len = mbuf->pkt_len;
+		struct rte_mbuf *tmp;
+		void *pkt;
+		tmp = mbuf->pool == umem->mb_pool ? mbuf : local_mbuf;
+
+		desc = xsk_ring_prod__tx_desc(&txq->tx, idx_tx);
+		desc->len = mbuf->pkt_len;
 
-			addr = (uint64_t)local_mbuf - (uint64_t)umem->buffer -
-					umem->mb_pool->header_size;
-			offset = rte_pktmbuf_mtod(local_mbuf, uint64_t) -
-					(uint64_t)local_mbuf +
-					umem->mb_pool->header_size;
+		addr = (uint64_t)tmp - (uint64_t)umem->buffer
+			- umem->mb_pool->header_size;
+		offset = rte_pktmbuf_mtod(tmp, uint64_t) - (uint64_t)tmp
+			+ umem->mb_pool->header_size;
+		offset = offset << XSK_UNALIGNED_BUF_OFFSET_SHIFT;
+		desc->addr = addr | offset;
+
+		if (mbuf->pool == umem->mb_pool) {
+			tx_bytes += mbuf->pkt_len;
+		} else {
 			pkt = xsk_umem__get_data(umem->buffer, addr + offset);
-			offset = offset << XSK_UNALIGNED_BUF_OFFSET_SHIFT;
-			desc->addr = addr | offset;
-			rte_memcpy(pkt, rte_pktmbuf_mtod(mbuf, void *),
-					desc->len);
+			rte_memcpy(pkt, rte_pktmbuf_mtod(mbuf, void *), desc->len);
 			tx_bytes += mbuf->pkt_len;
 			rte_pktmbuf_free(mbuf);
-			count++;
 		}
+		count++;
 	}
 
 out: