diff mbox series

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

Message ID 20240429013154.368118-2-lizhijian@fujitsu.com
State New, archived
Headers show
Series [1/2] cxl/region: Fix potential invalid pointer dereference | expand

Commit Message

Zhijian Li (Fujitsu) April 29, 2024, 1:31 a.m. UTC
>         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(-)

Comments

Zhijian Li (Fujitsu) April 29, 2024, 1:51 a.m. UTC | #1
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;
>   }
Markus Elfring April 29, 2024, 8 a.m. UTC | #2
> 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
Zhijian Li (Fujitsu) April 29, 2024, 8:26 a.m. UTC | #3
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
Zhijian Li (Fujitsu) April 29, 2024, 8:35 a.m. UTC | #4
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;
>   }
Dan Carpenter April 29, 2024, 10 a.m. UTC | #5
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
Zhijian Li (Fujitsu) April 29, 2024, 10:11 a.m. UTC | #6
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
>
Dan Carpenter April 29, 2024, 10:17 a.m. UTC | #7
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
Zhijian Li (Fujitsu) April 29, 2024, 10:26 a.m. UTC | #8
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
> 
>
Dan Carpenter April 29, 2024, 10:32 a.m. UTC | #9
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
Ira Weiny April 29, 2024, 4:14 p.m. UTC | #10
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
>
diff mbox series

Patch

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;
 }