* [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
* [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 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
* 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).