Re: [PATCH] nvdimm: Replace the usage of a variable by a direct function call in nd_pfn_validate()
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-6beb8492a...@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 >
Re: [PATCH] nvdimm: Replace the usage of a variable by a direct function call in nd_pfn_validate()
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(>dev), 16) != 0)
If parent_uuid is of uuid_t type, you better to replace memcmp() with
uuid_equal().
> return -ENODEV;
--
With Best Regards,
Andy Shevchenko
Re: [PATCH] nvdimm: Replace the usage of a variable by a direct function call in nd_pfn_validate()
>> 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-6beb8492a...@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
Re: [PATCH] nvdimm: Replace the usage of a variable by a direct function call in nd_pfn_validate()
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> ("")' - 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
> ---
> 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(>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(>dev), 16) != 0)
> return -ENODEV;
>
> if (__le16_to_cpu(pfn_sb->version_minor) < 1) {
> --
> 2.40.0
>
[PATCH] nvdimm: Replace the usage of a variable by a direct function call in nd_pfn_validate()
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
---
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(>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(>dev), 16) != 0)
return -ENODEV;
if (__le16_to_cpu(pfn_sb->version_minor) < 1) {
--
2.40.0
