[PATCH 00/23] nvmet: add context annotation for params in struct nvmet_ns

[PATCH 00/23] nvmet: add context annotation for params in struct nvmet_ns

[Nilay Shroff]

Hi,

This series adds Clang context annotations to fields in struct nvmet_ns
that are protected by nvmet_subsys::lock.

The goal is to incrementally introduce context annotations throughout
the NVMe target code. Covering the entire nvmet subsystem in a single
series would result in a very large patchset with dozens of patches,
making focused review difficult. To keep the changes manageable, I have
split the work into multiple series, each focusing on a specific subset
of context annotations.

This series focuses on fields in struct nvmet_ns protected by
nvmet_subsys::lock. Subsequent series will expand the scope to other
structures and locks.

The patches are organized so that each field is handled incrementally:
first, a field is annotated with __guarded_by(&subsys->lock), and then
any access violations reported by the Clang context analyzer are
addressed in follow-up patches. Not every field annotation necessarily
triggers analyzer warnings. In such cases, the series simply moves on
to the next field.

This approach keeps individual patches focused and makes it easier to
review the annotations and the corresponding locking changes
independently.

As always, review feedback, comments, and suggestions are very welcome!

Nilay Shroff (23):
  nvmet: add context annotation for nvmet_ns::bdev
  nvmet: fix nvmet_ns::bdev access using nvmet_subsys::lock
  nvmet: add context annotation for nvmet_ns::file
  nvmet: add context annotation for nvmet_ns::bdev_file
  nvmet: add context annotation for nvmet_ns::readonly
  nvmet: add context annotation for nvmet_ns::nsid
  nvmet: fix nvmet_ns::nsid access using nvmet_subsys::lock
  nvmet: add context annotation for nvmet_ns::blksize_shift
  nvmet: add context annotation for nvmet_ns::size
  nvmet: add context annotation for nvmet_ns::nguid
  nvmet: add context annotation for nvmet_ns::uuid
  nvmet: fix nvmet_ns::uuid access using nvmet_subsys::lock
  nvmet: add context annotation for nvmet_ns::buffered_io
  nvmet: fix nvmet_ns::buffered_io access using nvmet_subsys::lock
  nvmet: add context annotation for nvmet_ns::enabled
  nvmet: fix access to nvmet_ns::enable using nvmet_subsys::lock
  nvmet: add context annotation for nvmet_ns::device_path
  nvmet: fix nvmet_ns::device_path access using nvmet_subsys::lock
  nvmet: add context annotation for PCI P2P parameters in struct
    nvmet_ns
  nvmet: fix PCI P2P parameters access using nvmet_subsys::lock
  nvmet: add context annotation for integrity params in struct nvmet_ns
  nvmet: add context annotation for nvmet_ns::csi
  nvmet: add context annotation for nvmet_ns::debugfs_dir

 drivers/nvme/target/admin-cmd.c   | 143 ++++++++++++------
 drivers/nvme/target/configfs.c    |  52 ++++++-
 drivers/nvme/target/core.c        |  66 +++++++--
 drivers/nvme/target/debugfs.h     |   6 +-
 drivers/nvme/target/io-cmd-bdev.c |  30 ++--
 drivers/nvme/target/io-cmd-file.c |  49 +++---
 drivers/nvme/target/nvmet.h       | 237 +++++++++++++++++++-----------
 drivers/nvme/target/rdma.c        |   4 +-
 drivers/nvme/target/trace.h       |   2 +-
 drivers/nvme/target/zns.c         |  46 +++---
 10 files changed, 431 insertions(+), 204 deletions(-)


base-commit: 687f3014afcd5826fa3d8f40b445f647b5a379b6
-- 
2.53.0

[PATCH 01/23] nvmet: add context annotation for nvmet_ns::bdev

[Nilay Shroff]

The nvmet_ns::bdev field is protected by nvmet_subsys::lock. Annotate
it with __guarded_by(&subsys->lock) so that the clang context analyzer
can validate accesses to this field.

Some accesses to nvmet_ns::bdev in the I/O hotpath do not require
holding nvmet_subsys::lock, as the namespace reference guarantees that
the namespace remains valid and the field is not modified while I/O is
in flight. However, the clang context analyzer cannot express this
access pattern directly. So introduce nvmet_ns_bdev() as an inline
helper annotated with __context_unsafe() to allow these accesses
without requiring the subsystem lock. Convert the relevant accesses in
the I/O hotpath and other request-processing paths to use this helper.

Additionally, annotate namespace enable, disable, revalidation and
write protect functions with __must_hold() to make their locking
requirements explicit. This includes the block device and P2P memory
namespace operations.

When accessing the same subsystem lock through different expressions,
such as subsys->lock, ctrl->subsys->lock and ns->subsys->lock the clang
context analyzer cannot infer that they refer to the same lock. Use
__assume_ctx_lock() to explicitly assert the corresponding locking
capability at relevant call sites.

Since nvmet_ns_revalidate() requires callers to hold ns->subsys->lock,
acquire req->ns->subsys->lock before invoking it from
nvmet_execute_identify_ns() and nvmet_execute_identify_ns_zns().

Finally, update nvmet_set_feat_write_protect() to acquire
req->ns->subsys->lock instead of subsys->lock. Both expressions refer
to the same lock, but using the former allows the context analyzer to
correctly validate the locking requirement of
nvmet_write_protect_flush_sync().

Signed-off-by: Nilay Shroff <nilay@linux.ibm.com>
---
 drivers/nvme/target/admin-cmd.c   |  52 ++++++------
 drivers/nvme/target/core.c        |  27 +++++-
 drivers/nvme/target/io-cmd-bdev.c |  22 ++---
 drivers/nvme/target/nvmet.h       | 136 ++++++++++++++++++------------
 drivers/nvme/target/rdma.c        |   2 +-
 drivers/nvme/target/zns.c         |  44 +++++-----
 6 files changed, 170 insertions(+), 113 deletions(-)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 6732d6d078e3..2d4e21b3ee7d 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -259,15 +259,17 @@ static u16 nvmet_get_smart_log_nsid(struct nvmet_req *req,
 		return status;
 
 	/* we don't have the right data for file backed ns */
-	if (!req->ns->bdev)
+	if (!nvmet_ns_bdev(req->ns))
 		return NVME_SC_SUCCESS;
 
-	host_reads = part_stat_read(req->ns->bdev, ios[READ]);
+	host_reads = part_stat_read(nvmet_ns_bdev(req->ns), ios[READ]);
 	data_units_read =
-		DIV_ROUND_UP(part_stat_read(req->ns->bdev, sectors[READ]), 1000);
-	host_writes = part_stat_read(req->ns->bdev, ios[WRITE]);
+		DIV_ROUND_UP(part_stat_read(nvmet_ns_bdev(req->ns),
+				sectors[READ]), 1000);
+	host_writes = part_stat_read(nvmet_ns_bdev(req->ns), ios[WRITE]);
 	data_units_written =
-		DIV_ROUND_UP(part_stat_read(req->ns->bdev, sectors[WRITE]), 1000);
+		DIV_ROUND_UP(part_stat_read(nvmet_ns_bdev(req->ns),
+				sectors[WRITE]), 1000);
 
 	put_unaligned_le64(host_reads, &slog->host_reads[0]);
 	put_unaligned_le64(data_units_read, &slog->data_units_read[0]);
@@ -319,7 +321,7 @@ static void nvmet_execute_get_log_page_rmi(struct nvmet_req *req)
 	if (status)
 		goto out;
 
-	if (!req->ns->bdev || !bdev_rot(req->ns->bdev)) {
+	if (!nvmet_ns_bdev(req->ns) || !bdev_rot(nvmet_ns_bdev(req->ns))) {
 		status = NVME_SC_INVALID_FIELD | NVME_STATUS_DNR;
 		goto out;
 	}
@@ -336,7 +338,7 @@ static void nvmet_execute_get_log_page_rmi(struct nvmet_req *req)
 	}
 
 	log->endgid = req->cmd->get_log_page.lsi;
-	disk = req->ns->bdev->bd_disk;
+	disk = nvmet_ns_bdev(req->ns)->bd_disk;
 	if (disk && disk->ia_ranges)
 		log->numa = cpu_to_le16(disk->ia_ranges->nr_ia_ranges);
 	else
@@ -537,15 +539,17 @@ static void nvmet_execute_get_log_page_endgrp(struct nvmet_req *req)
 		goto out;
 	}
 
-	if (!req->ns->bdev)
+	if (!nvmet_ns_bdev(req->ns))
 		goto copy;
 
-	host_reads = part_stat_read(req->ns->bdev, ios[READ]);
+	host_reads = part_stat_read(nvmet_ns_bdev(req->ns), ios[READ]);
 	data_units_read =
-		DIV_ROUND_UP(part_stat_read(req->ns->bdev, sectors[READ]), 1000);
-	host_writes = part_stat_read(req->ns->bdev, ios[WRITE]);
+		DIV_ROUND_UP(part_stat_read(nvmet_ns_bdev(req->ns),
+				sectors[READ]), 1000);
+	host_writes = part_stat_read(nvmet_ns_bdev(req->ns), ios[WRITE]);
 	data_units_written =
-		DIV_ROUND_UP(part_stat_read(req->ns->bdev, sectors[WRITE]), 1000);
+		DIV_ROUND_UP(part_stat_read(nvmet_ns_bdev(req->ns),
+				sectors[WRITE]), 1000);
 
 	put_unaligned_le64(host_reads, &log->hrc[0]);
 	put_unaligned_le64(data_units_read, &log->dur[0]);
@@ -859,11 +863,10 @@ static void nvmet_execute_identify_ns(struct nvmet_req *req)
 		goto done;
 	}
 
-	if (nvmet_ns_revalidate(req->ns)) {
-		mutex_lock(&req->ns->subsys->lock);
+	mutex_lock(&req->ns->subsys->lock);
+	if (nvmet_ns_revalidate(req->ns))
 		nvmet_ns_changed(req->ns->subsys, req->ns->nsid);
-		mutex_unlock(&req->ns->subsys->lock);
-	}
+	mutex_unlock(&req->ns->subsys->lock);
 
 	/*
 	 * nuse = ncap = nsze isn't always true, but we have no way to find
@@ -880,8 +883,8 @@ static void nvmet_execute_identify_ns(struct nvmet_req *req)
 		break;
 	}
 
-	if (req->ns->bdev)
-		nvmet_bdev_set_limits(req->ns->bdev, id);
+	if (nvmet_ns_bdev(req->ns))
+		nvmet_bdev_set_limits(nvmet_ns_bdev(req->ns), id);
 
 	/*
 	 * We just provide a single LBA format that matches what the
@@ -1095,8 +1098,8 @@ static void nvme_execute_identify_ns_nvm(struct nvmet_req *req)
 		status = NVME_SC_INTERNAL;
 		goto out;
 	}
-	if (req->ns->bdev)
-		nvmet_bdev_set_nvm_limits(req->ns->bdev, id);
+	if (nvmet_ns_bdev(req->ns))
+		nvmet_bdev_set_nvm_limits(nvmet_ns_bdev(req->ns), id);
 	status = nvmet_copy_to_sgl(req, 0, id, sizeof(*id));
 	kfree(id);
 out:
@@ -1123,14 +1126,14 @@ static void nvmet_execute_id_cs_indep(struct nvmet_req *req)
 	id->nmic = NVME_NS_NMIC_SHARED;
 	if (req->ns->readonly)
 		id->nsattr |= NVME_NS_ATTR_RO;
-	if (req->ns->bdev && bdev_rot(req->ns->bdev))
+	if (nvmet_ns_bdev(req->ns) && bdev_rot(nvmet_ns_bdev(req->ns)))
 		id->nsfeat |= NVME_NS_ROTATIONAL;
 	/*
 	 * We need flush command to flush the file's metadata,
 	 * so report supporting vwc if backend is file, even
 	 * though buffered_io is disable.
 	 */
-	if (req->ns->bdev && !bdev_write_cache(req->ns->bdev))
+	if (nvmet_ns_bdev(req->ns) && !bdev_write_cache(nvmet_ns_bdev(req->ns)))
 		id->nsfeat |= NVME_NS_VWC_NOT_PRESENT;
 
 	status = nvmet_copy_to_sgl(req, 0, id, sizeof(*id));
@@ -1216,6 +1219,7 @@ static void nvmet_execute_abort(struct nvmet_req *req)
 }
 
 static u16 nvmet_write_protect_flush_sync(struct nvmet_req *req)
+	__must_hold(&req->ns->subsys->lock)
 {
 	u16 status;
 
@@ -1239,7 +1243,7 @@ static u16 nvmet_set_feat_write_protect(struct nvmet_req *req)
 	if (status)
 		return status;
 
-	mutex_lock(&subsys->lock);
+	mutex_lock(&req->ns->subsys->lock);
 	switch (write_protect) {
 	case NVME_NS_WRITE_PROTECT:
 		req->ns->readonly = true;
@@ -1257,7 +1261,7 @@ static u16 nvmet_set_feat_write_protect(struct nvmet_req *req)
 
 	if (!status)
 		nvmet_ns_changed(subsys, req->ns->nsid);
-	mutex_unlock(&subsys->lock);
+	mutex_unlock(&req->ns->subsys->lock);
 	return status;
 }
 
diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index cc1955bc8c84..c9837263898a 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -484,12 +484,14 @@ void nvmet_put_namespace(struct nvmet_ns *ns)
 }
 
 static void nvmet_ns_dev_disable(struct nvmet_ns *ns)
+	__must_hold(&ns->subsys->lock)
 {
 	nvmet_bdev_ns_disable(ns);
 	nvmet_file_ns_disable(ns);
 }
 
 static int nvmet_p2pmem_ns_enable(struct nvmet_ns *ns)
+	__must_hold(&ns->subsys->lock)
 {
 	int ret;
 	struct pci_dev *p2p_dev;
@@ -535,12 +537,19 @@ static int nvmet_p2pmem_ns_enable(struct nvmet_ns *ns)
 
 static void nvmet_p2pmem_ns_add_p2p(struct nvmet_ctrl *ctrl,
 				    struct nvmet_ns *ns)
+	__must_hold(&ctrl->subsys->lock)
 {
 	struct device *clients[2];
 	struct pci_dev *p2p_dev;
 	int ret;
 
 	lockdep_assert_held(&ctrl->subsys->lock);
+	/*
+	 * Both ctrl->subsys->lock and ns->subsys->lock refers to the same lock.
+	 * However there's no way for us to annotate such locking semantics
+	 * using clang context annotation. So explicitly assert the capability.
+	 */
+	__assume_ctx_lock(&ns->subsys->lock);
 
 	if (!ctrl->p2p_client || !ns->use_p2pmem)
 		return;
@@ -615,8 +624,16 @@ int nvmet_ns_enable(struct nvmet_ns *ns)
 	if (ret)
 		goto out_dev_disable;
 
-	list_for_each_entry(ctrl, &subsys->ctrls, subsys_entry)
+	list_for_each_entry(ctrl, &subsys->ctrls, subsys_entry) {
+		/*
+		 * Both subsys->lock and ctrl->subsys->lock refers to the same
+		 * lock. However there's no way for us to annotate such locking
+		 * semantics using clang context annotation. So explicitly
+		 * assert the capability.
+		 */
+		__assume_ctx_lock(&ctrl->subsys->lock);
 		nvmet_p2pmem_ns_add_p2p(ctrl, ns);
+	}
 
 	if (ns->pr.enable) {
 		ret = nvmet_pr_init_ns(ns);
@@ -1576,6 +1593,7 @@ bool nvmet_host_allowed(struct nvmet_subsys *subsys, const char *hostnqn)
 
 static void nvmet_setup_p2p_ns_map(struct nvmet_ctrl *ctrl,
 		struct device *p2p_client)
+	__must_hold(&ctrl->subsys->lock)
 {
 	struct nvmet_ns *ns;
 	unsigned long idx;
@@ -1733,6 +1751,13 @@ struct nvmet_ctrl *nvmet_alloc_ctrl(struct nvmet_alloc_ctrl_args *args)
 	if (ret)
 		goto init_pr_fail;
 	list_add_tail(&ctrl->subsys_entry, &subsys->ctrls);
+
+	/*
+	 * Both subsys->lock and ctrl->subsys->lock refers to the same lock.
+	 * However there's no way for us to annotate such locking semantics
+	 * using clang context annotation. So explicitly assert the capability.
+	 */
+	__assume_ctx_lock(&ctrl->subsys->lock);
 	nvmet_setup_p2p_ns_map(ctrl, args->p2p_client);
 	nvmet_debugfs_ctrl_setup(ctrl);
 	mutex_unlock(&subsys->lock);
diff --git a/drivers/nvme/target/io-cmd-bdev.c b/drivers/nvme/target/io-cmd-bdev.c
index 72618dced191..7c263cb169a8 100644
--- a/drivers/nvme/target/io-cmd-bdev.c
+++ b/drivers/nvme/target/io-cmd-bdev.c
@@ -73,6 +73,7 @@ void nvmet_bdev_ns_disable(struct nvmet_ns *ns)
 }
 
 static void nvmet_bdev_ns_enable_integrity(struct nvmet_ns *ns)
+	__must_hold(&ns->subsys->lock)
 {
 	struct blk_integrity *bi = bdev_get_integrity(ns->bdev);
 
@@ -206,7 +207,7 @@ static int nvmet_bdev_alloc_bip(struct nvmet_req *req, struct bio *bio,
 	int rc;
 	size_t resid, len;
 
-	bi = bdev_get_integrity(req->ns->bdev);
+	bi = bdev_get_integrity(nvmet_ns_bdev(req->ns));
 	if (unlikely(!bi)) {
 		pr_err("Unable to locate bio_integrity\n");
 		return -ENODEV;
@@ -291,11 +292,11 @@ static void nvmet_bdev_execute_rw(struct nvmet_req *req)
 
 	if (nvmet_use_inline_bvec(req)) {
 		bio = &req->b.inline_bio;
-		bio_init(bio, req->ns->bdev, req->inline_bvec,
+		bio_init(bio, nvmet_ns_bdev(req->ns), req->inline_bvec,
 			 ARRAY_SIZE(req->inline_bvec), opf);
 	} else {
-		bio = bio_alloc(req->ns->bdev, bio_max_segs(sg_cnt), opf,
-				GFP_KERNEL);
+		bio = bio_alloc(nvmet_ns_bdev(req->ns), bio_max_segs(sg_cnt),
+				opf, GFP_KERNEL);
 	}
 	bio->bi_iter.bi_sector = sector;
 	bio->bi_private = req;
@@ -320,8 +321,9 @@ static void nvmet_bdev_execute_rw(struct nvmet_req *req)
 				}
 			}
 
-			bio = bio_alloc(req->ns->bdev, bio_max_segs(sg_cnt),
-					opf, GFP_KERNEL);
+			bio = bio_alloc(nvmet_ns_bdev(req->ns),
+					bio_max_segs(sg_cnt), opf,
+					GFP_KERNEL);
 			bio->bi_iter.bi_sector = sector;
 
 			bio_chain(bio, prev);
@@ -349,7 +351,7 @@ static void nvmet_bdev_execute_flush(struct nvmet_req *req)
 {
 	struct bio *bio = &req->b.inline_bio;
 
-	if (!bdev_write_cache(req->ns->bdev)) {
+	if (!bdev_write_cache(nvmet_ns_bdev(req->ns))) {
 		nvmet_req_complete(req, NVME_SC_SUCCESS);
 		return;
 	}
@@ -357,7 +359,7 @@ static void nvmet_bdev_execute_flush(struct nvmet_req *req)
 	if (!nvmet_check_transfer_len(req, 0))
 		return;
 
-	bio_init(bio, req->ns->bdev, req->inline_bvec,
+	bio_init(bio, nvmet_ns_bdev(req->ns), req->inline_bvec,
 		 ARRAY_SIZE(req->inline_bvec), REQ_OP_WRITE | REQ_PREFLUSH);
 	bio->bi_private = req;
 	bio->bi_end_io = nvmet_bio_done;
@@ -392,7 +394,7 @@ static void nvmet_bdev_execute_discard(struct nvmet_req *req)
 
 		nr_sects = (sector_t)le32_to_cpu(range.nlb) <<
 			   (ns->blksize_shift - 9);
-		__blkdev_issue_discard(ns->bdev,
+		__blkdev_issue_discard(nvmet_ns_bdev(ns),
 				nvmet_lba_to_sect(ns, range.slba), nr_sects,
 				GFP_KERNEL, &bio);
 	}
@@ -442,7 +444,7 @@ static void nvmet_bdev_execute_write_zeroes(struct nvmet_req *req)
 	nr_sector = (((sector_t)le16_to_cpu(write_zeroes->length) + 1) <<
 		(req->ns->blksize_shift - 9));
 
-	ret = __blkdev_issue_zeroout(req->ns->bdev, sector, nr_sector,
+	ret = __blkdev_issue_zeroout(nvmet_ns_bdev(req->ns), sector, nr_sector,
 			GFP_KERNEL, &bio, 0);
 	if (bio) {
 		bio->bi_private = req;
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 2c2460ef5e2b..41912877ade3 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -98,10 +98,60 @@ struct nvmet_pr_per_ctrl_ref {
 	uuid_t			hostid;
 };
 
+struct nvmet_subsys {
+	enum nvme_subsys_type	type;
+
+	struct mutex		lock;
+	struct kref		ref;
+
+	struct xarray		namespaces;
+	unsigned int		nr_namespaces;
+	u32			max_nsid;
+	u16			cntlid_min;
+	u16			cntlid_max;
+
+	struct list_head	ctrls;
+
+	struct list_head	hosts;
+	bool			allow_any_host;
+#ifdef CONFIG_NVME_TARGET_DEBUGFS
+	struct dentry		*debugfs_dir;
+#endif
+	u16			max_qid;
+
+	u64			ver;
+	char			serial[NVMET_SN_MAX_SIZE];
+	bool			subsys_discovered;
+	char			*subsysnqn;
+	bool			pi_support;
+	bool			tbkas;
+
+	struct config_group	group;
+
+	struct config_group	namespaces_group;
+	struct config_group	allowed_hosts_group;
+
+	u16			vendor_id;
+	u16			subsys_vendor_id;
+	char			*model_number;
+	u32			ieee_oui;
+	char			*firmware_rev;
+
+#ifdef CONFIG_NVME_TARGET_PASSTHRU
+	struct nvmet_passthru	*passthru;
+	struct config_group	passthru_group;
+#endif /* CONFIG_NVME_TARGET_PASSTHRU */
+
+#ifdef CONFIG_BLK_DEV_ZONED
+	u8			zasl;
+#endif /* CONFIG_BLK_DEV_ZONED */
+};
+
 struct nvmet_ns {
 	struct percpu_ref	ref;
 	struct file		*bdev_file;
-	struct block_device	*bdev;
+	struct block_device	*bdev
+		__guarded_by(&subsys->lock);
 	struct file		*file;
 	bool			readonly;
 	u32			nsid;
@@ -136,12 +186,29 @@ struct nvmet_ns {
 #endif
 };
 
+/*
+ * The accessors generated by this macro are annotated with __context_unsafe()
+ * because they are intended for use in I/O hotpaths and request-processing
+ * paths where a namespace reference is held. These paths access namespace
+ * fields that are guaranteed not to change while the reference is held and the
+ * I/O or request processing is in progress.
+ */
+#define NVMET_NS_ACCESSOR(_type, _name)					    \
+static inline _type nvmet_ns_##_name(struct nvmet_ns *ns)		    \
+	__context_unsafe(/* access _name after getting ns reference */)	    \
+{									    \
+	return ns->_name;						    \
+}
+
+NVMET_NS_ACCESSOR(struct block_device *, bdev)
+
 static inline struct nvmet_ns *to_nvmet_ns(struct config_item *item)
 {
 	return container_of(to_config_group(item), struct nvmet_ns, group);
 }
 
 static inline struct device *nvmet_ns_dev(struct nvmet_ns *ns)
+	__must_hold(&ns->subsys->lock)
 {
 	return ns->bdev ? disk_to_dev(ns->bdev->bd_disk) : NULL;
 }
@@ -336,55 +403,6 @@ struct nvmet_passthru {
 	unsigned int		clear_ids;
 };
 
-struct nvmet_subsys {
-	enum nvme_subsys_type	type;
-
-	struct mutex		lock;
-	struct kref		ref;
-
-	struct xarray		namespaces;
-	unsigned int		nr_namespaces;
-	u32			max_nsid;
-	u16			cntlid_min;
-	u16			cntlid_max;
-
-	struct list_head	ctrls;
-
-	struct list_head	hosts;
-	bool			allow_any_host;
-#ifdef CONFIG_NVME_TARGET_DEBUGFS
-	struct dentry		*debugfs_dir;
-#endif
-	u16			max_qid;
-
-	u64			ver;
-	char			serial[NVMET_SN_MAX_SIZE];
-	bool			subsys_discovered;
-	char			*subsysnqn;
-	bool			pi_support;
-	bool			tbkas;
-
-	struct config_group	group;
-
-	struct config_group	namespaces_group;
-	struct config_group	allowed_hosts_group;
-
-	u16			vendor_id;
-	u16			subsys_vendor_id;
-	char			*model_number;
-	u32			ieee_oui;
-	char			*firmware_rev;
-
-#ifdef CONFIG_NVME_TARGET_PASSTHRU
-	struct nvmet_passthru	*passthru;
-	struct config_group	passthru_group;
-#endif /* CONFIG_NVME_TARGET_PASSTHRU */
-
-#ifdef CONFIG_BLK_DEV_ZONED
-	u8			zasl;
-#endif /* CONFIG_BLK_DEV_ZONED */
-};
-
 static inline struct nvmet_subsys *to_subsys(struct config_item *item)
 {
 	return container_of(to_config_group(item), struct nvmet_subsys, group);
@@ -737,19 +755,25 @@ extern struct rw_semaphore nvmet_ana_sem;
 
 bool nvmet_host_allowed(struct nvmet_subsys *subsys, const char *hostnqn);
 
-int nvmet_bdev_ns_enable(struct nvmet_ns *ns);
+int nvmet_bdev_ns_enable(struct nvmet_ns *ns)
+	__must_hold(&ns->subsys->lock);
 int nvmet_file_ns_enable(struct nvmet_ns *ns);
-void nvmet_bdev_ns_disable(struct nvmet_ns *ns);
+void nvmet_bdev_ns_disable(struct nvmet_ns *ns)
+	__must_hold(&ns->subsys->lock);
 void nvmet_file_ns_disable(struct nvmet_ns *ns);
-u16 nvmet_bdev_flush(struct nvmet_req *req);
+u16 nvmet_bdev_flush(struct nvmet_req *req)
+	__must_hold(&req->ns->subsys->lock);
 u16 nvmet_file_flush(struct nvmet_req *req);
 void nvmet_ns_changed(struct nvmet_subsys *subsys, u32 nsid);
-void nvmet_bdev_ns_revalidate(struct nvmet_ns *ns);
+void nvmet_bdev_ns_revalidate(struct nvmet_ns *ns)
+	__must_hold(&ns->subsys->lock);
 void nvmet_file_ns_revalidate(struct nvmet_ns *ns);
-bool nvmet_ns_revalidate(struct nvmet_ns *ns);
+bool nvmet_ns_revalidate(struct nvmet_ns *ns)
+	__must_hold(&ns->subsys->lock);
 u16 blk_to_nvme_status(struct nvmet_req *req, blk_status_t blk_sts);
 
-bool nvmet_bdev_zns_enable(struct nvmet_ns *ns);
+bool nvmet_bdev_zns_enable(struct nvmet_ns *ns)
+	__must_hold(&ns->subsys->lock);
 void nvmet_execute_identify_ctrl_zns(struct nvmet_req *req);
 void nvmet_execute_identify_ns_zns(struct nvmet_req *req);
 void nvmet_bdev_execute_zone_mgmt_recv(struct nvmet_req *req);
diff --git a/drivers/nvme/target/rdma.c b/drivers/nvme/target/rdma.c
index f386c388da18..0b351d39340c 100644
--- a/drivers/nvme/target/rdma.c
+++ b/drivers/nvme/target/rdma.c
@@ -594,7 +594,7 @@ static void nvmet_rdma_set_sig_attrs(struct nvmet_req *req,
 	u8 pi_type = req->ns->pi_type;
 	struct blk_integrity *bi;
 
-	bi = bdev_get_integrity(req->ns->bdev);
+	bi = bdev_get_integrity(nvmet_ns_bdev(req->ns));
 
 	memset(sig_attrs, 0, sizeof(*sig_attrs));
 
diff --git a/drivers/nvme/target/zns.c b/drivers/nvme/target/zns.c
index cea05b4bc740..ce5309453668 100644
--- a/drivers/nvme/target/zns.c
+++ b/drivers/nvme/target/zns.c
@@ -110,30 +110,29 @@ void nvmet_execute_identify_ns_zns(struct nvmet_req *req)
 	if (status)
 		goto out;
 
-	if (nvmet_ns_revalidate(req->ns)) {
-		mutex_lock(&req->ns->subsys->lock);
+	mutex_lock(&req->ns->subsys->lock);
+	if (nvmet_ns_revalidate(req->ns))
 		nvmet_ns_changed(req->ns->subsys, req->ns->nsid);
-		mutex_unlock(&req->ns->subsys->lock);
-	}
+	mutex_unlock(&req->ns->subsys->lock);
 
-	if (!req->ns->bdev || !bdev_is_zoned(req->ns->bdev)) {
+	if (!nvmet_ns_bdev(req->ns) || !bdev_is_zoned(nvmet_ns_bdev(req->ns))) {
 		status = NVME_SC_INVALID_FIELD | NVME_STATUS_DNR;
 		req->error_loc = offsetof(struct nvme_identify, nsid);
 		goto out;
 	}
 
-	zsze = (bdev_zone_sectors(req->ns->bdev) << 9) >>
+	zsze = (bdev_zone_sectors(nvmet_ns_bdev(req->ns)) << 9) >>
 					req->ns->blksize_shift;
 	id_zns->lbafe[0].zsze = cpu_to_le64(zsze);
 
-	mor = bdev_max_open_zones(req->ns->bdev);
+	mor = bdev_max_open_zones(nvmet_ns_bdev(req->ns));
 	if (!mor)
 		mor = U32_MAX;
 	else
 		mor--;
 	id_zns->mor = cpu_to_le32(mor);
 
-	mar = bdev_max_active_zones(req->ns->bdev);
+	mar = bdev_max_active_zones(nvmet_ns_bdev(req->ns));
 	if (!mar)
 		mar = U32_MAX;
 	else
@@ -151,7 +150,7 @@ static u16 nvmet_bdev_validate_zone_mgmt_recv(struct nvmet_req *req)
 	sector_t sect = nvmet_lba_to_sect(req->ns, req->cmd->zmr.slba);
 	u32 out_bufsize = (le32_to_cpu(req->cmd->zmr.numd) + 1) << 2;
 
-	if (sect >= get_capacity(req->ns->bdev->bd_disk)) {
+	if (sect >= get_capacity(nvmet_ns_bdev(req->ns)->bd_disk)) {
 		req->error_loc = offsetof(struct nvme_zone_mgmt_recv_cmd, slba);
 		return NVME_SC_LBA_RANGE | NVME_STATUS_DNR;
 	}
@@ -247,7 +246,8 @@ static unsigned long nvmet_req_nr_zones_from_slba(struct nvmet_req *req)
 {
 	unsigned int sect = nvmet_lba_to_sect(req->ns, req->cmd->zmr.slba);
 
-	return bdev_nr_zones(req->ns->bdev) - bdev_zone_no(req->ns->bdev, sect);
+	return bdev_nr_zones(nvmet_ns_bdev(req->ns)) -
+			bdev_zone_no(nvmet_ns_bdev(req->ns), sect);
 }
 
 static unsigned long get_nr_zones_from_buf(struct nvmet_req *req, u32 bufsize)
@@ -291,8 +291,8 @@ static void nvmet_bdev_zone_zmgmt_recv_work(struct work_struct *w)
 		goto out;
 	}
 
-	ret = blkdev_report_zones(req->ns->bdev, start_sect, req_slba_nr_zones,
-				 nvmet_bdev_report_zone_cb, &rz_data);
+	ret = blkdev_report_zones(nvmet_ns_bdev(req->ns), start_sect,
+			req_slba_nr_zones, nvmet_bdev_report_zone_cb, &rz_data);
 	if (ret < 0) {
 		status = NVME_SC_INTERNAL;
 		goto out;
@@ -415,7 +415,7 @@ static int zmgmt_send_scan_cb(struct blk_zone *z, unsigned i, void *d)
 
 static u16 nvmet_bdev_zone_mgmt_emulate_all(struct nvmet_req *req)
 {
-	struct block_device *bdev = req->ns->bdev;
+	struct block_device *bdev = nvmet_ns_bdev(req->ns);
 	unsigned int nr_zones = bdev_nr_zones(bdev);
 	struct bio *bio = NULL;
 	sector_t sector = 0;
@@ -471,8 +471,9 @@ static u16 nvmet_bdev_execute_zmgmt_send_all(struct nvmet_req *req)
 
 	switch (zsa_req_op(req->cmd->zms.zsa)) {
 	case REQ_OP_ZONE_RESET:
-		ret = blkdev_zone_mgmt(req->ns->bdev, REQ_OP_ZONE_RESET, 0,
-				       get_capacity(req->ns->bdev->bd_disk));
+		ret = blkdev_zone_mgmt(nvmet_ns_bdev(req->ns),
+				REQ_OP_ZONE_RESET, 0,
+				get_capacity(nvmet_ns_bdev(req->ns)->bd_disk));
 		if (ret < 0)
 			return blkdev_zone_mgmt_errno_to_nvme_status(ret);
 		break;
@@ -494,7 +495,7 @@ static void nvmet_bdev_zmgmt_send_work(struct work_struct *w)
 	struct nvmet_req *req = container_of(w, struct nvmet_req, z.zmgmt_work);
 	sector_t sect = nvmet_lba_to_sect(req->ns, req->cmd->zms.slba);
 	enum req_op op = zsa_req_op(req->cmd->zms.zsa);
-	struct block_device *bdev = req->ns->bdev;
+	struct block_device *bdev = nvmet_ns_bdev(req->ns);
 	sector_t zone_sectors = bdev_zone_sectors(bdev);
 	u16 status = NVME_SC_SUCCESS;
 	int ret;
@@ -560,13 +561,14 @@ void nvmet_bdev_execute_zone_append(struct nvmet_req *req)
 	u32 data_len = nvmet_rw_data_len(req);
 	struct bio *bio;
 	int sg_cnt;
+	struct block_device *bdev = nvmet_ns_bdev(req->ns);
 
 	/* Request is completed on len mismatch in nvmet_check_transfer_len() */
 	if (!nvmet_check_transfer_len(req, nvmet_rw_data_len(req)))
 		return;
 
 	if (data_len >
-	    bdev_max_zone_append_sectors(req->ns->bdev) << SECTOR_SHIFT) {
+	    bdev_max_zone_append_sectors(bdev) << SECTOR_SHIFT) {
 		req->error_loc = offsetof(struct nvme_rw_command, length);
 		status = NVME_SC_INVALID_FIELD | NVME_STATUS_DNR;
 		goto out;
@@ -577,13 +579,13 @@ void nvmet_bdev_execute_zone_append(struct nvmet_req *req)
 		return;
 	}
 
-	if (sect >= get_capacity(req->ns->bdev->bd_disk)) {
+	if (sect >= get_capacity(bdev->bd_disk)) {
 		req->error_loc = offsetof(struct nvme_rw_command, slba);
 		status = NVME_SC_LBA_RANGE | NVME_STATUS_DNR;
 		goto out;
 	}
 
-	if (sect & (bdev_zone_sectors(req->ns->bdev) - 1)) {
+	if (sect & (bdev_zone_sectors(bdev) - 1)) {
 		req->error_loc = offsetof(struct nvme_rw_command, slba);
 		status = NVME_SC_INVALID_FIELD | NVME_STATUS_DNR;
 		goto out;
@@ -591,10 +593,10 @@ void nvmet_bdev_execute_zone_append(struct nvmet_req *req)
 
 	if (nvmet_use_inline_bvec(req)) {
 		bio = &req->z.inline_bio;
-		bio_init(bio, req->ns->bdev, req->inline_bvec,
+		bio_init(bio, bdev, req->inline_bvec,
 			 ARRAY_SIZE(req->inline_bvec), opf);
 	} else {
-		bio = bio_alloc(req->ns->bdev, req->sg_cnt, opf, GFP_KERNEL);
+		bio = bio_alloc(bdev, req->sg_cnt, opf, GFP_KERNEL);
 	}
 
 	bio->bi_end_io = nvmet_bdev_zone_append_bio_done;
-- 
2.53.0

Re: [PATCH 01/23] nvmet: add context annotation for nvmet_ns::bdev

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [Critical] nvmet: race in nvmet_bdev_zone_append_bio_done() on
  inline_bio cleanup [1]
--

commit bd98b59eeb25b1fa920c66a35d00856fefddae58
Author: Nilay Shroff <nilay@linux.ibm.com>

nvmet: add context annotation for nvmet_ns::bdev

This commit annotates nvmet_ns::bdev with __guarded_by(&subsys->lock), adds
__must_hold() annotations to namespace management functions, and introduces
the nvmet_ns_bdev() helper for lockless I/O and request-processing paths.

> diff --git a/drivers/nvme/target/zns.c b/drivers/nvme/target/zns.c
> index cea05b4bc7409..ce53094536688 100644
> --- a/drivers/nvme/target/zns.c
> +++ b/drivers/nvme/target/zns.c

[ ... ]

> @@ -591,10 +593,10 @@ void nvmet_bdev_execute_zone_append(struct nvmet_req *req)
>  
>  	if (nvmet_use_inline_bvec(req)) {
>  		bio = &req->z.inline_bio;
> -		bio_init(bio, req->ns->bdev, req->inline_bvec,
> +		bio_init(bio, bdev, req->inline_bvec,
>  			 ARRAY_SIZE(req->inline_bvec), opf);
>  	} else {
> -		bio = bio_alloc(req->ns->bdev, req->sg_cnt, opf, GFP_KERNEL);
> +		bio = bio_alloc(bdev, req->sg_cnt, opf, GFP_KERNEL);
>  	}
>  
>  	bio->bi_end_io = nvmet_bdev_zone_append_bio_done;

[Severity: Critical]
This problem wasn't introduced by this patch, but can
nvmet_bdev_zone_append_bio_done() cause a use-after-free on req or corrupt
req->z.inline_bio [1]?

drivers/nvme/target/zns.c:nvmet_bdev_zone_append_bio_done() {
	...
	nvmet_req_complete(req, blk_to_nvme_status(req, bio->bi_status));
	nvmet_req_bio_put(req, bio);
}

nvmet_req_complete() invokes req->ops->queue_response(), allowing the
transport to immediately free req or reuse it for a new command before
nvmet_req_bio_put() runs:

CPU0                                  CPU1
nvmet_bdev_zone_append_bio_done()
  nvmet_req_complete(req, ...)
    req->ops->queue_response(req)
                                      bio_init(&req->z.inline_bio, ...)
                                      submit_bio(&req->z.inline_bio)
  nvmet_req_bio_put(req, bio)
    bio_uninit(bio)
      bio->bi_blkg = NULL
                                        blk_cgroup_bio_start(bio)
                                          <-- NULL pointer dereference

Saving bio->bi_status and calling nvmet_req_bio_put() before
nvmet_req_complete(), as nvmet_bio_done() does, would avoid touching req and
inline_bio after completion.

[1] https://sashiko.dev/#/bug/linux-26aa255a-7ec1-4018-9aa1-cc5794fc39c0

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109033102.982191-25-nilay@linux.ibm.com?part=1

Re: [PATCH 01/23] nvmet: add context annotation for nvmet_ns::bdev

[Bart Van Assche]

On 11/8/26 7:28 PM, Nilay Shroff wrote:
 > The nvmet_ns::bdev field is protected by nvmet_subsys::lock. Annotate
 > it with __guarded_by(&subsys->lock) so that the clang context analyzer
 > can validate accesses to this field.

This patch introduces a compiler error in nvmet_get_smart_log_all() when
built with CONFIG_WARN_CONTEXT_ANALYSIS=y:

   drivers/nvme/target/admin-cmd.c: error: reading variable 'bdev' requires
   holding mutex 'ns->subsys->lock' [-Werror,-Wthread-safety-precise]

This is only resolved in patch 02. Please make sure this series is
bisectable.

 > +	/*
 > +	 * Both ctrl->subsys->lock and ns->subsys->lock refers to the same 
lock.

"refers to" -> "refer to".

 > +	 * However there's no way for us to annotate such locking semantics
 > +	 * using clang context annotation. So explicitly assert the capability.
 > +	 */
 > +	__assume_ctx_lock(&ns->subsys->lock);

__assume_ctx_lock() wouldn't be necessary if subsys would be passed
as a function argument, isn't it? In general, I strongly prefer to
add a function argument rather than using __assume_ctx_lock().

Thanks,

Bart.

Re: [PATCH 01/23] nvmet: add context annotation for nvmet_ns::bdev

[Nilay Shroff]

On 11/10/26 3:16 AM, Bart Van Assche wrote:
> On 11/8/26 7:28 PM, Nilay Shroff wrote:
>  > The nvmet_ns::bdev field is protected by nvmet_subsys::lock. Annotate
>  > it with __guarded_by(&subsys->lock) so that the clang context analyzer
>  > can validate accesses to this field.
> 
> This patch introduces a compiler error in nvmet_get_smart_log_all() when
> built with CONFIG_WARN_CONTEXT_ANALYSIS=y:
> 
>    drivers/nvme/target/admin-cmd.c: error: reading variable 'bdev' requires
>    holding mutex 'ns->subsys->lock' [-Werror,-Wthread-safety-precise]
> 
> This is only resolved in patch 02. Please make sure this series is
> bisectable.
> 
Enabling CONFIG_WARN_CONTEXT_ANALYSIS=y only enables the context-analysis
infrastructure. The analysis is opt-in by default, and individual subsystems
or source files need to explicitly enable it through their Makefiles.

I haven't enabled context analysis for the NVMe target subsystem yet in this
series. My plan is to enable it by adding CONTEXT_ANALYSIS=y to the NVMe target
Makefile in a subsequent series, after completing the context annotations
throughout the subsystem.

Therefore, with only CONFIG_WARN_CONTEXT_ANALYSIS=y enabled (and without
CONFIG_WARN_CONTEXT_ANALYSIS_ALL=y), this patch should not trigger the reported error.
Could you check whether your build has CONFIG_WARN_CONTEXT_ANALYSIS_ALL enabled?

>  > +    /*
>  > +     * Both ctrl->subsys->lock and ns->subsys->lock refers to the same lock.
> 
> "refers to" -> "refer to".
Okay>
>  > +     * However there's no way for us to annotate such locking semantics
>  > +     * using clang context annotation. So explicitly assert the capability.
>  > +     */
>  > +    __assume_ctx_lock(&ns->subsys->lock);
> 
> __assume_ctx_lock() wouldn't be necessary if subsys would be passed
> as a function argument, isn't it? In general, I strongly prefer to
> add a function argument rather than using __assume_ctx_lock().
> 

I agree that passing the lock owner as a function argument is preferable whenever
possible. For this particular case, I tried passing subsys down the call chain to
nvmet_ns_dev() and annotating the function with __must_hold(&subsys->lock). However,
Clang still reports a locking violation when accessing ns->bdev:

drivers/nvme/target/nvmet.h:213:13: error: reading variable 'bdev'
requires holding mutex 'ns->subsys->lock'
[-Werror,-Wthread-safety-precise]
213 |         return ns->bdev ? disk_to_dev(ns->bdev->bd_disk) : NULL;
|                    ^
drivers/nvme/target/nvmet.h:213:13: note: found near match 'subsys->lock'

It appears that the context analyzer cannot infer that subsys->lock and ns->subsys->lock
refer to the same mutex, even though subsys and ns->subsys refer to the same subsystem.
Consequently, I couldn't avoid using __assume_ctx_lock() in this case to explicitly
establish the lock capability expected by the field annotation.

Thanks,
--Nilay

Re: [PATCH 01/23] nvmet: add context annotation for nvmet_ns::bdev

[Bart Van Assche]

[-- Attachment #1: Type: text/plain, Size: 586 bytes --]

On 11/9/26 10:38 PM, Nilay Shroff wrote:
> I agree that passing the lock owner as a function argument is preferable 
> whenever
> possible. For this particular case, I tried passing subsys down the call 
> chain to
> nvmet_ns_dev() and annotating the function with __must_hold(&subsys- 
>  >lock). However,
> Clang still reports a locking violation when accessing ns->bdev:

Please take a look at the attached patches and consider to add these to
the start of this series. These patches are needed to implement what I
suggested in my previous email in this email thread.

Thanks,

Bart.

[-- Attachment #2: 0001-nvme-Fix-lock-context-annotations-for-nvme_passthru_.patch --]
[-- Type: text/x-patch, Size: 3548 bytes --]

From b051b784d302cbf334803c3b1b71cc2dd55ce8a3 Mon Sep 17 00:00:00 2001
From: Bart Van Assche <bvanassche@acm.org>
Date: Tue, 10 Nov 2026 09:11:12 -0800
Subject: [PATCH 1/3] nvme: Fix lock context annotations for
 nvme_passthru_{start,end}()

Remove the __cond_acquires() and __cond_releases() annotations from the
nvme_passthru_start() and nvme_passthru_end() declarations in nvme.h.
Annotate nvme_passthru_start() and nvme_passthru_end() in core.c with
__context_unsafe(/* conditional locking */).

Note that nvme_passthru_start() acquires ctrl->scan_lock and
ctrl->subsys->lock only when (effects & NVME_CMD_EFFECTS_CSE_MASK) is
non-zero. Consider that commands frequently return non-zero effects
(such as NVME_CMD_EFFECTS_CSUPP) without setting
NVME_CMD_EFFECTS_CSE_MASK. Do not assume that every non-zero return value
implies that locks were acquired, which is what __cond_acquires(nonzero,
...) incorrectly assumes.

Observe also that nvme_passthru_end() conditionally releases locks based
on the "effects" parameter rather than its return value.

Acknowledge that Clang lock context analysis cannot model conditional
locking based on bitmask flags. Suppress lock context imbalance warnings
inside nvme_passthru_start() and nvme_passthru_end() by marking them with
__context_unsafe().

Fixes: a6732bd8003a ("nvme: add context annotations for nvme_passthru_{start|stop}")
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/nvme/host/core.c |  2 ++
 drivers/nvme/host/nvme.h | 10 ++--------
 2 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index f4aa02869b79..def9ac6e8b6a 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1259,6 +1259,7 @@ u32 nvme_command_effects(struct nvme_ctrl *ctrl, struct nvme_ns *ns, u8 opcode)
 EXPORT_SYMBOL_NS_GPL(nvme_command_effects, "NVME_TARGET_PASSTHRU");
 
 u32 nvme_passthru_start(struct nvme_ctrl *ctrl, struct nvme_ns *ns, u8 opcode)
+	__context_unsafe(/* conditional locking */)
 {
 	u32 effects = nvme_command_effects(ctrl, ns, opcode);
 
@@ -1280,6 +1281,7 @@ EXPORT_SYMBOL_NS_GPL(nvme_passthru_start, "NVME_TARGET_PASSTHRU");
 
 u32 nvme_passthru_end(struct nvme_ctrl *ctrl, struct nvme_ns *ns, u32 effects,
 		       struct nvme_command *cmd, int status)
+	__context_unsafe(/* conditional locking */)
 {
 	if (effects & NVME_CMD_EFFECTS_CSE_MASK) {
 		nvme_unfreeze(ctrl);
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 8833a463f1c8..9c3a65657afb 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -1334,16 +1334,10 @@ static inline void nvme_auth_revoke_tls_key(struct nvme_ctrl *ctrl) {};
 
 u32 nvme_command_effects(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
 			 u8 opcode);
-u32 nvme_passthru_start(struct nvme_ctrl *ctrl, struct nvme_ns *ns, u8 opcode)
-	__cond_acquires(nonzero, &ctrl->subsys->lock)
-	__cond_acquires(nonzero, &ctrl->scan_lock);
-
+u32 nvme_passthru_start(struct nvme_ctrl *ctrl, struct nvme_ns *ns, u8 opcode);
 int nvme_execute_rq(struct request *rq, bool at_head);
 u32 nvme_passthru_end(struct nvme_ctrl *ctrl, struct nvme_ns *ns, u32 effects,
-		       struct nvme_command *cmd, int status)
-		       __cond_releases(nonzero, &ctrl->scan_lock)
-		       __cond_releases(nonzero, &ctrl->subsys->lock);
-
+		      struct nvme_command *cmd, int status);
 struct nvme_ctrl *nvme_ctrl_from_file(struct file *file);
 struct nvme_ns *nvme_find_get_ns(struct nvme_ctrl *ctrl, unsigned nsid);
 bool nvme_get_ns(struct nvme_ns *ns);

[-- Attachment #3: 0002-nvme-Pass-the-subsystem-pointer-to-nvme_find_ns_head.patch --]
[-- Type: text/x-patch, Size: 2208 bytes --]

From f278680a0d4fa20538032390d6d7056f44c6ca40 Mon Sep 17 00:00:00 2001
From: Bart Van Assche <bvanassche@acm.org>
Date: Tue, 10 Nov 2026 09:13:21 -0800
Subject: [PATCH 2/3] nvme: Pass the subsystem pointer to nvme_find_ns_head()

Pass the struct nvme_subsystem pointer directly to nvme_find_ns_head().
Update the __must_hold() annotation and the lockdep assertion to check
&subsys->lock instead of &ctrl->subsys->lock, and traverse
&subsys->nsheads directly.

Notice that nvme_subsystem::nsheads is guarded by &nvme_subsystem::lock.
Observe that Clang lock context analysis cannot verify lock requirements
when accessing nsheads through the nested pointer ctrl->subsys. Enable
the compiler to verify that &subsys->lock is held while traversing
&subsys->nsheads by passing the subsystem pointer directly.

Fixes: d1fdf49b5f7f ("nvme: add context annotations for nvme_subsystem::lock")
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/nvme/host/core.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index def9ac6e8b6a..98c39fef0646 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -3910,14 +3910,15 @@ static const struct file_operations nvme_dev_fops = {
 };
 
 static struct nvme_ns_head *nvme_find_ns_head(struct nvme_ctrl *ctrl,
-		unsigned nsid)
-	__must_hold(&ctrl->subsys->lock)
+					      struct nvme_subsystem *subsys,
+					      unsigned nsid)
+	__must_hold(&subsys->lock)
 {
 	struct nvme_ns_head *h;
 
-	lockdep_assert_held(&ctrl->subsys->lock);
+	lockdep_assert_held(&subsys->lock);
 
-	list_for_each_entry(h, &ctrl->subsys->nsheads, entry) {
+	list_for_each_entry(h, &subsys->nsheads, entry) {
 		/*
 		 * Private namespaces can share NSIDs under some conditions.
 		 * In that case we can't use the same ns_head for namespaces
@@ -4192,7 +4193,7 @@ static int nvme_init_ns_head(struct nvme_ns *ns, struct nvme_ns_info *info)
 	}
 
 	mutex_lock(&ctrl->subsys->lock);
-	head = nvme_find_ns_head(ctrl, info->nsid);
+	head = nvme_find_ns_head(ctrl, ctrl->subsys, info->nsid);
 	if (!head) {
 		ret = nvme_subsys_check_duplicate_ids(ctrl->subsys, &info->ids);
 		if (ret) {

[-- Attachment #4: 0003-nvme-Pass-the-subsystem-pointer-to-nvme_alloc_ns_hea.patch --]
[-- Type: text/x-patch, Size: 3177 bytes --]

From f6b512ffdbcad8014d0981a36eb49e1289969c0f Mon Sep 17 00:00:00 2001
From: Bart Van Assche <bvanassche@acm.org>
Date: Tue, 10 Nov 2026 09:17:00 -0800
Subject: [PATCH 3/3] nvme: Pass the subsystem pointer to nvme_alloc_ns_head()

Pass the struct nvme_subsystem pointer directly to nvme_alloc_ns_head().
Update the __must_hold() annotation to check &subsys->lock instead of
&ns->ctrl->subsys->lock, and access subsys directly instead of through
ctrl->subsys.

Notice that nvme_subsystem::nsheads is guarded by &nvme_subsystem::lock.
Observe that Clang lock context analysis cannot verify lock requirements
when accessing nsheads through the nested pointer ns->ctrl->subsys.
Enable the compiler to verify that &subsys->lock is held while modifying
&subsys->nsheads by passing the subsystem pointer directly.

Fixes: d1fdf49b5f7f ("nvme: add context annotations for nvme_subsystem::lock")
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/nvme/host/core.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 98c39fef0646..c5ca25122f8e 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -4052,8 +4052,9 @@ static void nvme_add_ns_cdev(struct nvme_ns *ns)
 }
 
 static struct nvme_ns_head *nvme_alloc_ns_head(struct nvme_ns *ns,
-		struct nvme_ns_info *info)
-	__must_hold(&ns->ctrl->subsys->lock)
+					       struct nvme_subsystem *subsys,
+					       struct nvme_ns_info *info)
+	__must_hold(&subsys->lock)
 {
 	struct nvme_ctrl *ctrl = ns->ctrl;
 	struct nvme_ns_head *head;
@@ -4067,7 +4068,7 @@ static struct nvme_ns_head *nvme_alloc_ns_head(struct nvme_ns *ns,
 	head = kzalloc(size, GFP_KERNEL);
 	if (!head)
 		goto out;
-	ret = ida_alloc_min(&ctrl->subsys->ns_ida, 1, GFP_KERNEL);
+	ret = ida_alloc_min(&subsys->ns_ida, 1, GFP_KERNEL);
 	if (ret < 0)
 		goto out_free_head;
 	head->instance = ret;
@@ -4075,7 +4076,7 @@ static struct nvme_ns_head *nvme_alloc_ns_head(struct nvme_ns *ns,
 	ret = init_srcu_struct(&head->srcu);
 	if (ret)
 		goto out_ida_remove;
-	head->subsys = ctrl->subsys;
+	head->subsys = subsys;
 	head->ns_id = info->nsid;
 	head->ids = info->ids;
 	head->shared = info->is_shared;
@@ -4102,9 +4103,9 @@ static struct nvme_ns_head *nvme_alloc_ns_head(struct nvme_ns *ns,
 	if (ret)
 		goto out_cleanup_fdp;
 
-	list_add_tail(&head->entry, &ctrl->subsys->nsheads);
+	list_add_tail(&head->entry, &subsys->nsheads);
 
-	kref_get(&ctrl->subsys->ref);
+	kref_get(&subsys->ref);
 
 	return head;
 out_cleanup_fdp:
@@ -4112,7 +4113,7 @@ static struct nvme_ns_head *nvme_alloc_ns_head(struct nvme_ns *ns,
 out_cleanup_srcu:
 	cleanup_srcu_struct(&head->srcu);
 out_ida_remove:
-	ida_free(&ctrl->subsys->ns_ida, head->instance);
+	ida_free(&subsys->ns_ida, head->instance);
 out_free_head:
 	kfree(head);
 	ns->head = NULL;
@@ -4202,7 +4203,7 @@ static int nvme_init_ns_head(struct nvme_ns *ns, struct nvme_ns_info *info)
 				info->nsid);
 			goto out_unlock;
 		}
-		head = nvme_alloc_ns_head(ns, info);
+		head = nvme_alloc_ns_head(ns, ctrl->subsys, info);
 		if (IS_ERR(head)) {
 			ret = PTR_ERR(head);
 			goto out_unlock;

Re: [PATCH 01/23] nvmet: add context annotation for nvmet_ns::bdev

[Nilay Shroff]

[-- Attachment #1: Type: text/plain, Size: 3320 bytes --]

On 11/10/26 10:59 PM, Bart Van Assche wrote:
> On 11/9/26 10:38 PM, Nilay Shroff wrote:
>> I agree that passing the lock owner as a function argument is preferable whenever
>> possible. For this particular case, I tried passing subsys down the call chain to
>> nvmet_ns_dev() and annotating the function with __must_hold(&subsys-  >lock). However,
>> Clang still reports a locking violation when accessing ns->bdev:
> 
> Please take a look at the attached patches and consider to add these to
> the start of this series. These patches are needed to implement what I
> suggested in my previous email in this email thread.
> 
I reviewed the patches you attached and tried implementing your suggestion by passing
the struct nvmet_subsys pointer down the call chain to nvmet_ns_dev(), which accesses
ns->bdev protected by subsys->lock.

The relevant code looks like this:

struct nvmet_subsys {
     ...
     struct mutex lock;
     ...
}

struct nvmet_ns {
     ...
     struct block_device	*bdev __guarded_by(&subsys->lock);
     ...
     struct nvmet_subsys	*subsys;
     ...
}

static inline struct device *nvmet_ns_dev(struct nvmet_subsys *subsys,
		struct nvmet_ns *ns)
	__must_hold(&subsys->lock)
{
	return ns->bdev ? disk_to_dev(ns->bdev->bd_disk) : NULL;
}

Attached the complete patch for your reference. Even after making
this change, clang still complains about locking violation:

drivers/nvme/target/nvmet.h:255:13: error: reading variable 'bdev' requires holding mutex 'ns->subsys->lock' [-Werror,-Wthread-safety-precise]
   255 |         return ns->bdev ? disk_to_dev(ns->bdev->bd_disk) : NULL;
       |                    ^
drivers/nvme/target/nvmet.h:255:13: note: found near match 'subsys->lock'

 From this error, it appears that the Clang context analyzer expects accesses
to ns->bdev to be protected specifically by ns->subsys->lock. It recognizes
subsys->lock as a near match but cannot establish that both expressions refer
to the same mutex.

Therefore, passing the subsys pointer down to nvmet_ns_dev() and annotating the
function with __must_hold(&subsys->lock) does not resolve the warning in this case.

I also looked at the approach in your proposed patches for the NVMe host driver.
There, passing the struct nvme_subsystem pointer down the call chain works because
the list being protected, nsheads, is itself a member of struct nvme_subsystem:


struct nvme_subsystem {
     ....
     struct mutex lock;
     struct list_head nsheads __guarded_by(&lock);
     ....
}

In this case, the analyzer can associate accesses to subsys->nsheads directly with
subsys->lock, allowing __must_hold(&subsys->lock) to express the locking requirement
correctly.

The situation is different for nvmet_ns::bdev, where the lock protecting the field
belongs to the namespace's subsystem which is a separate struct nvmet_subsys. It seems
the analyzer cannot establish the equivalence between subsys->lock and ns->subsys->lock,
even when both pointers refer to the same subsystem.

Regarding your patches, I'm happy to include them in my series. However, my current
series focuses on adding context annotations to the NVMe target driver, whereas your
proposed patches address the NVMe host driver. So I'd suggest posting your changes as
a separate series so they can be reviewed independently.

Thanks,
--Nilay

[-- Attachment #2: nvmet-ns-bdev.diff --]
[-- Type: text/x-patch, Size: 5288 bytes --]

diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index c9837263898a..982749efa6e1 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -490,8 +490,9 @@ static void nvmet_ns_dev_disable(struct nvmet_ns *ns)
 	nvmet_file_ns_disable(ns);
 }
 
-static int nvmet_p2pmem_ns_enable(struct nvmet_ns *ns)
-	__must_hold(&ns->subsys->lock)
+static int nvmet_p2pmem_ns_enable(struct nvmet_subsys *subsys,
+		struct nvmet_ns *ns)
+	__must_hold(&subsys->lock)
 {
 	int ret;
 	struct pci_dev *p2p_dev;
@@ -511,7 +512,8 @@ static int nvmet_p2pmem_ns_enable(struct nvmet_ns *ns)
 	}
 
 	if (ns->p2p_dev) {
-		ret = pci_p2pdma_distance(ns->p2p_dev, nvmet_ns_dev(ns), true);
+		ret = pci_p2pdma_distance(ns->p2p_dev, nvmet_ns_dev(subsys, ns),
+				true);
 		if (ret < 0)
 			return -EINVAL;
 	} else {
@@ -522,7 +524,7 @@ static int nvmet_p2pmem_ns_enable(struct nvmet_ns *ns)
 		 * setup the controller when the port's device is available.
 		 */
 
-		p2p_dev = pci_p2pmem_find(nvmet_ns_dev(ns));
+		p2p_dev = pci_p2pmem_find(nvmet_ns_dev(subsys, ns));
 		if (!p2p_dev) {
 			pr_err("no peer-to-peer memory is available for %s\n",
 			       ns->device_path);
@@ -535,21 +537,16 @@ static int nvmet_p2pmem_ns_enable(struct nvmet_ns *ns)
 	return 0;
 }
 
-static void nvmet_p2pmem_ns_add_p2p(struct nvmet_ctrl *ctrl,
+static void nvmet_p2pmem_ns_add_p2p(struct nvmet_subsys *subsys,
+				    struct nvmet_ctrl *ctrl,
 				    struct nvmet_ns *ns)
-	__must_hold(&ctrl->subsys->lock)
+	__must_hold(&subsys->lock)
 {
 	struct device *clients[2];
 	struct pci_dev *p2p_dev;
 	int ret;
 
-	lockdep_assert_held(&ctrl->subsys->lock);
-	/*
-	 * Both ctrl->subsys->lock and ns->subsys->lock refers to the same lock.
-	 * However there's no way for us to annotate such locking semantics
-	 * using clang context annotation. So explicitly assert the capability.
-	 */
-	__assume_ctx_lock(&ns->subsys->lock);
+	lockdep_assert_held(&subsys->lock);
 
 	if (!ctrl->p2p_client || !ns->use_p2pmem)
 		return;
@@ -562,7 +559,7 @@ static void nvmet_p2pmem_ns_add_p2p(struct nvmet_ctrl *ctrl,
 		p2p_dev = pci_dev_get(ns->p2p_dev);
 	} else {
 		clients[0] = ctrl->p2p_client;
-		clients[1] = nvmet_ns_dev(ns);
+		clients[1] = nvmet_ns_dev(subsys, ns);
 
 		p2p_dev = pci_p2pmem_find_many(clients, ARRAY_SIZE(clients));
 		if (!p2p_dev) {
@@ -620,20 +617,12 @@ int nvmet_ns_enable(struct nvmet_ns *ns)
 	if (ret)
 		goto out_unlock;
 
-	ret = nvmet_p2pmem_ns_enable(ns);
+	ret = nvmet_p2pmem_ns_enable(subsys, ns);
 	if (ret)
 		goto out_dev_disable;
 
-	list_for_each_entry(ctrl, &subsys->ctrls, subsys_entry) {
-		/*
-		 * Both subsys->lock and ctrl->subsys->lock refers to the same
-		 * lock. However there's no way for us to annotate such locking
-		 * semantics using clang context annotation. So explicitly
-		 * assert the capability.
-		 */
-		__assume_ctx_lock(&ctrl->subsys->lock);
-		nvmet_p2pmem_ns_add_p2p(ctrl, ns);
-	}
+	list_for_each_entry(ctrl, &subsys->ctrls, subsys_entry)
+		nvmet_p2pmem_ns_add_p2p(subsys, ctrl, ns);
 
 	if (ns->pr.enable) {
 		ret = nvmet_pr_init_ns(ns);
@@ -1591,14 +1580,15 @@ bool nvmet_host_allowed(struct nvmet_subsys *subsys, const char *hostnqn)
 	return false;
 }
 
-static void nvmet_setup_p2p_ns_map(struct nvmet_ctrl *ctrl,
-		struct device *p2p_client)
-	__must_hold(&ctrl->subsys->lock)
+static void nvmet_setup_p2p_ns_map(struct nvmet_subsys *subsys,
+				   struct nvmet_ctrl *ctrl,
+				   struct device *p2p_client)
+	__must_hold(&subsys->lock)
 {
 	struct nvmet_ns *ns;
 	unsigned long idx;
 
-	lockdep_assert_held(&ctrl->subsys->lock);
+	lockdep_assert_held(&subsys->lock);
 
 	if (!p2p_client)
 		return;
@@ -1606,7 +1596,7 @@ static void nvmet_setup_p2p_ns_map(struct nvmet_ctrl *ctrl,
 	ctrl->p2p_client = get_device(p2p_client);
 
 	nvmet_for_each_enabled_ns(&ctrl->subsys->namespaces, idx, ns)
-		nvmet_p2pmem_ns_add_p2p(ctrl, ns);
+		nvmet_p2pmem_ns_add_p2p(subsys, ctrl, ns);
 }
 
 static void nvmet_release_p2p_ns_map(struct nvmet_ctrl *ctrl)
@@ -1752,13 +1742,7 @@ struct nvmet_ctrl *nvmet_alloc_ctrl(struct nvmet_alloc_ctrl_args *args)
 		goto init_pr_fail;
 	list_add_tail(&ctrl->subsys_entry, &subsys->ctrls);
 
-	/*
-	 * Both subsys->lock and ctrl->subsys->lock refers to the same lock.
-	 * However there's no way for us to annotate such locking semantics
-	 * using clang context annotation. So explicitly assert the capability.
-	 */
-	__assume_ctx_lock(&ctrl->subsys->lock);
-	nvmet_setup_p2p_ns_map(ctrl, args->p2p_client);
+	nvmet_setup_p2p_ns_map(subsys, ctrl, args->p2p_client);
 	nvmet_debugfs_ctrl_setup(ctrl);
 	mutex_unlock(&subsys->lock);
 	up_read(&nvmet_config_sem);
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 41912877ade3..270cb112e702 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -207,8 +207,9 @@ static inline struct nvmet_ns *to_nvmet_ns(struct config_item *item)
 	return container_of(to_config_group(item), struct nvmet_ns, group);
 }
 
-static inline struct device *nvmet_ns_dev(struct nvmet_ns *ns)
-	__must_hold(&ns->subsys->lock)
+static inline struct device *nvmet_ns_dev(struct nvmet_subsys *subsys,
+		struct nvmet_ns *ns)
+	__must_hold(&subsys->lock)
 {
 	return ns->bdev ? disk_to_dev(ns->bdev->bd_disk) : NULL;
 }

Re: [PATCH 01/23] nvmet: add context annotation for nvmet_ns::bdev

[Bart Van Assche]

On 11/10/26 11:25 PM, Nilay Shroff wrote:
> Attached the complete patch for your reference. Even after making
> this change, clang still complains about locking violation:
> 
> drivers/nvme/target/nvmet.h:255:13: error: reading variable 'bdev' 
> requires holding mutex 'ns->subsys->lock' [-Werror,-Wthread-safety-precise]
>    255 |         return ns->bdev ? disk_to_dev(ns->bdev->bd_disk) : NULL;
>        |                    ^
> drivers/nvme/target/nvmet.h:255:13: note: found near match 'subsys->lock'

This can be solved by adding a lockdep_assert_held(&ms->bdev->bd_disk)
statement, isn't it?

Thanks,

Bart.

Re: [PATCH 01/23] nvmet: add context annotation for nvmet_ns::bdev

[Nilay Shroff]

On 11/11/26 11:30 PM, Bart Van Assche wrote:
> On 11/10/26 11:25 PM, Nilay Shroff wrote:
>> Attached the complete patch for your reference. Even after making
>> this change, clang still complains about locking violation:
>>
>> drivers/nvme/target/nvmet.h:255:13: error: reading variable 'bdev' requires holding mutex 'ns->subsys->lock' [-Werror,-Wthread-safety-precise]
>>    255 |         return ns->bdev ? disk_to_dev(ns->bdev->bd_disk) : NULL;
>>        |                    ^
>> drivers/nvme/target/nvmet.h:255:13: note: found near match 'subsys->lock'
> 
> This can be solved by adding a lockdep_assert_held(&ms->bdev->bd_disk)
> statement, isn't it?
> I think there's a typo above. I assume you're suggesting adding
lockdep_assert_held(&ns->subsys->lock) here.

And yes, adding the lockdep assertion resolves the compilation error.
As I see now (frankly I was not aware about this earlier), lockdep
internally assumes the required capability, which allows Clang's context
analyzer to satisfy the corresponding lock annotation.

So as you suggested, for complex call paths involving multiple locks,
where the same lock may be accessed through different expressions, I'll
use the appropriate lockdep_assert_held() assertions instead of explicitly
adding __assume_ctx_lock() annotations as far as possible.

Thanks,
--Nilay

Re: [PATCH 01/23] nvmet: add context annotation for nvmet_ns::bdev

[Bart Van Assche]

On 11/12/26 1:04 AM, Nilay Shroff wrote:
> And yes, adding the lockdep assertion resolves the compilation error.
> As I see now (frankly I was not aware about this earlier), lockdep
> internally assumes the required capability, which allows Clang's context
> analyzer to satisfy the corresponding lock annotation.
> 
> So as you suggested, for complex call paths involving multiple locks,
> where the same lock may be accessed through different expressions, I'll
> use the appropriate lockdep_assert_held() assertions instead of explicitly
> adding __assume_ctx_lock() annotations as far as possible.

Thanks!

Something that you may want to know is that I'm proposing to remove 
__assume_ctx_lock() again from lockdep_assert_held() and to introduce
a new macro lockdep_assume_ctx_held(). So far no agreement has been
reached about these changes. See also "[PATCH v2 00/14] Stop
lockdep_assert_held*() from assuming lock ownership"
(https://lore.kernel.org/lkml/cover.1794250850.git.bvanassche@acm.org/).

Bart.

[PATCH 02/23] nvmet: fix nvmet_ns::bdev access using nvmet_subsys::lock

[Nilay Shroff]

nvmet_get_smart_log_all() iterates over enabled namespaces and accesses
ns->bdev to collect block device statistics without holding
nvmet_subsys::lock.

Since nvmet_ns::bdev is annotated with __guarded_by(&subsys->lock), the
clang context analyzer reports this access as a locking violation:

drivers/nvme/target/admin-cmd.c:274:12: error: reading variable 'bdev' requires holding mutex 'ns->subsys->lock' [-Werror,-Wthread-safety-precise]
  274 |                 if (!ns->bdev)
      |                          ^

Fix this by acquiring ctrl->subsys->lock while iterating over the
enabled namespaces and accessing their block device statistics.

Since ctrl->subsys->lock and ns->subsys->lock refer to the same lock,
explicitly use __assume_ctx_lock() to inform the clang context analyzer
of the corresponding locking capability.

Signed-off-by: Nilay Shroff <nilay@linux.ibm.com>
---
 drivers/nvme/target/admin-cmd.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 2d4e21b3ee7d..517f8cc78473 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -289,7 +289,15 @@ static u16 nvmet_get_smart_log_all(struct nvmet_req *req,
 	unsigned long idx;
 
 	ctrl = req->sq->ctrl;
+	mutex_lock(&ctrl->subsys->lock);
 	nvmet_for_each_enabled_ns(&ctrl->subsys->namespaces, idx, ns) {
+		/*
+		 * Both ctrl->subsys->lock and ns->subsys->lock refer to the
+		 * same lock. However there's no way for us to annotate such
+		 * locking semantics using clang context annotation. So
+		 * explicitly assert the capability.
+		 */
+		__assume_ctx_lock(&ns->subsys->lock);
 		/* we don't have the right data for file backed ns */
 		if (!ns->bdev)
 			continue;
@@ -300,6 +308,7 @@ static u16 nvmet_get_smart_log_all(struct nvmet_req *req,
 		data_units_written += DIV_ROUND_UP(
 			part_stat_read(ns->bdev, sectors[WRITE]), 1000);
 	}
+	mutex_unlock(&ctrl->subsys->lock);
 
 	put_unaligned_le64(host_reads, &slog->host_reads[0]);
 	put_unaligned_le64(data_units_read, &slog->data_units_read[0]);
-- 
2.53.0

Re: [PATCH 02/23] nvmet: fix nvmet_ns::bdev access using nvmet_subsys::lock

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [Critical] nvmet: use-after-free in nvmet_for_each_enabled_ns() [1]
--

commit c1fe0968cd1483b32634bdd9e21897a686ef88f2
Author: Nilay Shroff <nilay@linux.ibm.com>

nvmet: fix nvmet_ns::bdev access using nvmet_subsys::lock

This commit acquires ctrl->subsys->lock in nvmet_get_smart_log_all() while
iterating over enabled namespaces and reading ns->bdev statistics, and adds
an __assume_ctx_lock() annotation for the clang context analyzer.

> diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
> index 2d4e21b3ee7d1..517f8cc784734 100644
> --- a/drivers/nvme/target/admin-cmd.c
> +++ b/drivers/nvme/target/admin-cmd.c
> @@ -289,7 +289,15 @@ static u16 nvmet_get_smart_log_all(struct nvmet_req *req,
>  	unsigned long idx;
>  
>  	ctrl = req->sq->ctrl;
> +	mutex_lock(&ctrl->subsys->lock);
>  	nvmet_for_each_enabled_ns(&ctrl->subsys->namespaces, idx, ns) {

[Severity: Critical]
This problem wasn't introduced by this patch, but can the remaining
unlocked nvmet_for_each_enabled_ns() loops [1] in
nvmet_format_ana_group(), nvmet_execute_identify_endgrp_list(),
nvmet_execute_identify_nslist(), and nvmet_set_feat_resv_notif_mask()
still hit a use-after-free on ns when racing with namespace removal?

nvmet_for_each_enabled_ns() expands to xa_for_each_marked(), which drops
rcu_read_lock() before yielding each entry to the caller:

drivers/nvme/target/admin-cmd.c:nvmet_execute_identify_nslist() {
    ...
	nvmet_for_each_enabled_ns(&ctrl->subsys->namespaces, idx, ns) {
		if (ns->nsid <= min_nsid)
			continue;
    ...
}

Because those callers hold neither subsys->lock nor rcu_read_lock() across
the loop body, synchronize_rcu() in nvmet_ns_disable() does not wait for
them before nvmet_ns_free() frees ns:

CPU0                                  CPU1
nvmet_execute_identify_nslist()
  nvmet_for_each_enabled_ns()
    xa_for_each_marked()
      // drops rcu_read_lock()
                                      nvmet_ns_free()
                                        nvmet_ns_disable()
                                          synchronize_rcu()
                                        kfree(ns)
    if (ns->nsid <= min_nsid)  <-- use-after-free

Holding rcu_read_lock() (or ctrl->subsys->lock) around those loops would
prevent ns from being freed mid-iteration.

[1] https://sashiko.dev/#/bug/linux-dd6d8d28-fcd8-41b8-9469-54cca852794f

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109033102.982191-25-nilay@linux.ibm.com?part=2

Re: [PATCH 02/23] nvmet: fix nvmet_ns::bdev access using nvmet_subsys::lock

[Bart Van Assche]

On 11/8/26 7:28 PM, Nilay Shroff wrote:
> +		__assume_ctx_lock(&ns->subsys->lock);

Please pass "subsys" as an additional argument to 
nvmet_get_smart_log_all() instead of using __assume_ctx_lock().

Thanks,

Bart.

Re: [PATCH 02/23] nvmet: fix nvmet_ns::bdev access using nvmet_subsys::lock

[Nilay Shroff]

On 11/10/26 3:17 AM, Bart Van Assche wrote:
> On 11/8/26 7:28 PM, Nilay Shroff wrote:
>> +        __assume_ctx_lock(&ns->subsys->lock);
> 
> Please pass "subsys" as an additional argument to nvmet_get_smart_log_all() instead of using __assume_ctx_lock().
> As I stated in my previous mail, it appears that the context analyzer recognizes
subsys->lock as a near match, but cannot establish that it is the same mutex as
ns->subsys->lock. That's why passing subsys alone doesn't resolve the warning,
and I used __assume_ctx_lock() to explicitly establish the capability expected
by the field annotation.

If there's another way to express this lock equivalence to the analyzer, I'm happy
to try it.

Thanks,
--Nilay

[PATCH 03/23] nvmet: add context annotation for nvmet_ns::file

[Nilay Shroff]

The nvmet_ns::file field is protected by nvmet_subsys::lock.
Annotate it with __guarded_by(&subsys->lock) so that the clang
context analyzer can validate accesses to this field.

Some accesses to nvmet_ns::file in the I/O hotpath do not require
holding nvmet_subsys::lock, as the namespace reference guarantees
that the namespace remains valid and the field is not modified while
I/O is in flight. However, the clang context analyzer cannot express
this access pattern directly.

Introduce nvmet_ns_file() as an inline helper annotated with
__context_unsafe() to allow these accesses without requiring the
subsystem lock. Convert the relevant accesses in the I/O hotpath and
request-processing paths to use this helper.

Additionally, annotate nvmet_file_ns_enable(), nvmet_file_ns_disable()
nvmet_file_flush() and nvmet_file_ns_revalidate() with
__must_hold(&ns->subsys->lock) to document and validate their locking
requirements.

Finally, nvmet_file_flush_work() invokes nvmet_file_flush() from the
I/O hotpath after acquiring a namespace reference. Explicitly assert
the ns->subsys->lock capability using __assume_ctx_lock() to satisfy
the context analyzer, since nvmet_file_flush() requires callers to
hold this lock.

Signed-off-by: Nilay Shroff <nilay@linux.ibm.com>
---
 drivers/nvme/target/core.c        |  2 +-
 drivers/nvme/target/io-cmd-file.c | 25 +++++++++++++++++++------
 drivers/nvme/target/nvmet.h       | 16 +++++++++++-----
 3 files changed, 31 insertions(+), 12 deletions(-)

diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index c9837263898a..ef99c45f0f37 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -1171,7 +1171,7 @@ static u16 nvmet_parse_io_cmd(struct nvmet_req *req)
 
 	switch (req->ns->csi) {
 	case NVME_CSI_NVM:
-		if (req->ns->file)
+		if (nvmet_ns_file(req->ns))
 			ret = nvmet_file_parse_io_cmd(req);
 		else
 			ret = nvmet_bdev_parse_io_cmd(req);
diff --git a/drivers/nvme/target/io-cmd-file.c b/drivers/nvme/target/io-cmd-file.c
index 695963dd932e..4307232c9160 100644
--- a/drivers/nvme/target/io-cmd-file.c
+++ b/drivers/nvme/target/io-cmd-file.c
@@ -84,17 +84,17 @@ static ssize_t nvmet_file_submit_bvec(struct nvmet_req *req, loff_t pos,
 	if (req->cmd->rw.opcode == nvme_cmd_write) {
 		if (req->cmd->rw.control & cpu_to_le16(NVME_RW_FUA))
 			ki_flags |= IOCB_DSYNC;
-		call_iter = req->ns->file->f_op->write_iter;
+		call_iter = nvmet_ns_file(req->ns)->f_op->write_iter;
 		rw = ITER_SOURCE;
 	} else {
-		call_iter = req->ns->file->f_op->read_iter;
+		call_iter = nvmet_ns_file(req->ns)->f_op->read_iter;
 		rw = ITER_DEST;
 	}
 
 	iov_iter_bvec(&iter, rw, req->f.bvec, nr_segs, count);
 
 	iocb->ki_pos = pos;
-	iocb->ki_filp = req->ns->file;
+	iocb->ki_filp = nvmet_ns_file(req->ns);
 	iocb->ki_flags = ki_flags | iocb->ki_filp->f_iocb_flags;
 
 	return call_iter(iocb, &iter);
@@ -241,7 +241,7 @@ static void nvmet_file_execute_rw(struct nvmet_req *req)
 
 	if (req->ns->buffered_io) {
 		if (likely(!req->f.mpool_alloc) &&
-		    (req->ns->file->f_mode & FMODE_NOWAIT) &&
+		    (nvmet_ns_file(req->ns)->f_mode & FMODE_NOWAIT) &&
 		    nvmet_file_execute_io(req, IOCB_NOWAIT))
 			return;
 		nvmet_file_submit_buffered_io(req);
@@ -258,6 +258,19 @@ static void nvmet_file_flush_work(struct work_struct *w)
 {
 	struct nvmet_req *req = container_of(w, struct nvmet_req, f.work);
 
+	/*
+	 * The flush work is scheduled from the I/O hotpath after acquiring
+	 * a reference to the namespace. This ensures that the namespace
+	 * cannot be modified while the I/O is in flight.
+	 *
+	 * nvmet_file_flush() requires callers to hold ns->subsys->lock.
+	 * Since we already hold a namespace reference, explicitly assert
+	 * the lock capability here to satisfy the context analyzer.
+	 *
+	 * Other callers of nvmet_file_flush() that do not hold a namespace
+	 * reference must acquire ns->subsys->lock before invoking it.
+	 */
+	__assume_ctx_lock(&req->ns->subsys->lock);
 	nvmet_req_complete(req, nvmet_file_flush(req));
 }
 
@@ -293,7 +306,7 @@ static void nvmet_file_execute_discard(struct nvmet_req *req)
 			break;
 		}
 
-		ret = vfs_fallocate(req->ns->file, mode, offset, len);
+		ret = vfs_fallocate(nvmet_ns_file(req->ns), mode, offset, len);
 		if (ret && ret != -EOPNOTSUPP) {
 			req->error_slba = le64_to_cpu(range.slba);
 			status = errno_to_nvme_status(req, ret);
@@ -347,7 +360,7 @@ static void nvmet_file_write_zeroes_work(struct work_struct *w)
 		return;
 	}
 
-	ret = vfs_fallocate(req->ns->file, mode, offset, len);
+	ret = vfs_fallocate(nvmet_ns_file(req->ns), mode, offset, len);
 	nvmet_req_complete(req, ret < 0 ? errno_to_nvme_status(req, ret) : 0);
 }
 
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 41912877ade3..74b066ce9ab0 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -152,7 +152,8 @@ struct nvmet_ns {
 	struct file		*bdev_file;
 	struct block_device	*bdev
 		__guarded_by(&subsys->lock);
-	struct file		*file;
+	struct file		*file
+		__guarded_by(&subsys->lock);
 	bool			readonly;
 	u32			nsid;
 	u32			blksize_shift;
@@ -201,6 +202,7 @@ static inline _type nvmet_ns_##_name(struct nvmet_ns *ns)		    \
 }
 
 NVMET_NS_ACCESSOR(struct block_device *, bdev)
+NVMET_NS_ACCESSOR(struct file *, file)
 
 static inline struct nvmet_ns *to_nvmet_ns(struct config_item *item)
 {
@@ -757,17 +759,21 @@ bool nvmet_host_allowed(struct nvmet_subsys *subsys, const char *hostnqn);
 
 int nvmet_bdev_ns_enable(struct nvmet_ns *ns)
 	__must_hold(&ns->subsys->lock);
-int nvmet_file_ns_enable(struct nvmet_ns *ns);
+int nvmet_file_ns_enable(struct nvmet_ns *ns)
+	__must_hold(&ns->subsys->lock);
 void nvmet_bdev_ns_disable(struct nvmet_ns *ns)
 	__must_hold(&ns->subsys->lock);
-void nvmet_file_ns_disable(struct nvmet_ns *ns);
+void nvmet_file_ns_disable(struct nvmet_ns *ns)
+	__must_hold(&ns->subsys->lock);
 u16 nvmet_bdev_flush(struct nvmet_req *req)
 	__must_hold(&req->ns->subsys->lock);
-u16 nvmet_file_flush(struct nvmet_req *req);
+u16 nvmet_file_flush(struct nvmet_req *req)
+	__must_hold(&req->ns->subsys->lock);
 void nvmet_ns_changed(struct nvmet_subsys *subsys, u32 nsid);
 void nvmet_bdev_ns_revalidate(struct nvmet_ns *ns)
 	__must_hold(&ns->subsys->lock);
-void nvmet_file_ns_revalidate(struct nvmet_ns *ns);
+void nvmet_file_ns_revalidate(struct nvmet_ns *ns)
+	__must_hold(&ns->subsys->lock);
 bool nvmet_ns_revalidate(struct nvmet_ns *ns)
 	__must_hold(&ns->subsys->lock);
 u16 blk_to_nvme_status(struct nvmet_req *req, blk_status_t blk_sts);
-- 
2.53.0

Re: [PATCH 03/23] nvmet: add context annotation for nvmet_ns::file

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [Critical] nvmet: slba shift overflow in nvmet_file_execute_discard()
  [1]
--

commit 0b0e76ffccc07fe576097865269793cd78b566d9
Author: Nilay Shroff <nilay@linux.ibm.com>

nvmet: add context annotation for nvmet_ns::file

This commit annotates nvmet_ns::file with __guarded_by(&subsys->lock),
introduces the nvmet_ns_file() helper for lockless I/O hotpath accesses,
and adds __must_hold(&ns->subsys->lock) annotations to the file-backed
namespace helpers.

> diff --git a/drivers/nvme/target/io-cmd-file.c b/drivers/nvme/target/io-cmd-file.c
> index 695963dd932ee..4307232c9160c 100644
> --- a/drivers/nvme/target/io-cmd-file.c
> +++ b/drivers/nvme/target/io-cmd-file.c

[ ... ]

> @@ -293,7 +306,7 @@ static void nvmet_file_execute_discard(struct nvmet_req *req)
>  			break;
>  		}
>  
> -		ret = vfs_fallocate(req->ns->file, mode, offset, len);
> +		ret = vfs_fallocate(nvmet_ns_file(req->ns), mode, offset, len);
>  		if (ret && ret != -EOPNOTSUPP) {
>  			req->error_slba = le64_to_cpu(range.slba);
>  			status = errno_to_nvme_status(req, ret);
> @@ -347,7 +360,7 @@ static void nvmet_file_write_zeroes_work(struct work_struct *w)
>  		return;
>  	}
>  
> -	ret = vfs_fallocate(req->ns->file, mode, offset, len);
> +	ret = vfs_fallocate(nvmet_ns_file(req->ns), mode, offset, len);
>  	nvmet_req_complete(req, ret < 0 ? errno_to_nvme_status(req, ret) : 0);
>  }

[Severity: Critical]
This problem wasn't introduced by this patch, but can shifting slba by
req->ns->blksize_shift in nvmet_file_execute_discard() and
nvmet_file_write_zeroes_work() overflow offset [1]?

drivers/nvme/target/io-cmd-file.c:nvmet_file_execute_discard() {
	...
		offset = le64_to_cpu(range.slba) << req->ns->blksize_shift;
		len = le32_to_cpu(range.nlb);
		len <<= req->ns->blksize_shift;
		if (offset + len > req->ns->size) {
	...
}

drivers/nvme/target/io-cmd-file.c:nvmet_file_write_zeroes_work() {
	...
	offset = le64_to_cpu(write_zeroes->slba) << req->ns->blksize_shift;
	len = (((sector_t)le16_to_cpu(write_zeroes->length) + 1) <<
			req->ns->blksize_shift);

	if (unlikely(offset + len > req->ns->size)) {
	...
}

When a Discard or Write Zeroes command carries non-zero bits in the upper
blksize_shift bits of slba, the left shift discards those bits or wraps
the signed loff_t offset (or offset + len).  If the truncated result falls
within [0, req->ns->size - len], the bounds check is bypassed and
vfs_fallocate() punches holes or zeroes data at an unintended offset in
the backing file instead of failing with NVME_SC_CAP_EXCEEDED.  If offset
wraps to a negative value, vfs_fallocate() fails with -EINVAL and the
request completes with NVME_SC_INTERNAL rather than NVME_SC_CAP_EXCEEDED.

[1] https://sashiko.dev/#/bug/linux-db20adf5-cf8b-4a83-bea6-a689b8eaa9de

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109033102.982191-25-nilay@linux.ibm.com?part=3

Re: [PATCH 03/23] nvmet: add context annotation for nvmet_ns::file

[Bart Van Assche]

On 11/8/26 7:28 PM, Nilay Shroff wrote:
 > @@ -258,6 +258,19 @@ static void nvmet_file_flush_work(struct 
work_struct *w)
 >  {
 >  	struct nvmet_req *req = container_of(w, struct nvmet_req, f.work);
 >
 > +	/*
 > +	 * The flush work is scheduled from the I/O hotpath after acquiring
 > +	 * a reference to the namespace. This ensures that the namespace
 > +	 * cannot be modified while the I/O is in flight.
 > +	 *
 > +	 * nvmet_file_flush() requires callers to hold ns->subsys->lock.
 > +	 * Since we already hold a namespace reference, explicitly assert
 > +	 * the lock capability here to satisfy the context analyzer.
 > +	 *
 > +	 * Other callers of nvmet_file_flush() that do not hold a namespace
 > +	 * reference must acquire ns->subsys->lock before invoking it.
 > +	 */
 > +	__assume_ctx_lock(&req->ns->subsys->lock);
 >  	nvmet_req_complete(req, nvmet_file_flush(req));
 >  }

This assertion is incorrect. __assume_ctx_lock() informs the compiler
that a capability is held when it is actually held (e.g. through an alias).
Here, nvmet_file_flush_work() runs in a workqueue without holding
subsys->lock. If lockdep_assert_held() were used here, Lockdep would
trigger a warning.

nvmet_file_flush() should not be annotated with 
__must_hold(&req->ns->subsys->lock).

Thanks,

Bart.

Re: [PATCH 03/23] nvmet: add context annotation for nvmet_ns::file

[Nilay Shroff]

On 11/10/26 3:23 AM, Bart Van Assche wrote:
> On 11/8/26 7:28 PM, Nilay Shroff wrote:
>  > @@ -258,6 +258,19 @@ static void nvmet_file_flush_work(struct work_struct *w)
>  >  {
>  >      struct nvmet_req *req = container_of(w, struct nvmet_req, f.work);
>  >
>  > +    /*
>  > +     * The flush work is scheduled from the I/O hotpath after acquiring
>  > +     * a reference to the namespace. This ensures that the namespace
>  > +     * cannot be modified while the I/O is in flight.
>  > +     *
>  > +     * nvmet_file_flush() requires callers to hold ns->subsys->lock.
>  > +     * Since we already hold a namespace reference, explicitly assert
>  > +     * the lock capability here to satisfy the context analyzer.
>  > +     *
>  > +     * Other callers of nvmet_file_flush() that do not hold a namespace
>  > +     * reference must acquire ns->subsys->lock before invoking it.
>  > +     */
>  > +    __assume_ctx_lock(&req->ns->subsys->lock);
>  >      nvmet_req_complete(req, nvmet_file_flush(req));
>  >  }
> 
> This assertion is incorrect. __assume_ctx_lock() informs the compiler
> that a capability is held when it is actually held (e.g. through an alias).
> Here, nvmet_file_flush_work() runs in a workqueue without holding
> subsys->lock. If lockdep_assert_held() were used here, Lockdep would
> trigger a warning.
> 
> nvmet_file_flush() should not be annotated with __must_hold(&req->ns->subsys->lock).
> This was a tricky case because nvmet_file_flush() is called from both the I/O
hotpath and the control path. The I/O hotpath holds a namespace reference, so
it does not necessarily need to hold subsys->lock. However, the control path
invokes nvmet_file_flush() while holding subsys->lock.

I agree that using __assume_ctx_lock() in nvmet_file_flush_work() is incorrect,
since the workqueue callback does not hold subsys->lock. I'll remove the
__must_hold() annotation from nvmet_file_flush() and update the code accordingly
in the next revision.

Thanks,
--Nilay

[PATCH 04/23] nvmet: add context annotation for nvmet_ns::bdev_file

[Nilay Shroff]

The nvmet_ns::bdev_file field is protected by nvmet_subsys::lock.
Annotate it with __guarded_by(&subsys->lock) so that the clang
context analyzer can validate accesses to this field.

Signed-off-by: Nilay Shroff <nilay@linux.ibm.com>
---
 drivers/nvme/target/nvmet.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 74b066ce9ab0..b63664250ef4 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -149,7 +149,8 @@ struct nvmet_subsys {
 
 struct nvmet_ns {
 	struct percpu_ref	ref;
-	struct file		*bdev_file;
+	struct file		*bdev_file
+		__guarded_by(&subsys->lock);
 	struct block_device	*bdev
 		__guarded_by(&subsys->lock);
 	struct file		*file
-- 
2.53.0

[PATCH 05/23] nvmet: add context annotation for nvmet_ns::readonly

[Nilay Shroff]

The nvmet_ns::readonly field is protected by nvmet_subsys::lock.
Annotate it with __guarded_by(&subsys->lock) to allow the Clang
context analyzer to validate accesses to this field.

The admin command handlers nvmet_get_feat_write_protect() and
nvmet_set_feat_write_protect() access nvmet_ns::readonly while
holding ns->subsys->lock. However, the context analyzer cannot
recognize that subsys->lock and ns->subsys->lock refer to the
same mutex. Use ns->subsys->lock directly in these functions
to avoid false positive warnings.

The I/O hotpath functions, nvmet_execute_identify_ns(),
nvmet_execute_id_cs_indep() and nvmet_io_cmd_check_access(),
read nvmet_ns::readonly without holding the lock. These lockless
reads may race with updates to the field, potentially observing a
transient value. Such races are acceptable for these accesses.

Introduce nvmet_ns_readonly() to perform these lockless reads
using data_race(READ_ONCE()). This suppresses KCSAN reports for
the intentional races and informs the context analyzer that the
access is intentionally performed without holding the lock.

Use WRITE_ONCE() for updates to nvmet_ns::readonly in
nvmet_set_feat_write_protect() to pair with the lockless
READ_ONCE() accesses.

Signed-off-by: Nilay Shroff <nilay@linux.ibm.com>
---
 drivers/nvme/target/admin-cmd.c | 15 +++++++--------
 drivers/nvme/target/core.c      |  2 +-
 drivers/nvme/target/nvmet.h     | 16 +++++++++++++++-
 3 files changed, 23 insertions(+), 10 deletions(-)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 517f8cc78473..6b0ba383106d 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -938,7 +938,7 @@ static void nvmet_execute_identify_ns(struct nvmet_req *req)
 		id->lbaf[0].ms = cpu_to_le16(req->ns->metadata_size);
 	}
 
-	if (req->ns->readonly)
+	if (nvmet_ns_readonly(req->ns))
 		id->nsattr |= NVME_NS_ATTR_RO;
 done:
 	if (!status)
@@ -1133,7 +1133,7 @@ static void nvmet_execute_id_cs_indep(struct nvmet_req *req)
 	id->nstat = NVME_NSTAT_NRDY;
 	id->anagrpid = cpu_to_le32(req->ns->anagrpid);
 	id->nmic = NVME_NS_NMIC_SHARED;
-	if (req->ns->readonly)
+	if (nvmet_ns_readonly(req->ns))
 		id->nsattr |= NVME_NS_ATTR_RO;
 	if (nvmet_ns_bdev(req->ns) && bdev_rot(nvmet_ns_bdev(req->ns)))
 		id->nsfeat |= NVME_NS_ROTATIONAL;
@@ -1255,13 +1255,13 @@ static u16 nvmet_set_feat_write_protect(struct nvmet_req *req)
 	mutex_lock(&req->ns->subsys->lock);
 	switch (write_protect) {
 	case NVME_NS_WRITE_PROTECT:
-		req->ns->readonly = true;
+		WRITE_ONCE(req->ns->readonly, true);
 		status = nvmet_write_protect_flush_sync(req);
 		if (status)
-			req->ns->readonly = false;
+			WRITE_ONCE(req->ns->readonly, false);
 		break;
 	case NVME_NS_NO_WRITE_PROTECT:
-		req->ns->readonly = false;
+		WRITE_ONCE(req->ns->readonly, false);
 		status = 0;
 		break;
 	default:
@@ -1451,20 +1451,19 @@ void nvmet_execute_set_features(struct nvmet_req *req)
 
 static u16 nvmet_get_feat_write_protect(struct nvmet_req *req)
 {
-	struct nvmet_subsys *subsys = nvmet_req_subsys(req);
 	u32 result;
 
 	result = nvmet_req_find_ns(req);
 	if (result)
 		return result;
 
-	mutex_lock(&subsys->lock);
+	mutex_lock(&req->ns->subsys->lock);
 	if (req->ns->readonly == true)
 		result = NVME_NS_WRITE_PROTECT;
 	else
 		result = NVME_NS_NO_WRITE_PROTECT;
 	nvmet_set_result(req, result);
-	mutex_unlock(&subsys->lock);
+	mutex_unlock(&req->ns->subsys->lock);
 
 	return 0;
 }
diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index ef99c45f0f37..116b1b500790 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -1090,7 +1090,7 @@ static inline u16 nvmet_check_ana_state(struct nvmet_port *port,
 
 static inline u16 nvmet_io_cmd_check_access(struct nvmet_req *req)
 {
-	if (unlikely(req->ns->readonly)) {
+	if (unlikely(nvmet_ns_readonly(req->ns))) {
 		switch (req->cmd->common.opcode) {
 		case nvme_cmd_read:
 		case nvme_cmd_flush:
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index b63664250ef4..b76671672aca 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -155,7 +155,8 @@ struct nvmet_ns {
 		__guarded_by(&subsys->lock);
 	struct file		*file
 		__guarded_by(&subsys->lock);
-	bool			readonly;
+	bool			readonly
+		__guarded_by(&subsys->lock);
 	u32			nsid;
 	u32			blksize_shift;
 	loff_t			size;
@@ -205,6 +206,19 @@ static inline _type nvmet_ns_##_name(struct nvmet_ns *ns)		    \
 NVMET_NS_ACCESSOR(struct block_device *, bdev)
 NVMET_NS_ACCESSOR(struct file *, file)
 
+/*
+ * These accessors use data_race() and READ_ONCE() to allow lockless access to
+ * the corresponding fields. Use them only in code paths where a concurrent
+ * update to the field is harmless and the resulting data race is acceptable.
+ */
+#define NVMET_NS_ACCESSOR_RACE(_type, _name)				\
+static inline _type nvmet_ns_##_name(struct nvmet_ns *ns)		\
+{									\
+	return data_race(READ_ONCE(ns->_name));				\
+}									\
+
+NVMET_NS_ACCESSOR_RACE(bool, readonly)
+
 static inline struct nvmet_ns *to_nvmet_ns(struct config_item *item)
 {
 	return container_of(to_config_group(item), struct nvmet_ns, group);
-- 
2.53.0

[PATCH 06/23] nvmet: add context annotation for nvmet_ns::nsid

[Nilay Shroff]

The nvmet_ns::nsid field is protected by nvmet_subsys::lock.
Annotate it with __guarded_by(&subsys->lock) to allow the Clang
context analyzer to validate accesses to this field.

The I/O hotpath also accesses nvmet_ns::nsid without holding
subsys->lock. This is safe because the namespace reference held by
the I/O request ensures that the namespace cannot be modified while
the I/O is in flight.

Since the context analyzer cannot distinguish between accesses from
the I/O hotpath and the control path, introduce nvmet_ns_nsid(),
annotated with context_unsafe, to allow the I/O hotpath to access
nvmet_ns::nsid without triggering context analyzer warnings. Use
this accessor for accesses from the I/O hotpath.

Additionally, nvmet_max_nsid() and nvmet_ns_alloc() access
nvmet_ns::nsid while holding subsys->lock. Although subsys->lock
and ns->subsys->lock refer to the same lock, the context analyzer
cannot recognize this relationship. Explicitly assert the
ns->subsys->lock capability around these accesses to avoid false
positive warnings.

Annotate nvmet_ns_changed() and nvmet_debugfs_ns_setup() with
__must_hold() to make their locking requirements explicit.

Finally, nvmet_set_feat_write_protect() calls nvmet_ns_changed()
while holding ns->subsys->lock. Since the context analyzer
cannot recognize that this is the same lock as subsys->lock,
explicitly assert the subsys->lock capability before calling
nvmet_ns_changed().

Signed-off-by: Nilay Shroff <nilay@linux.ibm.com>
---
 drivers/nvme/target/admin-cmd.c | 12 ++++++++++--
 drivers/nvme/target/core.c      | 21 +++++++++++++++++++--
 drivers/nvme/target/debugfs.h   |  3 ++-
 drivers/nvme/target/nvmet.h     |  7 +++++--
 4 files changed, 36 insertions(+), 7 deletions(-)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 6b0ba383106d..590ca68588f8 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -922,7 +922,7 @@ static void nvmet_execute_identify_ns(struct nvmet_req *req)
 	 * Since we don't know any better, every namespace is its own endurance
 	 * group.
 	 */
-	id->endgid = cpu_to_le16(req->ns->nsid);
+	id->endgid = cpu_to_le16(nvmet_ns_nsid(req->ns));
 
 	memcpy(&id->nguid, &req->ns->nguid, sizeof(id->nguid));
 
@@ -1268,8 +1268,16 @@ static u16 nvmet_set_feat_write_protect(struct nvmet_req *req)
 		break;
 	}
 
-	if (!status)
+	if (!status) {
+		/*
+		 * Both ns->subsys->lock and subsys->lock refers to the same
+		 * lock. However there's no way for us to annotate such locking
+		 * semantics using clang context annotation. So explicitly
+		 * assert the capability.
+		 */
+		__assume_ctx_lock(&subsys->lock);
 		nvmet_ns_changed(subsys, req->ns->nsid);
+	}
 	mutex_unlock(&req->ns->subsys->lock);
 	return status;
 }
diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index 116b1b500790..2913ed76bc6a 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -118,13 +118,22 @@ u16 nvmet_zero_sgl(struct nvmet_req *req, off_t off, size_t len)
 }
 
 static u32 nvmet_max_nsid(struct nvmet_subsys *subsys)
+	__must_hold(&subsys->lock)
 {
 	struct nvmet_ns *cur;
 	unsigned long idx;
 	u32 nsid = 0;
 
-	nvmet_for_each_enabled_ns(&subsys->namespaces, idx, cur)
+	nvmet_for_each_enabled_ns(&subsys->namespaces, idx, cur) {
+		/*
+		 * Both ns->subsys->lock and subsys->lock refers to the same
+		 * lock. However there's no way for us to annotate such locking
+		 * semantics using clang context annotation. So explicitly
+		 * assert the capability.
+		 */
+		__assume_ctx_lock(&cur->subsys->lock);
 		nsid = cur->nsid;
+	}
 
 	return nsid;
 }
@@ -746,6 +755,12 @@ struct nvmet_ns *nvmet_ns_alloc(struct nvmet_subsys *subsys, u32 nsid)
 
 	init_completion(&ns->disable_done);
 
+	/*
+	 * Both subsys->lock and ns->subsys->lock refers to the same lock.
+	 * However there's no way for us to annotate such locking semantics
+	 * using clang context annotation. So explicitly assert the capability.
+	 */
+	__assume_ctx_lock(&ns->subsys->lock);
 	ns->nsid = nsid;
 	ns->subsys = subsys;
 
@@ -757,6 +772,7 @@ struct nvmet_ns *nvmet_ns_alloc(struct nvmet_subsys *subsys, u32 nsid)
 
 	subsys->nr_namespaces++;
 
+	__release_ctx_lock(&ns->subsys->lock);
 	mutex_unlock(&subsys->lock);
 
 	down_write(&nvmet_ana_sem);
@@ -1361,7 +1377,8 @@ static struct pci_dev *nvmet_req_find_p2p_dev(struct nvmet_req *req)
 	if (!IS_ENABLED(CONFIG_PCI_P2PDMA) ||
 	    !req->sq->ctrl || !req->sq->qid || !req->ns)
 		return NULL;
-	return radix_tree_lookup(&req->sq->ctrl->p2p_ns_map, req->ns->nsid);
+	return radix_tree_lookup(&req->sq->ctrl->p2p_ns_map,
+			nvmet_ns_nsid(req->ns));
 }
 
 int nvmet_req_alloc_sgls(struct nvmet_req *req)
diff --git a/drivers/nvme/target/debugfs.h b/drivers/nvme/target/debugfs.h
index b559d254fc2a..2dde79e8a2ab 100644
--- a/drivers/nvme/target/debugfs.h
+++ b/drivers/nvme/target/debugfs.h
@@ -14,7 +14,8 @@ int nvmet_debugfs_subsys_setup(struct nvmet_subsys *subsys);
 void nvmet_debugfs_subsys_free(struct nvmet_subsys *subsys);
 int nvmet_debugfs_ctrl_setup(struct nvmet_ctrl *ctrl);
 void nvmet_debugfs_ctrl_free(struct nvmet_ctrl *ctrl);
-void nvmet_debugfs_ns_setup(struct nvmet_ns *ns);
+void nvmet_debugfs_ns_setup(struct nvmet_ns *ns)
+	__must_hold(&ns->subsys->lock);
 void nvmet_debugfs_ns_free(struct nvmet_ns *ns);
 
 int __init nvmet_init_debugfs(void);
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index b76671672aca..d03e059d6d16 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -157,7 +157,8 @@ struct nvmet_ns {
 		__guarded_by(&subsys->lock);
 	bool			readonly
 		__guarded_by(&subsys->lock);
-	u32			nsid;
+	u32			nsid
+		__guarded_by(&subsys->lock);
 	u32			blksize_shift;
 	loff_t			size;
 	u8			nguid[16];
@@ -205,6 +206,7 @@ static inline _type nvmet_ns_##_name(struct nvmet_ns *ns)		    \
 
 NVMET_NS_ACCESSOR(struct block_device *, bdev)
 NVMET_NS_ACCESSOR(struct file *, file)
+NVMET_NS_ACCESSOR(u32, nsid)
 
 /*
  * These accessors use data_race() and READ_ONCE() to allow lockless access to
@@ -784,7 +786,8 @@ u16 nvmet_bdev_flush(struct nvmet_req *req)
 	__must_hold(&req->ns->subsys->lock);
 u16 nvmet_file_flush(struct nvmet_req *req)
 	__must_hold(&req->ns->subsys->lock);
-void nvmet_ns_changed(struct nvmet_subsys *subsys, u32 nsid);
+void nvmet_ns_changed(struct nvmet_subsys *subsys, u32 nsid)
+	__must_hold(&subsys->lock);
 void nvmet_bdev_ns_revalidate(struct nvmet_ns *ns)
 	__must_hold(&ns->subsys->lock);
 void nvmet_file_ns_revalidate(struct nvmet_ns *ns)
-- 
2.53.0

Re: [PATCH 06/23] nvmet: add context annotation for nvmet_ns::nsid

[Bart Van Assche]

On 11/8/26 7:28 PM, Nilay Shroff wrote:
 > [ ... ]

1. Bisectability:
    This patch causes Clang analyzer errors in nvmet_format_ana_group(),
    nvmet_execute_identify_endgrp_list(), and 
nvmet_execute_identify_nslist(),
    which are only resolved in patch 07. Please reorder or squash patch 07.

2. Design:
    ns->nsid is set once during nvmet_ns_alloc() and is immutable throughout
    the lifetime of the namespace. It is never modified under 
subsys->lock or
    any other lock. Marking it __guarded_by(&subsys->lock) and creating
    nvmet_ns_nsid() to bypass the annotation throughout the driver adds
    unnecessary churn. Please drop the annotation on nsid.

 > @@ -746,6 +755,12 @@ struct nvmet_ns *nvmet_ns_alloc(struct 
nvmet_subsys *subsys, u32 nsid)
 >
 >  	init_completion(&ns->disable_done);
 >
 > +	__assume_ctx_lock(&ns->subsys->lock);
 >  	ns->nsid = nsid;
 >  	ns->subsys = subsys;

Here and elsewhere, lockdep_assert_held() should be used instead of
__assume_ctx_lock(). Additionally, the lock state should only be
verified after the ns->subsys assignment.

Thanks,

Bart.

Re: [PATCH 06/23] nvmet: add context annotation for nvmet_ns::nsid

[Nilay Shroff]

On 11/10/26 3:26 AM, Bart Van Assche wrote:
> On 11/8/26 7:28 PM, Nilay Shroff wrote:
>  > [ ... ]
> 
> 1. Bisectability:
>     This patch causes Clang analyzer errors in nvmet_format_ana_group(),
>     nvmet_execute_identify_endgrp_list(), and nvmet_execute_identify_nslist(),
>     which are only resolved in patch 07. Please reorder or squash patch 07.
> 
As mentioned in your next comment, I agree that the nsid annotation is unnecessary
and will drop patch 06/23. Since patch 07/23 only addresses access violations
introduced by annotation added in patch 06/23, I'll drop patch 07/23 as well.

That said, patch 07/23 also addresses a separate locking issue: we should hold
subsys->lock while iterating over subsys->namespaces. However, I'll address this
when annotating subsys->namespaces with __guarded_by(&subsys->lock) in a
subsequent series.

> 2. Design:
>     ns->nsid is set once during nvmet_ns_alloc() and is immutable throughout
>     the lifetime of the namespace. It is never modified under subsys->lock or
>     any other lock. Marking it __guarded_by(&subsys->lock) and creating
>     nvmet_ns_nsid() to bypass the annotation throughout the driver adds
>     unnecessary churn. Please drop the annotation on nsid.

Agreed. Although ns->nsid is initialized under subsys->lock in nvmet_ns_alloc(),
this happens before the namespace is enabled or published. Since ns->nsid is never
modified afterwards in any other code path throughout the namespace's lifetime,
annotating it with __guarded_by(&subsys->lock) is unnecessary. I'll drop this
annotation.

>  > @@ -746,6 +755,12 @@ struct nvmet_ns *nvmet_ns_alloc(struct nvmet_subsys *subsys, u32 nsid)
>  >
>  >      init_completion(&ns->disable_done);
>  >
>  > +    __assume_ctx_lock(&ns->subsys->lock);
>  >      ns->nsid = nsid;
>  >      ns->subsys = subsys;
> 
> Here and elsewhere, lockdep_assert_held() should be used instead of
> __assume_ctx_lock(). Additionally, the lock state should only be
> verified after the ns->subsys assignment.
Yes correct, but if we drop this patch then above change would not be needed.

Thanks,
--Nilay

[PATCH 07/23] nvmet: fix nvmet_ns::nsid access using nvmet_subsys::lock

[Nilay Shroff]

nvmet_ns::nsid is protected by nvmet_subsys::lock. However, the Clang
context analyzer reports errors in nvmet_format_ana_group(),
nvmet_execute_identify_endgrp_list() and nvmet_execute_identify_nslist()
because these functions access ns->nsid while iterating over enabled
namespaces without holding the corresponding lock:

drivers/nvme/target/admin-cmd.c:493:44: error: reading variable 'nsid' requires holding mutex 'ns->subsys->lock'
      [-Werror,-Wthread-safety-precise]
  493 |                                 desc->nsids[count++] = cpu_to_le32(ns->nsid);
      |                                                                        ^

drivers/nvme/target/admin-cmd.c:955:11: error: reading variable 'nsid' requires holding mutex 'ns->subsys->lock'
      [-Werror,-Wthread-safety-analysis]
  955 |                 if (ns->nsid <= min_endgid)
      |                         ^

drivers/nvme/target/admin-cmd.c:1002:31: error: reading variable 'nsid' requires holding mutex 'ns->subsys->lock'
      [-Werror,-Wthread-safety-analysis]
 1002 |                 list[i++] = cpu_to_le32(ns->nsid);

Fix above errors, acquiring ctrl->subsys->lock while iterating over
enabled namespaces in these functions to protect accesses to namespace
fields.

Since the context analyzer cannot infer that ctrl->subsys->lock and
ns->subsys->lock refer to the same mutex, explicitly assert the
ns->subsys->lock capability using __assume_ctx_lock() inside each
iteration.

Signed-off-by: Nilay Shroff <nilay@linux.ibm.com>
---
 drivers/nvme/target/admin-cmd.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 590ca68588f8..761caac84255 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -512,10 +512,19 @@ static u32 nvmet_format_ana_group(struct nvmet_req *req, u32 grpid,
 	u32 count = 0;
 
 	if (!(req->cmd->get_log_page.lsp & NVME_ANA_LOG_RGO)) {
+		mutex_lock(&ctrl->subsys->lock);
 		nvmet_for_each_enabled_ns(&ctrl->subsys->namespaces, idx, ns) {
+			/*
+			 * Both ctrl->subsys->lock and ns->subsys->lock refers
+			 * to the same lock. However there's no way for us to
+			 * annotate such locking semantics using clang context
+			 * annotation. So explicitly assert the capability.
+			 */
+			__assume_ctx_lock(&ns->subsys->lock);
 			if (ns->anagrpid == grpid)
 				desc->nsids[count++] = cpu_to_le32(ns->nsid);
 		}
+		mutex_unlock(&ctrl->subsys->lock);
 	}
 
 	desc->grpid = cpu_to_le32(grpid);
@@ -966,7 +975,16 @@ static void nvmet_execute_identify_endgrp_list(struct nvmet_req *req)
 		goto out;
 	}
 
+	mutex_lock(&ctrl->subsys->lock);
 	nvmet_for_each_enabled_ns(&ctrl->subsys->namespaces, idx, ns) {
+		/*
+		 * Both ctrl->subsys->lock and ns->subsys->lock refers to the
+		 * same lock. However there's no way for us to annotate such
+		 * locking semantics using clang context annotation. So
+		 * explicitly assert the capability.
+		 */
+		__assume_ctx_lock(&ns->subsys->lock);
+
 		if (ns->nsid <= min_endgid)
 			continue;
 
@@ -974,6 +992,7 @@ static void nvmet_execute_identify_endgrp_list(struct nvmet_req *req)
 		if (i == buf_size / sizeof(__le16))
 			break;
 	}
+	mutex_unlock(&ctrl->subsys->lock);
 
 	list[0] = cpu_to_le16(i - 1);
 	status = nvmet_copy_to_sgl(req, 0, list, buf_size);
@@ -1009,7 +1028,16 @@ static void nvmet_execute_identify_nslist(struct nvmet_req *req, bool match_css)
 		goto out;
 	}
 
+	mutex_lock(&ctrl->subsys->lock);
 	nvmet_for_each_enabled_ns(&ctrl->subsys->namespaces, idx, ns) {
+		/*
+		 * Both ctrl->subsys->lock and ns->subsys->lock refers to the
+		 * same lock. However there's no way for us to annotate such
+		 * locking semantics using clang context annotation. So
+		 * explicitly assert the capability.
+		 */
+		__assume_ctx_lock(&ns->subsys->lock);
+
 		if (ns->nsid <= min_nsid)
 			continue;
 		if (match_css && ns->csi != req->cmd->identify.csi)
@@ -1018,6 +1046,7 @@ static void nvmet_execute_identify_nslist(struct nvmet_req *req, bool match_css)
 		if (i == buf_size / sizeof(__le32))
 			break;
 	}
+	mutex_unlock(&ctrl->subsys->lock);
 
 	status = nvmet_copy_to_sgl(req, 0, list, buf_size);
 
-- 
2.53.0

[PATCH 08/23] nvmet: add context annotation for nvmet_ns::blksize_shift

[Nilay Shroff]

The nvmet_ns::blksize_shift field is protected by nvmet_subsys::lock.
Annotate it with __guarded_by(&subsys->lock) to allow the Clang context
analyzer to validate accesses to this field.

The I/O hotpath also accesses nvmet_ns::blksize_shift without holding
subsys->lock. This is safe because the namespace reference held by the
I/O request prevents the namespace from being modified while the I/O is
in flight.

Introduce nvmet_ns_blksize_shift(), annotated with __context_unsafe(),
to allow these lockless accesses without triggering context analyzer
warnings. Use this accessor at all relevant call sites that access
blksize_shift without holding subsys->lock, including the namespace
identify, block I/O, file I/O, ZNS, and common LBA/sector conversion
paths.

Signed-off-by: Nilay Shroff <nilay@linux.ibm.com>
---
 drivers/nvme/target/admin-cmd.c   |  4 ++--
 drivers/nvme/target/io-cmd-bdev.c |  4 ++--
 drivers/nvme/target/io-cmd-file.c | 12 +++++++-----
 drivers/nvme/target/nvmet.h       | 10 ++++++----
 drivers/nvme/target/zns.c         |  2 +-
 5 files changed, 18 insertions(+), 14 deletions(-)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 761caac84255..4302dba7c000 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -891,7 +891,7 @@ static void nvmet_execute_identify_ns(struct nvmet_req *req)
 	 * that out from the underlying device.
 	 */
 	id->ncap = id->nsze =
-		cpu_to_le64(req->ns->size >> req->ns->blksize_shift);
+		cpu_to_le64(req->ns->size >> nvmet_ns_blksize_shift(req->ns));
 	switch (req->port->ana_state[req->ns->anagrpid]) {
 	case NVME_ANA_INACCESSIBLE:
 	case NVME_ANA_PERSISTENT_LOSS:
@@ -935,7 +935,7 @@ static void nvmet_execute_identify_ns(struct nvmet_req *req)
 
 	memcpy(&id->nguid, &req->ns->nguid, sizeof(id->nguid));
 
-	id->lbaf[0].ds = req->ns->blksize_shift;
+	id->lbaf[0].ds = nvmet_ns_blksize_shift(req->ns);
 
 	if (req->sq->ctrl->pi_support && nvmet_ns_has_pi(req->ns)) {
 		id->dpc = NVME_NS_DPC_PI_FIRST | NVME_NS_DPC_PI_LAST |
diff --git a/drivers/nvme/target/io-cmd-bdev.c b/drivers/nvme/target/io-cmd-bdev.c
index 7c263cb169a8..e950b442ffc6 100644
--- a/drivers/nvme/target/io-cmd-bdev.c
+++ b/drivers/nvme/target/io-cmd-bdev.c
@@ -393,7 +393,7 @@ static void nvmet_bdev_execute_discard(struct nvmet_req *req)
 			break;
 
 		nr_sects = (sector_t)le32_to_cpu(range.nlb) <<
-			   (ns->blksize_shift - 9);
+			   (nvmet_ns_blksize_shift(ns) - 9);
 		__blkdev_issue_discard(nvmet_ns_bdev(ns),
 				nvmet_lba_to_sect(ns, range.slba), nr_sects,
 				GFP_KERNEL, &bio);
@@ -442,7 +442,7 @@ static void nvmet_bdev_execute_write_zeroes(struct nvmet_req *req)
 
 	sector = nvmet_lba_to_sect(req->ns, write_zeroes->slba);
 	nr_sector = (((sector_t)le16_to_cpu(write_zeroes->length) + 1) <<
-		(req->ns->blksize_shift - 9));
+		(nvmet_ns_blksize_shift(req->ns) - 9));
 
 	ret = __blkdev_issue_zeroout(nvmet_ns_bdev(req->ns), sector, nr_sector,
 			GFP_KERNEL, &bio, 0);
diff --git a/drivers/nvme/target/io-cmd-file.c b/drivers/nvme/target/io-cmd-file.c
index 4307232c9160..90dbfbd16978 100644
--- a/drivers/nvme/target/io-cmd-file.c
+++ b/drivers/nvme/target/io-cmd-file.c
@@ -131,7 +131,7 @@ static bool nvmet_file_execute_io(struct nvmet_req *req, int ki_flags)
 	if (req->f.mpool_alloc && nr_bvec > NVMET_MAX_MPOOL_BVEC)
 		is_sync = true;
 
-	pos = le64_to_cpu(req->cmd->rw.slba) << req->ns->blksize_shift;
+	pos = le64_to_cpu(req->cmd->rw.slba) << nvmet_ns_blksize_shift(req->ns);
 	if (unlikely(pos + req->transfer_len > req->ns->size)) {
 		ret = -ENOSPC;
 		goto complete;
@@ -297,9 +297,10 @@ static void nvmet_file_execute_discard(struct nvmet_req *req)
 		if (status)
 			break;
 
-		offset = le64_to_cpu(range.slba) << req->ns->blksize_shift;
+		offset = le64_to_cpu(range.slba) <<
+				nvmet_ns_blksize_shift(req->ns);
 		len = le32_to_cpu(range.nlb);
-		len <<= req->ns->blksize_shift;
+		len <<= nvmet_ns_blksize_shift(req->ns);
 		if (offset + len > req->ns->size) {
 			req->error_slba = le64_to_cpu(range.slba);
 			status = errno_to_nvme_status(req, -ENOSPC);
@@ -351,9 +352,10 @@ static void nvmet_file_write_zeroes_work(struct work_struct *w)
 	loff_t len;
 	int ret;
 
-	offset = le64_to_cpu(write_zeroes->slba) << req->ns->blksize_shift;
+	offset = le64_to_cpu(write_zeroes->slba) <<
+			nvmet_ns_blksize_shift(req->ns);
 	len = (((sector_t)le16_to_cpu(write_zeroes->length) + 1) <<
-			req->ns->blksize_shift);
+			nvmet_ns_blksize_shift(req->ns));
 
 	if (unlikely(offset + len > req->ns->size)) {
 		nvmet_req_complete(req, errno_to_nvme_status(req, -ENOSPC));
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index d03e059d6d16..1ea0566b8eea 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -159,7 +159,8 @@ struct nvmet_ns {
 		__guarded_by(&subsys->lock);
 	u32			nsid
 		__guarded_by(&subsys->lock);
-	u32			blksize_shift;
+	u32			blksize_shift
+		__guarded_by(&subsys->lock);
 	loff_t			size;
 	u8			nguid[16];
 	uuid_t			uuid;
@@ -207,6 +208,7 @@ static inline _type nvmet_ns_##_name(struct nvmet_ns *ns)		    \
 NVMET_NS_ACCESSOR(struct block_device *, bdev)
 NVMET_NS_ACCESSOR(struct file *, file)
 NVMET_NS_ACCESSOR(u32, nsid)
+NVMET_NS_ACCESSOR(u32, blksize_shift)
 
 /*
  * These accessors use data_race() and READ_ONCE() to allow lockless access to
@@ -807,7 +809,7 @@ void nvmet_bdev_execute_zone_append(struct nvmet_req *req);
 static inline u32 nvmet_rw_data_len(struct nvmet_req *req)
 {
 	return ((u32)le16_to_cpu(req->cmd->rw.length) + 1) <<
-			req->ns->blksize_shift;
+			nvmet_ns_blksize_shift(req->ns);
 }
 
 static inline u32 nvmet_rw_metadata_len(struct nvmet_req *req)
@@ -1000,12 +1002,12 @@ static inline bool nvmet_ns_has_pi(struct nvmet_ns *ns)
 
 static inline __le64 nvmet_sect_to_lba(struct nvmet_ns *ns, sector_t sect)
 {
-	return cpu_to_le64(sect >> (ns->blksize_shift - SECTOR_SHIFT));
+	return cpu_to_le64(sect >> (nvmet_ns_blksize_shift(ns) - SECTOR_SHIFT));
 }
 
 static inline sector_t nvmet_lba_to_sect(struct nvmet_ns *ns, __le64 lba)
 {
-	return le64_to_cpu(lba) << (ns->blksize_shift - SECTOR_SHIFT);
+	return le64_to_cpu(lba) << (nvmet_ns_blksize_shift(ns) - SECTOR_SHIFT);
 }
 
 static inline bool nvmet_use_inline_bvec(struct nvmet_req *req)
diff --git a/drivers/nvme/target/zns.c b/drivers/nvme/target/zns.c
index ce5309453668..9afea37862ef 100644
--- a/drivers/nvme/target/zns.c
+++ b/drivers/nvme/target/zns.c
@@ -122,7 +122,7 @@ void nvmet_execute_identify_ns_zns(struct nvmet_req *req)
 	}
 
 	zsze = (bdev_zone_sectors(nvmet_ns_bdev(req->ns)) << 9) >>
-					req->ns->blksize_shift;
+					nvmet_ns_blksize_shift(req->ns);
 	id_zns->lbafe[0].zsze = cpu_to_le64(zsze);
 
 	mor = bdev_max_open_zones(nvmet_ns_bdev(req->ns));
-- 
2.53.0

[PATCH 09/23] nvmet: add context annotation for nvmet_ns::size

[Nilay Shroff]

The nvmet_ns::size field is protected by nvmet_subsys::lock. Annotate
it with __guarded_by(&subsys->lock) to allow the Clang context analyzer
to validate accesses to this field.

The I/O hotpath also accesses ns->size without holding subsys->lock.
Unlike ns->bdev, which cannot be updated while I/O is in flight due to
the namespace reference, ns->size can be modified concurrently by
nvmet_ns_revalidate_size_store() or nvmet_file_ns_revalidate().
Consequently, lockless reads may observe a transient value of ns->size,
which is acceptable for these access paths.

Introduce nvmet_ns_size() to perform these lockless reads using
data_race(READ_ONCE()) which would silence KCSAN splat as well as help
avoid clang context analyser warning. Use this accessor at the relevant
call sites where accessing ns->size without holding subsys->lock is
harmless despite concurrent updates.

Pair these lockless reads with WRITE_ONCE() for all updates to ns->size.
Also use READ_ONCE() in nvmet_ns_revalidate() when reading the old and
new size values to detect changes.

Signed-off-by: Nilay Shroff <nilay@linux.ibm.com>
---
 drivers/nvme/target/admin-cmd.c   |  3 ++-
 drivers/nvme/target/core.c        |  4 ++--
 drivers/nvme/target/io-cmd-bdev.c |  4 ++--
 drivers/nvme/target/io-cmd-file.c | 10 +++++-----
 drivers/nvme/target/nvmet.h       |  4 +++-
 5 files changed, 14 insertions(+), 11 deletions(-)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 4302dba7c000..7cdf13351678 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -891,7 +891,8 @@ static void nvmet_execute_identify_ns(struct nvmet_req *req)
 	 * that out from the underlying device.
 	 */
 	id->ncap = id->nsze =
-		cpu_to_le64(req->ns->size >> nvmet_ns_blksize_shift(req->ns));
+		cpu_to_le64(nvmet_ns_size(req->ns) >>
+				nvmet_ns_blksize_shift(req->ns));
 	switch (req->port->ana_state[req->ns->anagrpid]) {
 	case NVME_ANA_INACCESSIBLE:
 	case NVME_ANA_PERSISTENT_LOSS:
diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index 2913ed76bc6a..77e2ef211711 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -591,14 +591,14 @@ static void nvmet_p2pmem_ns_add_p2p(struct nvmet_ctrl *ctrl,
 
 bool nvmet_ns_revalidate(struct nvmet_ns *ns)
 {
-	loff_t oldsize = ns->size;
+	loff_t oldsize = READ_ONCE(ns->size);
 
 	if (ns->bdev)
 		nvmet_bdev_ns_revalidate(ns);
 	else
 		nvmet_file_ns_revalidate(ns);
 
-	return oldsize != ns->size;
+	return oldsize != READ_ONCE(ns->size);
 }
 
 int nvmet_ns_enable(struct nvmet_ns *ns)
diff --git a/drivers/nvme/target/io-cmd-bdev.c b/drivers/nvme/target/io-cmd-bdev.c
index e950b442ffc6..0ed691387f1c 100644
--- a/drivers/nvme/target/io-cmd-bdev.c
+++ b/drivers/nvme/target/io-cmd-bdev.c
@@ -115,7 +115,7 @@ int nvmet_bdev_ns_enable(struct nvmet_ns *ns)
 		return ret;
 	}
 	ns->bdev = file_bdev(ns->bdev_file);
-	ns->size = bdev_nr_bytes(ns->bdev);
+	WRITE_ONCE(ns->size, bdev_nr_bytes(ns->bdev));
 	ns->blksize_shift = blksize_bits(bdev_logical_block_size(ns->bdev));
 
 	ns->pi_type = 0;
@@ -136,7 +136,7 @@ int nvmet_bdev_ns_enable(struct nvmet_ns *ns)
 
 void nvmet_bdev_ns_revalidate(struct nvmet_ns *ns)
 {
-	ns->size = bdev_nr_bytes(ns->bdev);
+	WRITE_ONCE(ns->size, bdev_nr_bytes(ns->bdev));
 }
 
 u16 blk_to_nvme_status(struct nvmet_req *req, blk_status_t blk_sts)
diff --git a/drivers/nvme/target/io-cmd-file.c b/drivers/nvme/target/io-cmd-file.c
index 90dbfbd16978..224b7423b780 100644
--- a/drivers/nvme/target/io-cmd-file.c
+++ b/drivers/nvme/target/io-cmd-file.c
@@ -15,7 +15,7 @@
 
 void nvmet_file_ns_revalidate(struct nvmet_ns *ns)
 {
-	ns->size = i_size_read(ns->file->f_mapping->host);
+	WRITE_ONCE(ns->size, i_size_read(ns->file->f_mapping->host));
 }
 
 void nvmet_file_ns_disable(struct nvmet_ns *ns)
@@ -68,7 +68,7 @@ int nvmet_file_ns_enable(struct nvmet_ns *ns)
 err:
 	fput(ns->file);
 	ns->file = NULL;
-	ns->size = 0;
+	WRITE_ONCE(ns->size, 0);
 	ns->blksize_shift = 0;
 	return ret;
 }
@@ -132,7 +132,7 @@ static bool nvmet_file_execute_io(struct nvmet_req *req, int ki_flags)
 		is_sync = true;
 
 	pos = le64_to_cpu(req->cmd->rw.slba) << nvmet_ns_blksize_shift(req->ns);
-	if (unlikely(pos + req->transfer_len > req->ns->size)) {
+	if (unlikely(pos + req->transfer_len > nvmet_ns_size(req->ns))) {
 		ret = -ENOSPC;
 		goto complete;
 	}
@@ -301,7 +301,7 @@ static void nvmet_file_execute_discard(struct nvmet_req *req)
 				nvmet_ns_blksize_shift(req->ns);
 		len = le32_to_cpu(range.nlb);
 		len <<= nvmet_ns_blksize_shift(req->ns);
-		if (offset + len > req->ns->size) {
+		if (offset + len > nvmet_ns_size(req->ns)) {
 			req->error_slba = le64_to_cpu(range.slba);
 			status = errno_to_nvme_status(req, -ENOSPC);
 			break;
@@ -357,7 +357,7 @@ static void nvmet_file_write_zeroes_work(struct work_struct *w)
 	len = (((sector_t)le16_to_cpu(write_zeroes->length) + 1) <<
 			nvmet_ns_blksize_shift(req->ns));
 
-	if (unlikely(offset + len > req->ns->size)) {
+	if (unlikely(offset + len > nvmet_ns_size(req->ns))) {
 		nvmet_req_complete(req, errno_to_nvme_status(req, -ENOSPC));
 		return;
 	}
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 1ea0566b8eea..59adf99470f8 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -161,7 +161,8 @@ struct nvmet_ns {
 		__guarded_by(&subsys->lock);
 	u32			blksize_shift
 		__guarded_by(&subsys->lock);
-	loff_t			size;
+	loff_t			size
+		__guarded_by(&subsys->lock);
 	u8			nguid[16];
 	uuid_t			uuid;
 	u32			anagrpid;
@@ -222,6 +223,7 @@ static inline _type nvmet_ns_##_name(struct nvmet_ns *ns)		\
 }									\
 
 NVMET_NS_ACCESSOR_RACE(bool, readonly)
+NVMET_NS_ACCESSOR_RACE(loff_t, size)
 
 static inline struct nvmet_ns *to_nvmet_ns(struct config_item *item)
 {
-- 
2.53.0

[PATCH 10/23] nvmet: add context annotation for nvmet_ns::nguid

[Nilay Shroff]

The nvmet_ns::nguid field is protected by nvmet_subsys::lock. Annotate
it with __guarded_by(&subsys->lock) so that the clang context analyzer
can validate accesses to this field.

Some accesses to nvmet_ns::nguid in the I/O hotpath do not require
holding nvmet_subsys::lock, as the namespace reference guarantees that
the namespace remains valid and the field is not modified while I/O is
in flight. However, the clang context analyzer cannot express this
access pattern directly.

Introduce nvmet_ns_nguid() as an inline helper annotated with
__context_unsafe() to allow these accesses without requiring the
subsystem lock. Convert the relevant accesses in the I/O hotpath and
request-processing paths to use this helper.

Additionally, acquire ns->subsys->lock in nvmet_ns_device_nguid_show()
before accessing ns->nguid, since this configfs control path does not
hold the lock.

Signed-off-by: Nilay Shroff <nilay@linux.ibm.com>
---
 drivers/nvme/target/admin-cmd.c | 7 ++++---
 drivers/nvme/target/configfs.c  | 9 ++++++++-
 drivers/nvme/target/nvmet.h     | 4 +++-
 3 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 7cdf13351678..d784498ca720 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -934,7 +934,7 @@ static void nvmet_execute_identify_ns(struct nvmet_req *req)
 	 */
 	id->endgid = cpu_to_le16(nvmet_ns_nsid(req->ns));
 
-	memcpy(&id->nguid, &req->ns->nguid, sizeof(id->nguid));
+	memcpy(&id->nguid, nvmet_ns_nguid(req->ns), sizeof(id->nguid));
 
 	id->lbaf[0].ds = nvmet_ns_blksize_shift(req->ns);
 
@@ -1094,10 +1094,11 @@ static void nvmet_execute_identify_desclist(struct nvmet_req *req)
 		if (status)
 			goto out;
 	}
-	if (memchr_inv(req->ns->nguid, 0, sizeof(req->ns->nguid))) {
+	if (memchr_inv(nvmet_ns_nguid(req->ns), 0, sizeof(req->ns->nguid))) {
 		status = nvmet_copy_ns_identifier(req, NVME_NIDT_NGUID,
 						  NVME_NIDT_NGUID_LEN,
-						  &req->ns->nguid, &off);
+						  nvmet_ns_nguid(req->ns),
+						  &off);
 		if (status)
 			goto out;
 	}
diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index 27c50fa533e7..cd2c301bc69a 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -643,7 +643,14 @@ CONFIGFS_ATTR(nvmet_ns_, device_uuid);
 
 static ssize_t nvmet_ns_device_nguid_show(struct config_item *item, char *page)
 {
-	return sprintf(page, "%pUb\n", &to_nvmet_ns(item)->nguid);
+	struct nvmet_ns *ns = to_nvmet_ns(item);
+	ssize_t ret;
+
+	mutex_lock(&ns->subsys->lock);
+	ret = sprintf(page, "%pUb\n", &ns->nguid);
+	mutex_unlock(&ns->subsys->lock);
+
+	return ret;
 }
 
 static ssize_t nvmet_ns_device_nguid_store(struct config_item *item,
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 59adf99470f8..21ed96b39a31 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -163,7 +163,8 @@ struct nvmet_ns {
 		__guarded_by(&subsys->lock);
 	loff_t			size
 		__guarded_by(&subsys->lock);
-	u8			nguid[16];
+	u8			nguid[16]
+		__guarded_by(&subsys->lock);
 	uuid_t			uuid;
 	u32			anagrpid;
 
@@ -210,6 +211,7 @@ NVMET_NS_ACCESSOR(struct block_device *, bdev)
 NVMET_NS_ACCESSOR(struct file *, file)
 NVMET_NS_ACCESSOR(u32, nsid)
 NVMET_NS_ACCESSOR(u32, blksize_shift)
+NVMET_NS_ACCESSOR(u8 *, nguid)
 
 /*
  * These accessors use data_race() and READ_ONCE() to allow lockless access to
-- 
2.53.0

[PATCH 11/23] nvmet: add context annotation for nvmet_ns::uuid

[Nilay Shroff]

The nvmet_ns::uuid field is protected by nvmet_subsys::lock. Annotate
it with __guarded_by(&subsys->lock) so that the clang context analyzer
can validate accesses to this field.

Some accesses to nvmet_ns::uuid in the I/O hotpath do not require
holding nvmet_subsys::lock, as the namespace reference guarantees that
the namespace remains valid and the field is not modified while I/O is
in flight. However, the clang context analyzer cannot express this
access pattern directly.

Introduce nvmet_ns_uuid() as an inline helper annotated with
__context_unsafe() to allow these accesses without requiring the
subsystem lock. Convert the relevant accesses in the I/O hotpath and
request-processing paths to use this helper.

Additionally, acquire ns->subsys->lock in nvmet_ns_device_uuid_show()
before accessing ns->uuid, since this configfs control path does not
hold the lock.

Signed-off-by: Nilay Shroff <nilay@linux.ibm.com>
---
 drivers/nvme/target/admin-cmd.c | 6 ++++--
 drivers/nvme/target/configfs.c  | 9 ++++++++-
 drivers/nvme/target/nvmet.h     | 4 +++-
 3 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index d784498ca720..b8015054de46 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -1082,15 +1082,17 @@ static void nvmet_execute_identify_desclist(struct nvmet_req *req)
 {
 	off_t off = 0;
 	u16 status;
+	uuid_t uuid;
 
 	status = nvmet_req_find_ns(req);
 	if (status)
 		goto out;
 
-	if (memchr_inv(&req->ns->uuid, 0, sizeof(req->ns->uuid))) {
+	uuid = nvmet_ns_uuid(req->ns);
+	if (memchr_inv(&uuid, 0, sizeof(uuid))) {
 		status = nvmet_copy_ns_identifier(req, NVME_NIDT_UUID,
 						  NVME_NIDT_UUID_LEN,
-						  &req->ns->uuid, &off);
+						  &uuid, &off);
 		if (status)
 			goto out;
 	}
diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index cd2c301bc69a..f8a4d0d88946 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -615,7 +615,14 @@ CONFIGFS_ATTR(nvmet_ns_, p2pmem);
 
 static ssize_t nvmet_ns_device_uuid_show(struct config_item *item, char *page)
 {
-	return sprintf(page, "%pUb\n", &to_nvmet_ns(item)->uuid);
+	struct nvmet_ns *ns = to_nvmet_ns(item);
+	ssize_t size;
+
+	mutex_lock(&ns->subsys->lock);
+	size = sprintf(page, "%pUb\n", &ns->uuid);
+	mutex_unlock(&ns->subsys->lock);
+
+	return size;
 }
 
 static ssize_t nvmet_ns_device_uuid_store(struct config_item *item,
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 21ed96b39a31..3e18c36e83c8 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -165,7 +165,8 @@ struct nvmet_ns {
 		__guarded_by(&subsys->lock);
 	u8			nguid[16]
 		__guarded_by(&subsys->lock);
-	uuid_t			uuid;
+	uuid_t			uuid
+		__guarded_by(&subsys->lock);
 	u32			anagrpid;
 
 	bool			buffered_io;
@@ -212,6 +213,7 @@ NVMET_NS_ACCESSOR(struct file *, file)
 NVMET_NS_ACCESSOR(u32, nsid)
 NVMET_NS_ACCESSOR(u32, blksize_shift)
 NVMET_NS_ACCESSOR(u8 *, nguid)
+NVMET_NS_ACCESSOR(uuid_t, uuid)
 
 /*
  * These accessors use data_race() and READ_ONCE() to allow lockless access to
-- 
2.53.0

Re: [PATCH 11/23] nvmet: add context annotation for nvmet_ns::uuid

[Bart Van Assche]

On 11/8/26 7:28 PM, Nilay Shroff wrote:
> [ ... ]

This patch breaks compilation in nvmet_ns_alloc() because 
uuid_gen(&ns->uuid)
is called without holding subsys->lock. This is fixed only in patch 12.
Please move patch 12 before patch 11 (or squash them).
In nvmet_ns_device_uuid_show(), please use guard(mutex)(&ns->subsys->lock).

Thanks,

Bart.

Re: [PATCH 11/23] nvmet: add context annotation for nvmet_ns::uuid

[Nilay Shroff]

On 11/10/26 3:28 AM, Bart Van Assche wrote:
> On 11/8/26 7:28 PM, Nilay Shroff wrote:
>> [ ... ]
> 
> This patch breaks compilation in nvmet_ns_alloc() because uuid_gen(&ns->uuid)
> is called without holding subsys->lock. This is fixed only in patch 12.
> Please move patch 12 before patch 11 (or squash them).

The intention of this series is to first annotate a field in struct nvmet_ns
and then address any locking violations reported by the clang context analyzer
in a subsequent patch. I split the annotation and the corresponding locking
changes into separate patches to make the changes easier to review.

Also, since I haven't enabled CONTEXT_ANALYSIS:= y in the NVMe target Makefile yet,
enabling CONFIG_WARN_CONTEXT_ANALYSIS alone should not cause this compilation
failure.

> In nvmet_ns_device_uuid_show(), please use guard(mutex)(&ns->subsys->lock).
> Do you think using guard(...) is more appropriate then simple mutex_lock()
and mutex_unlock() APIs? I find using conventional mutex_lock()/mutex_unlcok()
APIs more readable and easy to reason about then using guard(...). That said,
if others as well prefer using guard() them I'm open to switch to it.

Thanks,
--Nilay

[PATCH 12/23] nvmet: fix nvmet_ns::uuid access using nvmet_subsys::lock

[Nilay Shroff]

The nvmet_ns::uuid field is protected by nvmet_subsys::lock. However,
nvmet_ns_alloc() calls uuid_gen() without holding this lock, triggering
a Clang context analyzer warning:

drivers/nvme/target/core.c:781:16: error: passing pointer to variable 'uuid' requires holding mutex '_res->subsys->lock'
      [-Werror,-Wthread-safety-pointer]
  781 |         uuid_gen(&ns->uuid);
      |                       ^

Fix this by moving the uuid_gen() call into the existing subsys->lock
critical section in nvmet_ns_alloc().

Signed-off-by: Nilay Shroff <nilay@linux.ibm.com>
---
 drivers/nvme/target/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index 77e2ef211711..08a70439fd55 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -771,6 +771,7 @@ struct nvmet_ns *nvmet_ns_alloc(struct nvmet_subsys *subsys, u32 nsid)
 		goto out_exit;
 
 	subsys->nr_namespaces++;
+	uuid_gen(&ns->uuid);
 
 	__release_ctx_lock(&ns->subsys->lock);
 	mutex_unlock(&subsys->lock);
@@ -780,7 +781,6 @@ struct nvmet_ns *nvmet_ns_alloc(struct nvmet_subsys *subsys, u32 nsid)
 	nvmet_ana_group_enabled[ns->anagrpid]++;
 	up_write(&nvmet_ana_sem);
 
-	uuid_gen(&ns->uuid);
 	ns->buffered_io = false;
 	ns->csi = NVME_CSI_NVM;
 
-- 
2.53.0

[PATCH 13/23] nvmet: add context annotation for nvmet_ns::buffered_io

[Nilay Shroff]

The nvmet_ns::buffered_io field is protected by nvmet_subsys::lock.
Annotate it with __guarded_by(&subsys->lock) so that the clang context
analyzer can validate accesses to this field.

Some accesses to nvmet_ns::buffered_io in the I/O hotpath do not require
holding nvmet_subsys::lock, as the namespace reference guarantees that
the namespace remains valid and the field is not modified while I/O is
in flight. However, the clang context analyzer cannot express this
access pattern directly.

Introduce nvmet_ns_buffered_io() as an inline helper annotated with
__context_unsafe() to allow these accesses without requiring the
subsystem lock. Convert the relevant accesses in the I/O hotpath and
request-processing paths to use this helper.

Signed-off-by: Nilay Shroff <nilay@linux.ibm.com>
---
 drivers/nvme/target/io-cmd-file.c | 2 +-
 drivers/nvme/target/nvmet.h       | 4 +++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/target/io-cmd-file.c b/drivers/nvme/target/io-cmd-file.c
index 224b7423b780..295203f10c2e 100644
--- a/drivers/nvme/target/io-cmd-file.c
+++ b/drivers/nvme/target/io-cmd-file.c
@@ -239,7 +239,7 @@ static void nvmet_file_execute_rw(struct nvmet_req *req)
 	} else
 		req->f.mpool_alloc = false;
 
-	if (req->ns->buffered_io) {
+	if (nvmet_ns_buffered_io(req->ns)) {
 		if (likely(!req->f.mpool_alloc) &&
 		    (nvmet_ns_file(req->ns)->f_mode & FMODE_NOWAIT) &&
 		    nvmet_file_execute_io(req, IOCB_NOWAIT))
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 3e18c36e83c8..572f495b06df 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -169,7 +169,8 @@ struct nvmet_ns {
 		__guarded_by(&subsys->lock);
 	u32			anagrpid;
 
-	bool			buffered_io;
+	bool			buffered_io
+		__guarded_by(&subsys->lock);
 	bool			enabled;
 #define NVMET_NS_IO_LIVE	0
 	unsigned long		flags;
@@ -214,6 +215,7 @@ NVMET_NS_ACCESSOR(u32, nsid)
 NVMET_NS_ACCESSOR(u32, blksize_shift)
 NVMET_NS_ACCESSOR(u8 *, nguid)
 NVMET_NS_ACCESSOR(uuid_t, uuid)
+NVMET_NS_ACCESSOR(bool, buffered_io)
 
 /*
  * These accessors use data_race() and READ_ONCE() to allow lockless access to
-- 
2.53.0

[PATCH 14/23] nvmet: fix nvmet_ns::buffered_io access using nvmet_subsys::lock

[Nilay Shroff]

The nvmet_ns::buffered_io field is protected by nvmet_subsys::lock.
However, nvmet_ns_alloc() initializes this field to false without
holding the corresponding lock, triggering a Clang context analyzer
warning:

drivers/nvme/target/core.c:782:6: error: writing variable 'buffered_io' requires holding mutex '_res->subsys->lock' exclusively
      [-Werror,-Wthread-safety-analysis]
  782 |         ns->buffered_io = false;
      |             ^

Fix this by removing the initialization of ns->buffered_io as we zero
alloc namespace struct and so ideally we do not need to initialize
ns->buffered_io to false in nvmet_ns_alloc().

Additionally, nvmet_ns_buffered_io_show() accesses ns->buffered_io
without holding subsys->lock, triggering another context analyzer
warning:

drivers/nvme/target/configfs.c:774:50: error: reading variable 'buffered_io' requires holding mutex 'to_nvmet_ns(item).subsys->lock'
      [-Werror,-Wthread-safety-analysis]
  774 |         return sprintf(page, "%d\n", to_nvmet_ns(item)->buffered_io);

Fix this by acquiring ns->subsys->lock before reading the field.

Signed-off-by: Nilay Shroff <nilay@linux.ibm.com>
---
 drivers/nvme/target/configfs.c | 9 ++++++++-
 drivers/nvme/target/core.c     | 1 -
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index f8a4d0d88946..34d79fadbf15 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -770,7 +770,14 @@ CONFIGFS_ATTR(nvmet_ns_, enable);
 
 static ssize_t nvmet_ns_buffered_io_show(struct config_item *item, char *page)
 {
-	return sprintf(page, "%d\n", to_nvmet_ns(item)->buffered_io);
+	struct nvmet_ns *ns = to_nvmet_ns(item);
+	ssize_t ret;
+
+	mutex_lock(&ns->subsys->lock);
+	ret = sprintf(page, "%d\n", ns->buffered_io);
+	mutex_unlock(&ns->subsys->lock);
+
+	return ret;
 }
 
 static ssize_t nvmet_ns_buffered_io_store(struct config_item *item,
diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index 08a70439fd55..805b5f70eb06 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -781,7 +781,6 @@ struct nvmet_ns *nvmet_ns_alloc(struct nvmet_subsys *subsys, u32 nsid)
 	nvmet_ana_group_enabled[ns->anagrpid]++;
 	up_write(&nvmet_ana_sem);
 
-	ns->buffered_io = false;
 	ns->csi = NVME_CSI_NVM;
 
 	return ns;
-- 
2.53.0

[PATCH 15/23] nvmet: add context annotation for nvmet_ns::enabled

[Nilay Shroff]

The nvmet_ns::enabled field is protected by nvmet_subsys::lock.
Annotate it with __guarded_by(&subsys->lock) so that the clang
context analyzer can validate accesses to this field.

Signed-off-by: Nilay Shroff <nilay@linux.ibm.com>
---
 drivers/nvme/target/nvmet.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 572f495b06df..a6fb3aab0c5d 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -171,7 +171,8 @@ struct nvmet_ns {
 
 	bool			buffered_io
 		__guarded_by(&subsys->lock);
-	bool			enabled;
+	bool			enabled
+		__guarded_by(&subsys->lock);
 #define NVMET_NS_IO_LIVE	0
 	unsigned long		flags;
 	struct nvmet_subsys	*subsys;
-- 
2.53.0

[PATCH 16/23] nvmet: fix access to nvmet_ns::enable using nvmet_subsys::lock

[Nilay Shroff]

nvmet_ns::enable is protected by nvmet_subsys::lock. The clang
context analyser complains about accessing ns->enabled without
subsys->lock in nvmet_ns_enable_show():

drivers/nvme/target/configfs.c:741:50: error: reading variable 'enabled' requires holding mutex 'to_nvmet_ns(item).subsys->lock'
      [-Werror,-Wthread-safety-analysis]
  741 |         return sprintf(page, "%d\n", to_nvmet_ns(item)->enabled);
      |                                                         ^

Fix the above error by acquiring ns->susbsys->lock before
accessing ns->enabled in nvmet_ns_enable_show().

Signed-off-by: Nilay Shroff <nilay@linux.ibm.com>
---
 drivers/nvme/target/configfs.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index 34d79fadbf15..ba44d404ad56 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -737,7 +737,14 @@ CONFIGFS_ATTR(nvmet_ns_, ana_grpid);
 
 static ssize_t nvmet_ns_enable_show(struct config_item *item, char *page)
 {
-	return sprintf(page, "%d\n", to_nvmet_ns(item)->enabled);
+	struct nvmet_ns *ns = to_nvmet_ns(item);
+	ssize_t ret;
+
+	mutex_lock(&ns->subsys->lock);
+	ret = sprintf(page, "%d\n", ns->enabled);
+	mutex_unlock(&ns->subsys->lock);
+
+	return ret;
 }
 
 static ssize_t nvmet_ns_enable_store(struct config_item *item,
-- 
2.53.0

[PATCH 17/23] nvmet: add context annotation for nvmet_ns::device_path

[Nilay Shroff]

The nvmet_ns::device_path field is protected by
nvmet_subsys::lock. Annotate it with
__guarded_by(&subsys->lock) to allow the Clang context
analyzer to validate accesses to this field.

Some request-processing paths access ns->device_path without
holding subsys->lock. This is safe because the namespace
reference held by the request prevents the ns->device_path from
being modified while I/O is in flight. However, the context
analyzer cannot express this access pattern directly.

Introduce nvmet_ns_device_path() as an inline helper
annotated with __context_unsafe() to allow these lockless
accesses without triggering context analyzer warnings.

Additionally, update __assign_req_name() to use this accessor
when copying device_path into the trace event's request name.
This is safe because the tracing function is invoked after
the namespace reference has been acquired.

Signed-off-by: Nilay Shroff <nilay@linux.ibm.com>
---
 drivers/nvme/target/nvmet.h | 4 +++-
 drivers/nvme/target/trace.h | 2 +-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index a6fb3aab0c5d..fa932c1fbf29 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -176,7 +176,8 @@ struct nvmet_ns {
 #define NVMET_NS_IO_LIVE	0
 	unsigned long		flags;
 	struct nvmet_subsys	*subsys;
-	const char		*device_path;
+	const char		*device_path
+		__guarded_by(&subsys->lock);
 
 	struct config_group	device_group;
 	struct config_group	group;
@@ -217,6 +218,7 @@ NVMET_NS_ACCESSOR(u32, blksize_shift)
 NVMET_NS_ACCESSOR(u8 *, nguid)
 NVMET_NS_ACCESSOR(uuid_t, uuid)
 NVMET_NS_ACCESSOR(bool, buffered_io)
+NVMET_NS_ACCESSOR(const char *, device_path)
 
 /*
  * These accessors use data_race() and READ_ONCE() to allow lockless access to
diff --git a/drivers/nvme/target/trace.h b/drivers/nvme/target/trace.h
index 7f7ebf9558e5..b15294e24793 100644
--- a/drivers/nvme/target/trace.h
+++ b/drivers/nvme/target/trace.h
@@ -59,7 +59,7 @@ static inline void __assign_req_name(char *name, struct nvmet_req *req)
 		return;
 	}
 
-	strscpy_pad(name, req->ns->device_path, DISK_NAME_LEN);
+	strscpy_pad(name, nvmet_ns_device_path(req->ns), DISK_NAME_LEN);
 }
 #endif
 
-- 
2.53.0

[PATCH 18/23] nvmet: fix nvmet_ns::device_path access using nvmet_subsys::lock

[Nilay Shroff]

The nvmet_ns::device_path field is protected by nvmet_subsys::lock.
However, nvmet_ns_device_path_show() accesses ns->device_path without
holding the corresponding lock, triggering a Clang context analyzer
error:

drivers/nvme/target/configfs.c:535:50: error: reading variable 'device_path' requires holding mutex 'to_nvmet_ns(item).subsys->lock'
      [-Werror,-Wthread-safety-analysis]
  535 |         return sprintf(page, "%s\n", to_nvmet_ns(item)->device_path);
      |                                                         ^

Fix this by acquiring ns->subsys->lock before accessing ns->device_path
in nvmet_ns_device_path_show().

Additionally, nvmet_ns_free() frees ns->device_path without holding
subsys->lock, triggering another context analyzer error:

drivers/nvme/target/core.c:737:12: error: reading variable 'device_path' requires holding mutex 'ns->subsys->lock'
      [-Werror,-Wthread-safety-analysis]
  737 |         kfree(ns->device_path);
      |                   ^

Fix the above error, by moving the kfree() call into the existing
subsys->lock critical section to ensure that the field is accessed
while holding the required lock.

Signed-off-by: Nilay Shroff <nilay@linux.ibm.com>
---
 drivers/nvme/target/configfs.c | 9 ++++++++-
 drivers/nvme/target/core.c     | 2 +-
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index ba44d404ad56..a87917381093 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -531,7 +531,14 @@ CONFIGFS_ATTR(nvmet_, addr_tsas);
  */
 static ssize_t nvmet_ns_device_path_show(struct config_item *item, char *page)
 {
-	return sprintf(page, "%s\n", to_nvmet_ns(item)->device_path);
+	struct nvmet_ns *ns = to_nvmet_ns(item);
+	ssize_t ret;
+
+	mutex_lock(&ns->subsys->lock);
+	ret = sprintf(page, "%s\n", ns->device_path);
+	mutex_unlock(&ns->subsys->lock);
+
+	return ret;
 }
 
 static ssize_t nvmet_ns_device_path_store(struct config_item *item,
diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index 805b5f70eb06..6b11beb2e2b2 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -730,13 +730,13 @@ void nvmet_ns_free(struct nvmet_ns *ns)
 		subsys->max_nsid = nvmet_max_nsid(subsys);
 
 	subsys->nr_namespaces--;
+	kfree(ns->device_path);
 	mutex_unlock(&subsys->lock);
 
 	down_write(&nvmet_ana_sem);
 	nvmet_ana_group_enabled[ns->anagrpid]--;
 	up_write(&nvmet_ana_sem);
 
-	kfree(ns->device_path);
 	kfree(ns);
 }
 
-- 
2.53.0

Re: [PATCH 18/23] nvmet: fix nvmet_ns::device_path access using nvmet_subsys::lock

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [Critical] nvmet: use-after-free in nvmet_req_find_ns() [1]
--

commit a468df0df47780f4511f3eba5f46e2ca65355d7b
Author: Nilay Shroff <nilay@linux.ibm.com>

nvmet: fix nvmet_ns::device_path access using nvmet_subsys::lock

This commit acquires subsys->lock in nvmet_ns_device_path_show() and moves
kfree(ns->device_path) under subsys->lock in nvmet_ns_free() to satisfy
Clang thread-safety analysis for nvmet_ns::device_path.

> diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
> index 805b5f70eb066..6b11beb2e2b20 100644
> --- a/drivers/nvme/target/core.c
> +++ b/drivers/nvme/target/core.c
> @@ -730,13 +730,13 @@ void nvmet_ns_free(struct nvmet_ns *ns)
>  		subsys->max_nsid = nvmet_max_nsid(subsys);
>  
>  	subsys->nr_namespaces--;
> +	kfree(ns->device_path);
>  	mutex_unlock(&subsys->lock);
>  
>  	down_write(&nvmet_ana_sem);
>  	nvmet_ana_group_enabled[ns->anagrpid]--;
>  	up_write(&nvmet_ana_sem);
>  
> -	kfree(ns->device_path);
>  	kfree(ns);
>  }

[Severity: Critical]
This problem wasn't introduced by this patch, but can freeing ns immediately
after xa_erase() in nvmet_ns_free() cause a use-after-free in
nvmet_req_find_ns() [1]?

nvmet_ns_disable() waits for the RCU grace period (via percpu_ref_kill())
while ns is still published in subsys->namespaces.  nvmet_ns_free() then
erases the XArray entry and synchronously calls kfree(ns) without waiting
for another RCU grace period:

CPU0                                    CPU1
nvmet_ns_free()
  nvmet_ns_disable()
    clear_bit(NVMET_NS_IO_LIVE, ...)
    percpu_ref_kill(&ns->ref)
    wait_for_completion(...)
                                        nvmet_req_find_ns()
                                          rcu_read_lock()
                                          req->ns = xa_load(...)
  xa_erase(&subsys->namespaces, ...)
  ...
  kfree(ns)
                                          test_bit(NVMET_NS_IO_LIVE,
                                                   &req->ns->flags)
                                            <-- use-after-free

drivers/nvme/target/core.c:nvmet_req_find_ns() {
	rcu_read_lock();
	req->ns = xa_load(&subsys->namespaces, nsid);
	if (unlikely(!req->ns) ||
	    !test_bit(NVMET_NS_IO_LIVE, &req->ns->flags) ||
	    !percpu_ref_tryget_live_rcu(&req->ns->ref)) {
    ...
}

Because ns remains visible to xa_load() until xa_erase() runs in
nvmet_ns_free(), a reader entering nvmet_req_find_ns() after
nvmet_ns_disable() finishes (or when the namespace was never enabled) can
load req->ns just before xa_erase() and dereference it after kfree(ns).
Freeing ns with kfree_rcu() or waiting for an RCU grace period after
xa_erase() would prevent this race.

[1] https://sashiko.dev/#/bug/linux-a674b331-ed9a-4029-b6d0-2a1abec4aaf0

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109033102.982191-25-nilay@linux.ibm.com?part=18

[PATCH 19/23] nvmet: add context annotation for PCI P2P parameters in struct nvmet_ns

[Nilay Shroff]

nvmet_ns::p2p_dev and nvmet_ns::use_p2pmem are protected by
nvmet_subsys::lock. Annotate it with __guarded_by(&subsys->lock) so
that the clang context analyzer can validate accesses to this field.

Signed-off-by: Nilay Shroff <nilay@linux.ibm.com>
---
 drivers/nvme/target/nvmet.h | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index fa932c1fbf29..9e7a3992bb76 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -185,8 +185,10 @@ struct nvmet_ns {
 	struct completion	disable_done;
 	mempool_t		*bvec_pool;
 
-	struct pci_dev		*p2p_dev;
-	int			use_p2pmem;
+	struct pci_dev		*p2p_dev
+		__guarded_by(&subsys->lock);
+	int			use_p2pmem
+		__guarded_by(&subsys->lock);
 	int			pi_type;
 	int			metadata_size;
 	u8			csi;
-- 
2.53.0

[PATCH 20/23] nvmet: fix PCI P2P parameters access using nvmet_subsys::lock

[Nilay Shroff]

The nvmet_ns::p2p_dev and nvmet_ns::use_p2pmem fields are protected by
nvmet_subsys::lock. However, nvmet_ns_p2pmem_show() accesses these
fields without holding the corresponding lock, triggering Clang context
analyzer errors:

drivers/nvme/target/configfs.c:587:42: error: reading variable 'p2p_dev' requires holding mutex 'to_nvmet_ns(item).subsys->lock'
      [-Werror,-Wthread-safety-analysis]
  587 |         return pci_p2pdma_enable_show(page, ns->p2p_dev, ns->use_p2pmem);
      |                                                 ^
drivers/nvme/target/configfs.c:587:55: error: reading variable 'use_p2pmem' requires holding mutex 'to_nvmet_ns(item).subsys->lock'
      [-Werror,-Wthread-safety-analysis]
  587 |         return pci_p2pdma_enable_show(page, ns->p2p_dev, ns->use_p2pmem);
      |                                                              ^

Fix the above errors by acquiring ns->subsys->lock before accessing
PCI P2P parameters in nvmet_ns_p2pmem_show().

Signed-off-by: Nilay Shroff <nilay@linux.ibm.com>
---
 drivers/nvme/target/configfs.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index a87917381093..7245db64b8dc 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -582,8 +582,13 @@ CONFIGFS_ATTR(nvmet_ns_, device_path);
 static ssize_t nvmet_ns_p2pmem_show(struct config_item *item, char *page)
 {
 	struct nvmet_ns *ns = to_nvmet_ns(item);
+	ssize_t ret;
 
-	return pci_p2pdma_enable_show(page, ns->p2p_dev, ns->use_p2pmem);
+	mutex_lock(&ns->subsys->lock);
+	ret = pci_p2pdma_enable_show(page, ns->p2p_dev, ns->use_p2pmem);
+	mutex_unlock(&ns->subsys->lock);
+
+	return ret;
 }
 
 static ssize_t nvmet_ns_p2pmem_store(struct config_item *item,
-- 
2.53.0

Re: [PATCH 20/23] nvmet: fix PCI P2P parameters access using nvmet_subsys::lock

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Low] nvmet: missing Fixes trailer for nvmet_ns_p2pmem_show() lock fix
--

commit bf26a5b394d2fe3a4b6f9ef28cd2b4e1c3857ec6
Author: Nilay Shroff <nilay@linux.ibm.com>

> nvmet: fix PCI P2P parameters access using nvmet_subsys::lock
>
> The nvmet_ns::p2p_dev and nvmet_ns::use_p2pmem fields are protected by
> nvmet_subsys::lock. However, nvmet_ns_p2pmem_show() accesses these
> fields without holding the corresponding lock, triggering Clang context
> analyzer errors:
>
> drivers/nvme/target/configfs.c:587:42: error: reading variable 'p2p_dev' requires holding mutex 'to_nvmet_ns(item).subsys->lock'
>       [-Werror,-Wthread-safety-analysis]
>   587 |         return pci_p2pdma_enable_show(page, ns->p2p_dev, ns->use_p2pmem);
>       |                                                 ^
> drivers/nvme/target/configfs.c:587:55: error: reading variable 'use_p2pmem' requires holding mutex 'to_nvmet_ns(item).subsys->lock'
>       [-Werror,-Wthread-safety-analysis]
>   587 |         return pci_p2pdma_enable_show(page, ns->p2p_dev, ns->use_p2pmem);
>       |                                                              ^
>
> Fix the above errors by acquiring ns->subsys->lock before accessing
> PCI P2P parameters in nvmet_ns_p2pmem_show().
>
> Signed-off-by: Nilay Shroff <nilay@linux.ibm.com>

[Severity: Low]
Since nvmet_ns_p2pmem_store() updates ns->use_p2pmem and ns->p2p_dev and
calls pci_dev_put(ns->p2p_dev) under ns->subsys->lock, this fixes a real
unlocked access and concurrent show/store race in nvmet_ns_p2pmem_show().
Should this include:

  Fixes: c6925093d0b2 ("nvmet: Optionally use PCI P2P memory")

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109033102.982191-25-nilay@linux.ibm.com?part=20

[PATCH 21/23] nvmet: add context annotation for integrity params in struct nvmet_ns

[Nilay Shroff]

The nvmet_ns::pi_type and nvmet_ns::metadata_size fields are protected
by nvmet_subsys::lock. Annotate them with __guarded_by(&subsys->lock)
to allow the Clang context analyzer to validate accesses to these fields.

Some request-processing and I/O paths access these fields without
holding subsys->lock. This is safe because the namespace reference held
by the request prevents the integrity parameters from being modified
while I/O is in flight. However, the context analyzer cannot express
this access pattern directly.

Introduce nvmet_ns_pi_type() and nvmet_ns_metadata_size() as inline
helpers annotated with __context_unsafe() to allow these lockless
accesses without triggering context analyzer warnings.

Use these helpers in the namespace identify path, metadata length
calculation, PI capability check, and RDMA signature attribute setup.

Signed-off-by: Nilay Shroff <nilay@linux.ibm.com>
---
 drivers/nvme/target/admin-cmd.c |  4 ++--
 drivers/nvme/target/nvmet.h     | 13 +++++++++----
 drivers/nvme/target/rdma.c      |  2 +-
 3 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index b8015054de46..e40fc361f5b6 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -943,9 +943,9 @@ static void nvmet_execute_identify_ns(struct nvmet_req *req)
 			  NVME_NS_DPC_PI_TYPE1 | NVME_NS_DPC_PI_TYPE2 |
 			  NVME_NS_DPC_PI_TYPE3;
 		id->mc = NVME_MC_EXTENDED_LBA;
-		id->dps = req->ns->pi_type;
+		id->dps = nvmet_ns_pi_type(req->ns);
 		id->flbas = NVME_NS_FLBAS_META_EXT;
-		id->lbaf[0].ms = cpu_to_le16(req->ns->metadata_size);
+		id->lbaf[0].ms = cpu_to_le16(nvmet_ns_metadata_size(req->ns));
 	}
 
 	if (nvmet_ns_readonly(req->ns))
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 9e7a3992bb76..082d848ea2b0 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -189,8 +189,10 @@ struct nvmet_ns {
 		__guarded_by(&subsys->lock);
 	int			use_p2pmem
 		__guarded_by(&subsys->lock);
-	int			pi_type;
-	int			metadata_size;
+	int			pi_type
+		__guarded_by(&subsys->lock);
+	int			metadata_size
+		__guarded_by(&subsys->lock);
 	u8			csi;
 	struct nvmet_pr		pr;
 	struct xarray		pr_per_ctrl_refs;
@@ -221,6 +223,8 @@ NVMET_NS_ACCESSOR(u8 *, nguid)
 NVMET_NS_ACCESSOR(uuid_t, uuid)
 NVMET_NS_ACCESSOR(bool, buffered_io)
 NVMET_NS_ACCESSOR(const char *, device_path)
+NVMET_NS_ACCESSOR(int, pi_type)
+NVMET_NS_ACCESSOR(int, metadata_size)
 
 /*
  * These accessors use data_race() and READ_ONCE() to allow lockless access to
@@ -830,7 +834,7 @@ static inline u32 nvmet_rw_metadata_len(struct nvmet_req *req)
 	if (!IS_ENABLED(CONFIG_BLK_DEV_INTEGRITY))
 		return 0;
 	return ((u32)le16_to_cpu(req->cmd->rw.length) + 1) *
-			req->ns->metadata_size;
+			nvmet_ns_metadata_size(req->ns);
 }
 
 static inline u32 nvmet_dsm_len(struct nvmet_req *req)
@@ -1010,7 +1014,8 @@ static inline bool nvmet_ns_has_pi(struct nvmet_ns *ns)
 {
 	if (!IS_ENABLED(CONFIG_BLK_DEV_INTEGRITY))
 		return false;
-	return ns->pi_type && ns->metadata_size == sizeof(struct t10_pi_tuple);
+	return nvmet_ns_pi_type(ns) &&
+	       nvmet_ns_metadata_size(ns) == sizeof(struct t10_pi_tuple);
 }
 
 static inline __le64 nvmet_sect_to_lba(struct nvmet_ns *ns, sector_t sect)
diff --git a/drivers/nvme/target/rdma.c b/drivers/nvme/target/rdma.c
index 0b351d39340c..2cec457f12fd 100644
--- a/drivers/nvme/target/rdma.c
+++ b/drivers/nvme/target/rdma.c
@@ -591,7 +591,7 @@ static void nvmet_rdma_set_sig_attrs(struct nvmet_req *req,
 {
 	struct nvme_command *cmd = req->cmd;
 	u16 control = le16_to_cpu(cmd->rw.control);
-	u8 pi_type = req->ns->pi_type;
+	u8 pi_type = nvmet_ns_pi_type(req->ns);
 	struct blk_integrity *bi;
 
 	bi = bdev_get_integrity(nvmet_ns_bdev(req->ns));
-- 
2.53.0

[PATCH 22/23] nvmet: add context annotation for nvmet_ns::csi

[Nilay Shroff]

The nvmet_ns::csi field is protected by nvmet_subsys::lock. Annotate
it with __guarded_by(&subsys->lock) so that the clang context analyzer
can validate accesses to this field.

Some accesses to nvmet_ns::csi in the I/O hotpath do not require
holding nvmet_subsys::lock, as the namespace reference guarantees that
the namespace remains valid and the field is not modified while I/O is
in flight. However, the clang context analyzer cannot express this
access pattern directly.

Introduce nvmet_ns_csi() as an inline helper annotated with
__context_unsafe() to allow these accesses without requiring the
subsystem lock. Convert the relevant accesses in the I/O hotpath and
request-processing paths to use this helper.

Additionally, nvmet_ns_alloc() initializes ns->csi to NVME_CSI_NVM
outside the subsystem lock, triggering a context analyzer error.
Although this initialization could be omitted because both the nvmet_ns
is zero-allocated and NVME_CSI_NVM is also defined as zero, explicitly
initializing the ns->csi to NVME_CSI_NVM makes the intended default
value much clear. So move the ns->csi initialization into the existing
subsys->lock critical section instead.

Signed-off-by: Nilay Shroff <nilay@linux.ibm.com>
---
 drivers/nvme/target/admin-cmd.c | 4 +++-
 drivers/nvme/target/core.c      | 5 ++---
 drivers/nvme/target/nvmet.h     | 4 +++-
 3 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index e40fc361f5b6..74e76e12cf26 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -1083,6 +1083,7 @@ static void nvmet_execute_identify_desclist(struct nvmet_req *req)
 	off_t off = 0;
 	u16 status;
 	uuid_t uuid;
+	u8 csi;
 
 	status = nvmet_req_find_ns(req);
 	if (status)
@@ -1105,9 +1106,10 @@ static void nvmet_execute_identify_desclist(struct nvmet_req *req)
 			goto out;
 	}
 
+	csi = nvmet_ns_csi(req->ns);
 	status = nvmet_copy_ns_identifier(req, NVME_NIDT_CSI,
 					  NVME_NIDT_CSI_LEN,
-					  &req->ns->csi, &off);
+					  &csi, &off);
 	if (status)
 		goto out;
 
diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index 6b11beb2e2b2..1e55ca344c96 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -772,6 +772,7 @@ struct nvmet_ns *nvmet_ns_alloc(struct nvmet_subsys *subsys, u32 nsid)
 
 	subsys->nr_namespaces++;
 	uuid_gen(&ns->uuid);
+	ns->csi = NVME_CSI_NVM;
 
 	__release_ctx_lock(&ns->subsys->lock);
 	mutex_unlock(&subsys->lock);
@@ -781,8 +782,6 @@ struct nvmet_ns *nvmet_ns_alloc(struct nvmet_subsys *subsys, u32 nsid)
 	nvmet_ana_group_enabled[ns->anagrpid]++;
 	up_write(&nvmet_ana_sem);
 
-	ns->csi = NVME_CSI_NVM;
-
 	return ns;
 out_exit:
 	subsys->max_nsid = nvmet_max_nsid(subsys);
@@ -1184,7 +1183,7 @@ static u16 nvmet_parse_io_cmd(struct nvmet_req *req)
 			return ret;
 	}
 
-	switch (req->ns->csi) {
+	switch (nvmet_ns_csi(req->ns)) {
 	case NVME_CSI_NVM:
 		if (nvmet_ns_file(req->ns))
 			ret = nvmet_file_parse_io_cmd(req);
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 082d848ea2b0..77d518739f91 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -193,7 +193,8 @@ struct nvmet_ns {
 		__guarded_by(&subsys->lock);
 	int			metadata_size
 		__guarded_by(&subsys->lock);
-	u8			csi;
+	u8			csi
+		__guarded_by(&subsys->lock);
 	struct nvmet_pr		pr;
 	struct xarray		pr_per_ctrl_refs;
 #ifdef CONFIG_NVME_TARGET_DEBUGFS
@@ -225,6 +226,7 @@ NVMET_NS_ACCESSOR(bool, buffered_io)
 NVMET_NS_ACCESSOR(const char *, device_path)
 NVMET_NS_ACCESSOR(int, pi_type)
 NVMET_NS_ACCESSOR(int, metadata_size)
+NVMET_NS_ACCESSOR(u8, csi)
 
 /*
  * These accessors use data_race() and READ_ONCE() to allow lockless access to
-- 
2.53.0

[PATCH 23/23] nvmet: add context annotation for nvmet_ns::debugfs_dir

[Nilay Shroff]

The nvmet_ns::debugfs_dir field is protected by nvmet_subsys::lock.
Annotate it with __guarded_by(&subsys->lock) so that the clang context
analyzer can validate accesses to this field.

Also, mark nvmet_debugfs_ns_free() with __must_hold(&subsys->lock) to
make its locking requirement explicit.

Signed-off-by: Nilay Shroff <nilay@linux.ibm.com>
---
 drivers/nvme/target/debugfs.h | 3 ++-
 drivers/nvme/target/nvmet.h   | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/target/debugfs.h b/drivers/nvme/target/debugfs.h
index 2dde79e8a2ab..ebab05ddf3e6 100644
--- a/drivers/nvme/target/debugfs.h
+++ b/drivers/nvme/target/debugfs.h
@@ -16,7 +16,8 @@ int nvmet_debugfs_ctrl_setup(struct nvmet_ctrl *ctrl);
 void nvmet_debugfs_ctrl_free(struct nvmet_ctrl *ctrl);
 void nvmet_debugfs_ns_setup(struct nvmet_ns *ns)
 	__must_hold(&ns->subsys->lock);
-void nvmet_debugfs_ns_free(struct nvmet_ns *ns);
+void nvmet_debugfs_ns_free(struct nvmet_ns *ns)
+	__must_hold(&ns->subsys->lock);
 
 int __init nvmet_init_debugfs(void);
 void nvmet_exit_debugfs(void);
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 77d518739f91..a85874992c19 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -198,7 +198,8 @@ struct nvmet_ns {
 	struct nvmet_pr		pr;
 	struct xarray		pr_per_ctrl_refs;
 #ifdef CONFIG_NVME_TARGET_DEBUGFS
-	struct dentry		*debugfs_dir;
+	struct dentry		*debugfs_dir
+		__guarded_by(&subsys->lock);
 #endif
 };
 
-- 
2.53.0