Fixes for issues detected by static analyzers

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

This patch avoids that sparse complains about missing declarations.

---
 drivers/nvme/host/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 2db33a752e2b..63932dea74a1 100644
@@ -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

Looks good.

From: Linux-nvme <[email protected]> on behalf of Bart Van Assche <[email protected]>
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.

---
?drivers/nvme/host/core.c | 2 +-
?1 file changed, 1 insertion(+), 1 deletion(-)

index 2db33a752e2b..63932dea74a1 100644
@@ -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

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

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.

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

index 63932dea74a1..04138223fad6 100644
@@ -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

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.

---
 drivers/nvme/host/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 04138223fad6..3746fe49c382 100644
@@ -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

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

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.

---
 drivers/nvme/host/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 3746fe49c382..2ce3e9d15618 100644
@@ -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

Looks good.

From: Linux-nvme <[email protected]> on behalf of Bart Van Assche <[email protected]>
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.

---
?drivers/nvme/host/core.c | 2 +-
?1 file changed, 1 insertion(+), 1 deletion(-)

index 3746fe49c382..2ce3e9d15618 100644
@@ -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

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.

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

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

---
 drivers/nvme/host/pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index d668682f91df..450481c2fd17 100644
@@ -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

Looks fine.

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

---
 drivers/nvme/host/fc.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

index 9d201b35397d..d838987fffe1 100644
@@ -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

Looks good.

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

---
 drivers/nvme/host/fc.c | 29 ++++++++++++++++++-----------
 1 file changed, 18 insertions(+), 11 deletions(-)

index d838987fffe1..fdadc9464f6f 100644
@@ -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

Looks good.

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.

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

index fdadc9464f6f..e52b9d3c0bd6 100644
@@ -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

Looks good.

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

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

index ef286b72d958..409081a03b24 100644
@@ -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

Looks good.

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)

      ^

---
 drivers/nvme/target/Makefile | 2 ++
 1 file changed, 2 insertions(+)

index 8118c93391c6..ed578a73a376 100644
@@ -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

Looks good.

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

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)

---
 drivers/nvme/target/core.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

index b5ec96abd048..0acdff9e6842 100644
@@ -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

This patch avoids that smatch reports the following warning:

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

---
 drivers/nvme/target/discovery.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

index eae29f493a07..08e7e284655c 100644
@@ -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

I have already posted this fix.

Please see :-

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

From: Linux-nvme <[email protected]> on behalf of Bart Van Assche <[email protected]>
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.

---
?drivers/nvme/target/discovery.c | 5 +----
?1 file changed, 1 insertion(+), 4 deletions(-)

index eae29f493a07..08e7e284655c 100644
@@ -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

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

Bart.

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".

---
 drivers/nvme/target/admin-cmd.c | 2 +-
 drivers/nvme/target/discovery.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

index 7a45f4477679..1179f6314323 100644
@@ -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) +

index 08e7e284655c..ff52ab84d0e3 100644
@@ -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

Looks good.

From: Linux-nvme <[email protected]> on behalf of Bart Van Assche <[email protected]>
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".

---
?drivers/nvme/target/admin-cmd.c | 2 +-
?drivers/nvme/target/discovery.c | 2 +-
?2 files changed, 2 insertions(+), 2 deletions(-)

index 7a45f4477679..1179f6314323 100644
@@ -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) +

index 08e7e284655c..ff52ab84d0e3 100644
@@ -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

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?

---
 drivers/nvme/target/io-cmd-file.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

index 81a9dc5290a8..39d972e2595f 100644
@@ -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

Looks good.

From: Bart Van Assche <[email protected]>
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?

---
?drivers/nvme/target/io-cmd-file.c | 3 ++-
?1 file changed, 2 insertions(+), 1 deletion(-)

index 81a9dc5290a8..39d972e2595f 100644
@@ -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

This patch avoids that sparse complains about missing declarations.

---
 drivers/nvme/target/rdma.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 5becca88ccbe..bd265aceb90c 100644
@@ -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

From: Linux-nvme <[email protected]> on behalf of Bart Van Assche <[email protected]>
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.

---
?drivers/nvme/target/rdma.c | 2 +-
?1 file changed, 1 insertion(+), 1 deletion(-)

index 5becca88ccbe..bd265aceb90c 100644
@@ -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

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

---
 drivers/nvme/host/rdma.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

index dc042017c293..e7be903041a8 100644
@@ -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

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.

Thanks,

applied 6-9 as well now.