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

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

Posted by Sungwoo Kim 2 days, 12 hours ago

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@sung-woo.kim>
---
V1: https://lore.kernel.org/linux-cxl/20260309000810.2632065-2-iam@sung-woo.kim/
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);
 }
 
 static int add_dpa_res(struct device *dev, struct resource *parent,
@@ -499,16 +505,8 @@ int devm_cxl_dpa_reserve(struct cxl_endpoint_decoder *cxled,
 				resource_size_t base, resource_size_t len,
 				resource_size_t skipped)
 {
-	struct cxl_port *port = cxled_to_port(cxled);
-	int rc;
-
-	scoped_guard(rwsem_write, &cxl_rwsem.dpa)
-		rc = __cxl_dpa_reserve(cxled, base, len, skipped);
-
-	if (rc)
-		return rc;
-
-	return devm_add_action_or_reset(&port->dev, cxl_dpa_release, cxled);
+	guard(rwsem_write)(&cxl_rwsem.dpa);
+	return __cxl_dpa_reserve(cxled, base, len, skipped);
 }
 EXPORT_SYMBOL_NS_GPL(devm_cxl_dpa_reserve, "CXL");
 
@@ -606,7 +604,8 @@ static int __cxl_dpa_alloc(struct cxl_endpoint_decoder *cxled, u64 size)
 	struct resource *p, *last;
 	int part;
 
-	guard(rwsem_write)(&cxl_rwsem.dpa);
+	lockdep_assert_held_write(&cxl_rwsem.dpa);
+
 	if (cxled->cxld.region) {
 		dev_dbg(dev, "decoder attached to %s\n",
 			dev_name(&cxled->cxld.region->dev));
@@ -669,14 +668,8 @@ static int __cxl_dpa_alloc(struct cxl_endpoint_decoder *cxled, u64 size)
 
 int cxl_dpa_alloc(struct cxl_endpoint_decoder *cxled, u64 size)
 {
-	struct cxl_port *port = cxled_to_port(cxled);
-	int rc;
-
-	rc = __cxl_dpa_alloc(cxled, size);
-	if (rc)
-		return rc;
-
-	return devm_add_action_or_reset(&port->dev, cxl_dpa_release, cxled);
+	guard(rwsem_write)(&cxl_rwsem.dpa);
+	return __cxl_dpa_alloc(cxled, size);
 }
 
 static void cxld_set_interleave(struct cxl_decoder *cxld, u32 *ctrl)
-- 
2.47.3

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

Posted by Dan Williams 2 days, 3 hours ago

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@sung-woo.kim>
> ---
> V1: https://lore.kernel.org/linux-cxl/20260309000810.2632065-2-iam@sung-woo.kim/
> 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;

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

Posted by Dave Jiang 2 days, 3 hours ago


On 3/30/26 6:01 AM, 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@sung-woo.kim>
> ---
> V1: https://lore.kernel.org/linux-cxl/20260309000810.2632065-2-iam@sung-woo.kim/
> 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);

With this function added the devm_add_action_or_reset(), I think it needs to be renamed to __devm_cxl_add_dpa_reserve().

DJ

>  }
>  
>  static int add_dpa_res(struct device *dev, struct resource *parent,
> @@ -499,16 +505,8 @@ int devm_cxl_dpa_reserve(struct cxl_endpoint_decoder *cxled,
>  				resource_size_t base, resource_size_t len,
>  				resource_size_t skipped)
>  {
> -	struct cxl_port *port = cxled_to_port(cxled);
> -	int rc;
> -
> -	scoped_guard(rwsem_write, &cxl_rwsem.dpa)
> -		rc = __cxl_dpa_reserve(cxled, base, len, skipped);
> -
> -	if (rc)
> -		return rc;
> -
> -	return devm_add_action_or_reset(&port->dev, cxl_dpa_release, cxled);
> +	guard(rwsem_write)(&cxl_rwsem.dpa);
> +	return __cxl_dpa_reserve(cxled, base, len, skipped);
>  }
>  EXPORT_SYMBOL_NS_GPL(devm_cxl_dpa_reserve, "CXL");
>  
> @@ -606,7 +604,8 @@ static int __cxl_dpa_alloc(struct cxl_endpoint_decoder *cxled, u64 size)
>  	struct resource *p, *last;
>  	int part;
>  
> -	guard(rwsem_write)(&cxl_rwsem.dpa);
> +	lockdep_assert_held_write(&cxl_rwsem.dpa);
> +
>  	if (cxled->cxld.region) {
>  		dev_dbg(dev, "decoder attached to %s\n",
>  			dev_name(&cxled->cxld.region->dev));
> @@ -669,14 +668,8 @@ static int __cxl_dpa_alloc(struct cxl_endpoint_decoder *cxled, u64 size)
>  
>  int cxl_dpa_alloc(struct cxl_endpoint_decoder *cxled, u64 size)
>  {
> -	struct cxl_port *port = cxled_to_port(cxled);
> -	int rc;
> -
> -	rc = __cxl_dpa_alloc(cxled, size);
> -	if (rc)
> -		return rc;
> -
> -	return devm_add_action_or_reset(&port->dev, cxl_dpa_release, cxled);
> +	guard(rwsem_write)(&cxl_rwsem.dpa);
> +	return __cxl_dpa_alloc(cxled, size);
>  }
>  
>  static void cxld_set_interleave(struct cxl_decoder *cxld, u32 *ctrl)