Re: [PATCH v7 4/6] eal: fix async IPC memory leaks on partial failure

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

On Wed, Jul 01, 2026 at 11:47:05AM +0200, David Marchand wrote:
> On Fri, 26 Jun 2026 at 12:34, Anatoly Burakov <anatoly.burakov@intel.com> wrote:
> >
> > When rte_mp_request_async() fails to send requests to all peers,
> > copy and param can lose ownership and leak.
> >
> > However, we cannot simply free them unconditionally, as "partial failure"
> > means some requests were already queued and thus still reference `copy` and
> > `param`, so freeing them directly on the error path can cause
> > use-after-free when those requests are later handled by the async timeout.
> >
> > Fix this by rolling back queued requests from the current batch, and reset
> > nb_sent to 0. Freeing the requests is now safe even if some requests were
> > sent, as any responses or timeouts will not find the request ID in the
> > queue and will safely exit without doing anything.
> >
> > Coverity issue: 501503
> >
> > Fixes: f05e26051c15 ("eal: add IPC asynchronous request")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> > ---
> >  lib/eal/common/eal_common_proc.c | 31 +++++++++++++++++++++++++++++++
> >  1 file changed, 31 insertions(+)
> >
> > diff --git a/lib/eal/common/eal_common_proc.c b/lib/eal/common/eal_common_proc.c
> > index 991bf215a3..0cffc7a127 100644
> > --- a/lib/eal/common/eal_common_proc.c
> > +++ b/lib/eal/common/eal_common_proc.c
> > @@ -1245,6 +1245,32 @@ rte_mp_request_async(struct rte_mp_msg *req, const struct timespec *ts,
> >                 } else if (mp_request_async(path, copy, param, ts))
> >                         ret = -1;
> >         }
> > +
> > +       /*
> > +        * On partial failure, roll back all queued requests. We hold the lock
> > +        * so no one else touches the queue. All requests in this batch share
> > +        * the same param pointer. Stale alarms will fire and harmlessly find
> > +        * nothing via ID-based lookup.
> > +        */
> > +       if (ret != 0 && reply->nb_sent > 0) {
> > +               struct pending_request *r, *next;
> > +
> > +               for (r = TAILQ_FIRST(&pending_requests.requests);
> > +                               r != NULL; r = next) {
> > +                       next = TAILQ_NEXT(r, next);
> > +                       if (r->type == REQUEST_TYPE_ASYNC &&
> > +                                       r->async.param == param) {
> > +                               TAILQ_REMOVE(&pending_requests.requests,
> > +                                               r, next);
> > +                               free(r->reply);
> > +                               /* r->request == copy, freed below after the loop */
> > +                               free(r);
> > +                       }
> > +               }
> > +               /* requests on the queue were removed so keep things consistent */
> > +               reply->nb_sent = 0;
> > +       }
> > +
> 
> Please, don't reimplement the safe macro.
> 
> I plan to update this with:
> 
> @@ -1252,15 +1252,11 @@ rte_mp_request_async(struct rte_mp_msg *req,
> const struct timespec *ts,
>          * nothing via ID-based lookup.
>          */
>         if (ret != 0 && reply->nb_sent > 0) {
> -               struct pending_request *r, *next;
> -
> -               for (r = TAILQ_FIRST(&pending_requests.requests);
> -                               r != NULL; r = next) {
> -                       next = TAILQ_NEXT(r, next);
> -                       if (r->type == REQUEST_TYPE_ASYNC &&
> -                                       r->async.param == param) {
> -                               TAILQ_REMOVE(&pending_requests.requests,
> -                                               r, next);
> +               struct pending_request *r, *tmp;
> +
> +               RTE_TAILQ_FOREACH_SAFE(r, &pending_requests.requests,
> next, tmp) {
> +                       if (r->type == REQUEST_TYPE_ASYNC &&
> r->async.param == param) {
> +
> TAILQ_REMOVE(&pending_requests.requests, r, next);
>                                 free(r->reply);
>                                 /* r->request == copy, freed below
> after the loop */
>                                 free(r);
> 
> Objection?
> If not, I'll update while applying.
> 
+1 to this suggestion from me.