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(&ndns->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(&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 >
[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(&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