nvme: don't rely on the changed namespace list log
Christoph Hellwig
maintainerPATCH 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.
--- drivers/nvme/host/core.c | 36 +++++++++++------------------------- 1 file changed, 11 insertions(+), 25 deletions(-)
drivers/nvme/host/core.c +11 -25
@@ -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
Christoph Hellwig
maintainerPATCH 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.
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.