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

[Bruce Richardson <bruce.richardson@intel.com>]

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