[PATCH 00/16] Fixes for issues detected by static analyzers

[PATCH 00/16] Fixes for issues detected by static analyzers

[Bart Van Assche]

Hi Keith, Christoph and Sagi,

This patch series addresses several issues reported by static analyzers like
sparse, smatch and Coverity. Please consider these patches for Linux kernel
v4.20.

Thanks,

Bart.

Bart Van Assche (16):
  nvme-core: Declare local symbols static
  nvme-core: Refuse out-of-range integrity data seeds
  nvme-core: Rework a NQN copying operation
  nvme-core: Complain if nvme_init_identify() fails
  nvme-pci: Fix nvme_suspend_queue() kernel-doc header
  nvme-fc: Fix kernel-doc headers
  nvme-fc: Introduce struct nvme_fcp_op_w_sgl
  nvme-fc: Rework the request initialization code
  nvmet-fc: Fix kernel-doc headers
  nvmet-fcloop: Suppress a compiler warning
  nvmet: Use strcmp() instead of strncmp() for subsystem lookup
  nvmet: Remove unreachable code from nvmet_parse_discovery_cmd()
  nvmet: Use strlcpy() instead of strcpy()
  nvmet: Avoid integer overflow in the discard code
  nvmet-rdma: Declare local symbols static
  nvmet-rdma: Check for timeout in nvme_rdma_wait_for_cm()

 drivers/nvme/host/core.c          | 14 +++++++---
 drivers/nvme/host/fc.c            | 45 ++++++++++++++++++-------------
 drivers/nvme/host/pci.c           |  2 +-
 drivers/nvme/host/rdma.c          |  9 ++++++-
 drivers/nvme/target/Makefile      |  2 ++
 drivers/nvme/target/admin-cmd.c   |  2 +-
 drivers/nvme/target/core.c        |  3 +--
 drivers/nvme/target/discovery.c   |  7 ++---
 drivers/nvme/target/fc.c          |  6 ++---
 drivers/nvme/target/io-cmd-file.c |  3 ++-
 drivers/nvme/target/rdma.c        |  2 +-
 11 files changed, 59 insertions(+), 36 deletions(-)

-- 
2.19.0.605.g01d371f741-goog

[PATCH 01/16] nvme-core: Declare local symbols static

[Bart Van Assche]

This patch avoids that sparse complains about missing declarations.

Signed-off-by: Bart Van Assche <bvanassche at acm.org>
---
 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 2db33a752e2b..63932dea74a1 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2729,7 +2729,7 @@ static umode_t nvme_ns_id_attrs_are_visible(struct kobject *kobj,
 	return a->mode;
 }
 
-const struct attribute_group nvme_ns_id_attr_group = {
+static const struct attribute_group nvme_ns_id_attr_group = {
 	.attrs		= nvme_ns_id_attrs,
 	.is_visible	= nvme_ns_id_attrs_are_visible,
 };
-- 
2.19.0.605.g01d371f741-goog

[PATCH 01/16] nvme-core: Declare local symbols static

[Chaitanya Kulkarni]

Looks good.

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni at wdc.com>


From: Linux-nvme <linux-nvme-bounces@lists.infradead.org> on behalf of Bart Van Assche <bvanassche@acm.org>
Sent: Monday, October 8, 2018 2:28 PM
To: Keith Busch
Cc: Bart Van Assche; Christoph Hellwig; linux-nvme at lists.infradead.org; Sagi Grimberg
Subject: [PATCH 01/16] nvme-core: Declare local symbols static
? 
 
This patch avoids that sparse complains about missing declarations.

Signed-off-by: Bart Van Assche <bvanassche at acm.org>
---
?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 2db33a752e2b..63932dea74a1 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2729,7 +2729,7 @@ static umode_t nvme_ns_id_attrs_are_visible(struct kobject *kobj,
???????? return a->mode;
?}
?
-const struct attribute_group nvme_ns_id_attr_group = {
+static const struct attribute_group nvme_ns_id_attr_group = {
???????? .attrs????????? = nvme_ns_id_attrs,
???????? .is_visible???? = nvme_ns_id_attrs_are_visible,
?};
-- 
2.19.0.605.g01d371f741-goog


_______________________________________________
Linux-nvme mailing list
Linux-nvme at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
    

[PATCH 01/16] nvme-core: Declare local symbols static

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <jthumshirn at suse.de>
-- 
Johannes Thumshirn                                          Storage
jthumshirn at suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

[PATCH 02/16] nvme-core: Refuse out-of-range integrity data seeds

[Bart Van Assche]

The nvme_user_io.slba field is 64 bits wide. That value is copied into the
32-bit bio_integrity_payload.bip_iter.bi_sector field. Refuse slba values
that exceed 32 bits. This patch avoids that Coverity complains about
implicit truncation. See also Coverity ID 1056486 on
http://scan.coverity.com/projects/linux.

Signed-off-by: Bart Van Assche <bvanassche at acm.org>
---
 drivers/nvme/host/core.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 63932dea74a1..04138223fad6 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1118,6 +1118,14 @@ static int nvme_submit_io(struct nvme_ns *ns, struct nvme_user_io __user *uio)
 			return -EINVAL;
 	}
 
+	/*
+	 * io.slba is 64 bits wide. Only the lower 32 bits are used as a seed.
+	 * Refuse seed values that exceed 32 bits instead of truncating the
+	 * seed value silently. See also nvme_add_user_metadata().
+	 */
+	if (io.slba >> 32 != 0)
+		return -EINVAL;
+
 	memset(&c, 0, sizeof(c));
 	c.rw.opcode = io.opcode;
 	c.rw.flags = io.flags;
-- 
2.19.0.605.g01d371f741-goog

[PATCH 02/16] nvme-core: Refuse out-of-range integrity data seeds

[Keith Busch]

On Mon, Oct 08, 2018@02:28:40PM -0700, Bart Van Assche wrote:
> The nvme_user_io.slba field is 64 bits wide. That value is copied into the
> 32-bit bio_integrity_payload.bip_iter.bi_sector field. Refuse slba values
> that exceed 32 bits. This patch avoids that Coverity complains about
> implicit truncation. See also Coverity ID 1056486 on
> http://scan.coverity.com/projects/linux.
> 
> Signed-off-by: Bart Van Assche <bvanassche at acm.org>
> ---
>  drivers/nvme/host/core.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 63932dea74a1..04138223fad6 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -1118,6 +1118,14 @@ static int nvme_submit_io(struct nvme_ns *ns, struct nvme_user_io __user *uio)
>  			return -EINVAL;
>  	}
>  
> +	/*
> +	 * io.slba is 64 bits wide. Only the lower 32 bits are used as a seed.
> +	 * Refuse seed values that exceed 32 bits instead of truncating the
> +	 * seed value silently. See also nvme_add_user_metadata().
> +	 */
> +	if (io.slba >> 32 != 0)
> +		return -EINVAL;
> +
>  	memset(&c, 0, sizeof(c));
>  	c.rw.opcode = io.opcode;
>  	c.rw.flags = io.flags;

The bip sector is supposed to wrap if it exceeds 32 bits. Just feed
"lower_32_bits(io.slba)" as the metadata seed.

[PATCH 03/16] nvme-core: Rework a NQN copying operation

[Bart Van Assche]

Although it is easy to see that the code in nvme_init_subnqn() guarantees that
the subsys->nqn string is '\0'-terminated, apparently Coverity is not smart
enough to see this. Make it easier for Coverity to analyze this code by changing
the strncpy() call into a strlcpy() call. This patch does not change the
behavior of the code but fixes Coveritiy ID 1423720.

Signed-off-by: Bart Van Assche <bvanassche at acm.org>
---
 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 04138223fad6..3746fe49c382 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2084,7 +2084,7 @@ static void nvme_init_subnqn(struct nvme_subsystem *subsys, struct nvme_ctrl *ct
 
 	nqnlen = strnlen(id->subnqn, NVMF_NQN_SIZE);
 	if (nqnlen > 0 && nqnlen < NVMF_NQN_SIZE) {
-		strncpy(subsys->subnqn, id->subnqn, NVMF_NQN_SIZE);
+		strlcpy(subsys->subnqn, id->subnqn, NVMF_NQN_SIZE);
 		return;
 	}
 
-- 
2.19.0.605.g01d371f741-goog

[PATCH 03/16] nvme-core: Rework a NQN copying operation

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <jthumshirn at suse.de>
-- 
Johannes Thumshirn                                          Storage
jthumshirn at suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

[PATCH 04/16] nvme-core: Complain if nvme_init_identify() fails

[Bart Van Assche]

This patch avoids that Coverity complains that some but not all callers of
nvme_init_identify() check the return value of that function. See also
Coverity ID 1423964.

Signed-off-by: Bart Van Assche <bvanassche at acm.org>
---
 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 3746fe49c382..2ce3e9d15618 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1213,7 +1213,7 @@ static void nvme_passthru_end(struct nvme_ctrl *ctrl, u32 effects)
 	if (effects & (NVME_CMD_EFFECTS_LBCC | NVME_CMD_EFFECTS_CSE_MASK))
 		nvme_unfreeze(ctrl);
 	if (effects & NVME_CMD_EFFECTS_CCC)
-		nvme_init_identify(ctrl);
+		WARN_ON_ONCE(nvme_init_identify(ctrl) < 0);
 	if (effects & (NVME_CMD_EFFECTS_NIC | NVME_CMD_EFFECTS_NCC))
 		nvme_queue_scan(ctrl);
 }
-- 
2.19.0.605.g01d371f741-goog

[PATCH 04/16] nvme-core: Complain if nvme_init_identify() fails

[Chaitanya Kulkarni]

Looks good.

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni at wdc.com>






From: Linux-nvme <linux-nvme-bounces@lists.infradead.org> on behalf of Bart Van Assche <bvanassche@acm.org>
Sent: Monday, October 8, 2018 2:28 PM
To: Keith Busch
Cc: Bart Van Assche; Christoph Hellwig; linux-nvme at lists.infradead.org; Sagi Grimberg
Subject: [PATCH 04/16] nvme-core: Complain if nvme_init_identify() fails
? 
 
This patch avoids that Coverity complains that some but not all callers of
nvme_init_identify() check the return value of that function. See also
Coverity ID 1423964.

Signed-off-by: Bart Van Assche <bvanassche at acm.org>
---
?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 3746fe49c382..2ce3e9d15618 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1213,7 +1213,7 @@ static void nvme_passthru_end(struct nvme_ctrl *ctrl, u32 effects)
???????? if (effects & (NVME_CMD_EFFECTS_LBCC | NVME_CMD_EFFECTS_CSE_MASK))
???????????????? nvme_unfreeze(ctrl);
???????? if (effects & NVME_CMD_EFFECTS_CCC)
-?????????????? nvme_init_identify(ctrl);
+?????????????? WARN_ON_ONCE(nvme_init_identify(ctrl) < 0);
???????? if (effects & (NVME_CMD_EFFECTS_NIC | NVME_CMD_EFFECTS_NCC))
???????????????? nvme_queue_scan(ctrl);
?}
-- 
2.19.0.605.g01d371f741-goog


_______________________________________________
Linux-nvme mailing list
Linux-nvme at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
    

[PATCH 04/16] nvme-core: Complain if nvme_init_identify() fails

[Keith Busch]

On Mon, Oct 08, 2018@02:28:42PM -0700, Bart Van Assche wrote:
> This patch avoids that Coverity complains that some but not all callers of
> nvme_init_identify() check the return value of that function. See also
> Coverity ID 1423964.

That seems like an odd thing to complain about. If we required the
return value be checked, we have the "__must_check" attribute. In this
particular path, we don't care if nvme_init_identify returns an error,
and the failure is logged in kernel messages within the function already.

Is there another way to suppress the complaint?

> Signed-off-by: Bart Van Assche <bvanassche at acm.org>
> ---
>  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 3746fe49c382..2ce3e9d15618 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -1213,7 +1213,7 @@ static void nvme_passthru_end(struct nvme_ctrl *ctrl, u32 effects)
>  	if (effects & (NVME_CMD_EFFECTS_LBCC | NVME_CMD_EFFECTS_CSE_MASK))
>  		nvme_unfreeze(ctrl);
>  	if (effects & NVME_CMD_EFFECTS_CCC)
> -		nvme_init_identify(ctrl);
> +		WARN_ON_ONCE(nvme_init_identify(ctrl) < 0);
>  	if (effects & (NVME_CMD_EFFECTS_NIC | NVME_CMD_EFFECTS_NCC))
>  		nvme_queue_scan(ctrl);
>  }

[PATCH 04/16] nvme-core: Complain if nvme_init_identify() fails

[Bart Van Assche]

On Mon, 2018-10-08@16:16 -0600, Keith Busch wrote:
> On Mon, Oct 08, 2018@02:28:42PM -0700, Bart Van Assche wrote:
> > This patch avoids that Coverity complains that some but not all callers of
> > nvme_init_identify() check the return value of that function. See also
> > Coverity ID 1423964.
> 
> That seems like an odd thing to complain about. If we required the
> return value be checked, we have the "__must_check" attribute. In this
> particular path, we don't care if nvme_init_identify returns an error,
> and the failure is logged in kernel messages within the function already.
> 
> Is there another way to suppress the complaint?

One way is to change nvme_init_identify(ctrl) into the following:

if (nvme_init_identify(ctrl) < 0) {
}

However, I'm not sure we should do that. Another possibility is to mark the
complaint in the Coverity database as "intended". That means that the return
value is ignored on purpose.

Bart.

[PATCH 04/16] nvme-core: Complain if nvme_init_identify() fails

[Keith Busch]

On Mon, Oct 08, 2018@03:26:42PM -0700, Bart Van Assche wrote:
> On Mon, 2018-10-08@16:16 -0600, Keith Busch wrote:
> > On Mon, Oct 08, 2018@02:28:42PM -0700, Bart Van Assche wrote:
> > > This patch avoids that Coverity complains that some but not all callers of
> > > nvme_init_identify() check the return value of that function. See also
> > > Coverity ID 1423964.
> > 
> > That seems like an odd thing to complain about. If we required the
> > return value be checked, we have the "__must_check" attribute. In this
> > particular path, we don't care if nvme_init_identify returns an error,
> > and the failure is logged in kernel messages within the function already.
> > 
> > Is there another way to suppress the complaint?
> 
> One way is to change nvme_init_identify(ctrl) into the following:
> 
> if (nvme_init_identify(ctrl) < 0) {
> }
> 
> However, I'm not sure we should do that. Another possibility is to mark the
> complaint in the Coverity database as "intended". That means that the return
> value is ignored on purpose.

Right, I was hoping for a way to suppress the complaint without changing
the code since it is intended as-is.

[PATCH 05/16] nvme-pci: Fix nvme_suspend_queue() kernel-doc header

[Bart Van Assche]

This patch avoids that the kernel-doc tool complains about the
nvme_suspend_queue() function header when building with W=1.

Signed-off-by: Bart Van Assche <bvanassche at acm.org>
---
 drivers/nvme/host/pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index d668682f91df..450481c2fd17 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -1249,7 +1249,7 @@ static void nvme_free_queues(struct nvme_dev *dev, int lowest)
 
 /**
  * nvme_suspend_queue - put queue into suspended state
- * @nvmeq - queue to suspend
+ * @nvmeq: queue to suspend
  */
 static int nvme_suspend_queue(struct nvme_queue *nvmeq)
 {
-- 
2.19.0.605.g01d371f741-goog

[PATCH 05/16] nvme-pci: Fix nvme_suspend_queue() kernel-doc header

[Keith Busch]

On Mon, Oct 08, 2018@02:28:43PM -0700, Bart Van Assche wrote:
> This patch avoids that the kernel-doc tool complains about the
> nvme_suspend_queue() function header when building with W=1.
> 
> Signed-off-by: Bart Van Assche <bvanassche at acm.org>

Looks fine.

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

> ---
>  drivers/nvme/host/pci.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index d668682f91df..450481c2fd17 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -1249,7 +1249,7 @@ static void nvme_free_queues(struct nvme_dev *dev, int lowest)
>  
>  /**
>   * nvme_suspend_queue - put queue into suspended state
> - * @nvmeq - queue to suspend
> + * @nvmeq: queue to suspend
>   */
>  static int nvme_suspend_queue(struct nvme_queue *nvmeq)
>  {
> -- 
> 2.19.0.605.g01d371f741-goog
> 

[PATCH 06/16] nvme-fc: Fix kernel-doc headers

[Bart Van Assche]

This patch avoids that the kernel-doc tool complains about several
multiple function headers when building with W=1.

Cc: James Smart <james.smart at broadcom.com>
Signed-off-by: Bart Van Assche <bvanassche at acm.org>
---
 drivers/nvme/host/fc.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
index 9d201b35397d..d838987fffe1 100644
--- a/drivers/nvme/host/fc.c
+++ b/drivers/nvme/host/fc.c
@@ -317,7 +317,7 @@ nvme_fc_attach_to_unreg_lport(struct nvme_fc_port_info *pinfo,
  * @template:  LLDD entrypoints and operational parameters for the port
  * @dev:       physical hardware device node port corresponds to. Will be
  *             used for DMA mappings
- * @lport_p:   pointer to a local port pointer. Upon success, the routine
+ * @portptr:   pointer to a local port pointer. Upon success, the routine
  *             will allocate a nvme_fc_local_port structure and place its
  *             address in the local port pointer. Upon failure, local port
  *             pointer will be set to 0.
@@ -425,8 +425,7 @@ EXPORT_SYMBOL_GPL(nvme_fc_register_localport);
  * nvme_fc_unregister_localport - transport entry point called by an
  *                              LLDD to deregister/remove a previously
  *                              registered a NVME host FC port.
- * @localport: pointer to the (registered) local port that is to be
- *             deregistered.
+ * @portptr: pointer to the (registered) local port that is to be deregistered.
  *
  * Returns:
  * a completion status. Must be 0 upon success; a negative errno
@@ -632,7 +631,7 @@ __nvme_fc_set_dev_loss_tmo(struct nvme_fc_rport *rport,
  * @localport: pointer to the (registered) local port that the remote
  *             subsystem port is connected to.
  * @pinfo:     pointer to information about the port to be registered
- * @rport_p:   pointer to a remote port pointer. Upon success, the routine
+ * @portptr:   pointer to a remote port pointer. Upon success, the routine
  *             will allocate a nvme_fc_remote_port structure and place its
  *             address in the remote port pointer. Upon failure, remote port
  *             pointer will be set to 0.
@@ -809,8 +808,8 @@ nvme_fc_ctrl_connectivity_loss(struct nvme_fc_ctrl *ctrl)
  * nvme_fc_unregister_remoteport - transport entry point called by an
  *                              LLDD to deregister/remove a previously
  *                              registered a NVME subsystem FC port.
- * @remoteport: pointer to the (registered) remote port that is to be
- *              deregistered.
+ * @portptr: pointer to the (registered) remote port that is to be
+ *           deregistered.
  *
  * Returns:
  * a completion status. Must be 0 upon success; a negative errno
-- 
2.19.0.605.g01d371f741-goog

[PATCH 06/16] nvme-fc: Fix kernel-doc headers

[James Smart]

On 10/8/2018 2:28 PM, Bart Van Assche wrote:
> This patch avoids that the kernel-doc tool complains about several
> multiple function headers when building with W=1.
>
> Cc: James Smart <james.smart at broadcom.com>
> Signed-off-by: Bart Van Assche <bvanassche at acm.org>
> ---
>
Looks good.

Reviewed-by:? James Smart <james.smart at broadcom.com>

[PATCH 07/16] nvme-fc: Introduce struct nvme_fcp_op_w_sgl

[Bart Van Assche]

This patch does not change any functionality but makes the intent of the
code more clear.

Cc: James Smart <james.smart at broadcom.com>
Signed-off-by: Bart Van Assche <bvanassche at acm.org>
---
 drivers/nvme/host/fc.c | 29 ++++++++++++++++++-----------
 1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
index d838987fffe1..fdadc9464f6f 100644
--- a/drivers/nvme/host/fc.c
+++ b/drivers/nvme/host/fc.c
@@ -20,6 +20,7 @@
 #include <uapi/scsi/fc/fc_fs.h>
 #include <uapi/scsi/fc/fc_els.h>
 #include <linux/delay.h>
+#include <linux/overflow.h>
 
 #include "nvme.h"
 #include "fabrics.h"
@@ -104,6 +105,12 @@ struct nvme_fc_fcp_op {
 	struct nvme_fc_ersp_iu	rsp_iu;
 };
 
+struct nvme_fcp_op_w_sgl {
+	struct nvme_fc_fcp_op	op;
+	struct scatterlist	sgl[SG_CHUNK_SIZE];
+	uint8_t			priv[0];
+};
+
 struct nvme_fc_lport {
 	struct nvme_fc_local_port	localport;
 
@@ -1686,6 +1693,8 @@ __nvme_fc_init_request(struct nvme_fc_ctrl *ctrl,
 		struct nvme_fc_queue *queue, struct nvme_fc_fcp_op *op,
 		struct request *rq, u32 rqno)
 {
+	struct nvme_fcp_op_w_sgl *op_w_sgl =
+		container_of(op, typeof(*op_w_sgl), op);
 	struct nvme_fc_cmd_iu *cmdiu = &op->cmd_iu;
 	int ret = 0;
 
@@ -1695,7 +1704,7 @@ __nvme_fc_init_request(struct nvme_fc_ctrl *ctrl,
 	op->fcp_req.rspaddr = &op->rsp_iu;
 	op->fcp_req.rsplen = sizeof(op->rsp_iu);
 	op->fcp_req.done = nvme_fc_fcpio_done;
-	op->fcp_req.first_sgl = (struct scatterlist *)&op[1];
+	op->fcp_req.first_sgl = &op_w_sgl->sgl[0];
 	op->fcp_req.private = &op->fcp_req.first_sgl[SG_CHUNK_SIZE];
 	op->ctrl = ctrl;
 	op->queue = queue;
@@ -1734,12 +1743,12 @@ nvme_fc_init_request(struct blk_mq_tag_set *set, struct request *rq,
 		unsigned int hctx_idx, unsigned int numa_node)
 {
 	struct nvme_fc_ctrl *ctrl = set->driver_data;
-	struct nvme_fc_fcp_op *op = blk_mq_rq_to_pdu(rq);
+	struct nvme_fcp_op_w_sgl *op = blk_mq_rq_to_pdu(rq);
 	int queue_idx = (set == &ctrl->tag_set) ? hctx_idx + 1 : 0;
 	struct nvme_fc_queue *queue = &ctrl->queues[queue_idx];
 
 	nvme_req(rq)->ctrl = &ctrl->ctrl;
-	return __nvme_fc_init_request(ctrl, queue, op, rq, queue->rqcnt++);
+	return __nvme_fc_init_request(ctrl, queue, &op->op, rq, queue->rqcnt++);
 }
 
 static int
@@ -2423,10 +2432,9 @@ nvme_fc_create_io_queues(struct nvme_fc_ctrl *ctrl)
 	ctrl->tag_set.reserved_tags = 1; /* fabric connect */
 	ctrl->tag_set.numa_node = NUMA_NO_NODE;
 	ctrl->tag_set.flags = BLK_MQ_F_SHOULD_MERGE;
-	ctrl->tag_set.cmd_size = sizeof(struct nvme_fc_fcp_op) +
-					(SG_CHUNK_SIZE *
-						sizeof(struct scatterlist)) +
-					ctrl->lport->ops->fcprqst_priv_sz;
+	ctrl->tag_set.cmd_size =
+		struct_size((struct nvme_fcp_op_w_sgl *)NULL, priv,
+			    ctrl->lport->ops->fcprqst_priv_sz);
 	ctrl->tag_set.driver_data = ctrl;
 	ctrl->tag_set.nr_hw_queues = ctrl->ctrl.queue_count - 1;
 	ctrl->tag_set.timeout = NVME_IO_TIMEOUT;
@@ -3028,10 +3036,9 @@ nvme_fc_init_ctrl(struct device *dev, struct nvmf_ctrl_options *opts,
 	ctrl->admin_tag_set.queue_depth = NVME_AQ_MQ_TAG_DEPTH;
 	ctrl->admin_tag_set.reserved_tags = 2; /* fabric connect + Keep-Alive */
 	ctrl->admin_tag_set.numa_node = NUMA_NO_NODE;
-	ctrl->admin_tag_set.cmd_size = sizeof(struct nvme_fc_fcp_op) +
-					(SG_CHUNK_SIZE *
-						sizeof(struct scatterlist)) +
-					ctrl->lport->ops->fcprqst_priv_sz;
+	ctrl->admin_tag_set.cmd_size =
+		struct_size((struct nvme_fcp_op_w_sgl *)NULL, priv,
+			    ctrl->lport->ops->fcprqst_priv_sz);
 	ctrl->admin_tag_set.driver_data = ctrl;
 	ctrl->admin_tag_set.nr_hw_queues = 1;
 	ctrl->admin_tag_set.timeout = ADMIN_TIMEOUT;
-- 
2.19.0.605.g01d371f741-goog

[PATCH 07/16] nvme-fc: Introduce struct nvme_fcp_op_w_sgl

[James Smart]

On 10/8/2018 2:28 PM, Bart Van Assche wrote:
> This patch does not change any functionality but makes the intent of the
> code more clear.
>
> Cc: James Smart <james.smart at broadcom.com>
> Signed-off-by: Bart Van Assche <bvanassche at acm.org>

Looks good.

Reviewed-by:? James Smart <james.smart at broadcom.com>

[PATCH 08/16] nvme-fc: Rework the request initialization code

[Bart Van Assche]

Instead of setting and then clearing the first_sgl pointer for AEN requests,
leave that pointer zero. This patch does not change how requests are
initialized but avoids that Coverity reports the following complaint for
nvme_fc_init_aen_ops():

CID 1418400 (#1 of 1): Out-of-bounds access (OVERRUN)
4. overrun-buffer-val: Overrunning buffer pointed to by aen_op of 312 bytes by passing it to a function which accesses it at byte offset 312.

Cc: James Smart <james.smart at broadcom.com>
Signed-off-by: Bart Van Assche <bvanassche at acm.org>
---
 drivers/nvme/host/fc.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
index fdadc9464f6f..e52b9d3c0bd6 100644
--- a/drivers/nvme/host/fc.c
+++ b/drivers/nvme/host/fc.c
@@ -1704,7 +1704,6 @@ __nvme_fc_init_request(struct nvme_fc_ctrl *ctrl,
 	op->fcp_req.rspaddr = &op->rsp_iu;
 	op->fcp_req.rsplen = sizeof(op->rsp_iu);
 	op->fcp_req.done = nvme_fc_fcpio_done;
-	op->fcp_req.first_sgl = &op_w_sgl->sgl[0];
 	op->fcp_req.private = &op->fcp_req.first_sgl[SG_CHUNK_SIZE];
 	op->ctrl = ctrl;
 	op->queue = queue;
@@ -1746,9 +1745,14 @@ nvme_fc_init_request(struct blk_mq_tag_set *set, struct request *rq,
 	struct nvme_fcp_op_w_sgl *op = blk_mq_rq_to_pdu(rq);
 	int queue_idx = (set == &ctrl->tag_set) ? hctx_idx + 1 : 0;
 	struct nvme_fc_queue *queue = &ctrl->queues[queue_idx];
+	int res;
 
 	nvme_req(rq)->ctrl = &ctrl->ctrl;
-	return __nvme_fc_init_request(ctrl, queue, &op->op, rq, queue->rqcnt++);
+	res = __nvme_fc_init_request(ctrl, queue, &op->op, rq, queue->rqcnt++);
+	if (res)
+		return res;
+	op->op.fcp_req.first_sgl = &op->sgl[0];
+	return res;
 }
 
 static int
@@ -1778,7 +1782,6 @@ nvme_fc_init_aen_ops(struct nvme_fc_ctrl *ctrl)
 		}
 
 		aen_op->flags = FCOP_FLAGS_AEN;
-		aen_op->fcp_req.first_sgl = NULL; /* no sg list */
 		aen_op->fcp_req.private = private;
 
 		memset(sqe, 0, sizeof(*sqe));
-- 
2.19.0.605.g01d371f741-goog

[PATCH 08/16] nvme-fc: Rework the request initialization code

[James Smart]

On 10/8/2018 2:28 PM, Bart Van Assche wrote:
> Instead of setting and then clearing the first_sgl pointer for AEN requests,
> leave that pointer zero. This patch does not change how requests are
> initialized but avoids that Coverity reports the following complaint for
> nvme_fc_init_aen_ops():
>
> CID 1418400 (#1 of 1): Out-of-bounds access (OVERRUN)
> 4. overrun-buffer-val: Overrunning buffer pointed to by aen_op of 312 bytes by passing it to a function which accesses it at byte offset 312.
>
> Cc: James Smart <james.smart at broadcom.com>
> Signed-off-by: Bart Van Assche <bvanassche at acm.org>


Looks good.

Reviewed-by:? James Smart <james.smart at broadcom.com>

[PATCH 09/16] nvmet-fc: Fix kernel-doc headers

[Bart Van Assche]

This patch avoids that the kernel-doc tool complains about two function
headers when building with W=1.

Cc: James Smart <james.smart at broadcom.com>
Signed-off-by: Bart Van Assche <bvanassche at acm.org>
---
 drivers/nvme/target/fc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/nvme/target/fc.c b/drivers/nvme/target/fc.c
index ef286b72d958..409081a03b24 100644
--- a/drivers/nvme/target/fc.c
+++ b/drivers/nvme/target/fc.c
@@ -1245,8 +1245,8 @@ nvmet_fc_delete_ctrl(struct nvmet_ctrl *ctrl)
  * nvme_fc_unregister_targetport - transport entry point called by an
  *                              LLDD to deregister/remove a previously
  *                              registered a local NVME subsystem FC port.
- * @tgtport: pointer to the (registered) target port that is to be
- *           deregistered.
+ * @target_port: pointer to the (registered) target port that is to be
+ *               deregistered.
  *
  * Returns:
  * a completion status. Must be 0 upon success; a negative errno
@@ -1749,7 +1749,7 @@ nvmet_fc_handle_ls_rqst_work(struct work_struct *work)
  *
  * If this routine returns error, the LLDD should abort the exchange.
  *
- * @tgtport:    pointer to the (registered) target port the LS was
+ * @target_port: pointer to the (registered) target port the LS was
  *              received on.
  * @lsreq:      pointer to a lsreq request structure to be used to reference
  *              the exchange corresponding to the LS.
-- 
2.19.0.605.g01d371f741-goog

[PATCH 09/16] nvmet-fc: Fix kernel-doc headers

[James Smart]

On 10/8/2018 2:28 PM, Bart Van Assche wrote:
> This patch avoids that the kernel-doc tool complains about two function
> headers when building with W=1.
>
> Cc: James Smart <james.smart at broadcom.com>
> Signed-off-by: Bart Van Assche <bvanassche at acm.org>

Looks good.

Reviewed-by:? James Smart <james.smart at broadcom.com>

[PATCH 10/16] nvmet-fcloop: Suppress a compiler warning

[Bart Van Assche]

Building with W=1 enables the compiler warning -Wimplicit-fallthrough=3. That
option does not recognize the fall-through comment in the fcloop driver. Hence
relax that compiler option. This patch avoids that the compiler reports the
following warning when building with W=1:

drivers/nvme/target/fcloop.c:647:6: warning: this statement may fall through [-Wimplicit-fallthrough=]
   if (op == NVMET_FCOP_READDATA)
      ^

Cc: James Smart <james.smart at broadcom.com>
Signed-off-by: Bart Van Assche <bvanassche at acm.org>
---
 drivers/nvme/target/Makefile | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/nvme/target/Makefile b/drivers/nvme/target/Makefile
index 8118c93391c6..ed578a73a376 100644
--- a/drivers/nvme/target/Makefile
+++ b/drivers/nvme/target/Makefile
@@ -1,5 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0
 
+ccflags-y += $(call cc-option,-Wimplicit-fallthrough=2)
+
 obj-$(CONFIG_NVME_TARGET)		+= nvmet.o
 obj-$(CONFIG_NVME_TARGET_LOOP)		+= nvme-loop.o
 obj-$(CONFIG_NVME_TARGET_RDMA)		+= nvmet-rdma.o
-- 
2.19.0.605.g01d371f741-goog

[PATCH 10/16] nvmet-fcloop: Suppress a compiler warning

[James Smart]

On 10/8/2018 2:28 PM, Bart Van Assche wrote:
> Building with W=1 enables the compiler warning -Wimplicit-fallthrough=3. That
> option does not recognize the fall-through comment in the fcloop driver. Hence
> relax that compiler option. This patch avoids that the compiler reports the
> following warning when building with W=1:
>
> drivers/nvme/target/fcloop.c:647:6: warning: this statement may fall through [-Wimplicit-fallthrough=]
>     if (op == NVMET_FCOP_READDATA)
>        ^
>
> Cc: James Smart <james.smart at broadcom.com>
> Signed-off-by: Bart Van Assche <bvanassche at acm.org>

Looks good.

Reviewed-by:? James Smart <james.smart at broadcom.com>

[PATCH 10/16] nvmet-fcloop: Suppress a compiler warning

[Christoph Hellwig]

On Mon, Oct 08, 2018@02:28:48PM -0700, Bart Van Assche wrote:
> Building with W=1 enables the compiler warning -Wimplicit-fallthrough=3. That
> option does not recognize the fall-through comment in the fcloop driver. Hence
> relax that compiler option. This patch avoids that the compiler reports the
> following warning when building with W=1:
> 
> drivers/nvme/target/fcloop.c:647:6: warning: this statement may fall through [-Wimplicit-fallthrough=]
>    if (op == NVMET_FCOP_READDATA)

Can you please just fix up the comments to use normal /* FALLTHRU */
annotation?  I rather hate having special compiler flags in drivers.

[PATCH 11/16] nvmet: Use strcmp() instead of strncmp() for subsystem lookup

[Bart Van Assche]

strncmp() stops comparing when either the end of one of the first two arguments
is reached or when 'n' characters have been compared, whichever comes first.
That means that strncmp(s1, s2, n) is equivalent to strcmp(s1, s2) if n exceeds
the length of s1 or the length of s2. Since that is the case in
nvmet_find_get_subsys(), change strncmp() into strcmp(). This patch avoids that
the following warning is reported by smatch:

drivers/nvme/target/core.c:940:1 nvmet_find_get_subsys() error: strncmp() '"nqn.2014-08.org.nvmexpress.discovery"' too small (37 vs 223)

Signed-off-by: Bart Van Assche <bvanassche at acm.org>
---
 drivers/nvme/target/core.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index b5ec96abd048..0acdff9e6842 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -1105,8 +1105,7 @@ static struct nvmet_subsys *nvmet_find_get_subsys(struct nvmet_port *port,
 	if (!port)
 		return NULL;
 
-	if (!strncmp(NVME_DISC_SUBSYS_NAME, subsysnqn,
-			NVMF_NQN_SIZE)) {
+	if (!strcmp(NVME_DISC_SUBSYS_NAME, subsysnqn)) {
 		if (!kref_get_unless_zero(&nvmet_disc_subsys->ref))
 			return NULL;
 		return nvmet_disc_subsys;
-- 
2.19.0.605.g01d371f741-goog

[PATCH 12/16] nvmet: Remove unreachable code from nvmet_parse_discovery_cmd()

[Bart Van Assche]

This patch avoids that smatch reports the following warning:

drivers/nvme/target/discovery.c:228:1 nvmet_parse_discovery_cmd() info: ignoring unreachable code.

Signed-off-by: Bart Van Assche <bvanassche at acm.org>
---
 drivers/nvme/target/discovery.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/nvme/target/discovery.c b/drivers/nvme/target/discovery.c
index eae29f493a07..08e7e284655c 100644
--- a/drivers/nvme/target/discovery.c
+++ b/drivers/nvme/target/discovery.c
@@ -218,12 +218,9 @@ u16 nvmet_parse_discovery_cmd(struct nvmet_req *req)
 			       cmd->identify.cns);
 			return NVME_SC_INVALID_OPCODE | NVME_SC_DNR;
 		}
-	default:
-		pr_err("unsupported cmd %d\n", cmd->common.opcode);
-		return NVME_SC_INVALID_OPCODE | NVME_SC_DNR;
 	}
 
-	pr_err("unhandled cmd %d\n", cmd->common.opcode);
+	pr_err("unsupported cmd %d\n", cmd->common.opcode);
 	return NVME_SC_INVALID_OPCODE | NVME_SC_DNR;
 }
 
-- 
2.19.0.605.g01d371f741-goog

[PATCH 12/16] nvmet: Remove unreachable code from nvmet_parse_discovery_cmd()

[Chaitanya Kulkarni]

I have already posted this fix.

Please see :-

http://lists.infradead.org/pipermail/linux-nvme/2018-August/019767.html


From: Linux-nvme <linux-nvme-bounces@lists.infradead.org> on behalf of Bart Van Assche <bvanassche@acm.org>
Sent: Monday, October 8, 2018 2:28 PM
To: Keith Busch
Cc: Bart Van Assche; Christoph Hellwig; linux-nvme at lists.infradead.org; Sagi Grimberg
Subject: [PATCH 12/16] nvmet: Remove unreachable code from nvmet_parse_discovery_cmd()
? 
 
This patch avoids that smatch reports the following warning:

drivers/nvme/target/discovery.c:228:1 nvmet_parse_discovery_cmd() info: ignoring unreachable code.

Signed-off-by: Bart Van Assche <bvanassche at acm.org>
---
?drivers/nvme/target/discovery.c | 5 +----
?1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/nvme/target/discovery.c b/drivers/nvme/target/discovery.c
index eae29f493a07..08e7e284655c 100644
--- a/drivers/nvme/target/discovery.c
+++ b/drivers/nvme/target/discovery.c
@@ -218,12 +218,9 @@ u16 nvmet_parse_discovery_cmd(struct nvmet_req *req)
??????????????????????????????? cmd->identify.cns);
???????????????????????? return NVME_SC_INVALID_OPCODE | NVME_SC_DNR;
???????????????? }
-?????? default:
-?????????????? pr_err("unsupported cmd %d\n", cmd->common.opcode);
-?????????????? return NVME_SC_INVALID_OPCODE | NVME_SC_DNR;
???????? }
?
-?????? pr_err("unhandled cmd %d\n", cmd->common.opcode);
+?????? pr_err("unsupported cmd %d\n", cmd->common.opcode);
???????? return NVME_SC_INVALID_OPCODE | NVME_SC_DNR;
?}
?
-- 
2.19.0.605.g01d371f741-goog


_______________________________________________
Linux-nvme mailing list
Linux-nvme at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
    

[PATCH 12/16] nvmet: Remove unreachable code from nvmet_parse_discovery_cmd()

[Bart Van Assche]

On 10/8/18 3:02 PM, Chaitanya Kulkarni wrote:
> I have already posted this fix.
> 
> Please see :-
> 
> http://lists.infradead.org/pipermail/linux-nvme/2018-August/019767.html

Thanks for your reply. I had missed that patch. I'm fine with either patch.

Bart.

[PATCH 13/16] nvmet: Use strlcpy() instead of strcpy()

[Bart Van Assche]

Although the code modified by this patch looks fine to me, this patch avoids
that Coverity reports the following complaint (ID 1364971 and ID 1364973):
"You might overrun the 256-character fixed-size string id->subnqn".

Signed-off-by: Bart Van Assche <bvanassche at acm.org>
---
 drivers/nvme/target/admin-cmd.c | 2 +-
 drivers/nvme/target/discovery.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 7a45f4477679..1179f6314323 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -353,7 +353,7 @@ static void nvmet_execute_identify_ctrl(struct nvmet_req *req)
 	if (req->port->inline_data_size)
 		id->sgls |= cpu_to_le32(1 << 20);
 
-	strcpy(id->subnqn, ctrl->subsys->subsysnqn);
+	strlcpy(id->subnqn, ctrl->subsys->subsysnqn, sizeof(id->subnqn));
 
 	/* Max command capsule size is sqe + single page of in-capsule data */
 	id->ioccsz = cpu_to_le32((sizeof(struct nvme_command) +
diff --git a/drivers/nvme/target/discovery.c b/drivers/nvme/target/discovery.c
index 08e7e284655c..ff52ab84d0e3 100644
--- a/drivers/nvme/target/discovery.c
+++ b/drivers/nvme/target/discovery.c
@@ -174,7 +174,7 @@ static void nvmet_execute_identify_disc_ctrl(struct nvmet_req *req)
 	if (req->port->inline_data_size)
 		id->sgls |= cpu_to_le32(1 << 20);
 
-	strcpy(id->subnqn, ctrl->subsys->subsysnqn);
+	strlcpy(id->subnqn, ctrl->subsys->subsysnqn, sizeof(id->subnqn));
 
 	status = nvmet_copy_to_sgl(req, 0, id, sizeof(*id));
 
-- 
2.19.0.605.g01d371f741-goog

[PATCH 13/16] nvmet: Use strlcpy() instead of strcpy()

[Chaitanya Kulkarni]

Looks good.

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni at wdc.com>



From: Linux-nvme <linux-nvme-bounces@lists.infradead.org> on behalf of Bart Van Assche <bvanassche@acm.org>
Sent: Monday, October 8, 2018 2:28 PM
To: Keith Busch
Cc: Bart Van Assche; Christoph Hellwig; linux-nvme at lists.infradead.org; Sagi Grimberg
Subject: [PATCH 13/16] nvmet: Use strlcpy() instead of strcpy()
? 
 
Although the code modified by this patch looks fine to me, this patch avoids
that Coverity reports the following complaint (ID 1364971 and ID 1364973):
"You might overrun the 256-character fixed-size string id->subnqn".

Signed-off-by: Bart Van Assche <bvanassche at acm.org>
---
?drivers/nvme/target/admin-cmd.c | 2 +-
?drivers/nvme/target/discovery.c | 2 +-
?2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 7a45f4477679..1179f6314323 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -353,7 +353,7 @@ static void nvmet_execute_identify_ctrl(struct nvmet_req *req)
???????? if (req->port->inline_data_size)
???????????????? id->sgls |= cpu_to_le32(1 << 20);
?
-?????? strcpy(id->subnqn, ctrl->subsys->subsysnqn);
+?????? strlcpy(id->subnqn, ctrl->subsys->subsysnqn, sizeof(id->subnqn));
?
???????? /* Max command capsule size is sqe + single page of in-capsule data */
???????? id->ioccsz = cpu_to_le32((sizeof(struct nvme_command) +
diff --git a/drivers/nvme/target/discovery.c b/drivers/nvme/target/discovery.c
index 08e7e284655c..ff52ab84d0e3 100644
--- a/drivers/nvme/target/discovery.c
+++ b/drivers/nvme/target/discovery.c
@@ -174,7 +174,7 @@ static void nvmet_execute_identify_disc_ctrl(struct nvmet_req *req)
???????? if (req->port->inline_data_size)
???????????????? id->sgls |= cpu_to_le32(1 << 20);
?
-?????? strcpy(id->subnqn, ctrl->subsys->subsysnqn);
+?????? strlcpy(id->subnqn, ctrl->subsys->subsysnqn, sizeof(id->subnqn));
?
???????? status = nvmet_copy_to_sgl(req, 0, id, sizeof(*id));
?
-- 
2.19.0.605.g01d371f741-goog


_______________________________________________
Linux-nvme mailing list
Linux-nvme at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
    

[PATCH 14/16] nvmet: Avoid integer overflow in the discard code

[Bart Van Assche]

Although I'm not sure whether it is a good idea to support large discard
commands, I think integer overflow for discard ranges larger than 4 GB
should be avoided. This patch avoids that smatch reports the following:

drivers/nvme/target/io-cmd-file.c:249:1 nvmet_file_execute_discard() warn: should '((range.nlb)) << req->ns->blksize_shift' be a 64 bit type?

Fixes: d5eff33ee6f8 ("nvmet: add simple file backed ns support")
Cc: Chaitanya Kulkarni <chaitanya.kulkarni at wdc.com>
Signed-off-by: Bart Van Assche <bvanassche at acm.org>
---
 drivers/nvme/target/io-cmd-file.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/target/io-cmd-file.c b/drivers/nvme/target/io-cmd-file.c
index 81a9dc5290a8..39d972e2595f 100644
--- a/drivers/nvme/target/io-cmd-file.c
+++ b/drivers/nvme/target/io-cmd-file.c
@@ -246,7 +246,8 @@ static void nvmet_file_execute_discard(struct nvmet_req *req)
 			break;
 
 		offset = le64_to_cpu(range.slba) << req->ns->blksize_shift;
-		len = le32_to_cpu(range.nlb) << req->ns->blksize_shift;
+		len = le32_to_cpu(range.nlb);
+		len <<= req->ns->blksize_shift;
 		if (offset + len > req->ns->size) {
 			ret = NVME_SC_LBA_RANGE | NVME_SC_DNR;
 			break;
-- 
2.19.0.605.g01d371f741-goog

[PATCH 14/16] nvmet: Avoid integer overflow in the discard code

[Chaitanya Kulkarni]

Looks good.

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni at wdc.com>

From: Bart Van Assche <bvanassche@acm.org>
Sent: Monday, October 8, 2018 2:28 PM
To: Keith Busch
Cc: Christoph Hellwig; Sagi Grimberg; linux-nvme at lists.infradead.org; Bart Van Assche; Chaitanya Kulkarni
Subject: [PATCH 14/16] nvmet: Avoid integer overflow in the discard code
? 
 
Although I'm not sure whether it is a good idea to support large discard
commands, I think integer overflow for discard ranges larger than 4 GB
should be avoided. This patch avoids that smatch reports the following:

drivers/nvme/target/io-cmd-file.c:249:1 nvmet_file_execute_discard() warn: should '((range.nlb)) << req->ns->blksize_shift' be a 64 bit type?

Fixes: d5eff33ee6f8 ("nvmet: add simple file backed ns support")
Cc: Chaitanya Kulkarni <chaitanya.kulkarni at wdc.com>
Signed-off-by: Bart Van Assche <bvanassche at acm.org>
---
?drivers/nvme/target/io-cmd-file.c | 3 ++-
?1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/target/io-cmd-file.c b/drivers/nvme/target/io-cmd-file.c
index 81a9dc5290a8..39d972e2595f 100644
--- a/drivers/nvme/target/io-cmd-file.c
+++ b/drivers/nvme/target/io-cmd-file.c
@@ -246,7 +246,8 @@ static void nvmet_file_execute_discard(struct nvmet_req *req)
???????????????????????? break;
?
???????????????? offset = le64_to_cpu(range.slba) << req->ns->blksize_shift;
-?????????????? len = le32_to_cpu(range.nlb) << req->ns->blksize_shift;
+?????????????? len = le32_to_cpu(range.nlb);
+?????????????? len <<= req->ns->blksize_shift;
???????????????? if (offset + len > req->ns->size) {
???????????????????????? ret = NVME_SC_LBA_RANGE | NVME_SC_DNR;
???????????????????????? break;
-- 
2.19.0.605.g01d371f741-goog

    

[PATCH 15/16] nvmet-rdma: Declare local symbols static

[Bart Van Assche]

This patch avoids that sparse complains about missing declarations.

Signed-off-by: Bart Van Assche <bvanassche at acm.org>
---
 drivers/nvme/target/rdma.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvme/target/rdma.c b/drivers/nvme/target/rdma.c
index 5becca88ccbe..bd265aceb90c 100644
--- a/drivers/nvme/target/rdma.c
+++ b/drivers/nvme/target/rdma.c
@@ -122,7 +122,7 @@ struct nvmet_rdma_device {
 	int			inline_page_count;
 };
 
-struct workqueue_struct *nvmet_rdma_delete_wq;
+static struct workqueue_struct *nvmet_rdma_delete_wq;
 static bool nvmet_rdma_use_srq;
 module_param_named(use_srq, nvmet_rdma_use_srq, bool, 0444);
 MODULE_PARM_DESC(use_srq, "Use shared receive queue.");
-- 
2.19.0.605.g01d371f741-goog

[PATCH 15/16] nvmet-rdma: Declare local symbols static

[Chaitanya Kulkarni]

 Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni at wdc.com>





From: Linux-nvme <linux-nvme-bounces@lists.infradead.org> on behalf of Bart Van Assche <bvanassche@acm.org>
Sent: Monday, October 8, 2018 2:28 PM
To: Keith Busch
Cc: Bart Van Assche; Christoph Hellwig; linux-nvme at lists.infradead.org; Sagi Grimberg
Subject: [PATCH 15/16] nvmet-rdma: Declare local symbols static
? 

This patch avoids that sparse complains about missing declarations.

Signed-off-by: Bart Van Assche <bvanassche at acm.org>
---
?drivers/nvme/target/rdma.c | 2 +-
?1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvme/target/rdma.c b/drivers/nvme/target/rdma.c
index 5becca88ccbe..bd265aceb90c 100644
--- a/drivers/nvme/target/rdma.c
+++ b/drivers/nvme/target/rdma.c
@@ -122,7 +122,7 @@ struct nvmet_rdma_device {
???????? int???????????????????? inline_page_count;
?};
?
-struct workqueue_struct *nvmet_rdma_delete_wq;
+static struct workqueue_struct *nvmet_rdma_delete_wq;
?static bool nvmet_rdma_use_srq;
?module_param_named(use_srq, nvmet_rdma_use_srq, bool, 0444);
?MODULE_PARM_DESC(use_srq, "Use shared receive queue.");
-- 
2.19.0.605.g01d371f741-goog


_______________________________________________
Linux-nvme mailing list
Linux-nvme at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
    

[PATCH 16/16] nvmet-rdma: Check for timeout in nvme_rdma_wait_for_cm()

[Bart Van Assche]

Check whether queue->cm_error holds a value before reading it. This patch
addresses Coverity ID 1373774: unchecked return value.

Signed-off-by: Bart Van Assche <bvanassche at acm.org>
---
 drivers/nvme/host/rdma.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index dc042017c293..e7be903041a8 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -233,8 +233,15 @@ static void nvme_rdma_qp_event(struct ib_event *event, void *context)
 
 static int nvme_rdma_wait_for_cm(struct nvme_rdma_queue *queue)
 {
-	wait_for_completion_interruptible_timeout(&queue->cm_done,
+	int ret;
+
+	ret = wait_for_completion_interruptible_timeout(&queue->cm_done,
 			msecs_to_jiffies(NVME_RDMA_CONNECT_TIMEOUT_MS) + 1);
+	if (ret < 0)
+		return ret;
+	if (ret == 0)
+		return -ETIMEDOUT;
+	WARN_ON_ONCE(queue->cm_error > 0);
 	return queue->cm_error;
 }
 
-- 
2.19.0.605.g01d371f741-goog

[PATCH 00/16] Fixes for issues detected by static analyzers

[Christoph Hellwig]

On Mon, Oct 08, 2018@02:28:38PM -0700, Bart Van Assche wrote:
> Hi Keith, Christoph and Sagi,
> 
> This patch series addresses several issues reported by static analyzers like
> sparse, smatch and Coverity. Please consider these patches for Linux kernel
> v4.20.

Thanks Bart!

I've applied 1,3,5,11 and 13-16 for now.

This excludes the FC patches, for which I want to wait for feedback
from James, the integrity one that needs chnanges as noted by Keith,
the patch earlier submitted by Chaitanya, and the nvme_init_identify
that really looks odd - we have plenty of calls like that in the
kernel, so I have to disagree with that Coverity warning.

[PATCH 00/16] Fixes for issues detected by static analyzers

[Christoph Hellwig]

Thanks,

applied 6-9 as well now.