[PATCH 0/2] Fix use after free, and refactor af_xdp_tx_zc()

[PATCH 0/2] Fix use after free, and refactor af_xdp_tx_zc()

[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

[PATCH 1/2] net/af_xdp: fix use after free in af_xdp_tx_zc()

[Ariel Otilibili]

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

[PATCH 2/2] net/af_xdp: Refactor af_xdp_tx_zc()

[Ariel Otilibili]

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

Re: [PATCH 2/2] net/af_xdp: Refactor af_xdp_tx_zc()

[Stephen Hemminger]

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

Re: [PATCH 2/2] net/af_xdp: Refactor af_xdp_tx_zc()

[Ariel Otilibili]

[-- 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 --]

Re: [PATCH 2/2] net/af_xdp: Refactor af_xdp_tx_zc()

[Stephen Hemminger]

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.

Re: [PATCH 2/2] net/af_xdp: Refactor af_xdp_tx_zc()

[Ariel Otilibili]

[-- 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 --]

[PATCH v2 0/2] Fix use after free, and refactor af_xdp_tx_zc()

[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,
---
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

[PATCH v2 1/2] net/af_xdp: fix use after free in af_xdp_tx_zc()

[Ariel Otilibili]

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

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

[Ariel Otilibili]

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

Re: [PATCH v2 1/2] net/af_xdp: fix use after free in af_xdp_tx_zc()

[Maryam Tahhan]

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;

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

[Maryam Tahhan]

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.

Re: [PATCH v2 1/2] net/af_xdp: fix use after free in af_xdp_tx_zc()

[Ariel Otilibili]

[-- Attachment #1: Type: text/plain, Size: 279 bytes --]

Hi Maryam,

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

> 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;
>
 Thanks for your feedback. I'll change that.

[-- Attachment #2: Type: text/html, Size: 625 bytes --]

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

[Ariel Otilibili]

[-- Attachment #1: Type: text/plain, Size: 731 bytes --]

Hi Maryam,

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

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

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

[-- Attachment #2: Type: text/html, Size: 1231 bytes --]

[PATCH v3 0/2] Fix use after free, and refactor af_xdp_tx_zc()

[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,
---
v3
* reworded commit message of patch-1
* addressed feedback of Maryam Tahhan
v2 (https://inbox.dpdk.org/dev/20250116225151.188214-1-ariel.otilibili@6wind.com/)
* 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 | 131 ++++++++++++++++++----------
 1 file changed, 84 insertions(+), 47 deletions(-)

-- 
2.30.2

[PATCH v3 1/2] net/af_xdp: fix use after free in af_xdp_tx_zc()

[Ariel Otilibili]

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..092bcb73aa0a 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 += desc->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 += desc->len;
 			rte_pktmbuf_free(mbuf);
 			count++;
 		}
-
-		tx_bytes += mbuf->pkt_len;
 	}
 
 out:
-- 
2.30.2

[PATCH v3 2/2] net/af_xdp: Refactor af_xdp_tx_zc()

[Ariel Otilibili]

Both legs of the loop share the same logic: either to go out of the
function, or to fall through to the next instructions.

For that, maybe_cpy_pkt() is introduced.

Bugzilla ID: 1440
Signed-off-by: Ariel Otilibili <ariel.otilibili@6wind.com>
---
 drivers/net/af_xdp/rte_eth_af_xdp.c | 131 ++++++++++++++++++----------
 1 file changed, 84 insertions(+), 47 deletions(-)

diff --git a/drivers/net/af_xdp/rte_eth_af_xdp.c b/drivers/net/af_xdp/rte_eth_af_xdp.c
index 092bcb73aa0a..efe23283fb52 100644
--- a/drivers/net/af_xdp/rte_eth_af_xdp.c
+++ b/drivers/net/af_xdp/rte_eth_af_xdp.c
@@ -536,13 +536,80 @@ kick_tx(struct pkt_tx_queue *txq, struct xsk_ring_cons *cq)
 		}
 }
 
+static inline uint64_t
+update_addr(struct rte_mbuf *mbuf, struct xsk_umem_info *umem)
+{
+	return (uint64_t)mbuf - (uint64_t)umem->buffer
+		- umem->mb_pool->header_size;
+}
+
+static inline uint64_t
+update_offset(struct rte_mbuf *mbuf, struct xsk_umem_info *umem)
+{
+	uint64_t offset;
+
+	offset = rte_pktmbuf_mtod(mbuf, uint64_t) - (uint64_t)mbuf
+		+ umem->mb_pool->header_size;
+	return offset << XSK_UNALIGNED_BUF_OFFSET_SHIFT;
+}
+
+static struct rte_mbuf *
+maybe_kick_tx(struct pkt_tx_queue *txq, uint32_t *idx_tx, struct rte_mbuf *mbuf)
+{
+	struct rte_mbuf *ret = mbuf;
+
+	if (!xsk_ring_prod__reserve(&txq->tx, 1, idx_tx)) {
+		kick_tx(txq, &txq->pair->cq);
+		if (!xsk_ring_prod__reserve(&txq->tx, 1, idx_tx))
+			ret = NULL;
+	}
+
+	return ret;
+}
+
+static struct rte_mbuf *
+maybe_create_mbuf(struct pkt_tx_queue *txq, uint32_t *idx_tx,
+		  struct xsk_umem_info *umem) {
+	struct rte_mbuf *local_mbuf = rte_pktmbuf_alloc(umem->mb_pool);
+
+	if (local_mbuf == NULL)
+		goto out;
+
+	if (!xsk_ring_prod__reserve(&txq->tx, 1, idx_tx)) {
+		rte_pktmbuf_free(local_mbuf);
+		local_mbuf = NULL;
+		goto out;
+	}
+
+out:
+	return local_mbuf;
+}
+
+static void
+maybe_cpy_pkt(bool is_mbuf_equal, struct xsk_umem_info *umem,
+	      uint64_t addr_plus_offset, struct rte_mbuf *mbuf,
+	      struct xdp_desc *desc)
+{
+	void *pkt;
+
+	if(is_mbuf_equal)
+		goto out;
+
+	pkt = xsk_umem__get_data(umem->buffer, addr_plus_offset);
+	rte_memcpy(pkt, rte_pktmbuf_mtod(mbuf, void *), desc->len);
+	rte_pktmbuf_free(mbuf);
+
+out:
+	return;
+}
+
 #if defined(XDP_UMEM_UNALIGNED_CHUNK_FLAG)
 static uint16_t
 af_xdp_tx_zc(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 {
 	struct pkt_tx_queue *txq = queue;
 	struct xsk_umem_info *umem = txq->umem;
-	struct rte_mbuf *mbuf;
+	struct rte_mbuf *mbuf, *local_mbuf;
 	unsigned long tx_bytes = 0;
 	int i;
 	uint32_t idx_tx;
@@ -551,61 +618,31 @@ 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;
+	bool is_true;
 
 	if (xsk_cons_nb_avail(cq, free_thresh) >= free_thresh)
 		pull_umem_cq(umem, XSK_RING_CONS__DEFAULT_NUM_DESCS, cq);
 
 	for (i = 0; i < nb_pkts; i++) {
 		mbuf = bufs[i];
+		is_true = mbuf->pool == umem->mb_pool;
 
-		if (mbuf->pool == umem->mb_pool) {
-			if (!xsk_ring_prod__reserve(&txq->tx, 1, &idx_tx)) {
-				kick_tx(txq, cq);
-				if (!xsk_ring_prod__reserve(&txq->tx, 1,
-							    &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 += desc->len;
-			count++;
-		} else {
-			struct rte_mbuf *local_mbuf =
-					rte_pktmbuf_alloc(umem->mb_pool);
-			void *pkt;
-
-			if (local_mbuf == NULL)
-				goto out;
+		if (is_true)
+			local_mbuf = maybe_kick_tx(txq, &idx_tx, mbuf);
+		else
+			local_mbuf = maybe_create_mbuf(txq, &idx_tx, umem);
 
-			if (!xsk_ring_prod__reserve(&txq->tx, 1, &idx_tx)) {
-				rte_pktmbuf_free(local_mbuf);
-				goto out;
-			}
+		if (!local_mbuf)
+		  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 += desc->len;
-			rte_pktmbuf_free(mbuf);
-			count++;
-		}
+		desc = xsk_ring_prod__tx_desc(&txq->tx, idx_tx);
+		desc->len = mbuf->pkt_len;
+		addr = update_addr(local_mbuf, umem);
+		offset = update_offset(local_mbuf, umem);
+		desc->addr = addr | offset;
+		maybe_cpy_pkt(is_true, umem, addr + offset, mbuf, desc);
+		tx_bytes += desc->len;
+		count++;
 	}
 
 out:
-- 
2.30.2

Re: [PATCH v3 2/2] net/af_xdp: Refactor af_xdp_tx_zc()

[Maryam Tahhan]

On 28/01/2025 23:11, 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.
>
> For that, maybe_cpy_pkt() is introduced.
>
> Bugzilla ID: 1440
> Signed-off-by: Ariel Otilibili <ariel.otilibili@6wind.com>
> ---
>   drivers/net/af_xdp/rte_eth_af_xdp.c | 131 ++++++++++++++++++----------
>   1 file changed, 84 insertions(+), 47 deletions(-)
>
> diff --git a/drivers/net/af_xdp/rte_eth_af_xdp.c b/drivers/net/af_xdp/rte_eth_af_xdp.c
> index 092bcb73aa0a..efe23283fb52 100644
> --- a/drivers/net/af_xdp/rte_eth_af_xdp.c
> +++ b/drivers/net/af_xdp/rte_eth_af_xdp.c
> @@ -536,13 +536,80 @@ kick_tx(struct pkt_tx_queue *txq, struct xsk_ring_cons *cq)
>   		}
>   }
>   
> +static inline uint64_t
> +update_addr(struct rte_mbuf *mbuf, struct xsk_umem_info *umem)
> +{
> +	return (uint64_t)mbuf - (uint64_t)umem->buffer
> +		- umem->mb_pool->header_size;
> +}
> +
> +static inline uint64_t
> +update_offset(struct rte_mbuf *mbuf, struct xsk_umem_info *umem)
> +{
> +	uint64_t offset;
> +
> +	offset = rte_pktmbuf_mtod(mbuf, uint64_t) - (uint64_t)mbuf
> +		+ umem->mb_pool->header_size;
> +	return offset << XSK_UNALIGNED_BUF_OFFSET_SHIFT;
> +}
> +
> +static struct rte_mbuf *
> +maybe_kick_tx(struct pkt_tx_queue *txq, uint32_t *idx_tx, struct rte_mbuf *mbuf)
> +{
> +	struct rte_mbuf *ret = mbuf;
> +
> +	if (!xsk_ring_prod__reserve(&txq->tx, 1, idx_tx)) {
> +		kick_tx(txq, &txq->pair->cq);
> +		if (!xsk_ring_prod__reserve(&txq->tx, 1, idx_tx))
> +			ret = NULL;
> +	}
> +
> +	return ret;
> +}


[MT] I don't see why we are passing in mbuf here?


> +
> +static struct rte_mbuf *
> +maybe_create_mbuf(struct pkt_tx_queue *txq, uint32_t *idx_tx,
> +		  struct xsk_umem_info *umem) {
> +	struct rte_mbuf *local_mbuf = rte_pktmbuf_alloc(umem->mb_pool);
> +
> +	if (local_mbuf == NULL)
> +		goto out;
> +
> +	if (!xsk_ring_prod__reserve(&txq->tx, 1, idx_tx)) {
> +		rte_pktmbuf_free(local_mbuf);
> +		local_mbuf = NULL;
> +		goto out;
> +	}
> +
> +out:
> +	return local_mbuf;
> +}
> +
> +static void
> +maybe_cpy_pkt(bool is_mbuf_equal, struct xsk_umem_info *umem,
> +	      uint64_t addr_plus_offset, struct rte_mbuf *mbuf,
> +	      struct xdp_desc *desc)
> +{
> +	void *pkt;
> +
> +	if(is_mbuf_equal)
> +		goto out;
> +
> +	pkt = xsk_umem__get_data(umem->buffer, addr_plus_offset);
> +	rte_memcpy(pkt, rte_pktmbuf_mtod(mbuf, void *), desc->len);
> +	rte_pktmbuf_free(mbuf);
> +
> +out:
> +	return;
> +}


[MT] does this really need to be an inline function? it wasn't common 
code between the blocks?


> +
>   #if defined(XDP_UMEM_UNALIGNED_CHUNK_FLAG)
>   static uint16_t
>   af_xdp_tx_zc(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
>   {
>   	struct pkt_tx_queue *txq = queue;
>   	struct xsk_umem_info *umem = txq->umem;
> -	struct rte_mbuf *mbuf;
> +	struct rte_mbuf *mbuf, *local_mbuf;
>   	unsigned long tx_bytes = 0;
>   	int i;
>   	uint32_t idx_tx;
> @@ -551,61 +618,31 @@ 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;
> +	bool is_true;
>   
>   	if (xsk_cons_nb_avail(cq, free_thresh) >= free_thresh)
>   		pull_umem_cq(umem, XSK_RING_CONS__DEFAULT_NUM_DESCS, cq);
>   
>   	for (i = 0; i < nb_pkts; i++) {
>   		mbuf = bufs[i];
> +		is_true = mbuf->pool == umem->mb_pool;
>   
> -		if (mbuf->pool == umem->mb_pool) {
> -			if (!xsk_ring_prod__reserve(&txq->tx, 1, &idx_tx)) {
> -				kick_tx(txq, cq);
> -				if (!xsk_ring_prod__reserve(&txq->tx, 1,
> -							    &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 += desc->len;
> -			count++;
> -		} else {
> -			struct rte_mbuf *local_mbuf =
> -					rte_pktmbuf_alloc(umem->mb_pool);
> -			void *pkt;
> -
> -			if (local_mbuf == NULL)
> -				goto out;
> +		if (is_true)
> +			local_mbuf = maybe_kick_tx(txq, &idx_tx, mbuf);
> +		else
> +			local_mbuf = maybe_create_mbuf(txq, &idx_tx, umem);
>   
> -			if (!xsk_ring_prod__reserve(&txq->tx, 1, &idx_tx)) {
> -				rte_pktmbuf_free(local_mbuf);
> -				goto out;
> -			}
> +		if (!local_mbuf)
> +		  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 += desc->len;
> -			rte_pktmbuf_free(mbuf);
> -			count++;
> -		}
> +		desc = xsk_ring_prod__tx_desc(&txq->tx, idx_tx);
> +		desc->len = mbuf->pkt_len;
> +		addr = update_addr(local_mbuf, umem);
> +		offset = update_offset(local_mbuf, umem);
> +		desc->addr = addr | offset;
> +		maybe_cpy_pkt(is_true, umem, addr + offset, mbuf, desc);
> +		tx_bytes += desc->len;
> +		count++;
>   	}
>   
>   out:


Firstly thank you for your efforts to clean up the code. I believe a 
simpler cleanup approach would better balance readability + 
maintainability. This approach would focus on eliminating the repeated 
code in both branches of the conditional block. For me the main 
repetition between the branches is the code that reserves and fills the 
descriptor info, Please find an example below...

Note: The following is all untested code (don't even know if it 
compiles) it's just to give an idea around the cleanup I had in mind:

static inline int
reserve_and_fill_desc(struct pkt_tx_queue *txq, struct xdp_desc **desc,
                       struct rte_mbuf *mbuf, struct xsk_umem_info *umem, uint32_t *idx_tx)
{
     uint64_t addr, offset;

     if (!xsk_ring_prod__reserve(&txq->tx, 1, idx_tx))
         return -1;

     *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;

     return 0;
}


Then the main function would look something like:


static uint16_t
af_xdp_tx_zc(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
{
     struct pkt_tx_queue *txq = queue;
     struct xsk_umem_info *umem = txq->umem;
     struct rte_mbuf *mbuf;
     unsigned long tx_bytes = 0;
     int i;
     uint32_t idx_tx;
     uint16_t count = 0;
     struct xdp_desc *desc;
     struct xsk_ring_cons *cq = &txq->pair->cq;
     uint32_t free_thresh = cq->size >> 1;

     if (xsk_cons_nb_avail(cq, free_thresh) >= free_thresh)
         pull_umem_cq(umem, XSK_RING_CONS__DEFAULT_NUM_DESCS, cq);

     for (i = 0; i < nb_pkts; i++) {
         mbuf = bufs[i];

         if (mbuf->pool == umem->mb_pool) {
             if (reserve_and_fill_desc(txq, &desc, mbuf, umem, &idx_tx) < 0) {
                 kick_tx(txq, cq);
                 if (reserve_and_fill_desc(txq, &desc, mbuf, umem, &idx_tx) < 0)
                     goto out;
             }
             
             tx_bytes += desc->len;
             count++;
         } else {
             struct rte_mbuf *local_mbuf = rte_pktmbuf_alloc(umem->mb_pool);
             void *pkt;

             if (local_mbuf == NULL)
                 goto out;

             if (reserve_and_fill_desc(txq, &desc, local_mbuf, umem, &idx_tx) < 0) {
                 rte_pktmbuf_free(local_mbuf);
                 goto out;
             }

             pkt = xsk_umem__get_data(umem->buffer, (desc->addr & ~0xFFF) +
                                      (desc->addr & 0xFFF));
             rte_memcpy(pkt, rte_pktmbuf_mtod(mbuf, void *), desc->len);
             rte_pktmbuf_free(mbuf);
             tx_bytes += desc->len;
             count++;
         }


     }

out:
     xsk_ring_prod__submit(&txq->tx, count);
     kick_tx(txq, cq);

     txq->stats.tx_pkts += count;
     txq->stats.tx_bytes += tx_bytes;
     txq->stats.tx_dropped += nb_pkts - count;

     return count;
}


Please let me know your thoughts, and I’d be happy to discuss further

Re: [PATCH 1/2] net/af_xdp: fix use after free in af_xdp_tx_zc()

[Stephen Hemminger]

On Thu, 16 Jan 2025 20:56:38 +0100
Ariel Otilibili <ariel.otilibili@6wind.com> wrote:

> 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>
> ---

Acked-by: Stephen Hemminger <stephen@networkplumber.org>

[PATCH v4 0/2] Fix use after free, and refactor af_xdp_tx_zc

[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,
---
v4
* redid the refactor (Maryam Tahhan)
* marked the fix as acked (Stephen Hemminger)
* updated .mailmap, my main e-mail is @6wind.com (https://inbox.dpdk.org/dev/20250115121152.487360-4-otilibil@eurecom.fr/)
v3 (https://inbox.dpdk.org/dev/20250128231152.249497-1-ariel.otilibili@6wind.com/)
* reworded commit message of patch-1
* addressed feedback of Maryam Tahhan
v2 (https://inbox.dpdk.org/dev/20250116225151.188214-1-ariel.otilibili@6wind.com/)
* 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

 .mailmap                            |  2 +-
 drivers/net/af_xdp/rte_eth_af_xdp.c | 64 ++++++++++++++++-------------
 2 files changed, 36 insertions(+), 30 deletions(-)

-- 
2.30.2

[PATCH v4 1/2] net/af_xdp: Fix use after free in af_xdp_tx_zc

[Ariel Otilibili]

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>
Acked-by: Stephen Hemminger <stephen@networkplumber.org>
---
 .mailmap                            | 2 +-
 drivers/net/af_xdp/rte_eth_af_xdp.c | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/.mailmap b/.mailmap
index 8524952d2480..69e485deac55 100644
--- a/.mailmap
+++ b/.mailmap
@@ -134,7 +134,7 @@ Anupam Kapoor <anupam.kapoor@gmail.com>
 Apeksha Gupta <apeksha.gupta@nxp.com>
 Archana Muniganti <marchana@marvell.com> <muniganti.archana@caviumnetworks.com>
 Archit Pandey <architpandeynitk@gmail.com>
-Ariel Otilibili <otilibil@eurecom.fr> <ariel.otilibili@6wind.com>
+Ariel Otilibili <ariel.otilibili@6wind.com> <otilibil@eurecom.fr>
 Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
 Arkadiusz Kusztal <arkadiuszx.kusztal@intel.com>
 Arnaud Fiorini <arnaud.fiorini@polymtl.ca>
diff --git a/drivers/net/af_xdp/rte_eth_af_xdp.c b/drivers/net/af_xdp/rte_eth_af_xdp.c
index 814398ba4b44..092bcb73aa0a 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 += desc->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 += desc->len;
 			rte_pktmbuf_free(mbuf);
 			count++;
 		}
-
-		tx_bytes += mbuf->pkt_len;
 	}
 
 out:
-- 
2.30.2

[PATCH v4 2/2] net/af_xdp: Refactor af_xdp_tx_zc

[Ariel Otilibili]

Both legs of the loop share the same logic: the common parts are about
reserving and filling both address and length into the description.

This is moved into reserve_and_fill().

Bugzilla ID: 1440
Suggested-by: Maryam Tahhan <mtahhan@redhat.com>
Signed-off-by: Ariel Otilibili <ariel.otilibili@6wind.com>
---
 drivers/net/af_xdp/rte_eth_af_xdp.c | 62 ++++++++++++++++-------------
 1 file changed, 34 insertions(+), 28 deletions(-)

diff --git a/drivers/net/af_xdp/rte_eth_af_xdp.c b/drivers/net/af_xdp/rte_eth_af_xdp.c
index 092bcb73aa0a..3f5de4d9cbda 100644
--- a/drivers/net/af_xdp/rte_eth_af_xdp.c
+++ b/drivers/net/af_xdp/rte_eth_af_xdp.c
@@ -536,6 +536,31 @@ kick_tx(struct pkt_tx_queue *txq, struct xsk_ring_cons *cq)
 		}
 }
 
+static inline struct xdp_desc*
+reserve_and_fill(struct pkt_tx_queue *txq, struct rte_mbuf *mbuf,
+		 struct xsk_umem_info *umem)
+{
+	struct xdp_desc *desc = NULL;
+	uint32_t *idx_tx = NULL;
+	uint64_t addr, offset;
+
+	if (!xsk_ring_prod__reserve(&txq->tx, 1, 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;
+
+out:
+	return desc;
+}
+
 #if defined(XDP_UMEM_UNALIGNED_CHUNK_FLAG)
 static uint16_t
 af_xdp_tx_zc(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
@@ -545,10 +570,8 @@ af_xdp_tx_zc(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 	struct rte_mbuf *mbuf;
 	unsigned long tx_bytes = 0;
 	int i;
-	uint32_t idx_tx;
 	uint16_t count = 0;
 	struct xdp_desc *desc;
-	uint64_t addr, offset;
 	struct xsk_ring_cons *cq = &txq->pair->cq;
 	uint32_t free_thresh = cq->size >> 1;
 
@@ -559,21 +582,12 @@ af_xdp_tx_zc(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 		mbuf = bufs[i];
 
 		if (mbuf->pool == umem->mb_pool) {
-			if (!xsk_ring_prod__reserve(&txq->tx, 1, &idx_tx)) {
+			if (!(desc = reserve_and_fill(txq, mbuf, umem))) {
 				kick_tx(txq, cq);
-				if (!xsk_ring_prod__reserve(&txq->tx, 1,
-							    &idx_tx))
+				if (!(desc = reserve_and_fill(txq, mbuf, umem)))
 					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 += desc->len;
 			count++;
 		} else {
@@ -584,26 +598,18 @@ af_xdp_tx_zc(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 			if (local_mbuf == NULL)
 				goto out;
 
-			if (!xsk_ring_prod__reserve(&txq->tx, 1, &idx_tx)) {
+			if (!(desc = reserve_and_fill(txq, local_mbuf, umem))) {
 				rte_pktmbuf_free(local_mbuf);
 				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;
+			pkt = xsk_umem__get_data(umem->buffer,
+						 (desc->addr & ~0xFFF)
+						 + (desc->addr & 0xFFF));
 			rte_memcpy(pkt, rte_pktmbuf_mtod(mbuf, void *),
-					desc->len);
-			tx_bytes += desc->len;
+				   desc->len);
 			rte_pktmbuf_free(mbuf);
+			tx_bytes += desc->len;
 			count++;
 		}
 	}
-- 
2.30.2

Re: [PATCH 1/2] net/af_xdp: fix use after free in af_xdp_tx_zc()

[Ariel Otilibili]

[-- Attachment #1: Type: text/plain, Size: 361 bytes --]

Hello Stephen,

On Thu, Jan 30, 2025 at 7:24 PM Stephen Hemminger <
stephen@networkplumber.org> wrote:

> Acked-by: Stephen Hemminger <stephen@networkplumber.org>
>
Thanks for having looked into the patch; I have ACKed the fourth version by
your name,
https://inbox.dpdk.org/dev/20250130221853.789366-2-ariel.otilibili@6wind.com/

Regards,
Ariel

[-- Attachment #2: Type: text/html, Size: 891 bytes --]

Re: [PATCH v3 2/2] net/af_xdp: Refactor af_xdp_tx_zc()

[Ariel Otilibili]

[-- Attachment #1: Type: text/plain, Size: 3053 bytes --]

Hello Maryam,

On Wed, Jan 29, 2025 at 6:58 PM Maryam Tahhan <mtahhan@redhat.com> wrote:

> > +static struct rte_mbuf *
> > +maybe_kick_tx(struct pkt_tx_queue *txq, uint32_t *idx_tx, struct
> rte_mbuf *mbuf)
> > +{
> > +     struct rte_mbuf *ret = mbuf;
> > +
> > +     if (!xsk_ring_prod__reserve(&txq->tx, 1, idx_tx)) {
> > +             kick_tx(txq, &txq->pair->cq);
> > +             if (!xsk_ring_prod__reserve(&txq->tx, 1, idx_tx))
> > +                     ret = NULL;
> > +     }
> > +
> > +     return ret;
> > +}
>
>
> [MT] I don't see why we are passing in mbuf here?
>
 My aim was to use the output of maybe_kick_tx() for the if statement on
local_buf: the true leg would copy mbuf into local_mbuf, and the false
would create a fresh local_mbuf

> > +static void
> > +maybe_cpy_pkt(bool is_mbuf_equal, struct xsk_umem_info *umem,
> > +           uint64_t addr_plus_offset, struct rte_mbuf *mbuf,
> > +           struct xdp_desc *desc)
> > +{
> > +     void *pkt;
> > +
> > +     if(is_mbuf_equal)
> > +             goto out;
> > +
> > +     pkt = xsk_umem__get_data(umem->buffer, addr_plus_offset);
> > +     rte_memcpy(pkt, rte_pktmbuf_mtod(mbuf, void *), desc->len);
> > +     rte_pktmbuf_free(mbuf);
> > +
> > +out:
> > +     return;
> > +}
>
>
> [MT] does this really need to be an inline function? it wasn't common
> code between the blocks?
>
In order for the next statements to just fall through till the exit. The
loop would have read as such:

1. Some boiler plate to check if mbuf is equal to umem; and the creation of
local_mbuf accordingly
2. If local_mbuf is null, goto exit.
3. Else, update addr, offset, and description

> Firstly thank you for your efforts to clean up the code. I believe a
> simpler cleanup approach would better balance readability +
> maintainability. This approach would focus on eliminating the repeated
> code in both branches of the conditional block. For me the main
> repetition between the branches is the code that reserves and fills the
> descriptor info, Please find an example below...
>
Thanks to you, I do appreciate your honest feedback. From what I
understand, you would like to take the filling of addr, offset, and desc
off from af_xdp_tx_zc(), but keep its core logic.

I wanted the boiler plate to be into separate functions, so one could read
through the subsequent statements. So we could avoid the cascade of if
statements.

>
> Note: The following is all untested code (don't even know if it
> compiles) it's just to give an idea around the cleanup I had in mind:
>
The code did compile on the go, that was pretty neat. I cleared out few
warnings, and pushed out a new version
https://inbox.dpdk.org/dev/20250130221853.789366-3-ariel.otilibili@6wind.com/

>
> Please let me know your thoughts, and I’d be happy to discuss further
>
> I improved on the snippets your proposal. It has fewer lines, so fewer
changes.

What matters to me, is that the series be merged.

Have a good day,
Ariel

[-- Attachment #2: Type: text/html, Size: 4486 bytes --]

Re: [PATCH v4 2/2] net/af_xdp: Refactor af_xdp_tx_zc

[Stephen Hemminger]

On Thu, 30 Jan 2025 23:18:53 +0100
Ariel Otilibili <ariel.otilibili@6wind.com> wrote:

> +static inline struct xdp_desc*
Need space after xdp_desc to follow style used elsewhere in DPDK.

[PATCH v5 0/2] Fix use after free, and refactor af_xdp_tx_zc

[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,
---
v5
* fix style issues in the signature of reserve_and_fill() (Stephen Hemminger)

v4 (https://inbox.dpdk.org/dev/20250130221853.789366-1-ariel.otilibili@6wind.com/)
* redid the refactor (Maryam Tahhan)
* marked the fix as acked (Stephen Hemminger)
* updated .mailmap, my main e-mail is @6wind.com (https://inbox.dpdk.org/dev/20250115121152.487360-4-otilibil@eurecom.fr/)

v3 (https://inbox.dpdk.org/dev/20250128231152.249497-1-ariel.otilibili@6wind.com/)
* reworded commit message of patch-1
* addressed feedback of Maryam Tahhan

v2 (https://inbox.dpdk.org/dev/20250116225151.188214-1-ariel.otilibili@6wind.com/)
* 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

 .mailmap                            |  2 +-
 drivers/net/af_xdp/rte_eth_af_xdp.c | 64 ++++++++++++++++-------------
 2 files changed, 36 insertions(+), 30 deletions(-)

-- 
2.30.2

[PATCH v5 1/2] net/af_xdp: Fix use after free in af_xdp_tx_zc

[Ariel Otilibili]

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>
Acked-by: Stephen Hemminger <stephen@networkplumber.org>
---
 .mailmap                            | 2 +-
 drivers/net/af_xdp/rte_eth_af_xdp.c | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/.mailmap b/.mailmap
index 76f65e5114d4..42fcefacf573 100644
--- a/.mailmap
+++ b/.mailmap
@@ -134,7 +134,7 @@ Anupam Kapoor <anupam.kapoor@gmail.com>
 Apeksha Gupta <apeksha.gupta@nxp.com>
 Archana Muniganti <marchana@marvell.com> <muniganti.archana@caviumnetworks.com>
 Archit Pandey <architpandeynitk@gmail.com>
-Ariel Otilibili <otilibil@eurecom.fr> <ariel.otilibili@6wind.com>
+Ariel Otilibili <ariel.otilibili@6wind.com> <otilibil@eurecom.fr>
 Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
 Arkadiusz Kusztal <arkadiuszx.kusztal@intel.com>
 Arnaud Fiorini <arnaud.fiorini@polymtl.ca>
diff --git a/drivers/net/af_xdp/rte_eth_af_xdp.c b/drivers/net/af_xdp/rte_eth_af_xdp.c
index 814398ba4b44..092bcb73aa0a 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 += desc->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 += desc->len;
 			rte_pktmbuf_free(mbuf);
 			count++;
 		}
-
-		tx_bytes += mbuf->pkt_len;
 	}
 
 out:
-- 
2.30.2

[PATCH v5 2/2] net/af_xdp: Refactor af_xdp_tx_zc

[Ariel Otilibili]

Both legs of the loop share the same logic: the common parts are about
reserving and filling both address and length into the description.

This is moved into reserve_and_fill().

Bugzilla ID: 1440
Signed-off-by: Ariel Otilibili <ariel.otilibili@6wind.com>
---
 drivers/net/af_xdp/rte_eth_af_xdp.c | 62 ++++++++++++++++-------------
 1 file changed, 34 insertions(+), 28 deletions(-)

diff --git a/drivers/net/af_xdp/rte_eth_af_xdp.c b/drivers/net/af_xdp/rte_eth_af_xdp.c
index 092bcb73aa0a..840a12dbf508 100644
--- a/drivers/net/af_xdp/rte_eth_af_xdp.c
+++ b/drivers/net/af_xdp/rte_eth_af_xdp.c
@@ -536,6 +536,31 @@ kick_tx(struct pkt_tx_queue *txq, struct xsk_ring_cons *cq)
 		}
 }
 
+static inline struct xdp_desc *
+reserve_and_fill(struct pkt_tx_queue *txq, struct rte_mbuf *mbuf,
+		 struct xsk_umem_info *umem)
+{
+	struct xdp_desc *desc = NULL;
+	uint32_t *idx_tx = NULL;
+	uint64_t addr, offset;
+
+	if (!xsk_ring_prod__reserve(&txq->tx, 1, 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;
+
+out:
+	return desc;
+}
+
 #if defined(XDP_UMEM_UNALIGNED_CHUNK_FLAG)
 static uint16_t
 af_xdp_tx_zc(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
@@ -545,10 +570,8 @@ af_xdp_tx_zc(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 	struct rte_mbuf *mbuf;
 	unsigned long tx_bytes = 0;
 	int i;
-	uint32_t idx_tx;
 	uint16_t count = 0;
 	struct xdp_desc *desc;
-	uint64_t addr, offset;
 	struct xsk_ring_cons *cq = &txq->pair->cq;
 	uint32_t free_thresh = cq->size >> 1;
 
@@ -559,21 +582,12 @@ af_xdp_tx_zc(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 		mbuf = bufs[i];
 
 		if (mbuf->pool == umem->mb_pool) {
-			if (!xsk_ring_prod__reserve(&txq->tx, 1, &idx_tx)) {
+			if (!(desc = reserve_and_fill(txq, mbuf, umem))) {
 				kick_tx(txq, cq);
-				if (!xsk_ring_prod__reserve(&txq->tx, 1,
-							    &idx_tx))
+				if (!(desc = reserve_and_fill(txq, mbuf, umem)))
 					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 += desc->len;
 			count++;
 		} else {
@@ -584,26 +598,18 @@ af_xdp_tx_zc(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 			if (local_mbuf == NULL)
 				goto out;
 
-			if (!xsk_ring_prod__reserve(&txq->tx, 1, &idx_tx)) {
+			if (!(desc = reserve_and_fill(txq, local_mbuf, umem))) {
 				rte_pktmbuf_free(local_mbuf);
 				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;
+			pkt = xsk_umem__get_data(umem->buffer,
+						 (desc->addr & ~0xFFF)
+						 + (desc->addr & 0xFFF));
 			rte_memcpy(pkt, rte_pktmbuf_mtod(mbuf, void *),
-					desc->len);
-			tx_bytes += desc->len;
+				   desc->len);
 			rte_pktmbuf_free(mbuf);
+			tx_bytes += desc->len;
 			count++;
 		}
 	}
-- 
2.30.2

Re: [PATCH v4 2/2] net/af_xdp: Refactor af_xdp_tx_zc

[Ariel Otilibili]

[-- Attachment #1: Type: text/plain, Size: 272 bytes --]

On Fri, Jan 31, 2025 at 12:55 AM Stephen Hemminger <
stephen@networkplumber.org> wrote:

> Need space after xdp_desc to follow style used elsewhere in DPDK.
>

There you are, Stephen:
https://inbox.dpdk.org/dev/20250131183439.909831-3-ariel.otilibili@6wind.com/

[-- Attachment #2: Type: text/html, Size: 717 bytes --]