* [PATCH 0/2] Fix use after free, and refactor af_xdp_tx_zc()
@ 2025-01-16 19:56 Ariel Otilibili
2025-01-16 19:56 ` [PATCH 1/2] net/af_xdp: fix use after free in af_xdp_tx_zc() Ariel Otilibili
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Ariel Otilibili @ 2025-01-16 19:56 UTC (permalink / raw)
To: dev
Cc: stable, Stephen Hemminger, Thomas Monjalon, David Marchand,
Ciara Loftus, Maryam Tahhan, Ariel Otilibili
Hello,
The series addresses Bugzilla ID 1440 in two steps;
1. Fix use after free.
2. Refactor af_xdp_tx_zc().
Thank you,
Ariel Otilibili (2):
net/af_xdp: fix use after free in af_xdp_tx_zc()
net/af_xdp: Refactor af_xdp_tx_zc()
drivers/net/af_xdp/rte_eth_af_xdp.c | 53 ++++++++++++++---------------
1 file changed, 25 insertions(+), 28 deletions(-)
--
2.30.2
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH 1/2] net/af_xdp: fix use after free in af_xdp_tx_zc() 2025-01-16 19:56 [PATCH 0/2] Fix use after free, and refactor af_xdp_tx_zc() Ariel Otilibili @ 2025-01-16 19:56 ` Ariel Otilibili 2025-01-16 19:56 ` [PATCH 2/2] net/af_xdp: Refactor af_xdp_tx_zc() Ariel Otilibili 2025-01-16 22:51 ` [PATCH v2 0/2] Fix use after free, and refactor af_xdp_tx_zc() Ariel Otilibili 2 siblings, 0 replies; 12+ messages in thread From: Ariel Otilibili @ 2025-01-16 19:56 UTC (permalink / raw) To: dev Cc: stable, Stephen Hemminger, Thomas Monjalon, David Marchand, Ariel Otilibili, Ciara Loftus, Maryam Tahhan tx_bytes is computed after both branches are tested. This might produce a use after memory free. The computation is now moved into both branches. Bugzilla ID: 1440 Fixes: d8a210774e1d ("net/af_xdp: support unaligned umem chunks") Signed-off-by: Ariel Otilibili <ariel.otilibili@6wind.com> --- drivers/net/af_xdp/rte_eth_af_xdp.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/net/af_xdp/rte_eth_af_xdp.c b/drivers/net/af_xdp/rte_eth_af_xdp.c index 814398ba4b44..4326a29f7042 100644 --- a/drivers/net/af_xdp/rte_eth_af_xdp.c +++ b/drivers/net/af_xdp/rte_eth_af_xdp.c @@ -574,6 +574,7 @@ af_xdp_tx_zc(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts) 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 = @@ -601,11 +602,10 @@ af_xdp_tx_zc(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts) desc->addr = addr | offset; rte_memcpy(pkt, rte_pktmbuf_mtod(mbuf, void *), desc->len); + tx_bytes += mbuf->pkt_len; rte_pktmbuf_free(mbuf); count++; } - - tx_bytes += mbuf->pkt_len; } out: -- 2.30.2 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 2/2] net/af_xdp: Refactor af_xdp_tx_zc() 2025-01-16 19:56 [PATCH 0/2] Fix use after free, and refactor af_xdp_tx_zc() Ariel Otilibili 2025-01-16 19:56 ` [PATCH 1/2] net/af_xdp: fix use after free in af_xdp_tx_zc() Ariel Otilibili @ 2025-01-16 19:56 ` Ariel Otilibili 2025-01-16 21:47 ` Stephen Hemminger 2025-01-16 22:51 ` [PATCH v2 0/2] Fix use after free, and refactor af_xdp_tx_zc() Ariel Otilibili 2 siblings, 1 reply; 12+ messages in thread From: Ariel Otilibili @ 2025-01-16 19:56 UTC (permalink / raw) To: dev Cc: stable, Stephen Hemminger, Thomas Monjalon, David Marchand, Ariel Otilibili, Ciara Loftus, Maryam Tahhan Both branches of the loop share the same logic. Now each one is a goto dispatcher; either to out (end of function), or to stats (continuation of the loop). Bugzilla ID: 1440 Depends-on: patch-1 ("net/af_xdp: fix use after free in af_xdp_tx_zc()") Signed-off-by: Ariel Otilibili <ariel.otilibili@6wind.com> --- drivers/net/af_xdp/rte_eth_af_xdp.c | 57 ++++++++++++++--------------- 1 file changed, 27 insertions(+), 30 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..8b42704b6d9f 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,10 @@ 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++; + + goto stats; } 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; @@ -589,23 +579,30 @@ af_xdp_tx_zc(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts) goto out; } - 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; - 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); - tx_bytes += mbuf->pkt_len; - rte_pktmbuf_free(mbuf); - count++; + goto stats; } +stats: + 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)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); + rte_memcpy(pkt, rte_pktmbuf_mtod(mbuf, void *), desc->len); + tx_bytes += mbuf->pkt_len; + rte_pktmbuf_free(mbuf); + } + count++; } out: -- 2.30.2 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] net/af_xdp: Refactor af_xdp_tx_zc() 2025-01-16 19:56 ` [PATCH 2/2] net/af_xdp: Refactor af_xdp_tx_zc() Ariel Otilibili @ 2025-01-16 21:47 ` Stephen Hemminger 2025-01-16 22:20 ` Ariel Otilibili 0 siblings, 1 reply; 12+ messages in thread From: Stephen Hemminger @ 2025-01-16 21:47 UTC (permalink / raw) To: Ariel Otilibili Cc: dev, stable, Thomas Monjalon, David Marchand, Ciara Loftus, Maryam Tahhan On Thu, 16 Jan 2025 20:56:39 +0100 Ariel Otilibili <ariel.otilibili@6wind.com> wrote: > Both branches of the loop share the same logic. Now each one is a > goto dispatcher; either to out (end of function), or to > stats (continuation of the loop). > > Bugzilla ID: 1440 > Depends-on: patch-1 ("net/af_xdp: fix use after free in af_xdp_tx_zc()") > Signed-off-by: Ariel Otilibili <ariel.otilibili@6wind.com> > --- > drivers/net/af_xdp/rte_eth_af_xdp.c | 57 ++++++++++++++--------------- > 1 file changed, 27 insertions(+), 30 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..8b42704b6d9f 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,10 @@ 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++; > + > + goto stats; > } 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; > @@ -589,23 +579,30 @@ af_xdp_tx_zc(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts) > goto out; > } > > - 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; > - 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); > - tx_bytes += mbuf->pkt_len; > - rte_pktmbuf_free(mbuf); > - count++; > + goto stats; > } > +stats: > + 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)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); > + rte_memcpy(pkt, rte_pktmbuf_mtod(mbuf, void *), desc->len); > + tx_bytes += mbuf->pkt_len; > + rte_pktmbuf_free(mbuf); > + } > + count++; > } > > out: Indentation here is wrong, and looks suspect. Either stats tag should be outside of loop Or stats is inside loop, and both of those goto's are unnecessary ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] net/af_xdp: Refactor af_xdp_tx_zc() 2025-01-16 21:47 ` Stephen Hemminger @ 2025-01-16 22:20 ` Ariel Otilibili 2025-01-16 22:26 ` Stephen Hemminger 0 siblings, 1 reply; 12+ messages in thread From: Ariel Otilibili @ 2025-01-16 22:20 UTC (permalink / raw) To: stephen Cc: dev, stable, Thomas Monjalon, David Marchand, Ciara Loftus, Maryam Tahhan [-- Attachment #1: Type: text/plain, Size: 5219 bytes --] Hi Stephen, On Thu, Jan 16, 2025 at 10:47 PM Stephen Hemminger < stephen@networkplumber.org> wrote: > On Thu, 16 Jan 2025 20:56:39 +0100 > Ariel Otilibili <ariel.otilibili@6wind.com> wrote: > > > Both branches of the loop share the same logic. Now each one is a > > goto dispatcher; either to out (end of function), or to > > stats (continuation of the loop). > > > > Bugzilla ID: 1440 > > Depends-on: patch-1 ("net/af_xdp: fix use after free in af_xdp_tx_zc()") > > Signed-off-by: Ariel Otilibili <ariel.otilibili@6wind.com> > > --- > > drivers/net/af_xdp/rte_eth_af_xdp.c | 57 ++++++++++++++--------------- > > 1 file changed, 27 insertions(+), 30 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..8b42704b6d9f 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,10 @@ 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++; > > + > > + goto stats; > > } 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; > > @@ -589,23 +579,30 @@ af_xdp_tx_zc(void *queue, struct rte_mbuf **bufs, > uint16_t nb_pkts) > > goto out; > > } > > > > - 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; > > - 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); > > - tx_bytes += mbuf->pkt_len; > > - rte_pktmbuf_free(mbuf); > > - count++; > > + goto stats; > > } > > +stats: > > + 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)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); > > + rte_memcpy(pkt, rte_pktmbuf_mtod(mbuf, void *), desc->len); > > + tx_bytes += mbuf->pkt_len; > > + rte_pktmbuf_free(mbuf); > > + } > > + count++; > > } > > > > out: > > Indentation here is wrong, and looks suspect. > Either stats tag should be outside of loop > Or stats is inside loop, and both of those goto's are unnecessary > Thanks for the feedback; I am pushing a new series with an extra tab. So it be obvious that stats belongs to the the loop. [-- Attachment #2: Type: text/html, Size: 7024 bytes --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] net/af_xdp: Refactor af_xdp_tx_zc() 2025-01-16 22:20 ` Ariel Otilibili @ 2025-01-16 22:26 ` Stephen Hemminger 2025-01-16 22:36 ` Ariel Otilibili 0 siblings, 1 reply; 12+ messages in thread From: Stephen Hemminger @ 2025-01-16 22:26 UTC (permalink / raw) To: Ariel Otilibili Cc: dev, stable, Thomas Monjalon, David Marchand, Ciara Loftus, Maryam Tahhan On Thu, 16 Jan 2025 23:20:06 +0100 Ariel Otilibili <ariel.otilibili@6wind.com> wrote: > Hi Stephen, > > On Thu, Jan 16, 2025 at 10:47 PM Stephen Hemminger < > stephen@networkplumber.org> wrote: > > > On Thu, 16 Jan 2025 20:56:39 +0100 > > Ariel Otilibili <ariel.otilibili@6wind.com> wrote: > > > > > Both branches of the loop share the same logic. Now each one is a > > > goto dispatcher; either to out (end of function), or to > > > stats (continuation of the loop). > > > > > > Bugzilla ID: 1440 > > > Depends-on: patch-1 ("net/af_xdp: fix use after free in af_xdp_tx_zc()") > > > Signed-off-by: Ariel Otilibili <ariel.otilibili@6wind.com> > > > --- > > > drivers/net/af_xdp/rte_eth_af_xdp.c | 57 ++++++++++++++--------------- > > > 1 file changed, 27 insertions(+), 30 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..8b42704b6d9f 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,10 @@ 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++; > > > + > > > + goto stats; > > > } 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; > > > @@ -589,23 +579,30 @@ af_xdp_tx_zc(void *queue, struct rte_mbuf **bufs, > > uint16_t nb_pkts) > > > goto out; > > > } > > > > > > - 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; > > > - 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); > > > - tx_bytes += mbuf->pkt_len; > > > - rte_pktmbuf_free(mbuf); > > > - count++; > > > + goto stats; > > > } > > > +stats: > > > + 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)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); > > > + rte_memcpy(pkt, rte_pktmbuf_mtod(mbuf, void *), desc->len); > > > + tx_bytes += mbuf->pkt_len; > > > + rte_pktmbuf_free(mbuf); > > > + } > > > + count++; > > > } > > > > > > out: > > > > Indentation here is wrong, and looks suspect. > > Either stats tag should be outside of loop > > Or stats is inside loop, and both of those goto's are unnecessary > > > Thanks for the feedback; I am pushing a new series with an extra tab. > So it be obvious that stats belongs to the the loop. But the the goto;s aren't needed? Both legs of the If would fall through to that location. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] net/af_xdp: Refactor af_xdp_tx_zc() 2025-01-16 22:26 ` Stephen Hemminger @ 2025-01-16 22:36 ` Ariel Otilibili 0 siblings, 0 replies; 12+ messages in thread From: Ariel Otilibili @ 2025-01-16 22:36 UTC (permalink / raw) To: Stephen Hemminger Cc: dev, stable, Thomas Monjalon, David Marchand, Ciara Loftus, Maryam Tahhan [-- Attachment #1: Type: text/plain, Size: 6144 bytes --] On Thu, Jan 16, 2025 at 11:26 PM Stephen Hemminger < stephen@networkplumber.org> wrote: > On Thu, 16 Jan 2025 23:20:06 +0100 > Ariel Otilibili <ariel.otilibili@6wind.com> wrote: > > > Hi Stephen, > > > > On Thu, Jan 16, 2025 at 10:47 PM Stephen Hemminger < > > stephen@networkplumber.org> wrote: > > > > > On Thu, 16 Jan 2025 20:56:39 +0100 > > > Ariel Otilibili <ariel.otilibili@6wind.com> wrote: > > > > > > > Both branches of the loop share the same logic. Now each one is a > > > > goto dispatcher; either to out (end of function), or to > > > > stats (continuation of the loop). > > > > > > > > Bugzilla ID: 1440 > > > > Depends-on: patch-1 ("net/af_xdp: fix use after free in > af_xdp_tx_zc()") > > > > Signed-off-by: Ariel Otilibili <ariel.otilibili@6wind.com> > > > > --- > > > > drivers/net/af_xdp/rte_eth_af_xdp.c | 57 > ++++++++++++++--------------- > > > > 1 file changed, 27 insertions(+), 30 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..8b42704b6d9f 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,10 @@ 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++; > > > > + > > > > + goto stats; > > > > } 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; > > > > @@ -589,23 +579,30 @@ af_xdp_tx_zc(void *queue, struct rte_mbuf > **bufs, > > > uint16_t nb_pkts) > > > > goto out; > > > > } > > > > > > > > - 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; > > > > - 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); > > > > - tx_bytes += mbuf->pkt_len; > > > > - rte_pktmbuf_free(mbuf); > > > > - count++; > > > > + goto stats; > > > > } > > > > +stats: > > > > + 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)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); > > > > + rte_memcpy(pkt, rte_pktmbuf_mtod(mbuf, void *), > desc->len); > > > > + tx_bytes += mbuf->pkt_len; > > > > + rte_pktmbuf_free(mbuf); > > > > + } > > > > + count++; > > > > } > > > > > > > > out: > > > > > > Indentation here is wrong, and looks suspect. > > > Either stats tag should be outside of loop > > > Or stats is inside loop, and both of those goto's are unnecessary > > > > > Thanks for the feedback; I am pushing a new series with an extra tab. > > So it be obvious that stats belongs to the the loop. > > > But the the goto;s aren't needed? Both legs of the If would fall through > to that location. > You are right, Stephen; thanks for the heads up. I am pushing that change, without any goto in each leg, so fall through. Bear with me for some time. [-- Attachment #2: Type: text/html, Size: 8903 bytes --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 0/2] Fix use after free, and refactor af_xdp_tx_zc() 2025-01-16 19:56 [PATCH 0/2] Fix use after free, and refactor af_xdp_tx_zc() Ariel Otilibili 2025-01-16 19:56 ` [PATCH 1/2] net/af_xdp: fix use after free in af_xdp_tx_zc() Ariel Otilibili 2025-01-16 19:56 ` [PATCH 2/2] net/af_xdp: Refactor af_xdp_tx_zc() Ariel Otilibili @ 2025-01-16 22:51 ` Ariel Otilibili 2025-01-16 22:51 ` [PATCH v2 1/2] net/af_xdp: fix use after free in af_xdp_tx_zc() Ariel Otilibili 2025-01-16 22:51 ` [PATCH v2 2/2] net/af_xdp: Refactor af_xdp_tx_zc() Ariel Otilibili 2 siblings, 2 replies; 12+ messages in thread From: Ariel Otilibili @ 2025-01-16 22:51 UTC (permalink / raw) To: dev Cc: stable, Stephen Hemminger, Thomas Monjalon, David Marchand, Ariel Otilibili, Ciara Loftus, Maryam Tahhan Hello, The series addresses Bugzilla ID 1440 in two steps; 1. Fix use after free. 2. Refactor af_xdp_tx_zc(). Thank you, --- v2 * reworded commit messages * addressed feedback from Stephen Hemminger v1 (http://inbox.dpdk.org/dev/20250116195640.68885-1-ariel.otilibili@6wind.com/) Ariel Otilibili (2): net/af_xdp: fix use after free in af_xdp_tx_zc() net/af_xdp: Refactor af_xdp_tx_zc() drivers/net/af_xdp/rte_eth_af_xdp.c | 49 +++++++++++++---------------- 1 file changed, 22 insertions(+), 27 deletions(-) -- 2.30.2 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 1/2] net/af_xdp: fix use after free in af_xdp_tx_zc() 2025-01-16 22:51 ` [PATCH v2 0/2] Fix use after free, and refactor af_xdp_tx_zc() Ariel Otilibili @ 2025-01-16 22:51 ` Ariel Otilibili 2025-01-20 14:54 ` Maryam Tahhan 2025-01-16 22:51 ` [PATCH v2 2/2] net/af_xdp: Refactor af_xdp_tx_zc() Ariel Otilibili 1 sibling, 1 reply; 12+ messages in thread From: Ariel Otilibili @ 2025-01-16 22:51 UTC (permalink / raw) To: dev Cc: stable, Stephen Hemminger, Thomas Monjalon, David Marchand, Ariel Otilibili, Ciara Loftus, Maryam Tahhan tx_bytes is computed after both legs are tested. This might produce a use after memory free. The computation is now moved into each leg. Bugzilla ID: 1440 Fixes: d8a210774e1d ("net/af_xdp: support unaligned umem chunks") Signed-off-by: Ariel Otilibili <ariel.otilibili@6wind.com> --- drivers/net/af_xdp/rte_eth_af_xdp.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/net/af_xdp/rte_eth_af_xdp.c b/drivers/net/af_xdp/rte_eth_af_xdp.c index 814398ba4b44..4326a29f7042 100644 --- a/drivers/net/af_xdp/rte_eth_af_xdp.c +++ b/drivers/net/af_xdp/rte_eth_af_xdp.c @@ -574,6 +574,7 @@ af_xdp_tx_zc(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts) 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 = @@ -601,11 +602,10 @@ af_xdp_tx_zc(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts) desc->addr = addr | offset; rte_memcpy(pkt, rte_pktmbuf_mtod(mbuf, void *), desc->len); + tx_bytes += mbuf->pkt_len; rte_pktmbuf_free(mbuf); count++; } - - tx_bytes += mbuf->pkt_len; } out: -- 2.30.2 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/2] net/af_xdp: fix use after free in af_xdp_tx_zc() 2025-01-16 22:51 ` [PATCH v2 1/2] net/af_xdp: fix use after free in af_xdp_tx_zc() Ariel Otilibili @ 2025-01-20 14:54 ` Maryam Tahhan 0 siblings, 0 replies; 12+ messages in thread From: Maryam Tahhan @ 2025-01-20 14:54 UTC (permalink / raw) To: Ariel Otilibili, dev Cc: stable, Stephen Hemminger, Thomas Monjalon, David Marchand, Ciara Loftus On 16/01/2025 17:51, Ariel Otilibili wrote: > tx_bytes is computed after both legs are tested. This might > produce a use after memory free. > > The computation is now moved into each leg. > > Bugzilla ID: 1440 > Fixes: d8a210774e1d ("net/af_xdp: support unaligned umem chunks") > Signed-off-by: Ariel Otilibili <ariel.otilibili@6wind.com> > --- > drivers/net/af_xdp/rte_eth_af_xdp.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/af_xdp/rte_eth_af_xdp.c b/drivers/net/af_xdp/rte_eth_af_xdp.c > index 814398ba4b44..4326a29f7042 100644 > --- a/drivers/net/af_xdp/rte_eth_af_xdp.c > +++ b/drivers/net/af_xdp/rte_eth_af_xdp.c > @@ -574,6 +574,7 @@ af_xdp_tx_zc(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts) > 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 = > @@ -601,11 +602,10 @@ af_xdp_tx_zc(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts) > desc->addr = addr | offset; > rte_memcpy(pkt, rte_pktmbuf_mtod(mbuf, void *), > desc->len); > + tx_bytes += mbuf->pkt_len; > rte_pktmbuf_free(mbuf); > count++; > } > - > - tx_bytes += mbuf->pkt_len; > } > > out: I think that you could've just set tx_bytes to the desc->len as this is being set in all scenarios... tx_bytes += desc->len; ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 2/2] net/af_xdp: Refactor af_xdp_tx_zc() 2025-01-16 22:51 ` [PATCH v2 0/2] Fix use after free, and refactor af_xdp_tx_zc() Ariel Otilibili 2025-01-16 22:51 ` [PATCH v2 1/2] net/af_xdp: fix use after free in af_xdp_tx_zc() Ariel Otilibili @ 2025-01-16 22:51 ` Ariel Otilibili 2025-01-20 15:28 ` Maryam Tahhan 1 sibling, 1 reply; 12+ messages in thread From: Ariel Otilibili @ 2025-01-16 22:51 UTC (permalink / raw) To: dev Cc: stable, Stephen Hemminger, Thomas Monjalon, David Marchand, Ariel Otilibili, Ciara Loftus, Maryam Tahhan 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: -- 2.30.2 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/2] net/af_xdp: Refactor af_xdp_tx_zc() 2025-01-16 22:51 ` [PATCH v2 2/2] net/af_xdp: Refactor af_xdp_tx_zc() Ariel Otilibili @ 2025-01-20 15:28 ` Maryam Tahhan 0 siblings, 0 replies; 12+ messages in thread From: Maryam Tahhan @ 2025-01-20 15:28 UTC (permalink / raw) To: Ariel Otilibili, dev Cc: stable, Stephen Hemminger, Thomas Monjalon, David Marchand, Ciara Loftus 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2025-01-20 15:28 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2025-01-16 19:56 [PATCH 0/2] Fix use after free, and refactor af_xdp_tx_zc() Ariel Otilibili 2025-01-16 19:56 ` [PATCH 1/2] net/af_xdp: fix use after free in af_xdp_tx_zc() Ariel Otilibili 2025-01-16 19:56 ` [PATCH 2/2] net/af_xdp: Refactor af_xdp_tx_zc() Ariel Otilibili 2025-01-16 21:47 ` Stephen Hemminger 2025-01-16 22:20 ` Ariel Otilibili 2025-01-16 22:26 ` Stephen Hemminger 2025-01-16 22:36 ` Ariel Otilibili 2025-01-16 22:51 ` [PATCH v2 0/2] Fix use after free, and refactor af_xdp_tx_zc() Ariel Otilibili 2025-01-16 22:51 ` [PATCH v2 1/2] net/af_xdp: fix use after free in af_xdp_tx_zc() Ariel Otilibili 2025-01-20 14:54 ` Maryam Tahhan 2025-01-16 22:51 ` [PATCH v2 2/2] net/af_xdp: Refactor af_xdp_tx_zc() Ariel Otilibili 2025-01-20 15:28 ` Maryam Tahhan
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).