Linux-NVDIMM Archive on lore.kernel.org
 help / color / Atom feed
From: "Verma, Vishal L" <vishal.l.verma@intel.com>
To: "Williams, Dan J" <dan.j.williams@intel.com>,
	"dan.carpenter@oracle.com" <dan.carpenter@oracle.com>,
	"Weiny, Ira" <ira.weiny@intel.com>
Cc: "kernel-janitors@vger.kernel.org"
	<kernel-janitors@vger.kernel.org>,
	"linux-nvdimm@lists.01.org" <linux-nvdimm@lists.01.org>
Subject: Re: [PATCH] libnvdimm/namespace: Fix a signedness bug in __holder_class_store()
Date: Wed, 25 Sep 2019 17:49:08 +0000
Message-ID: <c4a097f38fb4a51c9bca183bf72b356f00826c10.camel@intel.com> (raw)
In-Reply-To: <2807E5FD2F6FDA4886F6618EAC48510E8991FB01@CRSMSX101.amr.corp.intel.com>

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

  reply index

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-09-25 11:00 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 [this message]
2019-09-25 18:11       ` Ira Weiny

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=c4a097f38fb4a51c9bca183bf72b356f00826c10.camel@intel.com \
    --to=vishal.l.verma@intel.com \
    --cc=dan.carpenter@oracle.com \
    --cc=dan.j.williams@intel.com \
    --cc=ira.weiny@intel.com \
    --cc=kernel-janitors@vger.kernel.org \
    --cc=linux-nvdimm@lists.01.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-NVDIMM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-nvdimm/0 linux-nvdimm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-nvdimm linux-nvdimm/ https://lore.kernel.org/linux-nvdimm \
		linux-nvdimm@lists.01.org
	public-inbox-index linux-nvdimm

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.01.lists.linux-nvdimm


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git