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 <[email protected]>
---
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
> 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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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;
> }
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
…
> 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
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
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 <[email protected]>
> ---
> 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;
> }
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
>> 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
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
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 }
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
>
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
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 }
>
>
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
>
>
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
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
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 <[email protected]>
> Fixes: a32320b71f08 ("cxl/region: Add region autodiscovery")
> Signed-off-by: Li Zhijian <[email protected]>
> ---
> 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
>
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 <[email protected]>
> ---
> 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
>
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 }
>
>