[PATCH 0/3] nvme: small fixes reported by smatch

[PATCH 0/3] nvme: small fixes reported by smatch

[Javier González]

Fix a number of small things reported by smatch on the nvme driver

Javier González (3):
  nvme: do not check for ns on rw path
  nvme: compare NQN string with right size
  nvme: fix eui_show() print format

 drivers/nvme/host/core.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

-- 
2.7.4

[PATCH 1/3] nvme: do not check for ns on rw path

[Javier González]

On the rw path, the ns is assumed to be set. However, a check is still
done, inherited from the time the code resided at nvme_queue_rq().

Eliminate this check, which also eliminates a smatch complain for not
doing proper NULL checks on ns.

Signed-off-by: Javier González <javier@cnexlabs.com>
---
 drivers/nvme/host/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 5a14cc7f28ee..bd1d5ff911c9 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -472,7 +472,7 @@ static inline blk_status_t nvme_setup_rw(struct nvme_ns *ns,
 	 * unless this namespace is formated such that the metadata can be
 	 * stripped/generated by the controller with PRACT=1.
 	 */
-	if (ns && ns->ms &&
+	if (ns->ms &&
 	    (!ns->pi_type || ns->ms != sizeof(struct t10_pi_tuple)) &&
 	    !blk_integrity_rq(req) && !blk_rq_is_passthrough(req))
 		return BLK_STS_NOTSUPP;
-- 
2.7.4

[PATCH 2/3] nvme: compare NQN string with right size

[Javier González]

Compare subnqns using NVMF_NQN_SIZE as it is < 256

Signed-off-by: Javier González <javier@cnexlabs.com>
---
 drivers/nvme/host/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index bd1d5ff911c9..ae8ab0a1ef0d 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1743,7 +1743,7 @@ static void nvme_init_subnqn(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
 
 	nqnlen = strnlen(id->subnqn, NVMF_NQN_SIZE);
 	if (nqnlen > 0 && nqnlen < NVMF_NQN_SIZE) {
-		strcpy(ctrl->subnqn, id->subnqn);
+		strncpy(ctrl->subnqn, id->subnqn, NVMF_NQN_SIZE);
 		return;
 	}
 
-- 
2.7.4

[PATCH 3/3] nvme: fix eui_show() print format

[Javier González]

Signed-off-by: Javier González <javier@cnexlabs.com>
---
 drivers/nvme/host/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index ae8ab0a1ef0d..f05c81774abf 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2108,7 +2108,7 @@ static ssize_t eui_show(struct device *dev, struct device_attribute *attr,
 								char *buf)
 {
 	struct nvme_ns *ns = nvme_get_ns_from_dev(dev);
-	return sprintf(buf, "%8phd\n", ns->eui);
+	return sprintf(buf, "%8phD\n", ns->eui);
 }
 static DEVICE_ATTR(eui, S_IRUGO, eui_show, NULL);
 
-- 
2.7.4

Re: [PATCH 1/3] nvme: do not check for ns on rw path

[Christoph Hellwig]

> -	if (ns && ns->ms &&
> +	if (ns->ms &&
>  	    (!ns->pi_type || ns->ms != sizeof(struct t10_pi_tuple)) &&
>  	    !blk_integrity_rq(req) && !blk_rq_is_passthrough(req))
>  		return BLK_STS_NOTSUPP;

blk_rq_is_passthrough also can't be true here.

How about:

	if (ns->ms && !blk_integrity_rq(req) &&
	    (!ns->pi_type || ns->ms != sizeof(struct t10_pi_tuple)))
		return BLK_STS_NOTSUPP;

Although I have to admit I don't really understand what this check
is even trying to do.  It basically checks for a namespace that has
a format with metadata that is not T10 protection information and
then rejects all I/O to it.  Why are we even creating a block device
node for such a thing?

Re: [PATCH 2/3] nvme: compare NQN string with right size

[Christoph Hellwig]

On Fri, Nov 03, 2017 at 11:02:49AM +0100, Javier González wrote:
> Compare subnqns using NVMF_NQN_SIZE as it is < 256
> 
> Signed-off-by: Javier González <javier@cnexlabs.com>
> ---
>  drivers/nvme/host/core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index bd1d5ff911c9..ae8ab0a1ef0d 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -1743,7 +1743,7 @@ static void nvme_init_subnqn(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
>  
>  	nqnlen = strnlen(id->subnqn, NVMF_NQN_SIZE);
>  	if (nqnlen > 0 && nqnlen < NVMF_NQN_SIZE) {
> -		strcpy(ctrl->subnqn, id->subnqn);
> +		strncpy(ctrl->subnqn, id->subnqn, NVMF_NQN_SIZE);
>  		return;
>  	}

This isn't a compare, but a copy.  Except for that it looks ok to me.

Re: [PATCH 3/3] nvme: fix eui_show() print format

[Christoph Hellwig]

On Fri, Nov 03, 2017 at 11:02:50AM +0100, Javier González wrote:
> Signed-off-by: Javier González <javier@cnexlabs.com>
> ---
>  drivers/nvme/host/core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index ae8ab0a1ef0d..f05c81774abf 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -2108,7 +2108,7 @@ static ssize_t eui_show(struct device *dev, struct device_attribute *attr,
>  								char *buf)
>  {
>  	struct nvme_ns *ns = nvme_get_ns_from_dev(dev);
> -	return sprintf(buf, "%8phd\n", ns->eui);
> +	return sprintf(buf, "%8phD\n", ns->eui);
>  }
>  static DEVICE_ATTR(eui, S_IRUGO, eui_show, NULL);

This looks correct.  I wonder what the old code printed - does someone
have a device with an EUI-64 at hand to quickly cross check what we
did before?

Re: [PATCH 2/3] nvme: compare NQN string with right size

[Javier González]

[-- Attachment #1: Type: text/plain, Size: 999 bytes --]

> On 3 Nov 2017, at 13.54, Christoph Hellwig <hch@lst.de> wrote:
> 
> On Fri, Nov 03, 2017 at 11:02:49AM +0100, Javier González wrote:
>> Compare subnqns using NVMF_NQN_SIZE as it is < 256
>> 
>> Signed-off-by: Javier González <javier@cnexlabs.com>
>> ---
>> drivers/nvme/host/core.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
>> index bd1d5ff911c9..ae8ab0a1ef0d 100644
>> --- a/drivers/nvme/host/core.c
>> +++ b/drivers/nvme/host/core.c
>> @@ -1743,7 +1743,7 @@ static void nvme_init_subnqn(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
>> 
>> 	nqnlen = strnlen(id->subnqn, NVMF_NQN_SIZE);
>> 	if (nqnlen > 0 && nqnlen < NVMF_NQN_SIZE) {
>> -		strcpy(ctrl->subnqn, id->subnqn);
>> +		strncpy(ctrl->subnqn, id->subnqn, NVMF_NQN_SIZE);
>> 		return;
>> 	}
> 
> This isn't a compare, but a copy.  Except for that it looks ok to me.

True. Can you change the message when picking it up?

[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/3] nvme: do not check for ns on rw path

[Javier González]

[-- Attachment #1: Type: text/plain, Size: 1099 bytes --]

> On 3 Nov 2017, at 13.53, Christoph Hellwig <hch@lst.de> wrote:
> 
>> -	if (ns && ns->ms &&
>> +	if (ns->ms &&
>> 	    (!ns->pi_type || ns->ms != sizeof(struct t10_pi_tuple)) &&
>> 	    !blk_integrity_rq(req) && !blk_rq_is_passthrough(req))
>> 		return BLK_STS_NOTSUPP;
> 
> blk_rq_is_passthrough also can't be true here.
> 
> How about:
> 
> 	if (ns->ms && !blk_integrity_rq(req) &&
> 	    (!ns->pi_type || ns->ms != sizeof(struct t10_pi_tuple)))
> 		return BLK_STS_NOTSUPP;
> 

Sure.

> Although I have to admit I don't really understand what this check
> is even trying to do.  It basically checks for a namespace that has
> a format with metadata that is not T10 protection information and
> then rejects all I/O to it.  Why are we even creating a block device
> node for such a thing?

Looking at the history (i) the check has changed location and (ii) some
checks have been added through time. So it looks like leftovers from
here and there.

If we end up not needing these checks at all here, you can just fix it
all in the same commit. Just wanted to get rid of sparse/smatch
complains...


[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/3] nvme: do not check for ns on rw path

[Keith Busch]

On Fri, Nov 03, 2017 at 01:53:40PM +0100, Christoph Hellwig wrote:
> > -	if (ns && ns->ms &&
> > +	if (ns->ms &&
> >  	    (!ns->pi_type || ns->ms != sizeof(struct t10_pi_tuple)) &&
> >  	    !blk_integrity_rq(req) && !blk_rq_is_passthrough(req))
> >  		return BLK_STS_NOTSUPP;
> 
> blk_rq_is_passthrough also can't be true here.
> 
> How about:
> 
> 	if (ns->ms && !blk_integrity_rq(req) &&
> 	    (!ns->pi_type || ns->ms != sizeof(struct t10_pi_tuple)))
> 		return BLK_STS_NOTSUPP;
> 
> Although I have to admit I don't really understand what this check
> is even trying to do.  It basically checks for a namespace that has
> a format with metadata that is not T10 protection information and
> then rejects all I/O to it.  Why are we even creating a block device
> node for such a thing?

If the namespace has metadata, but the request doesn't have a metadata
payload attached to it for whatever reason, we can't construct the command
for that format. We also can't have the controller strip/generate the
payload with PRACT bit set if it's not a T10 format, so we just fail
the command.

Re: [PATCH 3/3] nvme: fix eui_show() print format

[Keith Busch]

On Fri, Nov 03, 2017 at 01:55:16PM +0100, Christoph Hellwig wrote:
> On Fri, Nov 03, 2017 at 11:02:50AM +0100, Javier González wrote:
> > Signed-off-by: Javier González <javier@cnexlabs.com>
> > ---
> >  drivers/nvme/host/core.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> > index ae8ab0a1ef0d..f05c81774abf 100644
> > --- a/drivers/nvme/host/core.c
> > +++ b/drivers/nvme/host/core.c
> > @@ -2108,7 +2108,7 @@ static ssize_t eui_show(struct device *dev, struct device_attribute *attr,
> >  								char *buf)
> >  {
> >  	struct nvme_ns *ns = nvme_get_ns_from_dev(dev);
> > -	return sprintf(buf, "%8phd\n", ns->eui);
> > +	return sprintf(buf, "%8phD\n", ns->eui);
> >  }
> >  static DEVICE_ATTR(eui, S_IRUGO, eui_show, NULL);
> 
> This looks correct.  I wonder what the old code printed - does someone
> have a device with an EUI-64 at hand to quickly cross check what we
> did before?

It just prints the same as the 'ph' format, which would look like this:

  01 02 03 04 05 06 07 08

The change will make it look like this:

  01-02-03-04-05-06-07-08

I think that was the original intention.

Reviewed-by: Keith Busch <keith.busch@intel.com>

Re: [PATCH 3/3] nvme: fix eui_show() print format

[Joe Perches]

On Fri, 2017-11-03 at 13:55 +0100, Christoph Hellwig wrote:
> On Fri, Nov 03, 2017 at 11:02:50AM +0100, Javier González wrote:
> > Signed-off-by: Javier González <javier@cnexlabs.com>
[]
> > diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
[]
> > @@ -2108,7 +2108,7 @@ static ssize_t eui_show(struct device *dev, struct device_attribute *attr,
> >  								char *buf)
> >  {
> >  	struct nvme_ns *ns = nvme_get_ns_from_dev(dev);
> > -	return sprintf(buf, "%8phd\n", ns->eui);
> > +	return sprintf(buf, "%8phD\n", ns->eui);
> >  }
> >  static DEVICE_ATTR(eui, S_IRUGO, eui_show, NULL);
> 
> This looks correct.  I wonder what the old code printed - does someone
> have a device with an EUI-64 at hand to quickly cross check what we
> did before?

It uses spaces between bytes and not dashes.

The code has been this way a couple years now.

I think this proposal, while it might fix an
unintentional output style, could also be an API
and could cause user breakage if changed.

Perhaps this should just become

	%8ph

without D

Re: [PATCH 1/3] nvme: do not check for ns on rw path

[Christoph Hellwig]

On Fri, Nov 03, 2017 at 09:02:04AM -0600, Keith Busch wrote:
> If the namespace has metadata, but the request doesn't have a metadata
> payload attached to it for whatever reason, we can't construct the command
> for that format. We also can't have the controller strip/generate the
> payload with PRACT bit set if it's not a T10 format, so we just fail
> the command.

The only metadata payload a READ or WRITE request can have is a protection
information one.  For a namespace formatted with protection information
bio_integrity_prep as called from blk_mq_make_request will ensure we
always have metadata attached.

If a namespace is formatted with non-PI metadata we will never have
metadata attached to the bio/request and should not even present the
namespace to the kernel.

Re: [PATCH 3/3] nvme: fix eui_show() print format

[Javier González]

[-- Attachment #1: Type: text/plain, Size: 1220 bytes --]

> On 3 Nov 2017, at 16.16, Joe Perches <joe@perches.com> wrote:
> 
> On Fri, 2017-11-03 at 13:55 +0100, Christoph Hellwig wrote:
>> On Fri, Nov 03, 2017 at 11:02:50AM +0100, Javier González wrote:
>>> Signed-off-by: Javier González <javier@cnexlabs.com>
> []
>>> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> []
>>> @@ -2108,7 +2108,7 @@ static ssize_t eui_show(struct device *dev, struct device_attribute *attr,
>>> 								char *buf)
>>> {
>>> 	struct nvme_ns *ns = nvme_get_ns_from_dev(dev);
>>> -	return sprintf(buf, "%8phd\n", ns->eui);
>>> +	return sprintf(buf, "%8phD\n", ns->eui);
>>> }
>>> static DEVICE_ATTR(eui, S_IRUGO, eui_show, NULL);
>> 
>> This looks correct.  I wonder what the old code printed - does someone
>> have a device with an EUI-64 at hand to quickly cross check what we
>> did before?
> 
> It uses spaces between bytes and not dashes.
> 
> The code has been this way a couple years now.
> 
> I think this proposal, while it might fix an
> unintentional output style, could also be an API
> and could cause user breakage if changed.
> 
> Perhaps this should just become
> 
> 	%8ph
> 
> without D

That would be ok with me.

Javier.

[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/3] nvme: do not check for ns on rw path

[Keith Busch]

On Sat, Nov 04, 2017 at 09:18:25AM +0100, Christoph Hellwig wrote:
> On Fri, Nov 03, 2017 at 09:02:04AM -0600, Keith Busch wrote:
> > If the namespace has metadata, but the request doesn't have a metadata
> > payload attached to it for whatever reason, we can't construct the command
> > for that format. We also can't have the controller strip/generate the
> > payload with PRACT bit set if it's not a T10 format, so we just fail
> > the command.
> 
> The only metadata payload a READ or WRITE request can have is a protection
> information one.  For a namespace formatted with protection information
> bio_integrity_prep as called from blk_mq_make_request will ensure we
> always have metadata attached.
> 
> If a namespace is formatted with non-PI metadata we will never have
> metadata attached to the bio/request and should not even present the
> namespace to the kernel.

That's not quite right. For non-PI metadata formats, we use the
'nop_profile', which gets the metadata buffer allocated so we can safely
use a metadata formatted namespace. There's no in-kernel user of the
allocated payload, but we still need the metadata buffer in order to
use the namespace at all.

Re: [PATCH 1/3] nvme: do not check for ns on rw path

[Christoph Hellwig]

On Sat, Nov 04, 2017 at 09:38:45AM -0600, Keith Busch wrote:
> That's not quite right. For non-PI metadata formats, we use the
> 'nop_profile', which gets the metadata buffer allocated so we can safely
> use a metadata formatted namespace. There's no in-kernel user of the
> allocated payload, but we still need the metadata buffer in order to
> use the namespace at all.

You're right.  But that means we will indeed always have a matching
integrity payload here and the check should not be needed.

Are you fine with turning it into something like:

	if (WARN_ON_ONCE(ns->ms && !blk_integrity_rq(req)))
		return BLK_STS_IOERR;

?

Re: [PATCH 1/3] nvme: do not check for ns on rw path

[Keith Busch]

On Mon, Nov 06, 2017 at 10:13:24AM +0100, Christoph Hellwig wrote:
> On Sat, Nov 04, 2017 at 09:38:45AM -0600, Keith Busch wrote:
> > That's not quite right. For non-PI metadata formats, we use the
> > 'nop_profile', which gets the metadata buffer allocated so we can safely
> > use a metadata formatted namespace. There's no in-kernel user of the
> > allocated payload, but we still need the metadata buffer in order to
> > use the namespace at all.
> 
> You're right.  But that means we will indeed always have a matching
> integrity payload here and the check should not be needed.
> 
> Are you fine with turning it into something like:
> 
> 	if (WARN_ON_ONCE(ns->ms && !blk_integrity_rq(req)))
> 		return BLK_STS_IOERR;
> 
> ?

Yes, that looks fine. You're right that we are not supposed to be able
to get into this path for read/write since the driver sets the capacity
to 0 if a metadata format doesn't have integrity support, so hitting
this warning would indicate something has gone wrong.

Re: [PATCH 3/3] nvme: fix eui_show() print format

[Christoph Hellwig]

On Sat, Nov 04, 2017 at 12:22:20PM +0100, Javier González wrote:
> > Perhaps this should just become
> > 
> > 	%8ph
> > 
> > without D
> 
> That would be ok with me.

Can you resend a patch for that?

Re: [PATCH 3/3] nvme: fix eui_show() print format

[Javier González]

> On 7 Nov 2017, at 17.28, Christoph Hellwig <hch@lst.de> wrote:
> 
> On Sat, Nov 04, 2017 at 12:22:20PM +0100, Javier González wrote:
>>> Perhaps this should just become
>>> 
>>>    %8ph
>>> 
>>> without D
>> 
>> That would be ok with me.
> 
> Can you resend a patch for that?
> 

Sure. I’ll send it tomorrow together with the copy fix with the right commit message. 

Javier