[PATCH v2] cxl/region: refactor decoder allocation for region refs

[PATCH v2] cxl/region: refactor decoder allocation for region refs

[Vishal Verma]

When an intermediate port's decoders have been exhausted by existing
regions, and creating a new region with the port in question in it's
hierarchical path is attempted, cxl_port_attach_region() fails to find a
port decoder (as would be expected), and drops into the failure / cleanup
path.

However, during cleanup of the region reference, a sanity check attempts
to dereference the decoder, which in the above case didn't exist. This
causes a NULL pointer dereference BUG.

To fix this, refactor the decoder allocation and de-allocation into
helper routines, and in this 'free' routine, check that the decoder,
@cxld, is valid before attempting any operations on it.

Cc: Dan Williams <dan.j.williams@intel.com>
Suggested-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 drivers/cxl/core/region.c | 67 ++++++++++++++++++++++++---------------
 1 file changed, 41 insertions(+), 26 deletions(-)

Changes since v1[1]:
- Limit the new decoder alloc helper to only the decoder allocation for
  new cxl_region_ref objects, not retrieval for existing refs (Dan).

[1]: https://lore.kernel.org/linux-cxl/89acba4011d03582a1f81feb376915b826020cee.camel@intel.com


diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 401148016978..986855e93e71 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -686,18 +686,27 @@ static struct cxl_region_ref *alloc_region_ref(struct cxl_port *port,
 	return cxl_rr;
 }
 
-static void free_region_ref(struct cxl_region_ref *cxl_rr)
+static void cxl_rr_free_decoder(struct cxl_region_ref *cxl_rr)
 {
-	struct cxl_port *port = cxl_rr->port;
 	struct cxl_region *cxlr = cxl_rr->region;
 	struct cxl_decoder *cxld = cxl_rr->decoder;
 
+	if (!cxld)
+		return;
+
 	dev_WARN_ONCE(&cxlr->dev, cxld->region != cxlr, "region mismatch\n");
 	if (cxld->region == cxlr) {
 		cxld->region = NULL;
 		put_device(&cxlr->dev);
 	}
+}
 
+static void free_region_ref(struct cxl_region_ref *cxl_rr)
+{
+	struct cxl_port *port = cxl_rr->port;
+	struct cxl_region *cxlr = cxl_rr->region;
+
+	cxl_rr_free_decoder(cxl_rr);
 	xa_erase(&port->regions, (unsigned long)cxlr);
 	xa_destroy(&cxl_rr->endpoints);
 	kfree(cxl_rr);
@@ -728,6 +737,33 @@ static int cxl_rr_ep_add(struct cxl_region_ref *cxl_rr,
 	return 0;
 }
 
+static int cxl_rr_alloc_decoder(struct cxl_port *port, struct cxl_region *cxlr,
+				struct cxl_endpoint_decoder *cxled,
+				struct cxl_region_ref *cxl_rr)
+{
+	struct cxl_decoder *cxld;
+
+	if (port == cxled_to_port(cxled))
+		cxld = &cxled->cxld;
+	else
+		cxld = cxl_region_find_decoder(port, cxlr);
+	if (!cxld) {
+		dev_dbg(&cxlr->dev, "%s: no decoder available\n",
+			dev_name(&port->dev));
+		return -EBUSY;
+	}
+
+	if (cxld->region) {
+		dev_dbg(&cxlr->dev, "%s: %s already attached to %s\n",
+			dev_name(&port->dev), dev_name(&cxld->dev),
+			dev_name(&cxld->region->dev));
+		return -EBUSY;
+	}
+
+	cxl_rr->decoder = cxld;
+	return 0;
+}
+
 /**
  * cxl_port_attach_region() - track a region's interest in a port by endpoint
  * @port: port to add a new region reference 'struct cxl_region_ref'
@@ -794,12 +830,6 @@ static int cxl_port_attach_region(struct cxl_port *port,
 			cxl_rr->nr_targets++;
 			nr_targets_inc = true;
 		}
-
-		/*
-		 * The decoder for @cxlr was allocated when the region was first
-		 * attached to @port.
-		 */
-		cxld = cxl_rr->decoder;
 	} else {
 		cxl_rr = alloc_region_ref(port, cxlr);
 		if (IS_ERR(cxl_rr)) {
@@ -810,26 +840,11 @@ static int cxl_port_attach_region(struct cxl_port *port,
 		}
 		nr_targets_inc = true;
 
-		if (port == cxled_to_port(cxled))
-			cxld = &cxled->cxld;
-		else
-			cxld = cxl_region_find_decoder(port, cxlr);
-		if (!cxld) {
-			dev_dbg(&cxlr->dev, "%s: no decoder available\n",
-				dev_name(&port->dev));
+		rc = cxl_rr_alloc_decoder(port, cxlr, cxled, cxl_rr);
+		if (rc)
 			goto out_erase;
-		}
-
-		if (cxld->region) {
-			dev_dbg(&cxlr->dev, "%s: %s already attached to %s\n",
-				dev_name(&port->dev), dev_name(&cxld->dev),
-				dev_name(&cxld->region->dev));
-			rc = -EBUSY;
-			goto out_erase;
-		}
-
-		cxl_rr->decoder = cxld;
 	}
+	cxld = cxl_rr->decoder;
 
 	rc = cxl_rr_ep_add(cxl_rr, cxled);
 	if (rc) {
-- 
2.37.3

Re: [PATCH v2] cxl/region: refactor decoder allocation for region refs

[Dave Jiang]

On 11/1/2022 12:41 AM, Vishal Verma wrote:
> When an intermediate port's decoders have been exhausted by existing
> regions, and creating a new region with the port in question in it's
> hierarchical path is attempted, cxl_port_attach_region() fails to find a
> port decoder (as would be expected), and drops into the failure / cleanup
> path.
> 
> However, during cleanup of the region reference, a sanity check attempts
> to dereference the decoder, which in the above case didn't exist. This
> causes a NULL pointer dereference BUG.
> 
> To fix this, refactor the decoder allocation and de-allocation into
> helper routines, and in this 'free' routine, check that the decoder,
> @cxld, is valid before attempting any operations on it.
> 
> Cc: Dan Williams <dan.j.williams@intel.com>
> Suggested-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>

> ---
>   drivers/cxl/core/region.c | 67 ++++++++++++++++++++++++---------------
>   1 file changed, 41 insertions(+), 26 deletions(-)
> 
> Changes since v1[1]:
> - Limit the new decoder alloc helper to only the decoder allocation for
>    new cxl_region_ref objects, not retrieval for existing refs (Dan).
> 
> [1]: https://lore.kernel.org/linux-cxl/89acba4011d03582a1f81feb376915b826020cee.camel@intel.com
> 
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 401148016978..986855e93e71 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -686,18 +686,27 @@ static struct cxl_region_ref *alloc_region_ref(struct cxl_port *port,
>   	return cxl_rr;
>   }
>   
> -static void free_region_ref(struct cxl_region_ref *cxl_rr)
> +static void cxl_rr_free_decoder(struct cxl_region_ref *cxl_rr)
>   {
> -	struct cxl_port *port = cxl_rr->port;
>   	struct cxl_region *cxlr = cxl_rr->region;
>   	struct cxl_decoder *cxld = cxl_rr->decoder;
>   
> +	if (!cxld)
> +		return;
> +
>   	dev_WARN_ONCE(&cxlr->dev, cxld->region != cxlr, "region mismatch\n");
>   	if (cxld->region == cxlr) {
>   		cxld->region = NULL;
>   		put_device(&cxlr->dev);
>   	}
> +}
>   
> +static void free_region_ref(struct cxl_region_ref *cxl_rr)
> +{
> +	struct cxl_port *port = cxl_rr->port;
> +	struct cxl_region *cxlr = cxl_rr->region;
> +
> +	cxl_rr_free_decoder(cxl_rr);
>   	xa_erase(&port->regions, (unsigned long)cxlr);
>   	xa_destroy(&cxl_rr->endpoints);
>   	kfree(cxl_rr);
> @@ -728,6 +737,33 @@ static int cxl_rr_ep_add(struct cxl_region_ref *cxl_rr,
>   	return 0;
>   }
>   
> +static int cxl_rr_alloc_decoder(struct cxl_port *port, struct cxl_region *cxlr,
> +				struct cxl_endpoint_decoder *cxled,
> +				struct cxl_region_ref *cxl_rr)
> +{
> +	struct cxl_decoder *cxld;
> +
> +	if (port == cxled_to_port(cxled))
> +		cxld = &cxled->cxld;
> +	else
> +		cxld = cxl_region_find_decoder(port, cxlr);
> +	if (!cxld) {
> +		dev_dbg(&cxlr->dev, "%s: no decoder available\n",
> +			dev_name(&port->dev));
> +		return -EBUSY;
> +	}
> +
> +	if (cxld->region) {
> +		dev_dbg(&cxlr->dev, "%s: %s already attached to %s\n",
> +			dev_name(&port->dev), dev_name(&cxld->dev),
> +			dev_name(&cxld->region->dev));
> +		return -EBUSY;
> +	}
> +
> +	cxl_rr->decoder = cxld;
> +	return 0;
> +}
> +
>   /**
>    * cxl_port_attach_region() - track a region's interest in a port by endpoint
>    * @port: port to add a new region reference 'struct cxl_region_ref'
> @@ -794,12 +830,6 @@ static int cxl_port_attach_region(struct cxl_port *port,
>   			cxl_rr->nr_targets++;
>   			nr_targets_inc = true;
>   		}
> -
> -		/*
> -		 * The decoder for @cxlr was allocated when the region was first
> -		 * attached to @port.
> -		 */
> -		cxld = cxl_rr->decoder;
>   	} else {
>   		cxl_rr = alloc_region_ref(port, cxlr);
>   		if (IS_ERR(cxl_rr)) {
> @@ -810,26 +840,11 @@ static int cxl_port_attach_region(struct cxl_port *port,
>   		}
>   		nr_targets_inc = true;
>   
> -		if (port == cxled_to_port(cxled))
> -			cxld = &cxled->cxld;
> -		else
> -			cxld = cxl_region_find_decoder(port, cxlr);
> -		if (!cxld) {
> -			dev_dbg(&cxlr->dev, "%s: no decoder available\n",
> -				dev_name(&port->dev));
> +		rc = cxl_rr_alloc_decoder(port, cxlr, cxled, cxl_rr);
> +		if (rc)
>   			goto out_erase;
> -		}
> -
> -		if (cxld->region) {
> -			dev_dbg(&cxlr->dev, "%s: %s already attached to %s\n",
> -				dev_name(&port->dev), dev_name(&cxld->dev),
> -				dev_name(&cxld->region->dev));
> -			rc = -EBUSY;
> -			goto out_erase;
> -		}
> -
> -		cxl_rr->decoder = cxld;
>   	}
> +	cxld = cxl_rr->decoder;
>   
>   	rc = cxl_rr_ep_add(cxl_rr, cxled);
>   	if (rc) {

Re: [PATCH v2] cxl/region: refactor decoder allocation for region refs

[Jonathan Cameron]

On Tue,  1 Nov 2022 01:41:00 -0600
Vishal Verma <vishal.l.verma@intel.com> wrote:

> When an intermediate port's decoders have been exhausted by existing
> regions, and creating a new region with the port in question in it's
> hierarchical path is attempted, cxl_port_attach_region() fails to find a
> port decoder (as would be expected), and drops into the failure / cleanup
> path.
> 
> However, during cleanup of the region reference, a sanity check attempts
> to dereference the decoder, which in the above case didn't exist. This
> causes a NULL pointer dereference BUG.
> 
> To fix this, refactor the decoder allocation and de-allocation into
> helper routines, and in this 'free' routine, check that the decoder,
> @cxld, is valid before attempting any operations on it.
> 
> Cc: Dan Williams <dan.j.williams@intel.com>
> Suggested-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>

FWIW.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
>  drivers/cxl/core/region.c | 67 ++++++++++++++++++++++++---------------
>  1 file changed, 41 insertions(+), 26 deletions(-)
> 
> Changes since v1[1]:
> - Limit the new decoder alloc helper to only the decoder allocation for
>   new cxl_region_ref objects, not retrieval for existing refs (Dan).
> 
> [1]: https://lore.kernel.org/linux-cxl/89acba4011d03582a1f81feb376915b826020cee.camel@intel.com
> 
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 401148016978..986855e93e71 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -686,18 +686,27 @@ static struct cxl_region_ref *alloc_region_ref(struct cxl_port *port,
>  	return cxl_rr;
>  }
>  
> -static void free_region_ref(struct cxl_region_ref *cxl_rr)
> +static void cxl_rr_free_decoder(struct cxl_region_ref *cxl_rr)
>  {
> -	struct cxl_port *port = cxl_rr->port;
>  	struct cxl_region *cxlr = cxl_rr->region;
>  	struct cxl_decoder *cxld = cxl_rr->decoder;
>  
> +	if (!cxld)
> +		return;
> +
>  	dev_WARN_ONCE(&cxlr->dev, cxld->region != cxlr, "region mismatch\n");
>  	if (cxld->region == cxlr) {
>  		cxld->region = NULL;
>  		put_device(&cxlr->dev);
>  	}
> +}
>  
> +static void free_region_ref(struct cxl_region_ref *cxl_rr)
> +{
> +	struct cxl_port *port = cxl_rr->port;
> +	struct cxl_region *cxlr = cxl_rr->region;
> +
> +	cxl_rr_free_decoder(cxl_rr);
>  	xa_erase(&port->regions, (unsigned long)cxlr);
>  	xa_destroy(&cxl_rr->endpoints);
>  	kfree(cxl_rr);
> @@ -728,6 +737,33 @@ static int cxl_rr_ep_add(struct cxl_region_ref *cxl_rr,
>  	return 0;
>  }
>  
> +static int cxl_rr_alloc_decoder(struct cxl_port *port, struct cxl_region *cxlr,
> +				struct cxl_endpoint_decoder *cxled,
> +				struct cxl_region_ref *cxl_rr)
> +{
> +	struct cxl_decoder *cxld;
> +
> +	if (port == cxled_to_port(cxled))
> +		cxld = &cxled->cxld;
> +	else
> +		cxld = cxl_region_find_decoder(port, cxlr);
> +	if (!cxld) {
> +		dev_dbg(&cxlr->dev, "%s: no decoder available\n",
> +			dev_name(&port->dev));
> +		return -EBUSY;
> +	}
> +
> +	if (cxld->region) {
> +		dev_dbg(&cxlr->dev, "%s: %s already attached to %s\n",
> +			dev_name(&port->dev), dev_name(&cxld->dev),
> +			dev_name(&cxld->region->dev));
> +		return -EBUSY;
> +	}
> +
> +	cxl_rr->decoder = cxld;
> +	return 0;
> +}
> +
>  /**
>   * cxl_port_attach_region() - track a region's interest in a port by endpoint
>   * @port: port to add a new region reference 'struct cxl_region_ref'
> @@ -794,12 +830,6 @@ static int cxl_port_attach_region(struct cxl_port *port,
>  			cxl_rr->nr_targets++;
>  			nr_targets_inc = true;
>  		}
> -
> -		/*
> -		 * The decoder for @cxlr was allocated when the region was first
> -		 * attached to @port.
> -		 */
> -		cxld = cxl_rr->decoder;
>  	} else {
>  		cxl_rr = alloc_region_ref(port, cxlr);
>  		if (IS_ERR(cxl_rr)) {
> @@ -810,26 +840,11 @@ static int cxl_port_attach_region(struct cxl_port *port,
>  		}
>  		nr_targets_inc = true;
>  
> -		if (port == cxled_to_port(cxled))
> -			cxld = &cxled->cxld;
> -		else
> -			cxld = cxl_region_find_decoder(port, cxlr);
> -		if (!cxld) {
> -			dev_dbg(&cxlr->dev, "%s: no decoder available\n",
> -				dev_name(&port->dev));
> +		rc = cxl_rr_alloc_decoder(port, cxlr, cxled, cxl_rr);
> +		if (rc)
>  			goto out_erase;
> -		}
> -
> -		if (cxld->region) {
> -			dev_dbg(&cxlr->dev, "%s: %s already attached to %s\n",
> -				dev_name(&port->dev), dev_name(&cxld->dev),
> -				dev_name(&cxld->region->dev));
> -			rc = -EBUSY;
> -			goto out_erase;
> -		}
> -
> -		cxl_rr->decoder = cxld;
>  	}
> +	cxld = cxl_rr->decoder;
>  
>  	rc = cxl_rr_ep_add(cxl_rr, cxled);
>  	if (rc) {