* [PATCH] libnvdimm/namespace: Fix a signedness bug in __holder_class_store()
@ 2019-09-25 11:00 Dan Carpenter
2019-09-25 17:24 ` Verma, Vishal L
0 siblings, 1 reply; 5+ messages in thread
From: Dan Carpenter @ 2019-09-25 11:00 UTC (permalink / raw)
To: Dan Williams, Vishal Verma; +Cc: linux-nvdimm, kernel-janitors
The "ndns->claim_class" variable is an enum but in this case GCC will
treat it as an unsigned int so the error handling is never triggered.
Fixes: 14e494542636 ("libnvdimm, btt: BTT updates for UEFI 2.7 format")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
drivers/nvdimm/namespace_devs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/nvdimm/namespace_devs.c b/drivers/nvdimm/namespace_devs.c
index cca0a3ba1d2c..669985527716 100644
--- a/drivers/nvdimm/namespace_devs.c
+++ b/drivers/nvdimm/namespace_devs.c
@@ -1529,7 +1529,7 @@ static ssize_t __holder_class_store(struct device *dev, const char *buf)
return -EINVAL;
/* btt_claim_class() could've returned an error */
- if (ndns->claim_class < 0)
+ if ((int)ndns->claim_class < 0)
return ndns->claim_class;
return 0;
--
2.20.1
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] libnvdimm/namespace: Fix a signedness bug in __holder_class_store()
2019-09-25 11:00 [PATCH] libnvdimm/namespace: Fix a signedness bug in __holder_class_store() Dan Carpenter
@ 2019-09-25 17:24 ` Verma, Vishal L
2019-09-25 17:25 ` Weiny, Ira
0 siblings, 1 reply; 5+ messages in thread
From: Verma, Vishal L @ 2019-09-25 17:24 UTC (permalink / raw)
To: Williams, Dan J, dan.carpenter; +Cc: kernel-janitors, linux-nvdimm
On Wed, 2019-09-25 at 14:00 +0300, Dan Carpenter wrote:
> The "ndns->claim_class" variable is an enum but in this case GCC will
> treat it as an unsigned int so the error handling is never triggered.
>
> Fixes: 14e494542636 ("libnvdimm, btt: BTT updates for UEFI 2.7 format")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> drivers/nvdimm/namespace_devs.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/nvdimm/namespace_devs.c b/drivers/nvdimm/namespace_devs.c
> index cca0a3ba1d2c..669985527716 100644
> --- a/drivers/nvdimm/namespace_devs.c
> +++ b/drivers/nvdimm/namespace_devs.c
> @@ -1529,7 +1529,7 @@ static ssize_t __holder_class_store(struct device *dev, const char *buf)
> return -EINVAL;
>
> /* btt_claim_class() could've returned an error */
> - if (ndns->claim_class < 0)
> + if ((int)ndns->claim_class < 0)
> return ndns->claim_class;
>
> return 0;
Looks straightforward, and a good catch.
Reviewed-by: Vishal Verma <vishal.l.verma@intel.com>
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [PATCH] libnvdimm/namespace: Fix a signedness bug in __holder_class_store()
2019-09-25 17:24 ` Verma, Vishal L
@ 2019-09-25 17:25 ` Weiny, Ira
2019-09-25 17:49 ` Verma, Vishal L
0 siblings, 1 reply; 5+ messages in thread
From: Weiny, Ira @ 2019-09-25 17:25 UTC (permalink / raw)
To: Verma, Vishal L, Williams, Dan J, dan.carpenter
Cc: kernel-janitors, linux-nvdimm
>
> On Wed, 2019-09-25 at 14:00 +0300, Dan Carpenter wrote:
> > The "ndns->claim_class" variable is an enum but in this case GCC will
> > treat it as an unsigned int so the error handling is never triggered.
> >
> > Fixes: 14e494542636 ("libnvdimm, btt: BTT updates for UEFI 2.7
> > format")
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > ---
> > drivers/nvdimm/namespace_devs.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/nvdimm/namespace_devs.c
> > b/drivers/nvdimm/namespace_devs.c index cca0a3ba1d2c..669985527716
> > 100644
> > --- a/drivers/nvdimm/namespace_devs.c
> > +++ b/drivers/nvdimm/namespace_devs.c
> > @@ -1529,7 +1529,7 @@ static ssize_t __holder_class_store(struct device
> *dev, const char *buf)
> > return -EINVAL;
> >
> > /* btt_claim_class() could've returned an error */
> > - if (ndns->claim_class < 0)
> > + if ((int)ndns->claim_class < 0)
> > return ndns->claim_class;
> >
> > return 0;
>
> Looks straightforward, and a good catch.
> Reviewed-by: Vishal Verma <vishal.l.verma@intel.com>
I'm not sure this is really a good fix. This leaves ndns->claim_class set to an invalid value. Is that ok?
Ira
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] libnvdimm/namespace: Fix a signedness bug in __holder_class_store()
2019-09-25 17:25 ` Weiny, Ira
@ 2019-09-25 17:49 ` Verma, Vishal L
2019-09-25 18:11 ` Ira Weiny
0 siblings, 1 reply; 5+ messages in thread
From: Verma, Vishal L @ 2019-09-25 17:49 UTC (permalink / raw)
To: Williams, Dan J, dan.carpenter, Weiny, Ira; +Cc: kernel-janitors, linux-nvdimm
On Wed, 2019-09-25 at 11:25 -0600, Weiny, Ira wrote:
> > On Wed, 2019-09-25 at 14:00 +0300, Dan Carpenter wrote:
> > > The "ndns->claim_class" variable is an enum but in this case GCC will
> > > treat it as an unsigned int so the error handling is never triggered.
> > >
> > > Fixes: 14e494542636 ("libnvdimm, btt: BTT updates for UEFI 2.7
> > > format")
> > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > > ---
> > > drivers/nvdimm/namespace_devs.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/nvdimm/namespace_devs.c
> > > b/drivers/nvdimm/namespace_devs.c index cca0a3ba1d2c..669985527716
> > > 100644
> > > --- a/drivers/nvdimm/namespace_devs.c
> > > +++ b/drivers/nvdimm/namespace_devs.c
> > > @@ -1529,7 +1529,7 @@ static ssize_t __holder_class_store(struct device
> > *dev, const char *buf)
> > > return -EINVAL;
> > >
> > > /* btt_claim_class() could've returned an error */
> > > - if (ndns->claim_class < 0)
> > > + if ((int)ndns->claim_class < 0)
> > > return ndns->claim_class;
> > >
> > > return 0;
> >
> > Looks straightforward, and a good catch.
> > Reviewed-by: Vishal Verma <vishal.l.verma@intel.com>
>
> I'm not sure this is really a good fix. This leaves ndns->claim_class set to an invalid value. Is that ok?
>
Hm, it is just in a store operation for the holder_class sysfs
attribute. if claim_class was negative, that store will just fail.
From the userspace side, this means the 'mode' enforcement API will
fail. Typically, 'ndctl' doesn't require the enforcement to succeed,
since we can disambiguate namespaces even without it, so it doesn't
block namespace creation etc.
On the kernel side, claim_class gets used by btt_devs.c during probe,
and looks like this case, as it currently stands, would fail the BTT
probe (nd_btt_probe()). I think this is what we want?
I guess it can be made a bit clearer by storing NVDIMM_CCLASS_UNKNOWN
explicitly in holder_class_store(), but that can be a separate
improvement from what this patch is addressing.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] libnvdimm/namespace: Fix a signedness bug in __holder_class_store()
2019-09-25 17:49 ` Verma, Vishal L
@ 2019-09-25 18:11 ` Ira Weiny
0 siblings, 0 replies; 5+ messages in thread
From: Ira Weiny @ 2019-09-25 18:11 UTC (permalink / raw)
To: Verma, Vishal L; +Cc: linux-nvdimm, kernel-janitors, dan.carpenter
On Wed, Sep 25, 2019 at 10:49:08AM -0700, 'Vishal Verma' wrote:
> On Wed, 2019-09-25 at 11:25 -0600, Weiny, Ira wrote:
> > > On Wed, 2019-09-25 at 14:00 +0300, Dan Carpenter wrote:
> > > > The "ndns->claim_class" variable is an enum but in this case GCC will
> > > > treat it as an unsigned int so the error handling is never triggered.
> > > >
> > > > Fixes: 14e494542636 ("libnvdimm, btt: BTT updates for UEFI 2.7
> > > > format")
> > > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > > > ---
> > > > drivers/nvdimm/namespace_devs.c | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/nvdimm/namespace_devs.c
> > > > b/drivers/nvdimm/namespace_devs.c index cca0a3ba1d2c..669985527716
> > > > 100644
> > > > --- a/drivers/nvdimm/namespace_devs.c
> > > > +++ b/drivers/nvdimm/namespace_devs.c
> > > > @@ -1529,7 +1529,7 @@ static ssize_t __holder_class_store(struct device
> > > *dev, const char *buf)
> > > > return -EINVAL;
> > > >
> > > > /* btt_claim_class() could've returned an error */
> > > > - if (ndns->claim_class < 0)
> > > > + if ((int)ndns->claim_class < 0)
> > > > return ndns->claim_class;
> > > >
> > > > return 0;
> > >
> > > Looks straightforward, and a good catch.
> > > Reviewed-by: Vishal Verma <vishal.l.verma@intel.com>
> >
> > I'm not sure this is really a good fix. This leaves ndns->claim_class set to an invalid value. Is that ok?
> >
>
> Hm, it is just in a store operation for the holder_class sysfs
> attribute. if claim_class was negative, that store will just fail.
>
> From the userspace side, this means the 'mode' enforcement API will
> fail. Typically, 'ndctl' doesn't require the enforcement to succeed,
> since we can disambiguate namespaces even without it, so it doesn't
> block namespace creation etc.
>
> On the kernel side, claim_class gets used by btt_devs.c during probe,
> and looks like this case, as it currently stands, would fail the BTT
> probe (nd_btt_probe()). I think this is what we want?
>
> I guess it can be made a bit clearer by storing NVDIMM_CCLASS_UNKNOWN
> explicitly in holder_class_store(), but that can be a separate
> improvement from what this patch is addressing.
Fair enough I've sent a follow on patch.
Thanks,
Ira
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-09-25 18:11 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-25 11:00 [PATCH] libnvdimm/namespace: Fix a signedness bug in __holder_class_store() Dan Carpenter
2019-09-25 17:24 ` Verma, Vishal L
2019-09-25 17:25 ` Weiny, Ira
2019-09-25 17:49 ` Verma, Vishal L
2019-09-25 18:11 ` Ira Weiny
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).