RE: [PATCH v2 11/17] net/dpaa2: fix software taildrop buffer access

["Morten Brørup" <mb@smartsharesystems.com>]

> From: David Marchand [mailto:david.marchand@redhat.com]
> Sent: Friday, 27 February 2026 10.24
> 
> On Thu, 26 Feb 2026 at 20:22, Stephen Hemminger
> <stephen@networkplumber.org> wrote:
> >
> > On Thu, 26 Feb 2026 15:33:35 +0100
> > Maxime Leroy <maxime@leroys.fr> wrote:
> >
> > > The sw_td path in dpaa2_dev_tx() reads from the wrong position in
> the
> > > bufs array. When the goto fires, bufs has already been advanced
> past
> > > the num_tx successfully sent packets. The first loop then reads
> num_tx
> > > more entries starting from bufs, going past the end of the input
> array.
> > > Additionally, the buf_to_free segments for already-enqueued packets
> > > are never freed, leaking memory.
> > >
> > > Replace the buggy sw_td code with the same pattern used in
> > > dpaa2_dev_tx_ordered(): free buf_to_free segments first, then use
> > > rte_pktmbuf_free_bulk() to drop remaining unsent packets.
> > >
> > > Not tested, found by code review.
> > >
> > > Cc: stable@dpdk.org
> > > Fixes: c3ffe74d85be ("net/dpaa2: support software taildrop")
> > >
> > > Reported-by: Stephen Hemminger <stephen@networkplumber.org>
> > > Signed-off-by: Maxime Leroy <maxime@leroys.fr>
> > > ---
> > >  drivers/net/dpaa2/dpaa2_rxtx.c | 18 ++++++------------
> > >  1 file changed, 6 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/drivers/net/dpaa2/dpaa2_rxtx.c
> b/drivers/net/dpaa2/dpaa2_rxtx.c
> > > index 689e5e7ee7..8275ba9780 100644
> > > --- a/drivers/net/dpaa2/dpaa2_rxtx.c
> > > +++ b/drivers/net/dpaa2/dpaa2_rxtx.c
> > > @@ -1517,21 +1517,15 @@ dpaa2_dev_tx(void *queue, struct rte_mbuf
> **bufs, uint16_t nb_pkts)
> > >
> > >       return num_tx;
> > >  sw_td:
> > > -     loop = 0;
> > > -     while (loop < num_tx) {
> > > -             if (unlikely(RTE_MBUF_HAS_EXTBUF(*bufs)))
> > > -                     rte_pktmbuf_free(*bufs);
> > > -             bufs++;
> > > -             loop++;
> > > +     for (loop = 0; loop < free_count; loop++) {
> > > +             if (buf_to_free[loop].pkt_id < num_tx)
> > > +                     rte_pktmbuf_free_seg(buf_to_free[loop].seg);
> > >       }
> > >
> > >       /* free the pending buffers */
> > > -     while (nb_pkts) {
> > > -             rte_pktmbuf_free(*bufs);
> > > -             bufs++;
> > > -             nb_pkts--;
> > > -             num_tx++;
> > > -     }
> > > +     rte_pktmbuf_free_bulk(bufs, nb_pkts);
> >
> > It is unsafe to use free_bulk on transmit path because packets in
> > burst might come from different pools. An example would be a router
> > forwarding from two incoming NIC's to one outgoing NIC.
> 
> Is it?
> 
> IIUC, this helper was added specifically to handle mbufs from multiple
> mempools.
> 
> static void
> __rte_pktmbuf_free_seg_via_array(struct rte_mbuf *m,
>         struct rte_mbuf ** const pending, unsigned int * const
> nb_pending,
>         const unsigned int pending_sz)
> {
>         m = rte_pktmbuf_prefree_seg(m);
>         if (likely(m != NULL)) {
>                 if (*nb_pending == pending_sz ||
>                     (*nb_pending > 0 && m->pool != pending[0]->pool))
> {
>                         rte_mempool_put_bulk(pending[0]->pool,
>                                         (void **)pending,
> *nb_pending);
>                         *nb_pending = 0;
>                 }
> 
>                 pending[(*nb_pending)++] = m;
>         }
> }
> 
> void rte_pktmbuf_free_bulk(struct rte_mbuf **mbufs, unsigned int
> count)
>         for (idx = 0; idx < count; idx++) {
>                 m = mbufs[idx];
>                 do {
>                         m_next = m->next;
>                         __rte_pktmbuf_free_seg_via_array(m,
>                                         pending, &nb_pending,
>                                         RTE_PKTMBUF_FREE_PENDING_SZ);
>                         m = m_next;
>                 } while (m != NULL);
> }
> 
> 
> --
> David Marchand

I guess rte_pktmbuf_free_bulk() was mistaken for rte_mbuf_raw_free_bulk().

The first is perfectly good to free from a mix of multiple mempools, the latter takes the mempool as a parameter.