[PATCH 1/1] nvme-pci: check that sqid match in nvme cqe

[PATCH 1/1] nvme-pci: check that sqid match in nvme cqe

[Max Gurtovoy]

This patch protects against faulty drives (e.g: a drive might send
the completion on a wrong msix).

Signed-off-by: Max Gurtovoy <maxg at mellanox.com>
---
 drivers/nvme/host/pci.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 5a0bf6a..ab7ff34 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -983,6 +983,13 @@ static inline void nvme_handle_cqe(struct nvme_queue *nvmeq, u16 idx)
 	volatile struct nvme_completion *cqe = &nvmeq->cqes[idx];
 	struct request *req;
 
+	if (unlikely(le16_to_cpu(cqe->sq_id) != nvmeq->qid)) {
+		dev_warn(nvmeq->dev->ctrl.device,
+			 "got completion on sqid %d instead of sqid %d\n",
+			 nvmeq->qid, le16_to_cpu(cqe->sq_id));
+		return;
+	}
+
 	if (unlikely(cqe->command_id >= nvmeq->q_depth)) {
 		dev_warn(nvmeq->dev->ctrl.device,
 			"invalid id %d completed on queue %d\n",
-- 
1.8.3.1

[PATCH 1/1] nvme-pci: check that sqid match in nvme cqe

[Christoph Hellwig]

On Tue, Jan 01, 2019@03:37:18PM +0200, Max Gurtovoy wrote:
> This patch protects against faulty drives (e.g: a drive might send
> the completion on a wrong msix).

Where/how do you see this issue?

[PATCH 1/1] nvme-pci: check that sqid match in nvme cqe

[Max Gurtovoy]

On 1/4/2019 8:13 PM, Christoph Hellwig wrote:
> On Tue, Jan 01, 2019@03:37:18PM +0200, Max Gurtovoy wrote:
>> This patch protects against faulty drives (e.g: a drive might send
>> the completion on a wrong msix).
> Where/how do you see this issue?

maybe it wasn't a good example but let's say I can change 1 line in QEMU 
code (@hw/block/nvme.c) to trigger this issue.

if I'll change

req->cqe.sq_id = cpu_to_le16(sq->sqid);

to be

req->cqe.sq_id = 0;

we'll always get wrong sq_id except of admin queue commands.

This patch tries to protect against HW/FW bugs and we need to decide if 
it's in of our driver interest to do so.

if not, let's abandon this commit.

[PATCH 1/1] nvme-pci: check that sqid match in nvme cqe

[Christoph Hellwig]

On Sun, Jan 06, 2019@02:21:53PM +0200, Max Gurtovoy wrote:
> maybe it wasn't a good example but let's say I can change 1 line in QEMU 
> code (@hw/block/nvme.c) to trigger this issue.
>
> if I'll change
>
> req->cqe.sq_id = cpu_to_le16(sq->sqid);
>
> to be
>
> req->cqe.sq_id = 0;
>
> we'll always get wrong sq_id except of admin queue commands.
>
> This patch tries to protect against HW/FW bugs and we need to decide if 
> it's in of our driver interest to do so.
>
> if not, let's abandon this commit.

Yes, but hardware that gets this wrong will just blow up and timeout
anyway.  I'm not sure this case is worth adding a special detetion
in the slow path.

[PATCH 1/1] nvme-pci: check that sqid match in nvme cqe

[Christoph Hellwig]

On Wed, Jan 09, 2019@07:35:21PM +0100, Christoph Hellwig wrote:
> Yes, but hardware that gets this wrong will just blow up and timeout
> anyway.  I'm not sure this case is worth adding a special detetion
> in the slow path.

s/slow/fast/