nvme-pci: fix rety handling of host memory buffer allocation

PATCH New Akinobu Mita 2 participants 7 messages Sep 4, 2017 – Sep 5, 2017 NVMe

Share & tools

Akinobu Mita

author

2017-09-04 15:23 UTC #0 lore raw

cover nvme-pci: fix rety handling of host memory buffer allocation

I found a problem in rety handling of host memory buffer allocation when using v4.13 kernel with Toshiba NVMe device which supports HMB.

Akinobu Mita (2):
nvme-pci: avoid arrary index out of bounds in retry HMB allocation
nvme-pci: use appropriate initial chunk size for HMB allocation

 drivers/nvme/host/pci.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

Cc Keith Busch <keith.busch at intel.com>

Cc Jens Axboe <axboe at fb.com>

Cc Christoph Hellwig <hch at lst.de>

Cc Sagi Grimberg <sagi at grimberg.me>

Signature · 1 line

2.7.4

Akinobu Mita

author

2017-09-04 15:23 UTC #1 lore raw

PATCH 1/2 nvme-pci: avoid arrary index out of bounds in retry HMB allocation

The array index which is used for addressing each chunk of host memory buffer is decreased to -1 in the middle of retrying HMB allocation with a smaller chunk size. This results memory corruptions and the head of host memory buffer descriptor entry is lost.

This fixes it by resetting the index before restarting HMB allocation. And this also removes unnecessary conditional statements for retrying decision.

Fixes 87ad72a59a38 ("nvme-pci: implement host memory buffer support")

Cc Keith Busch <keith.busch at intel.com>

Cc Jens Axboe <axboe at fb.com>

Cc Christoph Hellwig <hch at lst.de>

Cc Sagi Grimberg <sagi at grimberg.me>

Signed-off-by Akinobu Mita <akinobu.mita at gmail.com>

---
 drivers/nvme/host/pci.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

drivers/nvme/host/pci.c +6 -4

index ea892e7..7e71cc9 100644
@@ -1614,13 +1614,15 @@ static int nvme_alloc_host_mem(struct nvme_dev *dev, u64 min, u64 preferred)
 	struct nvme_host_mem_buf_desc *descs;
 	u32 chunk_size, max_entries, len;
 	dma_addr_t descs_dma;
-	int i = 0;
+	int i;
 	void **bufs;
-	u64 size = 0, tmp;
+	u64 size, tmp;
 
 	/* start big and work our way down */
 	chunk_size = min(preferred, (u64)PAGE_SIZE << MAX_ORDER);
 retry:
+	i = 0;
+	size = 0;
 	tmp = (preferred + chunk_size - 1);
 	do_div(tmp, chunk_size);
 	max_entries = tmp;

@@ -1633,7 +1635,7 @@ static int nvme_alloc_host_mem(struct nvme_dev *dev, u64 min, u64 preferred)
 	if (!bufs)
 		goto out_free_descs;
 
-	for (size = 0; size < preferred; size += len) {
+	for (; size < preferred; size += len) {
 		dma_addr_t dma_addr;
 
 		len = min_t(u64, chunk_size, preferred - size);

@@ -1677,7 +1679,7 @@ static int nvme_alloc_host_mem(struct nvme_dev *dev, u64 min, u64 preferred)
 			descs_dma);
 out:
 	/* try a smaller chunk size if we failed early */
-	if (chunk_size >= PAGE_SIZE * 2 && (i == 0 || size < min)) {
+	if (chunk_size >= PAGE_SIZE * 2) {
 		chunk_size /= 2;
 		goto retry;
 	}

Signature · 1 line

2.7.4

Christoph Hellwig

maintainer

2017-09-04 20:03 UTC #2 lore raw

PATCH 1/2 nvme-pci: avoid arrary index out of bounds in retry HMB allocation

Hmm. I think I did a clear beginners mistake in that code in mixing up two loops. Can you try the version below that just adds a new function to separate the loops (and also gets rid of the weird early fail heuristic):

drivers/nvme/host/pci.c +30 ~~-18~~

index 74a124a06264..324b995a46ed 100644
@@ -1614,17 +1614,15 @@ static void nvme_free_host_mem(struct nvme_dev *dev)
 	dev->host_mem_descs = NULL;
 }
 
-static int nvme_alloc_host_mem(struct nvme_dev *dev, u64 min, u64 preferred)
+static int __nvme_alloc_host_mem(struct nvme_dev *dev, u64 preferred,
+		u32 chunk_size)
 {
 	struct nvme_host_mem_buf_desc *descs;
-	u32 chunk_size, max_entries, len;
+	u32 max_entries, len;
 	int i = 0;
 	void **bufs;
 	u64 size = 0, tmp;
 
-	/* start big and work our way down */
-	chunk_size = min(preferred, (u64)PAGE_SIZE << MAX_ORDER);
-retry:
 	tmp = (preferred + chunk_size - 1);
 	do_div(tmp, chunk_size);
 	max_entries = tmp;

@@ -1650,15 +1648,9 @@ static int nvme_alloc_host_mem(struct nvme_dev *dev, u64 min, u64 preferred)
 		i++;
 	}
 
-	if (!size || (min && size < min)) {
-		dev_warn(dev->ctrl.device,
-			"failed to allocate host memory buffer.\n");
+	if (!size)
 		goto out_free_bufs;
-	}
 
-	dev_info(dev->ctrl.device,
-		"allocated %lld MiB host memory buffer.\n",
-		size >> ilog2(SZ_1M));
 	dev->nr_host_mem_descs = i;
 	dev->host_mem_size = size;
 	dev->host_mem_descs = descs;

@@ -1677,15 +1669,28 @@ static int nvme_alloc_host_mem(struct nvme_dev *dev, u64 min, u64 preferred)
 out_free_descs:
 	kfree(descs);
 out:
-	/* try a smaller chunk size if we failed early */
-	if (chunk_size >= PAGE_SIZE * 2 && (i == 0 || size < min)) {
-		chunk_size /= 2;
-		goto retry;
-	}
 	dev->host_mem_descs = NULL;
 	return -ENOMEM;
 }
 
+static int nvme_alloc_host_mem(struct nvme_dev *dev, u64 min, u64 preferred)
+{
+	u32 chunk_size;
+
+	/* start big and work our way down */
+	for (chunk_size = min_t(u64, preferred, PAGE_SIZE << MAX_ORDER);
+	     chunk_size >= PAGE_SIZE * 2;
+	     chunk_size /= 2) {
+		if (!__nvme_alloc_host_mem(dev, preferred, chunk_size)) {
+			if (!min || dev->host_mem_size >= min)
+				return 0;
+			nvme_free_host_mem(dev);
+		}
+	}
+
+	return -ENOMEM;
+}
+
 static void nvme_setup_host_mem(struct nvme_dev *dev)
 {
 	u64 max = (u64)max_host_mem_size_mb * SZ_1M;

@@ -1713,8 +1718,15 @@ static void nvme_setup_host_mem(struct nvme_dev *dev)
 	}
 
 	if (!dev->host_mem_descs) {
-		if (nvme_alloc_host_mem(dev, min, preferred))
+		if (nvme_alloc_host_mem(dev, min, preferred)) {
+			dev_warn(dev->ctrl.device,
+				"failed to allocate host memory buffer.\n");
 			return;
+		}
+
+		dev_info(dev->ctrl.device,
+			"allocated %lld MiB host memory buffer.\n",
+			dev->host_mem_size >> ilog2(SZ_1M));
 	}
 
 	if (nvme_set_host_mem(dev, enable_bits))

Akinobu Mita

author

2017-09-05 12:56 UTC #3 lore raw

PATCH 1/2 nvme-pci: avoid arrary index out of bounds in retry HMB allocation

2017-09-05 5:03 GMT+09:00 Christoph Hellwig <hch at lst.de>:

Quoted 4 lines

Hmm. I think I did a clear beginners mistake in that code in mixing up two loops. Can you try the version below that just adds a new function to separate the loops (and also gets rid of the weird early fail heuristic):

I tested the patch and it works fine. (this patch isn't applied cleanly to v4.13, but the conflict was resolved easily)

Akinobu Mita

author

2017-09-04 15:23 UTC #4 lore raw

PATCH 2/2 nvme-pci: use appropriate initial chunk size for HMB allocation

In reply to Akinobu Mita

The initial chunk size for host memory buffer allocation is currently PAGE_SIZE << MAX_ORDER. MAX_ORDER order allocation is usually failed without CONFIG_DMA_CMA. So the HMB allocation is retried with chunk size PAGE_SIZE << (MAX_ORDER - 1) in general, but there is no problem if the retry allocation works correctly.

This change just avoids the retry and "failed to allocate host memory buffer." warning in the most common case.

Cc Keith Busch <keith.busch at intel.com>

Cc Jens Axboe <axboe at fb.com>

Cc Christoph Hellwig <hch at lst.de>

Cc Sagi Grimberg <sagi at grimberg.me>

Signed-off-by Akinobu Mita <akinobu.mita at gmail.com>

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

drivers/nvme/host/pci.c +1 -1

index 7e71cc9..49e2e7f 100644
@@ -1619,7 +1619,7 @@ static int nvme_alloc_host_mem(struct nvme_dev *dev, u64 min, u64 preferred)
 	u64 size, tmp;
 
 	/* start big and work our way down */
-	chunk_size = min(preferred, (u64)PAGE_SIZE << MAX_ORDER);
+	chunk_size = min(preferred, (u64)PAGE_SIZE * MAX_ORDER_NR_PAGES);
 retry:
 	i = 0;
 	size = 0;

Signature · 1 line

2.7.4

Christoph Hellwig

maintainer

2017-09-04 20:05 UTC #5 lore raw

PATCH 2/2 nvme-pci: use appropriate initial chunk size for HMB allocation

Looks fine,

Reviewed-by Christoph Hellwig <hch at lst.de>

(It might need some context changes depending on the fate of the previous patch, but I'll resend it together with other HMB bits if needed)

Akinobu Mita

author

2017-09-05 12:58 UTC #6 lore raw

PATCH 2/2 nvme-pci: use appropriate initial chunk size for HMB allocation

2017-09-05 5:05 GMT+09:00 Christoph Hellwig <hch at lst.de>:

Quoted 7 lines

Looks fine,
Reviewed-by Christoph Hellwig <hch at lst.de>
(It might need some context changes depending on the fate of the previous patch, but I'll resend it together with other HMB bits if needed)

Sounds good. Please send it together.