Re: [PATCH bpf-next v4 04/15] selftests/bpf: test_xsk: fix memory leak in testapp_stats_rx_dropped()

[Maciej Fijalkowski <maciej.fijalkowski@intel.com>]

On Fri, Sep 26, 2025 at 08:39:28AM +0200, Bastien Curutchet wrote:
> Hi Maciej,
> 
> On 9/25/25 3:32 PM, Maciej Fijalkowski wrote:
> > On Wed, Sep 24, 2025 at 04:49:39PM +0200, Bastien Curutchet (eBPF Foundation) wrote:
> > > testapp_stats_rx_dropped() generates pkt_stream twice. The last
> > > generated is released by pkt_stream_restore_default() at the end of the
> > > test but we lose the pointer of the first pkt_stream.
> > > 
> > > Release the 'middle' pkt_stream when it's getting replaced to prevent
> > > memory leaks.
> > > 
> > > Signed-off-by: Bastien Curutchet (eBPF Foundation) <bastien.curutchet@bootlin.com>
> > > ---
> > >   tools/testing/selftests/bpf/test_xsk.c | 7 +++++++
> > >   1 file changed, 7 insertions(+)
> > > 
> > > diff --git a/tools/testing/selftests/bpf/test_xsk.c b/tools/testing/selftests/bpf/test_xsk.c
> > > index 8d7c38eb32ca3537cb019f120c3350ebd9f8c6bc..eb18288ea1e4aa1c9337d16333b7174ecaed0999 100644
> > > --- a/tools/testing/selftests/bpf/test_xsk.c
> > > +++ b/tools/testing/selftests/bpf/test_xsk.c
> > > @@ -536,6 +536,13 @@ static void pkt_stream_receive_half(struct test_spec *test)
> > >   	struct pkt_stream *pkt_stream = test->ifobj_tx->xsk->pkt_stream;
> > >   	u32 i;
> > > +	if (test->ifobj_rx->xsk->pkt_stream != test->rx_pkt_stream_default)
> > > +		/* Packet stream has already been replaced so we have to release this one.
> > > +		 * The newly created one will be freed by the restore_default() at the
> > > +		 * end of the test
> > > +		 */
> > > +		pkt_stream_delete(test->ifobj_rx->xsk->pkt_stream);
> > 
> > I don't see why this one is not addressed within test case
> > (testapp_stats_rx_dropped()) and other fix is (testapp_xdp_shared_umem()).
> > 
> 
> pkt_stream_receive_half() can be used by other tests. I thought it would be

So is pkt_stream_replace_half() and other routines that eventually call
pkt_stream_generate() and overwrite the pkt_stream, right?

It just feels odd to have a special treatment in one function and other
are left as-is just because currently we don't have another abusive test
case.

Maybe it's enough of bike-shedding here, just wanted to clarify on my POV.

In the end don't get me wrong here, this interface is a bit PITA for me
and thanks for whole effort!

> more convenient for people writing testapp_*() functions if they didn't have
> to worry about releasing these kind of pointer themselves.
> 
> The same approach can't be used in testapp_xdp_shared_umem(), because we
> need to wait for the test to complete before releasing the pointers.
> 
> 
> -- 
> Bastien Curutchet, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
>