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

[Stephen Hemminger <stephen@networkplumber.org>]

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?