[PATCH 1/2] cxl/region: Fix potential invalid pointer dereference

[PATCH 1/2] cxl/region: Fix potential invalid pointer dereference

[Li Zhijian]

construct_region() could return a PTR_ERR() which cannot be derefernced.
Moving the dereference behind the error checking to make sure the
pointer is valid.

Fixes: a32320b71f08 ("cxl/region: Add region autodiscovery")
Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
---
 drivers/cxl/core/region.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 8535718a20f0..3c80aa263a65 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -3086,10 +3086,9 @@ int cxl_add_to_region(struct cxl_port *root, struct cxl_endpoint_decoder *cxled)
 	mutex_lock(&cxlrd->range_lock);
 	region_dev = device_find_child(&cxlrd->cxlsd.cxld.dev, hpa,
 				       match_region_by_range);
-	if (!region_dev) {
+	if (!region_dev)
 		cxlr = construct_region(cxlrd, cxled);
-		region_dev = &cxlr->dev;
-	} else
+	else
 		cxlr = to_cxl_region(region_dev);
 	mutex_unlock(&cxlrd->range_lock);
 
@@ -3097,6 +3096,9 @@ int cxl_add_to_region(struct cxl_port *root, struct cxl_endpoint_decoder *cxled)
 	if (rc)
 		goto out;
 
+	if (!region_dev)
+		region_dev = &cxlr->dev;
+
 	attach_target(cxlr, cxled, -1, TASK_UNINTERRUPTIBLE);
 
 	down_read(&cxl_region_rwsem);
-- 
2.29.2

[PATCH 2/2] cxl/region: Fix missing put_device(region_dev)

[Li Zhijian]

>         mutex_lock(&cxlrd->range_lock);
>         region_dev = device_find_child(&cxlrd->cxlsd.cxld.dev, hpa,
>                                        match_region_by_range);
>         if (!region_dev)
>                 cxlr = construct_region(cxlrd, cxled);
>         else
>                 cxlr = to_cxl_region(region_dev);
>         mutex_unlock(&cxlrd->range_lock);
>
>         rc = PTR_ERR_OR_ZERO(cxlr);
>         if (rc)
>                 goto out;
>
>         if (!region_dev)
>                 region_dev = &cxlr->dev;

When to_cxl_region(region_dev) fails, put_device(region_dev) should be
called to decrease the reference count added by device_find_child().

Simply put_device(region_dev) if region_dev is valid in the error path.

Fixes: a32320b71f08 ("cxl/region: Add region autodiscovery")
Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
---
 drivers/cxl/core/region.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 3c80aa263a65..75390865382f 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -3117,8 +3117,9 @@ int cxl_add_to_region(struct cxl_port *root, struct cxl_endpoint_decoder *cxled)
 				p->res);
 	}
 
-	put_device(region_dev);
 out:
+	if (region_dev)
+		put_device(region_dev);
 	put_device(cxlrd_dev);
 	return rc;
 }
-- 
2.29.2

Re: [PATCH 2/2] cxl/region: Fix missing put_device(region_dev)

[Zhijian Li (Fujitsu)]

On 29/04/2024 09:31, Li Zhijian wrote:
>>          mutex_lock(&cxlrd->range_lock);
>>          region_dev = device_find_child(&cxlrd->cxlsd.cxld.dev, hpa,
>>                                         match_region_by_range);
>>          if (!region_dev)
>>                  cxlr = construct_region(cxlrd, cxled);
>>          else
>>                  cxlr = to_cxl_region(region_dev);
>>          mutex_unlock(&cxlrd->range_lock);
>>
>>          rc = PTR_ERR_OR_ZERO(cxlr);


Think again, PTR_ERR_OR_ZERO() here should be IS_ERR_OR_NULL()?
PTR_ERR_OR_ZERO() returns 0 if cxlr is NULL, but the cxlr will be dereferenced later.



>>          if (rc)
>>                  goto out;
>>
>>          if (!region_dev)
>>                  region_dev = &cxlr->dev;
> 
> When to_cxl_region(region_dev) fails, put_device(region_dev) should be
> called to decrease the reference count added by device_find_child().
> 
> Simply put_device(region_dev) if region_dev is valid in the error path.
> 
> Fixes: a32320b71f08 ("cxl/region: Add region autodiscovery")
> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
> ---
>   drivers/cxl/core/region.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 3c80aa263a65..75390865382f 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -3117,8 +3117,9 @@ int cxl_add_to_region(struct cxl_port *root, struct cxl_endpoint_decoder *cxled)
>   				p->res);
>   	}
>   
> -	put_device(region_dev);
>   out:
> +	if (region_dev)
> +		put_device(region_dev);
>   	put_device(cxlrd_dev);
>   	return rc;
>   }

Re: [PATCH 1/2] cxl/region: Fix potential invalid pointer dereference

[Markus Elfring]

I would usually expect a corresponding cover letter for patch series.


> construct_region() could return a PTR_ERR() which cannot be derefernced.

I hope that a typo will be avoided in the last word of this sentence.


> Moving the dereference behind the error checking to make sure the
> pointer is valid.

Please choose an imperative wording for an improved change description.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.9-rc5#n94


…
> +++ b/drivers/cxl/core/region.c
> @@ -3086,10 +3086,9 @@ int cxl_add_to_region(struct cxl_port *root, struct cxl_endpoint_decoder *cxled)
>  	mutex_lock(&cxlrd->range_lock);
>  	region_dev = device_find_child(&cxlrd->cxlsd.cxld.dev, hpa,
>  				       match_region_by_range);
> -	if (!region_dev) {
> +	if (!region_dev)
>  		cxlr = construct_region(cxlrd, cxled);
> -		region_dev = &cxlr->dev;
> -	} else
> +	else
>  		cxlr = to_cxl_region(region_dev);
>  	mutex_unlock(&cxlrd->range_lock);

I suggest to simplify such source code by using a conditional operator expression.

Regards,
Markus

Re: [PATCH 2/2] cxl/region: Fix missing put_device(region_dev)

[Markus Elfring]

…
> Simply put_device(region_dev) if region_dev is valid in the error path.

Please improve the change description with a corresponding imperative wording.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.9-rc5#n94

Regards,
Markus

Re: [PATCH 2/2] cxl/region: Fix missing put_device(region_dev)

[Zhijian Li (Fujitsu)]

On 29/04/2024 16:00, Markus Elfring wrote:
> …
>> Simply put_device(region_dev) if region_dev is valid in the error path.
> 
> Please improve the change description with a corresponding imperative wording.

Yeah, I always overlook this point. thank you.

Thanks
Zhijian


> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.9-rc5#n94
> 
> Regards,
> Markus

Re: [PATCH 2/2] cxl/region: Fix missing put_device(region_dev)

[Zhijian Li (Fujitsu)]

TBH, even though this patch seems correct, there are still a few
to_cxl_region() callers where they dereference the return value directly
without checking if it's NULL.

So I'm fine to drop this one because to_cxl_region() will trigger a WARN before
returning NULL.

static struct cxl_region *to_cxl_region(struct device *dev)
{
         if (dev_WARN_ONCE(dev, dev->type != &cxl_region_type,
                           "not a cxl_region device\n"))
                 return NULL;
                                                                                 
         return container_of(dev, struct cxl_region, dev);
}


Let me know your thoughts.

Thanks
Zhijian

On 29/04/2024 09:31, Li Zhijian wrote:
>>          mutex_lock(&cxlrd->range_lock);
>>          region_dev = device_find_child(&cxlrd->cxlsd.cxld.dev, hpa,
>>                                         match_region_by_range);
>>          if (!region_dev)
>>                  cxlr = construct_region(cxlrd, cxled);
>>          else
>>                  cxlr = to_cxl_region(region_dev);
>>          mutex_unlock(&cxlrd->range_lock);
>>
>>          rc = PTR_ERR_OR_ZERO(cxlr);
>>          if (rc)
>>                  goto out;
>>
>>          if (!region_dev)
>>                  region_dev = &cxlr->dev;
> 
> When to_cxl_region(region_dev) fails, put_device(region_dev) should be
> called to decrease the reference count added by device_find_child().
> 
> Simply put_device(region_dev) if region_dev is valid in the error path.
> 
> Fixes: a32320b71f08 ("cxl/region: Add region autodiscovery")
> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
> ---
>   drivers/cxl/core/region.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 3c80aa263a65..75390865382f 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -3117,8 +3117,9 @@ int cxl_add_to_region(struct cxl_port *root, struct cxl_endpoint_decoder *cxled)
>   				p->res);
>   	}
>   
> -	put_device(region_dev);
>   out:
> +	if (region_dev)
> +		put_device(region_dev);
>   	put_device(cxlrd_dev);
>   	return rc;
>   }

Re: [PATCH 1/2] cxl/region: Fix potential invalid pointer dereference

[Zhijian Li (Fujitsu)]

On 29/04/2024 15:50, Markus Elfring wrote:
> I would usually expect a corresponding cover letter for patch series.



> 
> 
>> construct_region() could return a PTR_ERR() which cannot be derefernced.
> 
> I hope that a typo will be avoided in the last word of this sentence.

Thanks, hate my fat fingers, I will fix it later.




> 
> 
>> Moving the dereference behind the error checking to make sure the
>> pointer is valid.
> 
> Please choose an imperative wording for an improved change description.
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.9-rc5#n94
> 
> 
> …
>> +++ b/drivers/cxl/core/region.c
>> @@ -3086,10 +3086,9 @@ int cxl_add_to_region(struct cxl_port *root, struct cxl_endpoint_decoder *cxled)
>>   	mutex_lock(&cxlrd->range_lock);
>>   	region_dev = device_find_child(&cxlrd->cxlsd.cxld.dev, hpa,
>>   				       match_region_by_range);
>> -	if (!region_dev) {
>> +	if (!region_dev)
>>   		cxlr = construct_region(cxlrd, cxled);
>> -		region_dev = &cxlr->dev;
>> -	} else
>> +	else
>>   		cxlr = to_cxl_region(region_dev);
>>   	mutex_unlock(&cxlrd->range_lock);
> 
> I suggest to simplify such source code by using a conditional operator expression.

Thanks for your suggestion. Do you mean something like:
cxlr = region_dev ? to_cxl_region(region_dev) : construct_region(cxlrd, cxled);

If so, I'm open to this option, but the kernel does not always obey this convention.
For example,
static inline int __must_check PTR_ERR_OR_ZERO(__force const void *ptr)
{
         if (IS_ERR(ptr))
                 return PTR_ERR(ptr);
         else
                 return 0;
}

Thanks
Zhijian

> 
> Regards,
> Markus

Re: [1/2] cxl/region: Fix potential invalid pointer dereference

[Markus Elfring]

>> I suggest to simplify such source code by using a conditional operator expression.
>
> Thanks for your suggestion. Do you mean something like:
> cxlr = region_dev ? to_cxl_region(region_dev) : construct_region(cxlrd, cxled);

Yes.


> If so, I'm open to this option, but the kernel does not always obey this convention.

Involved developers present different coding style preferences.
I hope that further source code parts can become a bit more succinct.

Regards,
Markus

Re: [PATCH 2/2] cxl/region: Fix missing put_device(region_dev)

[Dan Carpenter]

On Mon, Apr 29, 2024 at 08:26:19AM +0000, Zhijian Li (Fujitsu) wrote:
> 
> 
> On 29/04/2024 16:00, Markus Elfring wrote:
> > …
> >> Simply put_device(region_dev) if region_dev is valid in the error path.
> > 
> > Please improve the change description with a corresponding imperative wording.
> 
> Yeah, I always overlook this point. thank you.
> 

Greg has a bot that responds to Markus's reviews for USB.

https://lore.kernel.org/all/2024042547-shimmy-guileless-c7f2@gregkh/

regards,
dan carpenter

Re: [PATCH 1/2] cxl/region: Fix potential invalid pointer dereference

[Dan Carpenter]

On Mon, Apr 29, 2024 at 09:31:53AM +0800, Li Zhijian wrote:
> construct_region() could return a PTR_ERR() which cannot be derefernced.
> Moving the dereference behind the error checking to make sure the
> pointer is valid.
> 

No, this patch is unnecessary.

drivers/cxl/core/region.c
  3080          /*
  3081           * Ensure that if multiple threads race to construct_region() for @hpa
  3082           * one does the construction and the others add to that.
  3083           */
  3084          mutex_lock(&cxlrd->range_lock);
  3085          region_dev = device_find_child(&cxlrd->cxlsd.cxld.dev, hpa,
  3086                                         match_region_by_range);
  3087          if (!region_dev) {
  3088                  cxlr = construct_region(cxlrd, cxled);
  3089                  region_dev = &cxlr->dev;
                                     ^^^^^^^^^^^
This is not a dereference, it's just pointer math.  In in this case it's
the same as saying:

		region_dev = (void *)cxlr;

  3090          } else
  3091                  cxlr = to_cxl_region(region_dev);
  3092          mutex_unlock(&cxlrd->range_lock);
  3093  
  3094          rc = PTR_ERR_OR_ZERO(cxlr);
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^
This check means that if cxlr is an error pointer then we will clean up
and return an error.

regards,
dan carpenter

  3095          if (rc)
  3096                  goto out;
  3097  
  3098          attach_target(cxlr, cxled, -1, TASK_UNINTERRUPTIBLE);
  3099  
  3100          down_read(&cxl_region_rwsem);
  3101          p = &cxlr->params;
  3102          attach = p->state == CXL_CONFIG_COMMIT;
  3103          up_read(&cxl_region_rwsem);
  3104  
  3105          if (attach) {
  3106                  /*
  3107                   * If device_attach() fails the range may still be active via
  3108                   * the platform-firmware memory map, otherwise the driver for
  3109                   * regions is local to this file, so driver matching can't fail.
  3110                   */
  3111                  if (device_attach(&cxlr->dev) < 0)
  3112                          dev_err(&cxlr->dev, "failed to enable, range: %pr\n",
  3113                                  p->res);
  3114          }
  3115  
  3116          put_device(region_dev);
  3117  out:
  3118          put_device(cxlrd_dev);
  3119          return rc;
  3120  }

Re: [PATCH 2/2] cxl/region: Fix missing put_device(region_dev)

[Zhijian Li (Fujitsu)]

On 29/04/2024 18:00, Dan Carpenter wrote:
> On Mon, Apr 29, 2024 at 08:26:19AM +0000, Zhijian Li (Fujitsu) wrote:
>>
>>
>> On 29/04/2024 16:00, Markus Elfring wrote:
>>> …
>>>> Simply put_device(region_dev) if region_dev is valid in the error path.
>>>
>>> Please improve the change description with a corresponding imperative wording.
>>
>> Yeah, I always overlook this point. thank you.
>>
> 
> Greg has a bot that responds to Markus's reviews for USB.> 
> https://lore.kernel.org/all/2024042547-shimmy-guileless-c7f2@gregkh/


Wow!! Thanks for your reminder.



> 
> regards,
> dan carpenter
> 

Re: [PATCH 2/2] cxl/region: Fix missing put_device(region_dev)

[Dan Carpenter]

On Mon, Apr 29, 2024 at 09:31:54AM +0800, Li Zhijian wrote:
> >         mutex_lock(&cxlrd->range_lock);
> >         region_dev = device_find_child(&cxlrd->cxlsd.cxld.dev, hpa,
> >                                        match_region_by_range);
> >         if (!region_dev)
> >                 cxlr = construct_region(cxlrd, cxled);
> >         else
> >                 cxlr = to_cxl_region(region_dev);
> >         mutex_unlock(&cxlrd->range_lock);
> >
> >         rc = PTR_ERR_OR_ZERO(cxlr);
> >         if (rc)
> >                 goto out;
> >
> >         if (!region_dev)
> >                 region_dev = &cxlr->dev;
> 
> When to_cxl_region(region_dev) fails,
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
to_cxl_region() will return NULL if "region_dev" is not a region device.

  2215  static struct cxl_region *to_cxl_region(struct device *dev)
  2216  {
  2217          if (dev_WARN_ONCE(dev, dev->type != &cxl_region_type,
  2218                            "not a cxl_region device\n"))
  2219                  return NULL;
  2220  
  2221          return container_of(dev, struct cxl_region, dev);
  2222  }

It won't fail.

If it does fail, we're already in bad shape and it's not worth worrying
about resource leaks at that point.

regards,
dan carpenter

Re: [PATCH 1/2] cxl/region: Fix potential invalid pointer dereference

[Zhijian Li (Fujitsu)]

On 29/04/2024 18:10, Dan Carpenter wrote:
> On Mon, Apr 29, 2024 at 09:31:53AM +0800, Li Zhijian wrote:
>> construct_region() could return a PTR_ERR() which cannot be derefernced.
>> Moving the dereference behind the error checking to make sure the
>> pointer is valid.
>>
> 
> No, this patch is unnecessary.

Agree,


> 
> drivers/cxl/core/region.c
>    3080          /*
>    3081           * Ensure that if multiple threads race to construct_region() for @hpa
>    3082           * one does the construction and the others add to that.
>    3083           */
>    3084          mutex_lock(&cxlrd->range_lock);
>    3085          region_dev = device_find_child(&cxlrd->cxlsd.cxld.dev, hpa,
>    3086                                         match_region_by_range);
>    3087          if (!region_dev) {
>    3088                  cxlr = construct_region(cxlrd, cxled);
>    3089                  region_dev = &cxlr->dev;
>                                       ^^^^^^^^^^^
> This is not a dereference, it's just pointer math.  In in this case it's
> the same as saying:
> 
> 		region_dev = (void *)cxlr;


You are right, a equivalent code could be:
		region_dev = ((char *)cxlr) + offsetof(struct cxl_region, dev);


Thanks


> 
>    3090          } else
>    3091                  cxlr = to_cxl_region(region_dev);
>    3092          mutex_unlock(&cxlrd->range_lock);
>    3093
>    3094          rc = PTR_ERR_OR_ZERO(cxlr);
>                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^
> This check means that if cxlr is an error pointer then we will clean up
> and return an error.
> 
> regards,
> dan carpenter
> 
>    3095          if (rc)
>    3096                  goto out;
>    3097
>    3098          attach_target(cxlr, cxled, -1, TASK_UNINTERRUPTIBLE);
>    3099
>    3100          down_read(&cxl_region_rwsem);
>    3101          p = &cxlr->params;
>    3102          attach = p->state == CXL_CONFIG_COMMIT;
>    3103          up_read(&cxl_region_rwsem);
>    3104
>    3105          if (attach) {
>    3106                  /*
>    3107                   * If device_attach() fails the range may still be active via
>    3108                   * the platform-firmware memory map, otherwise the driver for
>    3109                   * regions is local to this file, so driver matching can't fail.
>    3110                   */
>    3111                  if (device_attach(&cxlr->dev) < 0)
>    3112                          dev_err(&cxlr->dev, "failed to enable, range: %pr\n",
>    3113                                  p->res);
>    3114          }
>    3115
>    3116          put_device(region_dev);
>    3117  out:
>    3118          put_device(cxlrd_dev);
>    3119          return rc;
>    3120  }
> 
> 

Re: [PATCH 2/2] cxl/region: Fix missing put_device(region_dev)

[Zhijian Li (Fujitsu)]

On 29/04/2024 18:17, Dan Carpenter wrote:
> On Mon, Apr 29, 2024 at 09:31:54AM +0800, Li Zhijian wrote:
>>>          mutex_lock(&cxlrd->range_lock);
>>>          region_dev = device_find_child(&cxlrd->cxlsd.cxld.dev, hpa,
>>>                                         match_region_by_range);
>>>          if (!region_dev)
>>>                  cxlr = construct_region(cxlrd, cxled);
>>>          else
>>>                  cxlr = to_cxl_region(region_dev);
>>>          mutex_unlock(&cxlrd->range_lock);
>>>
>>>          rc = PTR_ERR_OR_ZERO(cxlr);
>>>          if (rc)
>>>                  goto out;
>>>
>>>          if (!region_dev)
>>>                  region_dev = &cxlr->dev;
>>
>> When to_cxl_region(region_dev) fails,
>    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> to_cxl_region() will return NULL if "region_dev" is not a region device.
> 
>    2215  static struct cxl_region *to_cxl_region(struct device *dev)
>    2216  {
>    2217          if (dev_WARN_ONCE(dev, dev->type != &cxl_region_type,
>    2218                            "not a cxl_region device\n"))
>    2219                  return NULL;
>    2220
>    2221          return container_of(dev, struct cxl_region, dev);
>    2222  }
> 
> It won't fail.
> 
> If it does fail, we're already in bad shape and it's not worth worrying
> about resource leaks at that point.
> 

Sounds good to me,

Thanks
Zhijian


> regards,
> dan carpenter
> 
> 

Re: [PATCH 1/2] cxl/region: Fix potential invalid pointer dereference

[Dan Carpenter]

On Mon, Apr 29, 2024 at 10:25:35AM +0000, Zhijian Li (Fujitsu) wrote:
> >    3084          mutex_lock(&cxlrd->range_lock);
> >    3085          region_dev = device_find_child(&cxlrd->cxlsd.cxld.dev, hpa,
> >    3086                                         match_region_by_range);
> >    3087          if (!region_dev) {
> >    3088                  cxlr = construct_region(cxlrd, cxled);
> >    3089                  region_dev = &cxlr->dev;
> >                                       ^^^^^^^^^^^
> > This is not a dereference, it's just pointer math.  In in this case it's
> > the same as saying:
> > 
> > 		region_dev = (void *)cxlr;
> 
> 
> You are right, a equivalent code could be:
> 		region_dev = ((char *)cxlr) + offsetof(struct cxl_region, dev);
> 
> 

Correct.  But offsetof() is zero.  It's the same math that to_cxl_region()
does.

regards,
dan carpenter

Re: [PATCH 2/2] cxl/region: Fix missing put_device(region_dev)

[Dan Carpenter]

Btw, I assume you're doing some kind of static analysis?  Looking for
missing NULL checks is very tricky and I haven't found a way to do it
well except for looking at failed allocations.  Allocations have to be
checked.

There are so many other functions where we leave off the NULL check
because the caller knows it can't fail.

regards,
dan carpenter

Re: [PATCH 1/2] cxl/region: Fix potential invalid pointer dereference

[Ira Weiny]

Li Zhijian wrote:
> construct_region() could return a PTR_ERR() which cannot be derefernced.
                                                             ^^^^
                                                             dereferenced

> Moving the dereference behind the error checking to make sure the
> pointer is valid.
> 

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

> Fixes: a32320b71f08 ("cxl/region: Add region autodiscovery")
> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
> ---
>  drivers/cxl/core/region.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 8535718a20f0..3c80aa263a65 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -3086,10 +3086,9 @@ int cxl_add_to_region(struct cxl_port *root, struct cxl_endpoint_decoder *cxled)
>  	mutex_lock(&cxlrd->range_lock);
>  	region_dev = device_find_child(&cxlrd->cxlsd.cxld.dev, hpa,
>  				       match_region_by_range);
> -	if (!region_dev) {
> +	if (!region_dev)
>  		cxlr = construct_region(cxlrd, cxled);
> -		region_dev = &cxlr->dev;
> -	} else
> +	else
>  		cxlr = to_cxl_region(region_dev);
>  	mutex_unlock(&cxlrd->range_lock);
>  
> @@ -3097,6 +3096,9 @@ int cxl_add_to_region(struct cxl_port *root, struct cxl_endpoint_decoder *cxled)
>  	if (rc)
>  		goto out;
>  
> +	if (!region_dev)
> +		region_dev = &cxlr->dev;
> +
>  	attach_target(cxlr, cxled, -1, TASK_UNINTERRUPTIBLE);
>  
>  	down_read(&cxl_region_rwsem);
> -- 
> 2.29.2
> 

Re: [PATCH 2/2] cxl/region: Fix missing put_device(region_dev)

[Ira Weiny]

Li Zhijian wrote:
> >         mutex_lock(&cxlrd->range_lock);
> >         region_dev = device_find_child(&cxlrd->cxlsd.cxld.dev, hpa,
> >                                        match_region_by_range);
> >         if (!region_dev)
> >                 cxlr = construct_region(cxlrd, cxled);
> >         else
> >                 cxlr = to_cxl_region(region_dev);
> >         mutex_unlock(&cxlrd->range_lock);
> >
> >         rc = PTR_ERR_OR_ZERO(cxlr);
> >         if (rc)
> >                 goto out;
> >
> >         if (!region_dev)
> >                 region_dev = &cxlr->dev;

This diff hunk in the commit message is very odd.  I would drop it.  We
know this builds on patch 1 where you made the above change.

> 
> When to_cxl_region(region_dev) fails, put_device(region_dev) should be
> called to decrease the reference count added by device_find_child().

I __think__ this is what Dan was pointing out but I'm not sure.

I wanted to point out that to_cxl_region() can't fail because the
device_find_child() is checking that the device is a region device.

Dan, is that what you were saying when you mentioned there were more
serious issues if to_cxl_region() were to fail?

Ira

> 
> Simply put_device(region_dev) if region_dev is valid in the error path.
> 
> Fixes: a32320b71f08 ("cxl/region: Add region autodiscovery")
> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
> ---
>  drivers/cxl/core/region.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 3c80aa263a65..75390865382f 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -3117,8 +3117,9 @@ int cxl_add_to_region(struct cxl_port *root, struct cxl_endpoint_decoder *cxled)
>  				p->res);
>  	}
>  
> -	put_device(region_dev);
>  out:
> +	if (region_dev)
> +		put_device(region_dev);
>  	put_device(cxlrd_dev);
>  	return rc;
>  }
> -- 
> 2.29.2
> 

Re: [PATCH 1/2] cxl/region: Fix potential invalid pointer dereference

[Ira Weiny]

Dan Carpenter wrote:
> On Mon, Apr 29, 2024 at 09:31:53AM +0800, Li Zhijian wrote:
> > construct_region() could return a PTR_ERR() which cannot be derefernced.
> > Moving the dereference behind the error checking to make sure the
> > pointer is valid.
> > 
> 
> No, this patch is unnecessary.
> 
> drivers/cxl/core/region.c
>   3080          /*
>   3081           * Ensure that if multiple threads race to construct_region() for @hpa
>   3082           * one does the construction and the others add to that.
>   3083           */
>   3084          mutex_lock(&cxlrd->range_lock);
>   3085          region_dev = device_find_child(&cxlrd->cxlsd.cxld.dev, hpa,
>   3086                                         match_region_by_range);
>   3087          if (!region_dev) {
>   3088                  cxlr = construct_region(cxlrd, cxled);
>   3089                  region_dev = &cxlr->dev;
>                                      ^^^^^^^^^^^
> This is not a dereference, it's just pointer math.  In in this case it's
> the same as saying:
> 
> 		region_dev = (void *)cxlr;

Ah...  OK I guess we can ignore the change.  Still odd to my eyes though.

Ira

> 
>   3090          } else
>   3091                  cxlr = to_cxl_region(region_dev);
>   3092          mutex_unlock(&cxlrd->range_lock);
>   3093  
>   3094          rc = PTR_ERR_OR_ZERO(cxlr);
>                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^
> This check means that if cxlr is an error pointer then we will clean up
> and return an error.
> 
> regards,
> dan carpenter
> 
>   3095          if (rc)
>   3096                  goto out;
>   3097  
>   3098          attach_target(cxlr, cxled, -1, TASK_UNINTERRUPTIBLE);
>   3099  
>   3100          down_read(&cxl_region_rwsem);
>   3101          p = &cxlr->params;
>   3102          attach = p->state == CXL_CONFIG_COMMIT;
>   3103          up_read(&cxl_region_rwsem);
>   3104  
>   3105          if (attach) {
>   3106                  /*
>   3107                   * If device_attach() fails the range may still be active via
>   3108                   * the platform-firmware memory map, otherwise the driver for
>   3109                   * regions is local to this file, so driver matching can't fail.
>   3110                   */
>   3111                  if (device_attach(&cxlr->dev) < 0)
>   3112                          dev_err(&cxlr->dev, "failed to enable, range: %pr\n",
>   3113                                  p->res);
>   3114          }
>   3115  
>   3116          put_device(region_dev);
>   3117  out:
>   3118          put_device(cxlrd_dev);
>   3119          return rc;
>   3120  }
> 
>