* [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; 10+ 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] 10+ 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; 10+ 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] 10+ 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; 10+ 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] 10+ 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; 10+ 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] 10+ 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; 10+ 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] 10+ 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; 10+ 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] 10+ 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; 10+ 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] 10+ 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; 10+ 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] 10+ 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-16 22:51 ` [PATCH v2 2/2] net/af_xdp: Refactor af_xdp_tx_zc() Ariel Otilibili 1 sibling, 0 replies; 10+ 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] 10+ 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 1 sibling, 0 replies; 10+ 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] 10+ messages in thread
end of thread, other threads:[~2025-01-16 22:52 UTC | newest] Thread overview: 10+ 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-16 22:51 ` [PATCH v2 2/2] net/af_xdp: Refactor af_xdp_tx_zc() Ariel Otilibili
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).