[PATCH] cxl/region: don't try to cleanup after cxl_region_setup_targets() fails

[PATCH] cxl/region: don't try to cleanup after cxl_region_setup_targets() fails

[Jim Harris]

Patch 5e42bcbc ("cxl/region: decrement ->nr_targets on error in
cxl_region_attach()") tried to avoid 'eiw' initialization errors when
->nr_targets exceeded 16, by just decrementing ->nr_targets when
cxl_region_setup_targets() failed. Patch 86987c76 ("cxl/region: Cleanup
target list on attach error") extended that cleanup to also clear
cxled->pos and p->targets[pos].

The initialization error was incidentally fixed separately by patch
8d4285425 ("cxl/region: Fix port setup uninitialized variable warnings")
which was merged a few days after 5e42bcbc.

But now the original cleanup when cxl_region_setup_targets() fails
prevents endpoint and switch decoder resources from being reused:

1) the cleanup does not set the decoder's region to NULL, which results
   in future dpa_size_store() calls returning -EBUSY
2) the decoder is not properly freed, which results in future commit
   errors associated with the upstream switch

Now that the initialization errors were fixed separately, the proper
cleanup for this case is to just return immediately. Then the resources
associated with this target get cleanup up as normal when the failed
region is deleted.

Tested by trying to create an invalid region for a 2 switch * 2 endpoint
topology, and then following up with creating a valid region.

Signed-off-by: Jim Harris <jim.harris@samsung.com>
---
 drivers/cxl/core/region.c |    8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 6d63b8798c29..315ca1640e06 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -1750,7 +1750,7 @@ static int cxl_region_attach(struct cxl_region *cxlr,
 	if (p->nr_targets == p->interleave_ways) {
 		rc = cxl_region_setup_targets(cxlr);
 		if (rc)
-			goto err_decrement;
+			return rc;
 		p->state = CXL_CONFIG_ACTIVE;
 	}
 
@@ -1762,12 +1762,6 @@ static int cxl_region_attach(struct cxl_region *cxlr,
 	};
 
 	return 0;
-
-err_decrement:
-	p->nr_targets--;
-	cxled->pos = -1;
-	p->targets[pos] = NULL;
-	return rc;
 }
 
 static int cxl_region_detach(struct cxl_endpoint_decoder *cxled)

Re: [PATCH] cxl/region: don't try to cleanup after cxl_region_setup_targets() fails

[Dan Carpenter]

On Tue, Oct 10, 2023 at 06:38:39PM +0000, Jim Harris wrote:
> Patch 5e42bcbc ("cxl/region: decrement ->nr_targets on error in
> cxl_region_attach()") tried to avoid 'eiw' initialization errors when
> ->nr_targets exceeded 16, by just decrementing ->nr_targets when
> cxl_region_setup_targets() failed.

I mean that's what I wrote but I fairly sure that I was concerned about
->nr_targets getting incremented to an invalid value.

drivers/cxl/core/region.c
  1746          p->targets[pos] = cxled;
                ^^^^^^^^^^^^^^^
This array has CXL_DECODER_MAX_INTERLEAVE (16) elements.

  1747          cxled->pos = pos;
  1748          p->nr_targets++;
  1749  
  1750          if (p->nr_targets == p->interleave_ways) {
                                     ^^^^^^^^^^^^^^^^^^
This is how many we want, but it's capped at 16 so we don't go over.
Like I guess we add one at a time until we hit the max and then when we
get everything added

  1751                  rc = cxl_region_setup_targets(cxlr);

Then we register stuff.

So if we decrement and try to attach another region then my idea was
that it would write over the last element in the array.  But if we don't
have the decrement and we try to attach another region it will go beyond
the end of the array.

  1752                  if (rc)
  1753                          goto err_decrement;
  1754                  p->state = CXL_CONFIG_ACTIVE;
  1755          }
  1756  
  1757          cxled->cxld.interleave_ways = p->interleave_ways;
  1758          cxled->cxld.interleave_granularity = p->interleave_granularity;
  1759          cxled->cxld.hpa_range = (struct range) {
  1760                  .start = p->res->start,
  1761                  .end = p->res->end,
  1762          };
  1763  
  1764          return 0;
  1765  
  1766  err_decrement:
  1767          p->nr_targets--;
  1768          cxled->pos = -1;
  1769          p->targets[pos] = NULL;
  1770          return rc;
  1771  }

But I was just going from static analysis and code review and not
testing and obviously you have tested this.  A simple fix for my
concern would be to do this:

regards,
dan carpenter

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 6d63b8798c29..5948c4a01745 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -1649,6 +1649,11 @@ static int cxl_region_attach(struct cxl_region *cxlr,
 		return -ENODEV;
 	}
 
+	if (p->nr_targets >= p->interleave_ways) {
+		dev_dbg(&cxlr->dev, "%s too many regions\n", dev_name(&cxled->cxld.dev));
+		return -EINVAL;
+	}
+
 	/* all full of members, or interleave config not established? */
 	if (p->state > CXL_CONFIG_INTERLEAVE_ACTIVE) {
 		dev_dbg(&cxlr->dev, "region already active\n");

Re: [PATCH] cxl/region: don't try to cleanup after cxl_region_setup_targets() fails

[Jim Harris]

> On Oct 11, 2023, at 7:04 AM, Dan Carpenter <dan.carpenter@linaro.org> wrote:
> 
> On Tue, Oct 10, 2023 at 06:38:39PM +0000, Jim Harris wrote:
>> Patch 5e42bcbc ("cxl/region: decrement ->nr_targets on error in
>> cxl_region_attach()") tried to avoid 'eiw' initialization errors when
>> ->nr_targets exceeded 16, by just decrementing ->nr_targets when
>> cxl_region_setup_targets() failed.
> 
> I mean that's what I wrote but I fairly sure that I was concerned about
> ->nr_targets getting incremented to an invalid value.
> 
> drivers/cxl/core/region.c
>  1746          p->targets[pos] = cxled;
>                ^^^^^^^^^^^^^^^
> This array has CXL_DECODER_MAX_INTERLEAVE (16) elements.

Agreed, we need to guard against the array overflow too.

> 
> But I was just going from static analysis and code review and not
> testing and obviously you have tested this.  A simple fix for my
> concern would be to do this:
> 
> regards,
> dan carpenter
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 6d63b8798c29..5948c4a01745 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -1649,6 +1649,11 @@ static int cxl_region_attach(struct cxl_region *cxlr,
> return -ENODEV;
> }
> 
> + if (p->nr_targets >= p->interleave_ways) {
> + dev_dbg(&cxlr->dev, "%s too many regions\n", dev_name(&cxled->cxld.dev));
> + return -EINVAL;
> + }
> +
> /* all full of members, or interleave config not established? */
> if (p->state > CXL_CONFIG_INTERLEAVE_ACTIVE) {
> dev_dbg(&cxlr->dev, "region already active\n”);

I’ll push a v2.

I had to convince myself that we didn’t also need a comparison against
CXL_DECODER_MAX_INTERLEAVE. But interleave_ways_store() will fail with
a value > 16 via the ways_to_eiw() call, so the p->interleave_ways check
is sufficient.

[PATCH v2] cxl/region: don't try to cleanup after cxl_region_setup_targets() fails

[Jim Harris]

Patch 5e42bcbc ("cxl/region: decrement ->nr_targets on error in
cxl_region_attach()") tried to avoid 'eiw' initialization errors when
->nr_targets exceeded 16, by just decrementing ->nr_targets when
cxl_region_setup_targets() failed. Patch 86987c76 ("cxl/region: Cleanup
target list on attach error") extended that cleanup to also clear
cxled->pos and p->targets[pos].

The initialization error was incidentally fixed separately by patch
8d4285425 ("cxl/region: Fix port setup uninitialized variable warnings")
which was merged a few days after 5e42bcbc.

But now the original cleanup when cxl_region_setup_targets() fails
prevents endpoint and switch decoder resources from being reused:

1) the cleanup does not set the decoder's region to NULL, which results
   in future dpa_size_store() calls returning -EBUSY
2) the decoder is not properly freed, which results in future commit
   errors associated with the upstream switch

Now that the initialization errors were fixed separately, the proper
cleanup for this case is to just return immediately. Then the resources
associated with this target get cleanup up as normal when the failed
region is deleted.

The ->nr_targets decrement in the error case also helped prevent
a p->targets[] array overflow, so add a new check to prevent against
that overflow.

Tested by trying to create an invalid region for a 2 switch * 2 endpoint
topology, and then following up with creating a valid region.

Signed-off-by: Jim Harris <jim.harris@samsung.com>
---
 drivers/cxl/core/region.c |   14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 6d63b8798c29..2b3b3c62d0a7 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -1658,6 +1658,12 @@ static int cxl_region_attach(struct cxl_region *cxlr,
 		return -ENXIO;
 	}
 
+	if (p->nr_targets >= p->interleave_ways) {
+		dev_dbg(&cxlr->dev, "region already has %d endpoints\n",
+			p->nr_targets);
+		return -EINVAL;
+	}
+
 	ep_port = cxled_to_port(cxled);
 	root_port = cxlrd_to_port(cxlrd);
 	dport = cxl_find_dport_by_dev(root_port, ep_port->host_bridge);
@@ -1750,7 +1756,7 @@ static int cxl_region_attach(struct cxl_region *cxlr,
 	if (p->nr_targets == p->interleave_ways) {
 		rc = cxl_region_setup_targets(cxlr);
 		if (rc)
-			goto err_decrement;
+			return rc;
 		p->state = CXL_CONFIG_ACTIVE;
 	}
 
@@ -1762,12 +1768,6 @@ static int cxl_region_attach(struct cxl_region *cxlr,
 	};
 
 	return 0;
-
-err_decrement:
-	p->nr_targets--;
-	cxled->pos = -1;
-	p->targets[pos] = NULL;
-	return rc;
 }
 
 static int cxl_region_detach(struct cxl_endpoint_decoder *cxled)

Re: [PATCH v2] cxl/region: don't try to cleanup after cxl_region_setup_targets() fails

[Dan Carpenter]

Acked-by: Dan Carpenter <dan.carpenter@linaro.org>

regards,
dan carpenter

Re: [PATCH v2] cxl/region: don't try to cleanup after cxl_region_setup_targets() fails

[Jonathan Cameron]

On Wed, 11 Oct 2023 14:51:31 +0000
Jim Harris <jim.harris@samsung.com> wrote:

> Patch 5e42bcbc ("cxl/region: decrement ->nr_targets on error in
> cxl_region_attach()") tried to avoid 'eiw' initialization errors when
> ->nr_targets exceeded 16, by just decrementing ->nr_targets when  
> cxl_region_setup_targets() failed. Patch 86987c76 ("cxl/region: Cleanup
> target list on attach error") extended that cleanup to also clear
> cxled->pos and p->targets[pos].
> 
> The initialization error was incidentally fixed separately by patch
> 8d4285425 ("cxl/region: Fix port setup uninitialized variable warnings")
> which was merged a few days after 5e42bcbc.
> 
> But now the original cleanup when cxl_region_setup_targets() fails
> prevents endpoint and switch decoder resources from being reused:
> 
> 1) the cleanup does not set the decoder's region to NULL, which results
>    in future dpa_size_store() calls returning -EBUSY
> 2) the decoder is not properly freed, which results in future commit
>    errors associated with the upstream switch
> 
> Now that the initialization errors were fixed separately, the proper
> cleanup for this case is to just return immediately. Then the resources
> associated with this target get cleanup up as normal when the failed
> region is deleted.
> 
> The ->nr_targets decrement in the error case also helped prevent
> a p->targets[] array overflow, so add a new check to prevent against
> that overflow.
> 
> Tested by trying to create an invalid region for a 2 switch * 2 endpoint
> topology, and then following up with creating a valid region.
> 
> Signed-off-by: Jim Harris <jim.harris@samsung.com>

I agree with your analysis and that this seems to fix the cases you've called out.
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>


> ---
>  drivers/cxl/core/region.c |   14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 6d63b8798c29..2b3b3c62d0a7 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -1658,6 +1658,12 @@ static int cxl_region_attach(struct cxl_region *cxlr,
>  		return -ENXIO;
>  	}
>  
> +	if (p->nr_targets >= p->interleave_ways) {
> +		dev_dbg(&cxlr->dev, "region already has %d endpoints\n",
> +			p->nr_targets);
> +		return -EINVAL;
> +	}
> +
>  	ep_port = cxled_to_port(cxled);
>  	root_port = cxlrd_to_port(cxlrd);
>  	dport = cxl_find_dport_by_dev(root_port, ep_port->host_bridge);
> @@ -1750,7 +1756,7 @@ static int cxl_region_attach(struct cxl_region *cxlr,
>  	if (p->nr_targets == p->interleave_ways) {
>  		rc = cxl_region_setup_targets(cxlr);
>  		if (rc)
> -			goto err_decrement;
> +			return rc;
>  		p->state = CXL_CONFIG_ACTIVE;
>  	}
>  
> @@ -1762,12 +1768,6 @@ static int cxl_region_attach(struct cxl_region *cxlr,
>  	};
>  
>  	return 0;
> -
> -err_decrement:
> -	p->nr_targets--;
> -	cxled->pos = -1;
> -	p->targets[pos] = NULL;
> -	return rc;
>  }
>  
>  static int cxl_region_detach(struct cxl_endpoint_decoder *cxled)
> 

Re: [PATCH v2] cxl/region: don't try to cleanup after cxl_region_setup_targets() fails

[Dave Jiang]

On 10/11/23 07:51, Jim Harris wrote:
> Patch 5e42bcbc ("cxl/region: decrement ->nr_targets on error in
> cxl_region_attach()") tried to avoid 'eiw' initialization errors when
> ->nr_targets exceeded 16, by just decrementing ->nr_targets when
> cxl_region_setup_targets() failed. Patch 86987c76 ("cxl/region: Cleanup
> target list on attach error") extended that cleanup to also clear
> cxled->pos and p->targets[pos].
> 
> The initialization error was incidentally fixed separately by patch
> 8d4285425 ("cxl/region: Fix port setup uninitialized variable warnings")
> which was merged a few days after 5e42bcbc.
> 
> But now the original cleanup when cxl_region_setup_targets() fails
> prevents endpoint and switch decoder resources from being reused:
> 
> 1) the cleanup does not set the decoder's region to NULL, which results
>    in future dpa_size_store() calls returning -EBUSY
> 2) the decoder is not properly freed, which results in future commit
>    errors associated with the upstream switch
> 
> Now that the initialization errors were fixed separately, the proper
> cleanup for this case is to just return immediately. Then the resources
> associated with this target get cleanup up as normal when the failed
> region is deleted.
> 
> The ->nr_targets decrement in the error case also helped prevent
> a p->targets[] array overflow, so add a new check to prevent against
> that overflow.
> 
> Tested by trying to create an invalid region for a 2 switch * 2 endpoint
> topology, and then following up with creating a valid region.
> 
> Signed-off-by: Jim Harris <jim.harris@samsung.com>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  drivers/cxl/core/region.c |   14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 6d63b8798c29..2b3b3c62d0a7 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -1658,6 +1658,12 @@ static int cxl_region_attach(struct cxl_region *cxlr,
>  		return -ENXIO;
>  	}
>  
> +	if (p->nr_targets >= p->interleave_ways) {
> +		dev_dbg(&cxlr->dev, "region already has %d endpoints\n",
> +			p->nr_targets);
> +		return -EINVAL;
> +	}
> +
>  	ep_port = cxled_to_port(cxled);
>  	root_port = cxlrd_to_port(cxlrd);
>  	dport = cxl_find_dport_by_dev(root_port, ep_port->host_bridge);
> @@ -1750,7 +1756,7 @@ static int cxl_region_attach(struct cxl_region *cxlr,
>  	if (p->nr_targets == p->interleave_ways) {
>  		rc = cxl_region_setup_targets(cxlr);
>  		if (rc)
> -			goto err_decrement;
> +			return rc;
>  		p->state = CXL_CONFIG_ACTIVE;
>  	}
>  
> @@ -1762,12 +1768,6 @@ static int cxl_region_attach(struct cxl_region *cxlr,
>  	};
>  
>  	return 0;
> -
> -err_decrement:
> -	p->nr_targets--;
> -	cxled->pos = -1;
> -	p->targets[pos] = NULL;
> -	return rc;
>  }
>  
>  static int cxl_region_detach(struct cxl_endpoint_decoder *cxled)
> 

RE: [PATCH v2] cxl/region: don't try to cleanup after cxl_region_setup_targets() fails

[Dan Williams]

Jim Harris wrote:
> Patch 5e42bcbc ("cxl/region: decrement ->nr_targets on error in
> cxl_region_attach()") tried to avoid 'eiw' initialization errors when
> ->nr_targets exceeded 16, by just decrementing ->nr_targets when
> cxl_region_setup_targets() failed. Patch 86987c76 ("cxl/region: Cleanup
> target list on attach error") extended that cleanup to also clear
> cxled->pos and p->targets[pos].
> 
> The initialization error was incidentally fixed separately by patch
> 8d4285425 ("cxl/region: Fix port setup uninitialized variable warnings")
> which was merged a few days after 5e42bcbc.

Patch looks good, but I did reflow the above paragraphs to have commit
references per checkpatch expectations. I believe it did not flag them
for you as it did not recognize "Patch <SHA>" as referring to a commit:

    Commit 5e42bcbc3fef ("cxl/region: decrement ->nr_targets on error in
    cxl_region_attach()") tried to avoid 'eiw' initialization errors when
    ->nr_targets exceeded 16, by just decrementing ->nr_targets when
    cxl_region_setup_targets() failed.
    
    Commit 86987c766276 ("cxl/region: Cleanup target list on attach error")
    extended that cleanup to also clear cxled->pos and p->targets[pos]. The
    initialization error was incidentally fixed separately by: 
    Commit 8d4285425714 ("cxl/region: Fix port setup uninitialized variable
    warnings") which was merged a few days after 5e42bcbc3fef.

I also went ahead and added:

    Fixes: 5e42bcbc3fef ("cxl/region: decrement ->nr_targets on error in cxl_region_attach()")
    Cc: <stable@vger.kernel.org>

Otherwise, good find, thanks Jim!

Re: [PATCH v2] cxl/region: don't try to cleanup after cxl_region_setup_targets() fails

[Jim Harris]

On Tue, Oct 24, 2023 at 04:01:19PM -0700, Dan Williams wrote:
> 
> Patch looks good, but I did reflow the above paragraphs to have commit
> references per checkpatch expectations. I believe it did not flag them
> for you as it did not recognize "Patch <SHA>" as referring to a commit:
> 
>     Commit 5e42bcbc3fef ("cxl/region: decrement ->nr_targets on error in
>     cxl_region_attach()") tried to avoid 'eiw' initialization errors when
>     ->nr_targets exceeded 16, by just decrementing ->nr_targets when
>     cxl_region_setup_targets() failed.
>     
>     Commit 86987c766276 ("cxl/region: Cleanup target list on attach error")
>     extended that cleanup to also clear cxled->pos and p->targets[pos]. The
>     initialization error was incidentally fixed separately by: 
>     Commit 8d4285425714 ("cxl/region: Fix port setup uninitialized variable
>     warnings") which was merged a few days after 5e42bcbc3fef.
> 
> I also went ahead and added:
> 
>     Fixes: 5e42bcbc3fef ("cxl/region: decrement ->nr_targets on error in cxl_region_attach()")
>     Cc: <stable@vger.kernel.org>
> 
Thanks Dan, I'll keep an eye out for these in the future.