[PATCH v1] common/idpf: fix heap use after free error

[PATCH v1] common/idpf: fix heap use after free error

[Praveen Shetty]

Heap use after free error is detected in AddressSanitizer while quitting
the testpmd application.Issue is due to accessing the empty control
queue in the idpf_ctlq_deinit function.idpf_ctlq_deinit function is called
during the rte_eal_cleanup routine.
This patch will fix this issue.

Fixes: fb4ac04e9bfa ("common/idpf: introduce common library")
Cc: stable@dpdk.org

Signed-off-by: Praveen Shetty <praveen.shetty@intel.com>
---
 drivers/common/idpf/base/idpf_controlq.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/common/idpf/base/idpf_controlq.c b/drivers/common/idpf/base/idpf_controlq.c
index 4f47759a4f..8f404d3083 100644
--- a/drivers/common/idpf/base/idpf_controlq.c
+++ b/drivers/common/idpf/base/idpf_controlq.c
@@ -248,9 +248,10 @@ int idpf_ctlq_init(struct idpf_hw *hw, u8 num_q,
 	return 0;
 
 init_destroy_qs:
-	LIST_FOR_EACH_ENTRY_SAFE(cq, tmp, &hw->cq_list_head,
-				 idpf_ctlq_info, cq_list)
+	while (!LIST_EMPTY(&hw->cq_list_head)) {
+		cq = LIST_FIRST(&hw->cq_list_head);
 		idpf_ctlq_remove(hw, cq);
+	}
 
 	return err;
 }
@@ -263,9 +264,10 @@ void idpf_ctlq_deinit(struct idpf_hw *hw)
 {
 	struct idpf_ctlq_info *cq = NULL, *tmp = NULL;
 
-	LIST_FOR_EACH_ENTRY_SAFE(cq, tmp, &hw->cq_list_head,
-				 idpf_ctlq_info, cq_list)
+	while (!LIST_EMPTY(&hw->cq_list_head)) {
+		cq = LIST_FIRST(&hw->cq_list_head);
 		idpf_ctlq_remove(hw, cq);
+	}
 }
 
 /**
-- 
2.34.1

Re: [PATCH v1] common/idpf: fix heap use after free error

[Stephen Hemminger]

On Mon, 13 Jan 2025 08:54:04 +0000
Praveen Shetty <praveen.shetty@intel.com> wrote:

> Heap use after free error is detected in AddressSanitizer while quitting
> the testpmd application.Issue is due to accessing the empty control
> queue in the idpf_ctlq_deinit function.idpf_ctlq_deinit function is called
> during the rte_eal_cleanup routine.
> This patch will fix this issue.
> 
> Fixes: fb4ac04e9bfa ("common/idpf: introduce common library")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Praveen Shetty <praveen.shetty@intel.com>

This should not be needed. LIST_FOR_EACH_ENTRY_SAFE part, don't understand.

Re: [PATCH v1] common/idpf: fix heap use after free error

[Bruce Richardson]

On Mon, Jan 13, 2025 at 08:30:01AM -0800, Stephen Hemminger wrote:
> On Mon, 13 Jan 2025 08:54:04 +0000
> Praveen Shetty <praveen.shetty@intel.com> wrote:
> 
> > Heap use after free error is detected in AddressSanitizer while quitting
> > the testpmd application.Issue is due to accessing the empty control
> > queue in the idpf_ctlq_deinit function.idpf_ctlq_deinit function is called
> > during the rte_eal_cleanup routine.
> > This patch will fix this issue.
> > 
> > Fixes: fb4ac04e9bfa ("common/idpf: introduce common library")
> > Cc: stable@dpdk.org
> > 
> > Signed-off-by: Praveen Shetty <praveen.shetty@intel.com>
> 
> This should not be needed. LIST_FOR_EACH_ENTRY_SAFE part, don't understand.

I would tend to agree. Is there an actual confirmed bug here? If so, then
either our standard list macros are broken, or the code using them is doing
something rather strange.

/Bruce

Re: [PATCH v1] common/idpf: fix heap use after free error

[Bruce Richardson]

On Mon, Jan 20, 2025 at 02:32:49PM +0000, Bruce Richardson wrote:
> On Mon, Jan 13, 2025 at 08:30:01AM -0800, Stephen Hemminger wrote:
> > On Mon, 13 Jan 2025 08:54:04 +0000
> > Praveen Shetty <praveen.shetty@intel.com> wrote:
> > 
> > > Heap use after free error is detected in AddressSanitizer while quitting
> > > the testpmd application.Issue is due to accessing the empty control
> > > queue in the idpf_ctlq_deinit function.idpf_ctlq_deinit function is called
> > > during the rte_eal_cleanup routine.
> > > This patch will fix this issue.
> > > 
> > > Fixes: fb4ac04e9bfa ("common/idpf: introduce common library")
> > > Cc: stable@dpdk.org
> > > 
> > > Signed-off-by: Praveen Shetty <praveen.shetty@intel.com>
> > 
> > This should not be needed. LIST_FOR_EACH_ENTRY_SAFE part, don't understand.
> 
> I would tend to agree. Is there an actual confirmed bug here? If so, then
> either our standard list macros are broken, or the code using them is doing
> something rather strange.
> 

I followed up on with with Praveen, and he went through the code and
possible solutions with me. The issue flagged by ASAN is correct, because
it turns out that the version of the _SAFE macro provided in this
particular driver is not actually safe! :-(

There are therefore two options to fixing this: 1) fix the macro/use a
different copy of the macro, or 2) rework the code as in this patch and drop
the macro. Copies of the driver in other OS use the style given in this patch,
so we will go with the second option. However, we will do a v2 to include
the removal of the bad macro, alongside fixing this. That should hopefully
prevent this issue from reoccurring.

Praveen, will review v2 when you send it.

/Bruce

Re: [PATCH v1] common/idpf: fix heap use after free error

[David Marchand]

On Thu, Jan 23, 2025 at 12:18 PM Bruce Richardson
<bruce.richardson@intel.com> wrote:
>
> On Mon, Jan 20, 2025 at 02:32:49PM +0000, Bruce Richardson wrote:
> > On Mon, Jan 13, 2025 at 08:30:01AM -0800, Stephen Hemminger wrote:
> > > On Mon, 13 Jan 2025 08:54:04 +0000
> > > Praveen Shetty <praveen.shetty@intel.com> wrote:
> > >
> > > > Heap use after free error is detected in AddressSanitizer while quitting
> > > > the testpmd application.Issue is due to accessing the empty control
> > > > queue in the idpf_ctlq_deinit function.idpf_ctlq_deinit function is called
> > > > during the rte_eal_cleanup routine.
> > > > This patch will fix this issue.
> > > >
> > > > Fixes: fb4ac04e9bfa ("common/idpf: introduce common library")
> > > > Cc: stable@dpdk.org
> > > >
> > > > Signed-off-by: Praveen Shetty <praveen.shetty@intel.com>
> > >
> > > This should not be needed. LIST_FOR_EACH_ENTRY_SAFE part, don't understand.
> >
> > I would tend to agree. Is there an actual confirmed bug here? If so, then
> > either our standard list macros are broken, or the code using them is doing
> > something rather strange.
> >
>
> I followed up on with with Praveen, and he went through the code and
> possible solutions with me. The issue flagged by ASAN is correct, because
> it turns out that the version of the _SAFE macro provided in this
> particular driver is not actually safe! :-(
>
> There are therefore two options to fixing this: 1) fix the macro/use a
> different copy of the macro, or 2) rework the code as in this patch and drop
> the macro. Copies of the driver in other OS use the style given in this patch,
> so we will go with the second option. However, we will do a v2 to include
> the removal of the bad macro, alongside fixing this. That should hopefully
> prevent this issue from reoccurring.
>
> Praveen, will review v2 when you send it.

Sorry, I am not following.

1) seems the best way as it does not require touching base driver code.
Afaiu, the LIST_FOR_EACH_ENTRY_SAFE macro is defined in the
"abstraction" header that is DPDK specific
(drivers/common/idpf/base/idpf_osdep.h).

There is already an implementation of LIST_FOR_EACH_ENTRY_SAFE in
driver/net/ice/base/ice_osdep.h.

(note that it may be worth providing such a macro in a common place in
DPDK and remove copies of it in various drivers).


-- 
David Marchand

Re: [PATCH v1] common/idpf: fix heap use after free error

[Bruce Richardson]

On Thu, Jan 23, 2025 at 12:43:50PM +0100, David Marchand wrote:
> On Thu, Jan 23, 2025 at 12:18 PM Bruce Richardson
> <bruce.richardson@intel.com> wrote:
> >
> > On Mon, Jan 20, 2025 at 02:32:49PM +0000, Bruce Richardson wrote:
> > > On Mon, Jan 13, 2025 at 08:30:01AM -0800, Stephen Hemminger wrote:
> > > > On Mon, 13 Jan 2025 08:54:04 +0000 Praveen Shetty
> > > > <praveen.shetty@intel.com> wrote:
> > > >
> > > > > Heap use after free error is detected in AddressSanitizer while
> > > > > quitting the testpmd application.Issue is due to accessing the
> > > > > empty control queue in the idpf_ctlq_deinit
> > > > > function.idpf_ctlq_deinit function is called during the
> > > > > rte_eal_cleanup routine.  This patch will fix this issue.
> > > > >
> > > > > Fixes: fb4ac04e9bfa ("common/idpf: introduce common library") Cc:
> > > > > stable@dpdk.org
> > > > >
> > > > > Signed-off-by: Praveen Shetty <praveen.shetty@intel.com>
> > > >
> > > > This should not be needed. LIST_FOR_EACH_ENTRY_SAFE part, don't
> > > > understand.
> > >
> > > I would tend to agree. Is there an actual confirmed bug here? If so,
> > > then either our standard list macros are broken, or the code using
> > > them is doing something rather strange.
> > >
> >
> > I followed up on with with Praveen, and he went through the code and
> > possible solutions with me. The issue flagged by ASAN is correct,
> > because it turns out that the version of the _SAFE macro provided in
> > this particular driver is not actually safe! :-(
> >
> > There are therefore two options to fixing this: 1) fix the macro/use a
> > different copy of the macro, or 2) rework the code as in this patch and
> > drop the macro. Copies of the driver in other OS use the style given in
> > this patch, so we will go with the second option. However, we will do a
> > v2 to include the removal of the bad macro, alongside fixing this. That
> > should hopefully prevent this issue from reoccurring.
> >
> > Praveen, will review v2 when you send it.
> 
> Sorry, I am not following.
> 
> 1) seems the best way as it does not require touching base driver code.
> Afaiu, the LIST_FOR_EACH_ENTRY_SAFE macro is defined in the "abstraction"
> header that is DPDK specific (drivers/common/idpf/base/idpf_osdep.h).
> 
> There is already an implementation of LIST_FOR_EACH_ENTRY_SAFE in
> driver/net/ice/base/ice_osdep.h.
> 
> (note that it may be worth providing such a macro in a common place in
> DPDK and remove copies of it in various drivers).
> 

Yes, that is correct, and double checking the IDPF shared code confirms
that is the best solution.

/Bruce

Re: [PATCH v1] common/idpf: fix heap use after free error

[Stephen Hemminger]

On Thu, 23 Jan 2025 12:43:50 +0100
David Marchand <david.marchand@redhat.com> wrote:

> On Thu, Jan 23, 2025 at 12:18 PM Bruce Richardson
> <bruce.richardson@intel.com> wrote:
> >
> > On Mon, Jan 20, 2025 at 02:32:49PM +0000, Bruce Richardson wrote:  
> > > On Mon, Jan 13, 2025 at 08:30:01AM -0800, Stephen Hemminger wrote:  
> > > > On Mon, 13 Jan 2025 08:54:04 +0000
> > > > Praveen Shetty <praveen.shetty@intel.com> wrote:
> > > >  
> > > > > Heap use after free error is detected in AddressSanitizer while quitting
> > > > > the testpmd application.Issue is due to accessing the empty control
> > > > > queue in the idpf_ctlq_deinit function.idpf_ctlq_deinit function is called
> > > > > during the rte_eal_cleanup routine.
> > > > > This patch will fix this issue.
> > > > >
> > > > > Fixes: fb4ac04e9bfa ("common/idpf: introduce common library")
> > > > > Cc: stable@dpdk.org
> > > > >
> > > > > Signed-off-by: Praveen Shetty <praveen.shetty@intel.com>  
> > > >
> > > > This should not be needed. LIST_FOR_EACH_ENTRY_SAFE part, don't understand.  
> > >
> > > I would tend to agree. Is there an actual confirmed bug here? If so, then
> > > either our standard list macros are broken, or the code using them is doing
> > > something rather strange.
> > >  
> >
> > I followed up on with with Praveen, and he went through the code and
> > possible solutions with me. The issue flagged by ASAN is correct, because
> > it turns out that the version of the _SAFE macro provided in this
> > particular driver is not actually safe! :-(
> >
> > There are therefore two options to fixing this: 1) fix the macro/use a
> > different copy of the macro, or 2) rework the code as in this patch and drop
> > the macro. Copies of the driver in other OS use the style given in this patch,
> > so we will go with the second option. However, we will do a v2 to include
> > the removal of the bad macro, alongside fixing this. That should hopefully
> > prevent this issue from reoccurring.
> >
> > Praveen, will review v2 when you send it.  
> 
> Sorry, I am not following.
> 
> 1) seems the best way as it does not require touching base driver code.
> Afaiu, the LIST_FOR_EACH_ENTRY_SAFE macro is defined in the
> "abstraction" header that is DPDK specific
> (drivers/common/idpf/base/idpf_osdep.h).
> 
> There is already an implementation of LIST_FOR_EACH_ENTRY_SAFE in
> driver/net/ice/base/ice_osdep.h.
> 
> (note that it may be worth providing such a macro in a common place in
> DPDK and remove copies of it in various drivers).


Yes, all the variants of LIST and TAILQ macros from FreeBSD should be
added, maybe a DPDK version of queue.h?