[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