[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

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

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

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