Skip to contentSkip to conversation

nvme: don't rely on the changed namespace list log

PATCH New Christoph Hellwig 2 participants 3 messages Jun 6, 2018 – Jun 11, 2018 NVMe

lore
Share & tools
lore

Christoph Hellwig

author maintainer
#0 lore raw

PATCH 1/1 nvme: don't rely on the changed namespace list log

Don't optimize our namespace rescan based on the changed namespace list log page as userspace might have changed the content through reading it.

Suggested-by Keith Busch <keith.busch at linux.intel.com>
Signed-off-by Christoph Hellwig <hch at lst.de>
---
 drivers/nvme/host/core.c | 36 +++++++++++-------------------------
 1 file changed, 11 insertions(+), 25 deletions(-)
drivers/nvme/host/core.c +11 -25
index 3250f8d72456..62d70419e12a 100644
@@ -3197,40 +3197,28 @@ static void nvme_scan_ns_sequential(struct nvme_ctrl *ctrl, unsigned nn)
 	nvme_remove_invalid_namespaces(ctrl, nn);
 }
 
-static bool nvme_scan_changed_ns_log(struct nvme_ctrl *ctrl)
+static void nvme_clear_changed_ns_log(struct nvme_ctrl *ctrl)
 {
 	size_t log_size = NVME_MAX_CHANGED_NAMESPACES * sizeof(__le32);
 	__le32 *log;
-	int error, i;
-	bool ret = false;
+	int error;
 
 	log = kzalloc(log_size, GFP_KERNEL);
 	if (!log)
-		return false;
+		return;
 
+	/*
+	 * We need to read the log to clear the AEN, but we don't want to rely
+	 * on it for the changed namespace information as userspace could have
+	 * raced with us in reading the log page, which could cause us to miss
+	 * updates.
+	 */
 	error = nvme_get_log(ctrl, NVME_LOG_CHANGED_NS, log, log_size);
-	if (error) {
+	if (error)
 		dev_warn(ctrl->device,
 			"reading changed ns log failed: %d\n", error);
-		goto out_free_log;
-	}
-
-	if (log[0] == cpu_to_le32(0xffffffff))
-		goto out_free_log;
 
-	for (i = 0; i < NVME_MAX_CHANGED_NAMESPACES; i++) {
-		u32 nsid = le32_to_cpu(log[i]);
-
-		if (nsid == 0)
-			break;
-		dev_info(ctrl->device, "rescanning namespace %d.\n", nsid);
-		nvme_validate_ns(ctrl, nsid);
-	}
-	ret = true;
-
-out_free_log:
 	kfree(log);
-	return ret;
 }
 
 static void nvme_scan_work(struct work_struct *work)
@@ -3246,9 +3234,8 @@ static void nvme_scan_work(struct work_struct *work)
 	WARN_ON_ONCE(!ctrl->tagset);
 
 	if (test_and_clear_bit(EVENT_NS_CHANGED, &ctrl->events)) {
-		if (nvme_scan_changed_ns_log(ctrl))
-			goto out_sort_namespaces;
 		dev_info(ctrl->device, "rescanning namespaces.\n");
+		nvme_clear_changed_ns_log(ctrl);
 	}
 
 	if (nvme_identify_ctrl(ctrl, &id))
@@ -3263,7 +3250,6 @@ static void nvme_scan_work(struct work_struct *work)
 	nvme_scan_ns_sequential(ctrl, nn);
 out_free_id:
 	kfree(id);
-out_sort_namespaces:
 	down_write(&ctrl->namespaces_rwsem);
 	list_sort(NULL, &ctrl->namespaces, ns_cmp);
 	up_write(&ctrl->namespaces_rwsem);
Signature · 1 line
2.14.2
author maintainer
#1 lore raw

PATCH 1/1 nvme: don't rely on the changed namespace list log

Keith, you asked for this so it would be good to get a review.

That being said I'm still not sure this patch is the right thing. Compared to userspace deleting I/O queues reading the changed ns log seems pretty harmless, especially given that we can easily force a manual rescan.

#2 lore raw

PATCH 1/1 nvme: don't rely on the changed namespace list log

Thanks, this looks fine. The main thing I want to prevent is reading from a namespace with the wrong block size, which may corrupt memory. I don't think it's possible to fix every scenario, but this patch at least narrows it down even if the scenario this is guarding against is pretty unlikely.

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