diff mbox series

nvdimm: Replace the usage of a variable by a direct function call in nd_pfn_validate()

Message ID d2403b7a-c6cd-4ee9-2a35-86ea57554eec@web.de (mailing list archive)
State New, archived
Headers show
Series nvdimm: Replace the usage of a variable by a direct function call in nd_pfn_validate() | expand

Commit Message

Markus Elfring April 14, 2023, 10:12 a.m. UTC 
Date: Fri, 14 Apr 2023 12:01:15 +0200

The address of a data structure member was determined before
a corresponding null pointer check in the implementation of
the function “nd_pfn_validate”.

Thus avoid the risk for undefined behaviour by replacing the usage of
the local variable “parent_uuid” by a direct function call within
a later condition check.

This issue was detected by using the Coccinelle software.

Fixes: d1c6e08e7503649e4a4f3f9e700e2c05300b6379 ("libnvdimm/labels: Add uuid helpers")
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/nvdimm/pfn_devs.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

--
2.40.0

Comments

Alison Schofield April 14, 2023, 3:22 p.m. UTC | #1 
On Fri, Apr 14, 2023 at 12:12:37PM +0200, Markus Elfring wrote:
> Date: Fri, 14 Apr 2023 12:01:15 +0200
> 
> The address of a data structure member was determined before
> a corresponding null pointer check in the implementation of
> the function “nd_pfn_validate”.
> 
> Thus avoid the risk for undefined behaviour by replacing the usage of
> the local variable “parent_uuid” by a direct function call within
> a later condition check.

Hi Markus,

I think I understand what you are saying above, but I don't follow
how that applies here. This change seems to be a nice simplification,
parent_uuid, is used once, just grab it when needed.

What is the risk of undefined behavior?

> 
> This issue was detected by using the Coccinelle software.
Which cocci script?

> 
> Fixes: d1c6e08e7503649e4a4f3f9e700e2c05300b6379 ("libnvdimm/labels: Add uuid helpers")

This fixes tag seems to be the wrong tag. It is a tag from when the
uuid helpers were introduce, not where parent_uuid was first introduced
and used. I'm not clear this warrants a Fixes tag anyway. Is there
really a bug here? Perhaps I'm missing something in the previous
explanation of risk.

checkpatch is WARNING on the tag format:
WARNING: Please use correct Fixes: style 'Fixes: <12 chars of sha1> ("<title line>")' - ie: 'Fixes: d1c6e08e7503 ("libnvdimm/labels: Add uuid helpers")'
#17:
    Fixes: d1c6e08e7503649e4a4f3f9e700e2c05300b6379 ("libnvdimm/labels: Add uuid helpers")

checkpatch is also WARNING on the commit msg:
WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#5:
    nvdimm: Replace the usage of a variable by a direct function call in nd_pfn_validate()

Also, possible only my pet peeve, the long commit message spoils my
pretty 80 column view. Please trim it to not wrap here:

$git log --oneline pfn_devs.c
52b639e56a46 nvdimm: Replace the usage of a variable by a direct function call in nd_pfn_validate()
c91d71363084 nvdimm: Support sizeof(struct page) > MAX_STRUCT_PAGE_SIZE
6e9f05dc66f9 libnvdimm/pfn_dev: increase MAX_STRUCT_PAGE_SIZE
81beea55cb74 nvdimm: Drop nd_device_lock()
4a0079bc7aae nvdimm: Replace lockdep_mutex with local lock classes
322cbb50de71 block: remove genhd.h

Alison


> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/nvdimm/pfn_devs.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/nvdimm/pfn_devs.c b/drivers/nvdimm/pfn_devs.c
> index af7d9301520c..f14cbfa500ed 100644
> --- a/drivers/nvdimm/pfn_devs.c
> +++ b/drivers/nvdimm/pfn_devs.c
> @@ -456,7 +456,6 @@ int nd_pfn_validate(struct nd_pfn *nd_pfn, const char *sig)
>  	unsigned long align, start_pad;
>  	struct nd_pfn_sb *pfn_sb = nd_pfn->pfn_sb;
>  	struct nd_namespace_common *ndns = nd_pfn->ndns;
> -	const uuid_t *parent_uuid = nd_dev_to_uuid(&ndns->dev);
> 
>  	if (!pfn_sb || !ndns)
>  		return -ENODEV;
> @@ -476,7 +475,7 @@ int nd_pfn_validate(struct nd_pfn *nd_pfn, const char *sig)
>  		return -ENODEV;
>  	pfn_sb->checksum = cpu_to_le64(checksum);
> 
> -	if (memcmp(pfn_sb->parent_uuid, parent_uuid, 16) != 0)
> +	if (memcmp(pfn_sb->parent_uuid, nd_dev_to_uuid(&ndns->dev), 16) != 0)
>  		return -ENODEV;
> 
>  	if (__le16_to_cpu(pfn_sb->version_minor) < 1) {
> --
> 2.40.0
>
Markus Elfring April 14, 2023, 4:50 p.m. UTC | #2 
>> The address of a data structure member was determined before
>> a corresponding null pointer check in the implementation of
>> the function “nd_pfn_validate”.
>>
>> Thus avoid the risk for undefined behaviour by replacing the usage of
>> the local variable “parent_uuid” by a direct function call within
>> a later condition check.
>
> Hi Markus,
>
> I think I understand what you are saying above, but I don't follow
> how that applies here. This change seems to be a nice simplification,
> parent_uuid, is used once, just grab it when needed.

Thanks for your positive feedback.


> What is the risk of undefined behavior?

See also:
https://wiki.sei.cmu.edu/confluence/display/c/EXP34-C.+Do+not+dereference+null+pointers?focusedCommentId=405504137#comment-405504137


>> This issue was detected by using the Coccinelle software.
> Which cocci script?

See also:
Reconsidering pointer dereferences before null pointer checks (with SmPL)
https://lore.kernel.org/cocci/1a11455f-ab57-dce0-1677-6beb8492a257@web.de/
https://sympa.inria.fr/sympa/arc/cocci/2023-04/msg00021.html


How do you think about to review and improve any similarly affected software components?

Regards,
Markus
Andy Shevchenko April 14, 2023, 5:15 p.m. UTC | #3 
On Fri, Apr 14, 2023 at 12:12:37PM +0200, Markus Elfring wrote:
> Date: Fri, 14 Apr 2023 12:01:15 +0200
> 
> The address of a data structure member was determined before
> a corresponding null pointer check in the implementation of
> the function “nd_pfn_validate”.
> 
> Thus avoid the risk for undefined behaviour by replacing the usage of
> the local variable “parent_uuid” by a direct function call within
> a later condition check.

> This issue was detected by using the Coccinelle software.
> 
> Fixes: d1c6e08e7503649e4a4f3f9e700e2c05300b6379 ("libnvdimm/labels: Add uuid helpers")

Same issues as per patch 1.

...

> -	if (memcmp(pfn_sb->parent_uuid, parent_uuid, 16) != 0)
> +	if (memcmp(pfn_sb->parent_uuid, nd_dev_to_uuid(&ndns->dev), 16) != 0)

If parent_uuid is of uuid_t type, you better to replace memcmp() with
uuid_equal().

>  		return -ENODEV;
Alison Schofield April 14, 2023, 7:14 p.m. UTC | #4 
On Fri, Apr 14, 2023 at 06:50:59PM +0200, Markus Elfring wrote:
> >> The address of a data structure member was determined before
> >> a corresponding null pointer check in the implementation of
> >> the function “nd_pfn_validate”.
> >>
> >> Thus avoid the risk for undefined behaviour by replacing the usage of
> >> the local variable “parent_uuid” by a direct function call within
> >> a later condition check.
> >
> > Hi Markus,
> >
> > I think I understand what you are saying above, but I don't follow
> > how that applies here. This change seems to be a nice simplification,
> > parent_uuid, is used once, just grab it when needed.
> 
> Thanks for your positive feedback.

Hi Markus,

FYI - I'm a tiny bit taken aback that in response to me applying, and
providing feedback, on your patch, you respond with 2 links for me to
follow and cut off a chunk of my feedback.

Seems like it would taken the same amount of time to just answer my
two questions directly.

Was this part of a larger patch set? Andy's comment seems to indicate
that. Would have been nice to be CC'd on the cover letter.


More below...

> 
> 
> > What is the risk of undefined behavior?
> 
> See also:
> https://wiki.sei.cmu.edu/confluence/display/c/EXP34-C.+Do+not+dereference+null+pointers?focusedCommentId=405504137#comment-405504137

Where is the NULL pointer dereference here?

> 
> 
> >> This issue was detected by using the Coccinelle software.
> > Which cocci script?
> 
> See also:
> Reconsidering pointer dereferences before null pointer checks (with SmPL)
> https://lore.kernel.org/cocci/1a11455f-ab57-dce0-1677-6beb8492a257@web.de/
> https://sympa.inria.fr/sympa/arc/cocci/2023-04/msg00021.html
> 

The cocci script linked above does not seem to apply here.

> 
> How do you think about to review and improve any similarly affected software components?
> 
> Regards,
> Markus
>
Markus Elfring April 15, 2023, 7:52 a.m. UTC | #5 
> FYI - I'm a tiny bit taken aback that in response to me applying,
> and providing feedback, on your patch,

This will probably trigger collateral evolution, won't it?


> you respond with 2 links for me to follow

I offered another bit of background information according to your enquiry.


> and cut off a chunk of my feedback.

Will this part become relevant for a subsequent patch?


> Seems like it would taken the same amount of time to just answer my
> two questions directly.

Do you find linked information sources also helpful?


> Was this part of a larger patch set?

Not for this software module.

But one of my scripts for the semantic patch language pointed several update candidates out.
Thus I sent 19 patches according to these change possibilities so far.
(Would you become interested to take another look by the means of mailing list archives?)


> Andy's comment seems to indicate that.

Andy Shevchenko was informed because he is involved also in the evolution of other components.


>>> What is the risk of undefined behavior?
>>
>> See also:
>> https://wiki.sei.cmu.edu/confluence/display/c/EXP34-C.+Do+not+dereference+null+pointers?focusedCommentId=405504137#comment-405504137
>
> Where is the NULL pointer dereference here?

I hope that you can become more aware that access attempts for data structure members
(also by using the arrow operator) can occasionally be problematic before null pointer checks.



>>>> This issue was detected by using the Coccinelle software.
>>> Which cocci script?
>>
>> See also:
>> Reconsidering pointer dereferences before null pointer checks (with SmPL)
>> https://lore.kernel.org/cocci/1a11455f-ab57-dce0-1677-6beb8492a257@web.de/
>> https://sympa.inria.fr/sympa/arc/cocci/2023-04/msg00021.html
>
> The cocci script linked above does not seem to apply here.

Which command did you try out?

Do you find the following data processing result reasonable?

Markus_Elfring@Sonne:…/Projekte/Linux/next-analyses> spatch …/Projekte/Coccinelle/janitor/show_pointer_dereferences_before_check7.cocci drivers/nvdimm/pfn_devs.c
…
@@ -456,9 +456,7 @@ int nd_pfn_validate(struct nd_pfn *nd_pf
        unsigned long align, start_pad;
        struct nd_pfn_sb *pfn_sb = nd_pfn->pfn_sb;
        struct nd_namespace_common *ndns = nd_pfn->ndns;
-       const uuid_t *parent_uuid = nd_dev_to_uuid(&ndns->dev);

-       if (!pfn_sb || !ndns)
                return -ENODEV;

        if (!is_memory(nd_pfn->dev.parent))


Regards,
Markus
diff mbox series

Patch

diff --git a/drivers/nvdimm/pfn_devs.c b/drivers/nvdimm/pfn_devs.c
index af7d9301520c..f14cbfa500ed 100644
--- a/drivers/nvdimm/pfn_devs.c
+++ b/drivers/nvdimm/pfn_devs.c
@@ -456,7 +456,6 @@  int nd_pfn_validate(struct nd_pfn *nd_pfn, const char *sig)
 	unsigned long align, start_pad;
 	struct nd_pfn_sb *pfn_sb = nd_pfn->pfn_sb;
 	struct nd_namespace_common *ndns = nd_pfn->ndns;
-	const uuid_t *parent_uuid = nd_dev_to_uuid(&ndns->dev);

 	if (!pfn_sb || !ndns)
 		return -ENODEV;
@@ -476,7 +475,7 @@  int nd_pfn_validate(struct nd_pfn *nd_pfn, const char *sig)
 		return -ENODEV;
 	pfn_sb->checksum = cpu_to_le64(checksum);

-	if (memcmp(pfn_sb->parent_uuid, parent_uuid, 16) != 0)
+	if (memcmp(pfn_sb->parent_uuid, nd_dev_to_uuid(&ndns->dev), 16) != 0)
 		return -ENODEV;

 	if (__le16_to_cpu(pfn_sb->version_minor) < 1) {