Re: [PATCH v2] cxl/hdm: fix a warning in devm_remove_action()

[Sungwoo Kim]

On Tue, Mar 31, 2026 at 6:58 AM Dan Williams <dan.j.williams@xxxxxxxxx> wrote:
>
> Sungwoo Kim wrote:
> > In the following race scenario, devm_remove_action() can be called
> > before devm_add_action(), triggering a warning because there is no
> > action to remove.
> >
> > To fix this, __cxl_dpa_reserve() performs devm_add_action(). This
> > extends the critical section that embraces cxled->dpa_res = res and
> > devm_add_action(), so cxl_dpa_free() cannot observe dpa_res without
> > the devres action.
> >
> > task 1:
> > cxl_dpa_alloc()
> >   __cxl_dpa_alloc()
> >     guard(&cxl_rwsem.dpa)
> >     cxled->dpa_res = res;  ...(1)
> >   devm_add_action()        ...(4)
> >
> > task 2:
> > cxl_dpa_free()
> >   guard(&cxl_rwsem.dpa)
> >   if (!cxled->dpa_res)     ...(2) pass, due to (1)
> >     return 0;
> >   devm_cxl_dpa_release()
> >     devm_remove_action()   ...(3) warning, no action is added yet
> >
> > Splat:
> >
> > WARNING: ./include/linux/device/devres.h:160 at devm_remove_action include/linux/device/devres.h:160 [inline], CPU#0: syz.1.6464/25993
> > WARNING: ./include/linux/device/devres.h:160 at devm_cxl_dpa_release drivers/cxl/core/hdm.c:290 [inline], CPU#0: syz.1.6464/25993
> > WARNING: ./include/linux/device/devres.h:160 at cxl_dpa_free+0x2a4/0x320 drivers/cxl/core/hdm.c:572, CPU#0: syz.1.6464/25993
> >
> > Fixes: cf880423b6a0 ("cxl/hdm: Add support for allocating DPA to an endpoint decoder")
> > Fixes: 9c57cde0dcbd ("cxl/hdm: Enumerate allocated DPA")
> > Signed-off-by: Sungwoo Kim <iam@xxxxxxxxxxxx>
> > ---
> > V1: https://lore.kernel.org/linux-cxl/20260309000810.2632065-2-iam@xxxxxxxxxxxx/
> > V1->V2:
> > - Let __cxl_dpa_reserve() handle devm_remove_action() to reduce
> >   duplicated patches. (Thanks to Dan for the suggestion)
> > - Make the comment concise (Reflect Dan and Alison's comments)
> > - Make a warning log in a commit message concise
> >
> >  drivers/cxl/core/hdm.c | 33 +++++++++++++--------------------
> >  1 file changed, 13 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> > index cb5d5a047a9d..63bd0d5f31df 100644
> > --- a/drivers/cxl/core/hdm.c
> > +++ b/drivers/cxl/core/hdm.c
> > @@ -408,7 +408,13 @@ static int __cxl_dpa_reserve(struct cxl_endpoint_decoder *cxled,
> >
> >       port->hdm_end++;
> >       get_device(&cxled->cxld.dev);
> > -     return 0;
> > +
> > +     /*
> > +      * Perform devres registration while holding cxl_rwsem.dpa so
> > +      * cxl_dpa_free() cannot observe dpa_res without a matching devres
> > +      * action.
> > +      */
> > +     return devm_add_action_or_reset(&port->dev, cxl_dpa_release, cxled);
>
> This needs the pattern you identified in v1 and comments should be
> reserved for cases where the code needs more explanation. Something
> like:
>
> /*
>  * Invoke the release action with the unlocked __cxl_dpa_release() since
>  * cxl_dpa_release() acquires the the dpa rwsem
>  */
> rc = devm_add_action(..., cxl_dpa_release())
> if (rc)
>     __cxl_dpa_release()
> return rc;
>

Dan,
Thank you for your review and suggestion. I will fix this and add a
comment in v3.

Sungwoo.