[PATCH] cxl/region: Fix region reference target accounting

[PATCH] cxl/region: Fix region reference target accounting

[Dan Williams]

Dan reports:

    The error handling in cxl_port_attach_region() looks like it might
    have a similar bug.  The cxl_rr->nr_targets++; might want a --.

    That function is more complicated.

Indeed cxl_rr->nr_targets leaks when cxl_rr_ep_add() fails, but that
flow is not clear. Fix the bug and the clarity by separating the 'new'
region-reference case from the 'extend' region-reference case. This also
moves the host-physical-address (HPA) validation, that the HPA of a new
region being accounted to the port is greater than the HPA of all other
regions associated with the port, to alloc_region_ref().

Introduce @nr_targets_inc to track when the error exit path needs to
clean up cxl_rr->nr_targets.

Fixes: 384e624bb211 ("cxl/region: Attach endpoint decoders")
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/core/region.c |   71 +++++++++++++++++++++++++++------------------
 1 file changed, 43 insertions(+), 28 deletions(-)

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index aff4f736b63c..cf5d5811fe4c 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -709,12 +709,26 @@ static struct cxl_decoder *cxl_region_find_decoder(struct cxl_port *port,
 static struct cxl_region_ref *alloc_region_ref(struct cxl_port *port,
 					       struct cxl_region *cxlr)
 {
-	struct cxl_region_ref *cxl_rr;
+	struct cxl_region_params *p = &cxlr->params;
+	struct cxl_region_ref *cxl_rr, *iter;
+	unsigned long index;
 	int rc;
 
+	xa_for_each(&port->regions, index, iter) {
+		struct cxl_region_params *ip = &iter->region->params;
+
+		if (ip->res->start > p->res->start) {
+			dev_dbg(&cxlr->dev,
+				"%s: HPA order violation %s:%pr vs %pr\n",
+				dev_name(&port->dev),
+				dev_name(&iter->region->dev), ip->res, p->res);
+			return ERR_PTR(-EBUSY);
+		}
+	}
+
 	cxl_rr = kzalloc(sizeof(*cxl_rr), GFP_KERNEL);
 	if (!cxl_rr)
-		return NULL;
+		return ERR_PTR(-ENOMEM);
 	cxl_rr->port = port;
 	cxl_rr->region = cxlr;
 	cxl_rr->nr_targets = 1;
@@ -726,7 +740,7 @@ static struct cxl_region_ref *alloc_region_ref(struct cxl_port *port,
 			"%s: failed to track region reference: %d\n",
 			dev_name(&port->dev), rc);
 		kfree(cxl_rr);
-		return NULL;
+		return ERR_PTR(rc);
 	}
 
 	return cxl_rr;
@@ -801,33 +815,25 @@ static int cxl_port_attach_region(struct cxl_port *port,
 {
 	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
 	struct cxl_ep *ep = cxl_ep_load(port, cxlmd);
-	struct cxl_region_ref *cxl_rr = NULL, *iter;
-	struct cxl_region_params *p = &cxlr->params;
-	struct cxl_decoder *cxld = NULL;
+	struct cxl_region_ref *cxl_rr;
+	bool nr_targets_inc = false;
+	struct cxl_decoder *cxld;
 	unsigned long index;
 	int rc = -EBUSY;
 
 	lockdep_assert_held_write(&cxl_region_rwsem);
 
-	xa_for_each(&port->regions, index, iter) {
-		struct cxl_region_params *ip = &iter->region->params;
-
-		if (iter->region == cxlr)
-			cxl_rr = iter;
-		if (ip->res->start > p->res->start) {
-			dev_dbg(&cxlr->dev,
-				"%s: HPA order violation %s:%pr vs %pr\n",
-				dev_name(&port->dev),
-				dev_name(&iter->region->dev), ip->res, p->res);
-			return -EBUSY;
-		}
-	}
-
+	cxl_rr = cxl_rr_load(port, cxlr);
 	if (cxl_rr) {
 		struct cxl_ep *ep_iter;
 		int found = 0;
 
-		cxld = cxl_rr->decoder;
+		/*
+		 * Walk the existing endpoints that have been attached to
+		 * @cxlr at @port and see if they share the same 'next' port
+		 * in the downstream direction. I.e. endpoints that share common
+		 * upstream switch.
+		 */
 		xa_for_each(&cxl_rr->endpoints, index, ep_iter) {
 			if (ep_iter == ep)
 				continue;
@@ -838,22 +844,29 @@ static int cxl_port_attach_region(struct cxl_port *port,
 		}
 
 		/*
-		 * If this is a new target or if this port is direct connected
-		 * to this endpoint then add to the target count.
+		 * New target port, or @port is an endpoint port that always
+		 * accounts its own local decode as a target.
 		 */
-		if (!found || !ep->next)
+		if (!found || !ep->next) {
 			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 (!cxl_rr) {
+		if (IS_ERR(cxl_rr)) {
 			dev_dbg(&cxlr->dev,
 				"%s: failed to allocate region reference\n",
 				dev_name(&port->dev));
-			return -ENOMEM;
+			return PTR_ERR(cxl_rr);
 		}
-	}
+		nr_targets_inc = true;
 
-	if (!cxld) {
 		if (port == cxled_to_port(cxled))
 			cxld = &cxled->cxld;
 		else
@@ -896,6 +909,8 @@ static int cxl_port_attach_region(struct cxl_port *port,
 
 	return 0;
 out_erase:
+	if (nr_targets_inc)
+		cxl_rr->nr_targets--;
 	if (cxl_rr->nr_eps == 0)
 		free_region_ref(cxl_rr);
 	return rc;

Re: [PATCH] cxl/region: Fix region reference target accounting

[Ira Weiny]

On Mon, Aug 01, 2022 at 04:00:21PM -0700, Dan Williams wrote:
> Dan reports:
> 
>     The error handling in cxl_port_attach_region() looks like it might
>     have a similar bug.  The cxl_rr->nr_targets++; might want a --.
> 
>     That function is more complicated.
> 
> Indeed cxl_rr->nr_targets leaks when cxl_rr_ep_add() fails, but that
> flow is not clear. Fix the bug and the clarity by separating the 'new'
                                         ^^^^^^^
                                         clarify?

> region-reference case from the 'extend' region-reference case. This also
> moves the host-physical-address (HPA) validation, that the HPA of a new
> region being accounted to the port is greater than the HPA of all other
> regions associated with the port, to alloc_region_ref().

Technically this means the kdoc 'steps list' is out of order a bit.  :-(

But I like that this is only done on creation.  And I'm not sure it matter much
because 'steps' is more of a list of things this function does.

> 
> Introduce @nr_targets_inc to track when the error exit path needs to
> clean up cxl_rr->nr_targets.
> 
> Fixes: 384e624bb211 ("cxl/region: Attach endpoint decoders")
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  drivers/cxl/core/region.c |   71 +++++++++++++++++++++++++++------------------
>  1 file changed, 43 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index aff4f736b63c..cf5d5811fe4c 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -709,12 +709,26 @@ static struct cxl_decoder *cxl_region_find_decoder(struct cxl_port *port,
>  static struct cxl_region_ref *alloc_region_ref(struct cxl_port *port,
>  					       struct cxl_region *cxlr)
>  {
> -	struct cxl_region_ref *cxl_rr;
> +	struct cxl_region_params *p = &cxlr->params;
> +	struct cxl_region_ref *cxl_rr, *iter;
> +	unsigned long index;
>  	int rc;
>  
> +	xa_for_each(&port->regions, index, iter) {
> +		struct cxl_region_params *ip = &iter->region->params;
> +
> +		if (ip->res->start > p->res->start) {
> +			dev_dbg(&cxlr->dev,
> +				"%s: HPA order violation %s:%pr vs %pr\n",
> +				dev_name(&port->dev),
> +				dev_name(&iter->region->dev), ip->res, p->res);
> +			return ERR_PTR(-EBUSY);
> +		}
> +	}
> +
>  	cxl_rr = kzalloc(sizeof(*cxl_rr), GFP_KERNEL);
>  	if (!cxl_rr)
> -		return NULL;
> +		return ERR_PTR(-ENOMEM);
>  	cxl_rr->port = port;
>  	cxl_rr->region = cxlr;
>  	cxl_rr->nr_targets = 1;
> @@ -726,7 +740,7 @@ static struct cxl_region_ref *alloc_region_ref(struct cxl_port *port,
>  			"%s: failed to track region reference: %d\n",
>  			dev_name(&port->dev), rc);
>  		kfree(cxl_rr);
> -		return NULL;
> +		return ERR_PTR(rc);
>  	}
>  
>  	return cxl_rr;
> @@ -801,33 +815,25 @@ static int cxl_port_attach_region(struct cxl_port *port,
>  {
>  	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
>  	struct cxl_ep *ep = cxl_ep_load(port, cxlmd);
> -	struct cxl_region_ref *cxl_rr = NULL, *iter;
> -	struct cxl_region_params *p = &cxlr->params;
> -	struct cxl_decoder *cxld = NULL;
> +	struct cxl_region_ref *cxl_rr;
> +	bool nr_targets_inc = false;
> +	struct cxl_decoder *cxld;
>  	unsigned long index;
>  	int rc = -EBUSY;
>  
>  	lockdep_assert_held_write(&cxl_region_rwsem);
>  
> -	xa_for_each(&port->regions, index, iter) {
> -		struct cxl_region_params *ip = &iter->region->params;
> -
> -		if (iter->region == cxlr)
> -			cxl_rr = iter;
> -		if (ip->res->start > p->res->start) {
> -			dev_dbg(&cxlr->dev,
> -				"%s: HPA order violation %s:%pr vs %pr\n",
> -				dev_name(&port->dev),
> -				dev_name(&iter->region->dev), ip->res, p->res);
> -			return -EBUSY;
> -		}
> -	}
> -
> +	cxl_rr = cxl_rr_load(port, cxlr);
>  	if (cxl_rr) {
>  		struct cxl_ep *ep_iter;
>  		int found = 0;
>  
> -		cxld = cxl_rr->decoder;
> +		/*
> +		 * Walk the existing endpoints that have been attached to
> +		 * @cxlr at @port and see if they share the same 'next' port
> +		 * in the downstream direction. I.e. endpoints that share common
> +		 * upstream switch.
> +		 */
>  		xa_for_each(&cxl_rr->endpoints, index, ep_iter) {
>  			if (ep_iter == ep)
>  				continue;
> @@ -838,22 +844,29 @@ static int cxl_port_attach_region(struct cxl_port *port,
>  		}
>  
>  		/*
> -		 * If this is a new target or if this port is direct connected
> -		 * to this endpoint then add to the target count.
> +		 * New target port, or @port is an endpoint port that always
> +		 * accounts its own local decode as a target.
>  		 */
> -		if (!found || !ep->next)
> +		if (!found || !ep->next) {
>  			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 (!cxl_rr) {
> +		if (IS_ERR(cxl_rr)) {
>  			dev_dbg(&cxlr->dev,
>  				"%s: failed to allocate region reference\n",
>  				dev_name(&port->dev));
> -			return -ENOMEM;
> +			return PTR_ERR(cxl_rr);
>  		}
> -	}
> +		nr_targets_inc = true;

I don't think this is technically needed but I would leave it if I were you.

I think the leak only occurs on the extend case but I think it is safe to do
this and does make the code cleaner.

Regardless of the kdoc subtlety and with the spelling fix.

Reviewed-by: Ira Weiny <ira.weiny@intel.com>

>  
> -	if (!cxld) {
>  		if (port == cxled_to_port(cxled))
>  			cxld = &cxled->cxld;
>  		else
> @@ -896,6 +909,8 @@ static int cxl_port_attach_region(struct cxl_port *port,
>  
>  	return 0;
>  out_erase:
> +	if (nr_targets_inc)
> +		cxl_rr->nr_targets--;
>  	if (cxl_rr->nr_eps == 0)
>  		free_region_ref(cxl_rr);
>  	return rc;
> 

Re: [PATCH] cxl/region: Fix region reference target accounting

[Dan Williams]

Ira Weiny wrote:
> On Mon, Aug 01, 2022 at 04:00:21PM -0700, Dan Williams wrote:
> > Dan reports:
> > 
> >     The error handling in cxl_port_attach_region() looks like it might
> >     have a similar bug.  The cxl_rr->nr_targets++; might want a --.
> > 
> >     That function is more complicated.
> > 
> > Indeed cxl_rr->nr_targets leaks when cxl_rr_ep_add() fails, but that
> > flow is not clear. Fix the bug and the clarity by separating the 'new'
>                                          ^^^^^^^
>                                          clarify?

It's a code 'clarity' fix.

> 
> > region-reference case from the 'extend' region-reference case. This also
> > moves the host-physical-address (HPA) validation, that the HPA of a new
> > region being accounted to the port is greater than the HPA of all other
> > regions associated with the port, to alloc_region_ref().
> 
> Technically this means the kdoc 'steps list' is out of order a bit.  :-(
> 
> But I like that this is only done on creation.  And I'm not sure it matter much
> because 'steps' is more of a list of things this function does.

Yeah, the steps are not compiled.

> 
> > 
> > Introduce @nr_targets_inc to track when the error exit path needs to
> > clean up cxl_rr->nr_targets.
> > 
> > Fixes: 384e624bb211 ("cxl/region: Attach endpoint decoders")
> > Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> > ---
> >  drivers/cxl/core/region.c |   71 +++++++++++++++++++++++++++------------------
> >  1 file changed, 43 insertions(+), 28 deletions(-)
> > 
> > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> > index aff4f736b63c..cf5d5811fe4c 100644
> > --- a/drivers/cxl/core/region.c
> > +++ b/drivers/cxl/core/region.c
> > @@ -709,12 +709,26 @@ static struct cxl_decoder *cxl_region_find_decoder(struct cxl_port *port,
> >  static struct cxl_region_ref *alloc_region_ref(struct cxl_port *port,
> >  					       struct cxl_region *cxlr)
> >  {
> > -	struct cxl_region_ref *cxl_rr;
> > +	struct cxl_region_params *p = &cxlr->params;
> > +	struct cxl_region_ref *cxl_rr, *iter;
> > +	unsigned long index;
> >  	int rc;
> >  
> > +	xa_for_each(&port->regions, index, iter) {
> > +		struct cxl_region_params *ip = &iter->region->params;
> > +
> > +		if (ip->res->start > p->res->start) {
> > +			dev_dbg(&cxlr->dev,
> > +				"%s: HPA order violation %s:%pr vs %pr\n",
> > +				dev_name(&port->dev),
> > +				dev_name(&iter->region->dev), ip->res, p->res);
> > +			return ERR_PTR(-EBUSY);
> > +		}
> > +	}
> > +
> >  	cxl_rr = kzalloc(sizeof(*cxl_rr), GFP_KERNEL);
> >  	if (!cxl_rr)
> > -		return NULL;
> > +		return ERR_PTR(-ENOMEM);
> >  	cxl_rr->port = port;
> >  	cxl_rr->region = cxlr;
> >  	cxl_rr->nr_targets = 1;
> > @@ -726,7 +740,7 @@ static struct cxl_region_ref *alloc_region_ref(struct cxl_port *port,
> >  			"%s: failed to track region reference: %d\n",
> >  			dev_name(&port->dev), rc);
> >  		kfree(cxl_rr);
> > -		return NULL;
> > +		return ERR_PTR(rc);
> >  	}
> >  
> >  	return cxl_rr;
> > @@ -801,33 +815,25 @@ static int cxl_port_attach_region(struct cxl_port *port,
> >  {
> >  	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
> >  	struct cxl_ep *ep = cxl_ep_load(port, cxlmd);
> > -	struct cxl_region_ref *cxl_rr = NULL, *iter;
> > -	struct cxl_region_params *p = &cxlr->params;
> > -	struct cxl_decoder *cxld = NULL;
> > +	struct cxl_region_ref *cxl_rr;
> > +	bool nr_targets_inc = false;
> > +	struct cxl_decoder *cxld;
> >  	unsigned long index;
> >  	int rc = -EBUSY;
> >  
> >  	lockdep_assert_held_write(&cxl_region_rwsem);
> >  
> > -	xa_for_each(&port->regions, index, iter) {
> > -		struct cxl_region_params *ip = &iter->region->params;
> > -
> > -		if (iter->region == cxlr)
> > -			cxl_rr = iter;
> > -		if (ip->res->start > p->res->start) {
> > -			dev_dbg(&cxlr->dev,
> > -				"%s: HPA order violation %s:%pr vs %pr\n",
> > -				dev_name(&port->dev),
> > -				dev_name(&iter->region->dev), ip->res, p->res);
> > -			return -EBUSY;
> > -		}
> > -	}
> > -
> > +	cxl_rr = cxl_rr_load(port, cxlr);
> >  	if (cxl_rr) {
> >  		struct cxl_ep *ep_iter;
> >  		int found = 0;
> >  
> > -		cxld = cxl_rr->decoder;
> > +		/*
> > +		 * Walk the existing endpoints that have been attached to
> > +		 * @cxlr at @port and see if they share the same 'next' port
> > +		 * in the downstream direction. I.e. endpoints that share common
> > +		 * upstream switch.
> > +		 */
> >  		xa_for_each(&cxl_rr->endpoints, index, ep_iter) {
> >  			if (ep_iter == ep)
> >  				continue;
> > @@ -838,22 +844,29 @@ static int cxl_port_attach_region(struct cxl_port *port,
> >  		}
> >  
> >  		/*
> > -		 * If this is a new target or if this port is direct connected
> > -		 * to this endpoint then add to the target count.
> > +		 * New target port, or @port is an endpoint port that always
> > +		 * accounts its own local decode as a target.
> >  		 */
> > -		if (!found || !ep->next)
> > +		if (!found || !ep->next) {
> >  			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 (!cxl_rr) {
> > +		if (IS_ERR(cxl_rr)) {
> >  			dev_dbg(&cxlr->dev,
> >  				"%s: failed to allocate region reference\n",
> >  				dev_name(&port->dev));
> > -			return -ENOMEM;
> > +			return PTR_ERR(cxl_rr);
> >  		}
> > -	}
> > +		nr_targets_inc = true;
> 
> I don't think this is technically needed but I would leave it if I were you.
> 
> I think the leak only occurs on the extend case but I think it is safe to do
> this and does make the code cleaner.

Appreciate the detailed look. You're right that it's ultimately
pointless to do the tracking in this case because the cxl_rr instance
will be freed shortly. For the casual reader though it saves them a trip
to go read the details to convice themselves there's no leak.

> Regardless of the kdoc subtlety and with the spelling fix.
> 
> Reviewed-by: Ira Weiny <ira.weiny@intel.com>

Thanks!