[PATCH 0/2] lib/reorder: fix drain/free issues

[PATCH 0/2] lib/reorder: fix drain/free issues

[Volodymyr Fialko]

This patch address issues with reorder drain/free,
discovered with enabled `RTE_LIBRTE_MEMPOOL_DEBUG`.

Volodymyr Fialko (2):
  reorder: invalidate buf from ready queue in drain
  test/reorder: fix double free of drained buffers

 app/test/test_reorder.c   | 2 ++
 lib/reorder/rte_reorder.c | 1 +
 2 files changed, 3 insertions(+)

-- 
2.34.1

[PATCH 1/2] reorder: invalidate buf from ready queue in drain

[Volodymyr Fialko]

Set drained buffers from ready queue to NULL, since their ownership
returned to user. Otherwise it's possible that both user and library
will attempt to free the packet.

Signed-off-by: Volodymyr Fialko <vfialko@marvell.com>
---
 lib/reorder/rte_reorder.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/reorder/rte_reorder.c b/lib/reorder/rte_reorder.c
index 385ee479da..b38e71f460 100644
--- a/lib/reorder/rte_reorder.c
+++ b/lib/reorder/rte_reorder.c
@@ -389,6 +389,7 @@ rte_reorder_drain(struct rte_reorder_buffer *b, struct rte_mbuf **mbufs,
 	/* Try to fetch requested number of mbufs from ready buffer */
 	while ((drain_cnt < max_mbufs) && (ready_buf->tail != ready_buf->head)) {
 		mbufs[drain_cnt++] = ready_buf->entries[ready_buf->tail];
+		ready_buf->entries[ready_buf->tail] = NULL;
 		ready_buf->tail = (ready_buf->tail + 1) & ready_buf->mask;
 	}
 
-- 
2.34.1

Re: [PATCH 1/2] reorder: invalidate buf from ready queue in drain

[Stephen Hemminger]

On Sat, 7 Jan 2023 16:19:38 +0100
Volodymyr Fialko <vfialko@marvell.com> wrote:

> Set drained buffers from ready queue to NULL, since their ownership
> returned to user. Otherwise it's possible that both user and library
> will attempt to free the packet.
> 
> Signed-off-by: Volodymyr Fialko <vfialko@marvell.com>

Looks correct to me.
Acked-by: Stephen Hemminger <stephen@networkplumber.org>

[PATCH 2/2] test/reorder: fix double free of drained buffers

[Volodymyr Fialko]

Set to zero array of drained buffers after free, to prevent freeing them
one more time.
Discovered with enabled `RTE_LIBRTE_MEMPOOL_DEBUG`.

Fixes: ecd867faa860 ("test/reorder: fix freeing mbuf twice")

Signed-off-by: Volodymyr Fialko <vfialko@marvell.com>
---
 app/test/test_reorder.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/app/test/test_reorder.c b/app/test/test_reorder.c
index f0714a5c18..7b5e590bac 100644
--- a/app/test/test_reorder.c
+++ b/app/test/test_reorder.c
@@ -278,6 +278,7 @@ test_reorder_drain(void)
 		goto exit;
 	}
 	rte_pktmbuf_free(robufs[0]);
+	memset(robufs, 0, sizeof(robufs));
 
 	/* Insert more packets
 	 * RB[] = {NULL, NULL, NULL, NULL}
@@ -313,6 +314,7 @@ test_reorder_drain(void)
 	for (i = 0; i < 3; i++) {
 		rte_pktmbuf_free(robufs[i]);
 	}
+	memset(robufs, 0, sizeof(robufs));
 
 	/*
 	 * RB[] = {NULL, NULL, NULL, NULL}
-- 
2.34.1

Re: [PATCH 2/2] test/reorder: fix double free of drained buffers

[David Marchand]

On Sat, Jan 7, 2023 at 4:20 PM Volodymyr Fialko <vfialko@marvell.com> wrote:
>
> Set to zero array of drained buffers after free, to prevent freeing them
> one more time.
> Discovered with enabled `RTE_LIBRTE_MEMPOOL_DEBUG`.

Good catch, having those debug options enabled in the CI could be interesting.
Cc: CI people, for info.


-- 
David Marchand

Re: [PATCH 2/2] test/reorder: fix double free of drained buffers

[Stephen Hemminger]

On Sat, 7 Jan 2023 16:19:39 +0100
Volodymyr Fialko <vfialko@marvell.com> wrote:

> Set to zero array of drained buffers after free, to prevent freeing them
> one more time.
> Discovered with enabled `RTE_LIBRTE_MEMPOOL_DEBUG`.
> 
> Fixes: ecd867faa860 ("test/reorder: fix freeing mbuf twice")
> 
> Signed-off-by: Volodymyr Fialko <vfialko@marvell.com>

Maybe include the actual test failure log next time.

Looks correct to me.
Acked-by: Stephen Hemminger <stephen@networkplumber.org>

Re: [PATCH 0/2] lib/reorder: fix drain/free issues

[David Marchand]

Hi Reshma,

On Sat, Jan 7, 2023 at 4:20 PM Volodymyr Fialko <vfialko@marvell.com> wrote:
>
> This patch address issues with reorder drain/free,
> discovered with enabled `RTE_LIBRTE_MEMPOOL_DEBUG`.
>
> Volodymyr Fialko (2):
>   reorder: invalidate buf from ready queue in drain
>   test/reorder: fix double free of drained buffers
>
>  app/test/test_reorder.c   | 2 ++
>  lib/reorder/rte_reorder.c | 1 +
>  2 files changed, 3 insertions(+)

Please review, thanks.


-- 
David Marchand

Re: [PATCH 0/2] lib/reorder: fix drain/free issues

[Thomas Monjalon]

Pïng for review

26/01/2023 16:47, David Marchand:
> Hi Reshma,
> 
> On Sat, Jan 7, 2023 at 4:20 PM Volodymyr Fialko <vfialko@marvell.com> wrote:
> >
> > This patch address issues with reorder drain/free,
> > discovered with enabled `RTE_LIBRTE_MEMPOOL_DEBUG`.
> >
> > Volodymyr Fialko (2):
> >   reorder: invalidate buf from ready queue in drain
> >   test/reorder: fix double free of drained buffers
> >
> >  app/test/test_reorder.c   | 2 ++
> >  lib/reorder/rte_reorder.c | 1 +
> >  2 files changed, 3 insertions(+)
> 
> Please review, thanks.

RE: [PATCH 0/2] lib/reorder: fix drain/free issues

[Volodymyr Fialko]

Hi All,

Kind reminder to all maintainers, please review and ack/comment.

> This patch address issues with reorder drain/free, discovered with enabled
> `RTE_LIBRTE_MEMPOOL_DEBUG`.

Re: [PATCH 0/2] lib/reorder: fix drain/free issues

[Thomas Monjalon]

07/01/2023 16:19, Volodymyr Fialko:
> This patch address issues with reorder drain/free,
> discovered with enabled `RTE_LIBRTE_MEMPOOL_DEBUG`.
> 
> Volodymyr Fialko (2):
>   reorder: invalidate buf from ready queue in drain
>   test/reorder: fix double free of drained buffers

Applied, thanks.