[PATCH v6 0/4] nvmet: avoid recursive configfs open

[PATCH v6 0/4] nvmet: avoid recursive configfs open

[Runyu Xiao]

Several configfs store callbacks open user-configured paths while holding
the item's frag_sem.  If a configured path resolves into configfs, the open
can re-enter __configfs_open_file() and try to acquire the same
non-recursive semaphore again.

Add helpers in configfs for opening non-configfs paths, then use them in the
nvmet file-backed namespace and passthru paths.  The series also pins the
target-core db_root path and uses the root-relative helper for ALUA and APTPL
metadata, so the helper has its target-core consumer in the same series.

Changes since v5:
- Add the target-core db_root patch as 4/4 and use configfs_open_root() for
  validation and metadata opens.
- Replace the verbose helper kerneldoc with concise interface descriptions.
- Keep the Reviewed-by tags on the unchanged nvmet patches.

Runyu Xiao (4):
  fs: configfs: add helpers for opening non-configfs paths
  nvmet: avoid recursive configfs open for file-backed namespaces
  nvmet: avoid recursive configfs open for passthru
  scsi: target: pin db_root for metadata writes

 drivers/nvme/target/io-cmd-file.c     |   3 +-
 drivers/nvme/target/passthru.c        |   3 +-
 drivers/target/target_core_alua.c     |  42 +++++-----
 drivers/target/target_core_configfs.c | 116 ++++++++++++++++++++------
 drivers/target/target_core_internal.h |   3 +
 drivers/target/target_core_pr.c       |  21 +++--
 fs/configfs/mount.c                   |  53 ++++++++++++
 include/linux/configfs.h              |   5 ++
 8 files changed, 189 insertions(+), 57 deletions(-)


base-commit: df2908090cda368b01ff43709f51890076c56157
-- 
2.34.1

[PATCH v6 1/4] fs: configfs: add helpers for opening non-configfs paths

[Runyu Xiao]

Configfs store callbacks hold frag_sem while they run.  Reopening a path
that resolves to configfs from such a callback can acquire the same
non-recursive semaphore again.

Add configfs_file_open() for configured pathnames and configfs_open_root()
for paths relative to a resolved root.  Reject configfs roots before using
file_open_root() so the normal open checks remain in place.

Assisted-by: LLM Codex
Signed-off-by: Runyu Xiao <runyu.xiao@seu.edu.cn>
---
 fs/configfs/mount.c      | 50 ++++++++++++++++++++++++++++++++++++++++
 include/linux/configfs.h |  5 ++++
 2 files changed, 58 insertions(+)

diff --git a/fs/configfs/mount.c b/fs/configfs/mount.c
index d8cac1cbf3bd5..8d3c809676640 100644
--- a/fs/configfs/mount.c
+++ b/fs/configfs/mount.c
@@ -13,6 +13,7 @@
 #include <linux/module.h>
 #include <linux/mount.h>
 #include <linux/fs_context.h>
+#include <linux/namei.h>
 #include <linux/pagemap.h>
 #include <linux/init.h>
 #include <linux/slab.h>
@@ -118,6 +119,58 @@ static struct file_system_type configfs_fs_type = {
 };
 MODULE_ALIAS_FS("configfs");
 
+/**
+ * configfs_open_root - open a path below a non-configfs root
+ * @root: resolved root path
+ * @name: path relative to @root
+ * @flags: open flags
+ * @mode: mode for a newly created file
+ *
+ * Use this from configfs store callbacks with a resolved non-configfs root
+ * to avoid re-entering configfs while the callback holds its fragment
+ * semaphore.
+ *
+ * Return: opened file, or an ERR_PTR() value.  Returns -EINVAL if @root
+ * is on configfs.
+ */
+struct file *configfs_open_root(const struct path *root, const char *name,
+				int flags, umode_t mode)
+{
+	if (root->dentry->d_sb->s_type == &configfs_fs_type)
+		return ERR_PTR(-EINVAL);
+
+	return file_open_root(root, name, flags, mode);
+}
+EXPORT_SYMBOL_GPL(configfs_open_root);
+
+/**
+ * configfs_file_open - open an existing non-configfs pathname
+ * @filename: pathname to open; it must already exist
+ * @flags: open flags for the existing pathname
+ * @mode: unused; creation is not supported
+ *
+ * Resolve @filename and reject configfs paths.  Use this from configfs
+ * store callbacks for existing configured paths.
+ *
+ * Return: opened file, or an ERR_PTR() value.  Returns -EINVAL if the
+ * resolved path is on configfs.
+ */
+struct file *configfs_file_open(const char *filename, int flags, umode_t mode)
+{
+	struct file *file;
+	struct path path;
+	int ret;
+
+	ret = kern_path(filename, LOOKUP_FOLLOW, &path);
+	if (ret)
+		return ERR_PTR(ret);
+
+	file = configfs_open_root(&path, "", flags, mode);
+	path_put(&path);
+	return file;
+}
+EXPORT_SYMBOL_GPL(configfs_file_open);
+
 struct dentry *configfs_pin_fs(void)
 {
 	int err = simple_pin_fs(&configfs_fs_type, &configfs_mount,
diff --git a/include/linux/configfs.h b/include/linux/configfs.h
index ef65c75beeaad..2a803bb836b4d 100644
--- a/include/linux/configfs.h
+++ b/include/linux/configfs.h
@@ -34,6 +34,8 @@ struct configfs_group_operations;
 struct configfs_attribute;
 struct configfs_bin_attribute;
 struct configfs_subsystem;
+struct file;
+struct path;
 
 struct config_item {
 	char			*ci_name;
@@ -243,6 +245,9 @@ void configfs_unregister_subsystem(struct configfs_subsystem *subsys);
 int configfs_register_group(struct config_group *parent_group,
 			    struct config_group *group);
 void configfs_unregister_group(struct config_group *group);
+struct file *configfs_open_root(const struct path *root, const char *name,
+				int flags, umode_t mode);
+struct file *configfs_file_open(const char *filename, int flags, umode_t mode);
 
 void configfs_remove_default_groups(struct config_group *group);
 
-- 
2.34.1

Re: [PATCH v6 1/4] fs: configfs: add helpers for opening non-configfs paths

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] fs: configfs: configfs_file_open() ignores O_NOFOLLOW flag
--

commit 76452a998ac4b5def9cd38ece0706188bd3e3ed4
Author: Runyu Xiao <runyu.xiao@seu.edu.cn>

fs: configfs: add helpers for opening non-configfs paths

This commit adds configfs_open_root() and configfs_file_open() to allow
configfs store callbacks to open non-configfs paths without re-entering
configfs and acquiring the same non-recursive semaphore again.

> diff --git a/fs/configfs/mount.c b/fs/configfs/mount.c
> --- a/fs/configfs/mount.c
> +++ b/fs/configfs/mount.c

[ ... ]

> @@ -118,6 +119,58 @@ static struct file_system_type configfs_fs_type = {

[ ... ]

> +struct file *configfs_file_open(const char *filename, int flags, umode_t mode)
> +{
> +	struct file *file;
> +	struct path path;
> +	int ret;
> +
> +	ret = kern_path(filename, LOOKUP_FOLLOW, &path);

[Severity: Medium]
Does hardcoding LOOKUP_FOLLOW here bypass the caller's O_NOFOLLOW intent if
it is provided in the flags?

If a caller invokes configfs_file_open() with flags containing O_NOFOLLOW
on a symlink, kern_path() will unconditionally resolve the symlink. The
resolved target is then passed to file_open_root() via configfs_open_root().
Because file_open_root() only checks if the resolved target itself is a
symlink, this successfully bypasses the O_NOFOLLOW intent, silently
traversing the symlink instead of failing.

> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	file = configfs_open_root(&path, "", flags, mode);
> +	path_put(&path);
> +	return file;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260926092148.128937-1-runyu.xiao@seu.edu.cn?part=1

[PATCH v6 2/4] nvmet: avoid recursive configfs open for file-backed namespaces

[Runyu Xiao]

nvmet_ns_enable_store() runs as a configfs store callback while configfs
holds the item frag_sem. File-backed namespace enable used filp_open() on
the configured device_path, so a path into configfs could re-enter
__configfs_open_file() and try to acquire the same semaphore again.

Use configfs_file_open() so the path is resolved before opening,
configfs-backed paths are rejected, and the resolved path is opened with
file_open_root() while retaining the normal open-time permission and
security checks.

Fixes: d5eff33ee6f8 ("nvmet: add simple file backed ns support")
Cc: stable@vger.kernel.org
Reviewed-by: Christoph Hellwig <hch@lst.de>
Assisted-by: LLM Codex
Signed-off-by: Runyu Xiao <runyu.xiao@seu.edu.cn>
---
 drivers/nvme/target/io-cmd-file.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/target/io-cmd-file.c b/drivers/nvme/target/io-cmd-file.c
index 0b22d183f9279..2a4f25de94ba1 100644
--- a/drivers/nvme/target/io-cmd-file.c
+++ b/drivers/nvme/target/io-cmd-file.c
@@ -8,6 +8,7 @@
 #include <linux/uio.h>
 #include <linux/falloc.h>
 #include <linux/file.h>
+#include <linux/configfs.h>
 #include <linux/fs.h>
 #include "nvmet.h"
 
@@ -38,7 +39,7 @@ int nvmet_file_ns_enable(struct nvmet_ns *ns)
 	if (!ns->buffered_io)
 		flags |= O_DIRECT;
 
-	ns->file = filp_open(ns->device_path, flags, 0);
+	ns->file = configfs_file_open(ns->device_path, flags, 0);
 	if (IS_ERR(ns->file)) {
 		ret = PTR_ERR(ns->file);
 		pr_err("failed to open file %s: (%d)\n",
-- 
2.34.1

[PATCH v6 3/4] nvmet: avoid recursive configfs open for passthru

[Runyu Xiao]

nvmet_passthru_enable_store() runs as a configfs store callback while
configfs holds the item frag_sem. Passthru enable used filp_open() on the
configured controller path, so a path into configfs could re-enter
__configfs_open_file() and try to acquire the same semaphore again.

Use configfs_file_open() so the path is resolved before opening,
configfs-backed paths are rejected, and the resolved path is opened with
file_open_root() while retaining the normal open-time permission and
security checks.

Fixes: cae5b01a2afc ("nvmet: introduce the passthru configfs interface")
Cc: stable@vger.kernel.org
Reviewed-by: Christoph Hellwig <hch@lst.de>
Assisted-by: LLM Codex
Signed-off-by: Runyu Xiao <runyu.xiao@seu.edu.cn>
---
 drivers/nvme/target/passthru.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/target/passthru.c b/drivers/nvme/target/passthru.c
index fa6527c537e26..d60256004e6cf 100644
--- a/drivers/nvme/target/passthru.c
+++ b/drivers/nvme/target/passthru.c
@@ -9,6 +9,7 @@
  */
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 #include <linux/module.h>
+#include <linux/configfs.h>
 
 #include "../host/nvme.h"
 #include "nvmet.h"
@@ -602,7 +603,7 @@ int nvmet_passthru_ctrl_enable(struct nvmet_subsys *subsys)
 		goto out_unlock;
 	}
 
-	file = filp_open(subsys->passthru_ctrl_path, O_RDWR, 0);
+	file = configfs_file_open(subsys->passthru_ctrl_path, O_RDWR, 0);
 	if (IS_ERR(file)) {
 		ret = PTR_ERR(file);
 		goto out_unlock;
-- 
2.34.1

Re: [PATCH v6 3/4] nvmet: avoid recursive configfs open for passthru

[Logan Gunthorpe]

On 2026-09-26 03:21, Runyu Xiao wrote:
> nvmet_passthru_enable_store() runs as a configfs store callback while
> configfs holds the item frag_sem. Passthru enable used filp_open() on the
> configured controller path, so a path into configfs could re-enter
> __configfs_open_file() and try to acquire the same semaphore again.
> 
> Use configfs_file_open() so the path is resolved before opening,
> configfs-backed paths are rejected, and the resolved path is opened with
> file_open_root() while retaining the normal open-time permission and
> security checks.
> 
> Fixes: cae5b01a2afc ("nvmet: introduce the passthru configfs interface")
> Cc: stable@vger.kernel.org
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Assisted-by: LLM Codex
> Signed-off-by: Runyu Xiao <runyu.xiao@seu.edu.cn>

The problem and solution make sense to me, thanks.

Reviewed-by: Logan Gunthorpe <logang@deltataee.com>

[PATCH v6 4/4] scsi: target: pin db_root for metadata writes

[Runyu Xiao]

ALUA and persistent reservation metadata files are derived from the
configurable db_root string and opened with filp_open().  If db_root
points at configfs, a metadata update from a configfs store callback can
re-enter configfs while the callback still holds frag_sem.

A one-time pathname check can also be bypassed by retargeting a symlink.
Resolve db_root once and retain the resulting path while target devices
use it.  Use configfs_open_root() both to reject configfs roots and to
open metadata files relative to the pinned root.

Resolve a new root outside target_devices_lock and recheck the device count
before publishing it, so the path walk does not occur under that lock.

Fixes: fdddf932269a ("target: use new "dbroot" target attribute")
Link: https://lore.kernel.org/r/20260818051442.1523210-1-runyu.xiao@seu.edu.cn
Cc: stable@vger.kernel.org
Assisted-by: LLM Codex
Signed-off-by: Runyu Xiao <runyu.xiao@seu.edu.cn>
---
 drivers/target/target_core_alua.c     |  42 +++++-----
 drivers/target/target_core_configfs.c | 116 ++++++++++++++++++++------
 drivers/target/target_core_internal.h |   3 +
 drivers/target/target_core_pr.c       |  21 +++--
 4 files changed, 127 insertions(+), 55 deletions(-)

diff --git a/drivers/target/target_core_alua.c b/drivers/target/target_core_alua.c
index 140154d93c430..601581f2071ff 100644
--- a/drivers/target/target_core_alua.c
+++ b/drivers/target/target_core_alua.c
@@ -18,7 +18,6 @@
 #include <linux/fcntl.h>
 #include <linux/file.h>
 #include <linux/fs.h>
-#include <linux/fs_struct.h>
 #include <linux/kthread.h>
 #include <scsi/scsi_proto.h>
 #include <linux/unaligned.h>
@@ -862,20 +861,23 @@ static int core_alua_write_tpg_metadata(
 	loff_t pos = 0;
 	int ret;
 
-	if (tsk_is_kthread(current)) {
-		scoped_with_init_fs()
-			file = filp_open(path, O_RDWR | O_CREAT | O_TRUNC, 0600);
-	} else {
-		file = filp_open(path, O_RDWR | O_CREAT | O_TRUNC, 0600);
+	if (!db_root_path.dentry) {
+		pr_err("db_root is not initialized for ALUA metadata path: %s/%s\n",
+		       db_root, path);
+		return -ENODEV;
 	}
 
+	file = configfs_open_root(&db_root_path, path,
+				  O_RDWR | O_CREAT | O_TRUNC, 0600);
 	if (IS_ERR(file)) {
-		pr_err("filp_open(%s) for ALUA metadata failed\n", path);
+		pr_err("configfs_open_root(%s/%s) for ALUA metadata failed\n",
+		       db_root, path);
 		return -ENODEV;
 	}
 	ret = kernel_write(file, md_buf, md_buf_len, &pos);
 	if (ret < 0)
-		pr_err("Error writing ALUA metadata file: %s\n", path);
+		pr_err("Error writing ALUA metadata file: %s/%s\n", db_root,
+		       path);
 	fput(file);
 	return (ret < 0) ? -EIO : 0;
 }
@@ -905,9 +907,9 @@ static int core_alua_update_tpg_primary_metadata(
 			tg_pt_gp->tg_pt_gp_alua_access_status);
 
 	rc = -ENOMEM;
-	path = kasprintf(GFP_KERNEL, "%s/alua/tpgs_%s/%s", db_root,
-			&wwn->unit_serial[0],
-			config_item_name(&tg_pt_gp->tg_pt_gp_group.cg_item));
+	path = kasprintf(GFP_KERNEL, "alua/tpgs_%s/%s",
+			 &wwn->unit_serial[0],
+			 config_item_name(&tg_pt_gp->tg_pt_gp_group.cg_item));
 	if (path) {
 		rc = core_alua_write_tpg_metadata(path, md_buf, len);
 		kfree(path);
@@ -1196,16 +1198,16 @@ static int core_alua_update_tpg_secondary_metadata(struct se_lun *lun)
 			lun->lun_tg_pt_secondary_stat);
 
 	if (se_tpg->se_tpg_tfo->tpg_get_tag != NULL) {
-		path = kasprintf(GFP_KERNEL, "%s/alua/%s/%s+%hu/lun_%llu",
-				db_root, se_tpg->se_tpg_tfo->fabric_name,
-				se_tpg->se_tpg_tfo->tpg_get_wwn(se_tpg),
-				se_tpg->se_tpg_tfo->tpg_get_tag(se_tpg),
-				lun->unpacked_lun);
+		path = kasprintf(GFP_KERNEL, "alua/%s/%s+%hu/lun_%llu",
+				 se_tpg->se_tpg_tfo->fabric_name,
+				 se_tpg->se_tpg_tfo->tpg_get_wwn(se_tpg),
+				 se_tpg->se_tpg_tfo->tpg_get_tag(se_tpg),
+				 lun->unpacked_lun);
 	} else {
-		path = kasprintf(GFP_KERNEL, "%s/alua/%s/%s/lun_%llu",
-				db_root, se_tpg->se_tpg_tfo->fabric_name,
-				se_tpg->se_tpg_tfo->tpg_get_wwn(se_tpg),
-				lun->unpacked_lun);
+		path = kasprintf(GFP_KERNEL, "alua/%s/%s/lun_%llu",
+				 se_tpg->se_tpg_tfo->fabric_name,
+				 se_tpg->se_tpg_tfo->tpg_get_wwn(se_tpg),
+				 lun->unpacked_lun);
 	}
 	if (!path) {
 		rc = -ENOMEM;
diff --git a/drivers/target/target_core_configfs.c b/drivers/target/target_core_configfs.c
index 2b19a956007b7..db1c56835f077 100644
--- a/drivers/target/target_core_configfs.c
+++ b/drivers/target/target_core_configfs.c
@@ -96,7 +96,41 @@ static ssize_t target_core_item_version_show(struct config_item *item,
 CONFIGFS_ATTR_RO(target_core_item_, version);
 
 char db_root[DB_ROOT_LEN] = DB_ROOT_DEFAULT;
-static char db_root_stage[DB_ROOT_LEN];
+struct path db_root_path;
+
+static int target_validate_db_root(const char *path_str, struct path *path)
+{
+	struct file *file;
+	int ret;
+
+	ret = kern_path(path_str, LOOKUP_FOLLOW | LOOKUP_DIRECTORY, path);
+	if (ret) {
+		pr_err("db_root: cannot open: %s\n", path_str);
+		if (ret == -ENOTDIR)
+			pr_err("db_root: not a directory: %s\n", path_str);
+		return ret;
+	}
+
+	file = configfs_open_root(path, "", O_RDONLY, 0);
+	if (IS_ERR(file)) {
+		ret = PTR_ERR(file);
+		path_put(path);
+		*path = (struct path){};
+		if (ret != -EINVAL)
+			return ret;
+
+		pr_err("db_root: configfs is not a valid target database root: %s\n",
+		       path_str);
+		return -EINVAL;
+	}
+
+	path_put(path);
+	*path = file->f_path;
+	path_get(path);
+	fput(file);
+
+	return 0;
+}
 
 static ssize_t target_core_item_dbroot_show(struct config_item *item,
 					    char *page)
@@ -107,46 +141,70 @@ static ssize_t target_core_item_dbroot_show(struct config_item *item,
 static ssize_t target_core_item_dbroot_store(struct config_item *item,
 					const char *page, size_t count)
 {
+	char *db_root_stage;
 	ssize_t read_bytes;
 	ssize_t r = -EINVAL;
 	struct path path = {};
+	struct path old_path = {};
+	bool have_old_path = false;
 
 	mutex_lock(&target_devices_lock);
 	if (target_devices) {
 		pr_err("db_root: cannot be changed because it's in use\n");
-		goto unlock;
+		mutex_unlock(&target_devices_lock);
+		return r;
 	}
+	mutex_unlock(&target_devices_lock);
 
 	if (count > (DB_ROOT_LEN - 1)) {
 		pr_err("db_root: count %d exceeds DB_ROOT_LEN-1: %u\n",
 		       (int)count, DB_ROOT_LEN - 1);
-		goto unlock;
+		return r;
 	}
 
+	db_root_stage = kmalloc(DB_ROOT_LEN, GFP_KERNEL);
+	if (!db_root_stage)
+		return -ENOMEM;
+
 	read_bytes = scnprintf(db_root_stage, DB_ROOT_LEN, "%s", page);
 	if (!read_bytes)
-		goto unlock;
+		goto free_stage;
 
 	if (db_root_stage[read_bytes - 1] == '\n')
 		db_root_stage[read_bytes - 1] = '\0';
 
 	/* validate new db root before accepting it */
-	r = kern_path(db_root_stage, LOOKUP_FOLLOW | LOOKUP_DIRECTORY, &path);
-	if (r) {
-		pr_err("db_root: cannot open: %s\n", db_root_stage);
-		if (r == -ENOTDIR)
-			pr_err("db_root: not a directory: %s\n", db_root_stage);
-		goto unlock;
+	r = target_validate_db_root(db_root_stage, &path);
+	if (r)
+		goto free_stage;
+
+	mutex_lock(&target_devices_lock);
+	if (target_devices) {
+		pr_err("db_root: cannot be changed because it's in use\n");
+		goto unlock_put;
 	}
-	path_put(&path);
 
+	have_old_path = db_root_path.dentry;
+	if (have_old_path)
+		old_path = db_root_path;
+	db_root_path = path;
+	path = (struct path){};
 	strscpy(db_root, db_root_stage);
 	pr_debug("Target_Core_ConfigFS: db_root set to %s\n", db_root);
 
 	r = read_bytes;
 
-unlock:
+unlock_put:
 	mutex_unlock(&target_devices_lock);
+	if (path.dentry)
+		path_put(&path);
+	if (have_old_path)
+		path_put(&old_path);
+	kfree(db_root_stage);
+	return r;
+
+free_stage:
+	kfree(db_root_stage);
 	return r;
 }
 
@@ -3722,21 +3780,20 @@ void target_setup_backend_cits(struct target_backend *tb)
 
 static void target_init_dbroot(void)
 {
-	struct file *fp;
+	const char *db_root_stage;
+	struct path path = {};
+	int ret;
 
-	snprintf(db_root_stage, DB_ROOT_LEN, DB_ROOT_PREFERRED);
-	fp = filp_open(db_root_stage, O_RDONLY, 0);
-	if (IS_ERR(fp)) {
-		pr_err("db_root: cannot open: %s\n", db_root_stage);
-		return;
-	}
-	if (!S_ISDIR(file_inode(fp)->i_mode)) {
-		filp_close(fp, NULL);
-		pr_err("db_root: not a valid directory: %s\n", db_root_stage);
-		return;
+	db_root_stage = DB_ROOT_PREFERRED;
+	ret = target_validate_db_root(db_root_stage, &path);
+	if (ret) {
+		db_root_stage = DB_ROOT_DEFAULT;
+		ret = target_validate_db_root(db_root_stage, &path);
+		if (ret)
+			return;
 	}
-	filp_close(fp, NULL);
 
+	db_root_path = path;
 	strscpy(db_root, db_root_stage);
 	pr_debug("Target_Core_ConfigFS: db_root set to %s\n", db_root);
 }
@@ -3797,6 +3854,10 @@ static int __init target_core_init_configfs(void)
 	/*
 	 * Register the target_core_mod subsystem with configfs.
 	 */
+	/* Resolve db_root before making the configfs attributes visible. */
+	scoped_with_kernel_creds()
+		target_init_dbroot();
+
 	ret = configfs_register_subsystem(subsys);
 	if (ret < 0) {
 		pr_err("Error %d while registering subsystem %s\n",
@@ -3821,9 +3882,6 @@ static int __init target_core_init_configfs(void)
 	if (ret < 0)
 		goto out;
 
-	scoped_with_kernel_creds()
-		target_init_dbroot();
-
 	return 0;
 
 out:
@@ -3832,6 +3890,8 @@ static int __init target_core_init_configfs(void)
 	core_dev_release_virtual_lun0();
 	rd_module_exit();
 out_global:
+	if (db_root_path.dentry)
+		path_put(&db_root_path);
 	if (default_lu_gp) {
 		core_alua_free_lu_gp(default_lu_gp);
 		default_lu_gp = NULL;
@@ -3861,6 +3921,8 @@ static void __exit target_core_exit_configfs(void)
 	core_dev_release_virtual_lun0();
 	rd_module_exit();
 	target_xcopy_release_pt();
+	if (db_root_path.dentry)
+		path_put(&db_root_path);
 	release_se_kmem_caches();
 }
 
diff --git a/drivers/target/target_core_internal.h b/drivers/target/target_core_internal.h
index f0886ea290345..c3e55f60cfb11 100644
--- a/drivers/target/target_core_internal.h
+++ b/drivers/target/target_core_internal.h
@@ -171,6 +171,9 @@ extern struct se_portal_group xcopy_pt_tpg;
 #define	DB_ROOT_DEFAULT		"/var/target"
 #define	DB_ROOT_PREFERRED	"/etc/target"
 
+struct path;
+
 extern char db_root[];
+extern struct path db_root_path;
 
 #endif /* TARGET_CORE_INTERNAL_H */
diff --git a/drivers/target/target_core_pr.c b/drivers/target/target_core_pr.c
index 25b1bcacc0c8f..0628622d916ba 100644
--- a/drivers/target/target_core_pr.c
+++ b/drivers/target/target_core_pr.c
@@ -18,7 +18,6 @@
 #include <linux/file.h>
 #include <linux/fcntl.h>
 #include <linux/fs.h>
-#include <linux/fs_struct.h>
 #include <scsi/scsi_proto.h>
 #include <linux/unaligned.h>
 
@@ -1965,16 +1964,21 @@ static int __core_scsi3_write_aptpl_to_file(
 	int ret;
 	loff_t pos = 0;
 
-	path = kasprintf(GFP_KERNEL, "%s/pr/aptpl_%s", db_root,
-			&wwn->unit_serial[0]);
+	path = kasprintf(GFP_KERNEL, "pr/aptpl_%s", &wwn->unit_serial[0]);
 	if (!path)
 		return -ENOMEM;
 
-	scoped_with_init_fs()
-		file = filp_open(path, flags, 0600);
+	if (!db_root_path.dentry) {
+		pr_err("db_root is not initialized for APTPL metadata path: %s/%s\n",
+		       db_root, path);
+		kfree(path);
+		return -ENODEV;
+	}
+
+	file = configfs_open_root(&db_root_path, path, flags, 0600);
 	if (IS_ERR(file)) {
-		pr_err("filp_open(%s) for APTPL metadata"
-			" failed\n", path);
+		pr_err("configfs_open_root(%s/%s) for APTPL metadata failed\n",
+		       db_root, path);
 		kfree(path);
 		return PTR_ERR(file);
 	}
@@ -1984,7 +1988,8 @@ static int __core_scsi3_write_aptpl_to_file(
 	ret = kernel_write(file, buf, pr_aptpl_buf_len, &pos);
 
 	if (ret < 0)
-		pr_debug("Error writing APTPL metadata file: %s\n", path);
+		pr_debug("Error writing APTPL metadata file: %s/%s\n", db_root,
+			 path);
 	fput(file);
 	kfree(path);
 
-- 
2.34.1

Re: [PATCH v6 4/4] scsi: target: pin db_root for metadata writes

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] target: use-after-free and torn read of db_root_path during concurrent configfs write
- [Medium] target: target_core_item_dbroot_store returns 0 on error
--

commit c0795bea7761bae07f37ffbb11cf3b1c3410422d
Author: Runyu Xiao <runyu.xiao@seu.edu.cn>

scsi: target: pin db_root for metadata writes

This commit addresses metadata file path resolution in the target subsystem
by resolving db_root once and pinning the resulting path. It aims to
prevent re-entering configfs while holding frag_sem and bypasses via
retargeted symlinks.

Link: https://lore.kernel.org/r/20260818051442.1523210-1-runyu.xiao@seu.edu.cn

> diff --git a/drivers/target/target_core_alua.c b/drivers/target/target_core_alua.c
> --- a/drivers/target/target_core_alua.c
> +++ b/drivers/target/target_core_alua.c
> @@ -862,20 +861,23 @@ static int core_alua_write_tpg_metadata(
>  	loff_t pos = 0;
>  	int ret;
>  
> -	if (tsk_is_kthread(current)) {
> -		scoped_with_init_fs()
> -			file = filp_open(path, O_RDWR | O_CREAT | O_TRUNC, 0600);
> -	} else {
> -		file = filp_open(path, O_RDWR | O_CREAT | O_TRUNC, 0600);
> +	if (!db_root_path.dentry) {

[Severity: High]
Is it possible for this lockless read of db_root_path to race with a
concurrent configfs store operation?

Since ALUA configuration state can be updated for a LUN even when there are
no backend devices present (meaning target_devices evaluates to 0), a
concurrent write to the dbroot configfs attribute wouldn't be blocked.
Could this lead to a torn read of the path structure?

> +		pr_err("db_root is not initialized for ALUA metadata path: %s/%s\n",
> +		       db_root, path);
> +		return -ENODEV;
>  	}

[ ... ]

> diff --git a/drivers/target/target_core_configfs.c b/drivers/target/target_core_configfs.c
> --- a/drivers/target/target_core_configfs.c
> +++ b/drivers/target/target_core_configfs.c
[ ... ]
> @@ -107,46 +141,70 @@ static ssize_t target_core_item_dbroot_show(struct config_item *item,
>  static ssize_t target_core_item_dbroot_store(struct config_item *item,
>  					const char *page, size_t count)
>  {
[ ... ]
> +	r = target_validate_db_root(db_root_stage, &path);
> +	if (r)
> +		goto free_stage;
> +
> +	mutex_lock(&target_devices_lock);
> +	if (target_devices) {
> +		pr_err("db_root: cannot be changed because it's in use\n");
> +		goto unlock_put;
>  	}

[Severity: Medium]
At this point in the execution flow, r is 0 because the call to
target_validate_db_root() succeeded. If we take this goto unlock_put path
due to target_devices being non-zero, doesn't this result in returning 0 to
userspace instead of an error code like -EINVAL?

> -	path_put(&path);
>  
> +	have_old_path = db_root_path.dentry;
> +	if (have_old_path)
> +		old_path = db_root_path;
> +	db_root_path = path;
> +	path = (struct path){};

[Severity: High]
Can this non-atomic update of db_root_path race with the lockless readers in
the metadata write paths?

Since the target_devices check above allows this write to proceed when there
are no backend devices, could this result in a use-after-free or invalid
pointer dereference if the old path is freed while being concurrently
accessed by ALUA or virtual_lun0 PR metadata writers?

>  	strscpy(db_root, db_root_stage);
>  	pr_debug("Target_Core_ConfigFS: db_root set to %s\n", db_root);
>  
>  	r = read_bytes;
>  
> -unlock:
> +unlock_put:
>  	mutex_unlock(&target_devices_lock);
> +	if (path.dentry)
> +		path_put(&path);
> +	if (have_old_path)
> +		path_put(&old_path);
> +	kfree(db_root_stage);
> +	return r;

[ ... ]

> diff --git a/drivers/target/target_core_pr.c b/drivers/target/target_core_pr.c
> --- a/drivers/target/target_core_pr.c
> +++ b/drivers/target/target_core_pr.c
> @@ -1965,16 +1964,21 @@ static int __core_scsi3_write_aptpl_to_file(
>  	int ret;
>  	loff_t pos = 0;
>  
> -	path = kasprintf(GFP_KERNEL, "%s/pr/aptpl_%s", db_root,
> -			&wwn->unit_serial[0]);
> +	path = kasprintf(GFP_KERNEL, "pr/aptpl_%s", &wwn->unit_serial[0]);
>  	if (!path)
>  		return -ENOMEM;
>  
> -	scoped_with_init_fs()
> -		file = filp_open(path, flags, 0600);
> +	if (!db_root_path.dentry) {

[Severity: High]
Similar to the ALUA metadata path, could this locklessly access db_root_path
while it is being updated?

For virtual_lun0, this function can execute when target_devices is 0,
potentially colliding with the configfs writer and leading to a torn read.

> +		pr_err("db_root is not initialized for APTPL metadata path: %s/%s\n",
> +		       db_root, path);
> +		kfree(path);
> +		return -ENODEV;
> +	}
> +
> +	file = configfs_open_root(&db_root_path, path, flags, 0600);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260926092148.128937-1-runyu.xiao@seu.edu.cn?part=4