[PATCH] scsi: target: file: reject configfs-backed paths in configfs stores

[PATCH] scsi: target: file: reject configfs-backed paths in configfs stores

[Runyu Xiao]

FILEIO paths can be opened from configfs store callbacks while configfs
holds the item's frag_sem.

target_dev_enable_store() reaches fd_configure_device() and
pi_prot_type_store() reaches fd_init_prot(). Both helpers call
filp_open() on user-controlled FILEIO paths. If either path resolves
inside configfs, the lookup can re-enter __configfs_open_file() and try
to take the same frag_sem again.

Reject configfs-backed FILEIO paths before calling filp_open(). Resolve
existing paths with kern_path(), and when O_CREAT may create the last
component, fall back to checking the parent directory with
kern_path_parent(). Keep reporting ordinary path lookup failures instead
of silently bypassing the existing FILEIO error paths.

Fixes: c66ac9db8d4a ("[SCSI] target: Add LIO target core v4.0.0-rc6")
Fixes: 0f5e2ec46dd6 ("target/file: Add DIF protection init/format support")
Cc: stable@vger.kernel.org
Signed-off-by: Runyu Xiao <runyu.xiao@seu.edu.cn>
---
 drivers/target/target_core_file.c | 50 +++++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/drivers/target/target_core_file.c b/drivers/target/target_core_file.c
index 2d78ef7..4351300 100644
--- a/drivers/target/target_core_file.c
+++ b/drivers/target/target_core_file.c
@@ -19,6 +19,7 @@
 #include <linux/module.h>
 #include <linux/vmalloc.h>
 #include <linux/falloc.h>
+#include <linux/namei.h>
 #include <linux/uio.h>
 #include <linux/scatterlist.h>
 #include <scsi/scsi_proto.h>
@@ -86,6 +87,33 @@ static struct se_device *fd_alloc_device(struct se_hba *hba, const char *name)
 	return &fd_dev->dev;
 }
 
+static int fd_validate_fileio_path(const char *path)
+{
+	struct path lookup_path = {};
+	struct dentry *dentry;
+	int ret;
+
+	ret = kern_path(path, LOOKUP_FOLLOW, &lookup_path);
+	if (!ret) {
+		ret = !strcmp(lookup_path.dentry->d_sb->s_type->name, "configfs") ?
+			-EINVAL : 0;
+		path_put(&lookup_path);
+		return ret;
+	}
+	if (ret != -ENOENT)
+		return ret;
+
+	dentry = kern_path_parent(path, &lookup_path);
+	if (IS_ERR(dentry))
+		return PTR_ERR(dentry);
+
+	ret = !strcmp(lookup_path.dentry->d_sb->s_type->name, "configfs") ?
+		-EINVAL : 0;
+	dput(dentry);
+	path_put(&lookup_path);
+	return ret;
+}
+
 static bool fd_configure_unmap(struct se_device *dev)
 {
 	struct file *file = FD_DEV(dev)->fd_file;
@@ -137,6 +165,17 @@ static int fd_configure_device(struct se_device *dev)
 		flags &= ~O_DSYNC;
 	}
 
+	ret = fd_validate_fileio_path(fd_dev->fd_dev_name);
+	if (ret) {
+		if (ret == -EINVAL)
+			pr_err("configfs-backed path is not valid for FILEIO backend: %s\n",
+			       fd_dev->fd_dev_name);
+		else
+			pr_err("FILEIO backend path lookup failed for %s: %d\n",
+			       fd_dev->fd_dev_name, ret);
+		goto fail;
+	}
+
 	file = filp_open(fd_dev->fd_dev_name, flags, 0600);
 	if (IS_ERR(file)) {
 		pr_err("filp_open(%s) failed\n", fd_dev->fd_dev_name);
@@ -847,6 +886,17 @@ static int fd_init_prot(struct se_device *dev)
 	snprintf(buf, FD_MAX_DEV_PROT_NAME, "%s.protection",
 		 fd_dev->fd_dev_name);
 
+	ret = fd_validate_fileio_path(buf);
+	if (ret) {
+		if (ret == -EINVAL)
+			pr_err("configfs-backed path is not valid for FILEIO protection: %s\n",
+			       buf);
+		else
+			pr_err("FILEIO protection path lookup failed for %s: %d\n",
+			       buf, ret);
+		return ret;
+	}
+
 	prot_file = filp_open(buf, flags, 0600);
 	if (IS_ERR(prot_file)) {
 		pr_err("filp_open(%s) failed\n", buf);
-- 
2.34.1

Re: [PATCH] scsi: target: file: reject configfs-backed paths in configfs stores

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

New issues:
- [High] A Time-of-Check to Time-of-Use (TOCTOU) race condition allows bypassing the configfs path validation.
- [High] Validation can be bypassed using stacked filesystems like OverlayFS.
- [Medium] Redundant parent directory validation logic guards against an impossible deadlock scenario.
- [Low] Call to unexported symbol `kern_path_parent` breaks modular builds.

Pre-existing issues:
- [High] Path validation performs VFS path lookups while holding `frag_sem`, creating an AB-BA deadlock with `configfs_rmdir`.
--

--- Patch [1]: [PATCH] scsi: target: file: reject configfs-backed paths in configfs stores ---
commit db2f5e66532db6f370e1ba2ab87399c4fbea7e7e
Author: Runyu Xiao <runyu.xiao@seu.edu.cn>
scsi: target: file: reject configfs-backed paths in configfs stores

This commit adds path validation to prevent FILEIO backend paths from
resolving inside configfs, which can cause a deadlock on frag_sem. It
introduces fd_validate_fileio_path() to check the filesystem type of
the resolved path and its parent before calling filp_open().

> diff --git a/drivers/target/target_core_file.c b/drivers/target/target_core_file.c
> index 62ced9f5102ff..ca41e217fa848 100644
> --- a/drivers/target/target_core_file.c
> +++ b/drivers/target/target_core_file.c
[ ... ]
> @@ -86,6 +87,33 @@ static struct se_device *fd_alloc_device(struct se_hba *hba, const char *name)
>  	return &fd_dev->dev;
>  }
>  
> +static int fd_validate_fileio_path(const char *path)
> +{
> +	struct path lookup_path = {};
> +	struct dentry *dentry;
> +	int ret;
> +
> +	ret = kern_path(path, LOOKUP_FOLLOW, &lookup_path);

[Severity: High]
This isn't a bug introduced by this patch, but does performing VFS path
walks here create an AB-BA deadlock with configfs_rmdir()?

A configfs store callback already holds frag_sem for reading. If kern_path()
or filp_open() traverses a configfs directory, it will block waiting for
i_rwsem to be shared. Concurrently, vfs_rmdir() on that configfs directory
holds i_rwsem exclusively and calls configfs_rmdir(), which will block
waiting for frag_sem exclusively.

> +	if (!ret) {
> +		ret = !strcmp(lookup_path.dentry->d_sb->s_type->name, "configfs") ?

[Severity: High]
Is this string comparison sufficient to catch configfs files accessed through
stacked filesystems?

If the path points to an OverlayFS mount with configfs as a lower directory,
the superblock name will be "overlay", allowing it to bypass this check. The
subsequent filp_open() would then delegate to the real configfs file, still
triggering the frag_sem deadlock.

> +			-EINVAL : 0;
> +		path_put(&lookup_path);
> +		return ret;
> +	}
> +	if (ret != -ENOENT)
> +		return ret;
> +
> +	dentry = kern_path_parent(path, &lookup_path);

[Severity: Low]
Does this break modular builds? kern_path_parent() is not an exported
symbol, so building with CONFIG_TCM_FILEIO=m will result in a linker error.

[Severity: Medium]
Is this parent directory validation necessary?

The configfs filesystem does not implement the .create callback in
configfs_dir_inode_operations. Because of this, if a file doesn't exist,
VFS lookup_open() will return -EACCES before ever calling
configfs_open_file(). It seems a deadlock is structurally impossible for
non-existent files.

> +	if (IS_ERR(dentry))
> +		return PTR_ERR(dentry);
> +
> +	ret = !strcmp(lookup_path.dentry->d_sb->s_type->name, "configfs") ?
> +		-EINVAL : 0;
> +	dput(dentry);
> +	path_put(&lookup_path);
> +	return ret;
> +}
> +
[ ... ]
> @@ -137,6 +165,17 @@ static int fd_configure_device(struct se_device *dev)
>  		flags &= ~O_DSYNC;
>  	}
>  
> +	ret = fd_validate_fileio_path(fd_dev->fd_dev_name);
> +	if (ret) {
> +		if (ret == -EINVAL)
> +			pr_err("configfs-backed path is not valid for FILEIO backend: %s\n",
> +			       fd_dev->fd_dev_name);
> +		else
> +			pr_err("FILEIO backend path lookup failed for %s: %d\n",
> +			       fd_dev->fd_dev_name, ret);
> +		goto fail;
> +	}
> +
>  	file = filp_open(fd_dev->fd_dev_name, flags, 0600);

[Severity: High]
Can a Time-of-Check to Time-of-Use (TOCTOU) race happen here?

There is a window between fd_validate_fileio_path() returning success
and filp_open() resolving the path again. A user could provide a path
containing a symlink, wait for the validation check to complete, and then
replace the symlink to point into configfs before filp_open() is called,
bypassing the protection.

[ ... ]
> @@ -847,6 +886,17 @@ static int fd_init_prot(struct se_device *dev)
>  	snprintf(buf, FD_MAX_DEV_PROT_NAME, "%s.protection",
>  		 fd_dev->fd_dev_name);
>  
> +	ret = fd_validate_fileio_path(buf);
> +	if (ret) {
> +		if (ret == -EINVAL)
> +			pr_err("configfs-backed path is not valid for FILEIO protection: %s\n",
> +			       buf);
> +		else
> +			pr_err("FILEIO protection path lookup failed for %s: %d\n",
> +			       buf, ret);
> +		return ret;
> +	}
> +
>  	prot_file = filp_open(buf, flags, 0600);

[Severity: High]
Does the same TOCTOU race exist here between path validation and
filp_open()?

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

[PATCH v2] scsi: target: file: avoid recursive configfs open in fd_init_prot()

[Runyu Xiao]

pi_prot_type_store() runs under the configfs item's frag_sem.  For
FILEIO devices it calls fd_init_prot(), which rebuilds a
"$FILE.protection" pathname from fd_dev_name and opens it with
filp_open().  If that pathname resolves inside configfs, the open path
can re-enter configfs while the store callback still holds frag_sem.

By the time fd_init_prot() runs, the backing file has already been
configured and pinned in fd_dev->fd_file.  Instead of reopening a
user-controlled pathname, derive the protection sidecar from the
configured backing file itself: reject configfs-backed backing files,
take the opened file's parent dentry, and open or create
"<resolved-name>.protection" there with direct dentry operations.

This keeps the PI sidecar tied to the configured backing file, avoids a
new pathname walk from the configfs store path, and also rejects
stacked filesystems whose real backing file lives on configfs.

Fixes: 0f5e2ec46dd6 ("target/file: Add DIF protection init/format support")
Cc: stable@vger.kernel.org
Signed-off-by: Runyu Xiao <runyu.xiao@seu.edu.cn>

v2:
- narrow the scope to 022 / fd_init_prot() only
- drop the earlier shared 022/023 pathname helper attempt
- use LOOKUP_CREATE so a missing sidecar can actually be created
- open or create the PI sidecar next to the configured backing file
---
 drivers/target/target_core_file.c | 102 +++++++++++++++++++++++++++---
 drivers/target/target_core_file.h |   1 -
 2 files changed, 94 insertions(+), 9 deletions(-)

diff --git a/drivers/target/target_core_file.c b/drivers/target/target_core_file.c
index 2d78ef74633c..b7810ccceb44 100644
--- a/drivers/target/target_core_file.c
+++ b/drivers/target/target_core_file.c
@@ -19,6 +19,10 @@
 #include <linux/module.h>
 #include <linux/vmalloc.h>
 #include <linux/falloc.h>
+#include <linux/limits.h>
+#include <linux/magic.h>
+#include <linux/mount.h>
+#include <linux/namei.h>
 #include <linux/uio.h>
 #include <linux/scatterlist.h>
 #include <scsi/scsi_proto.h>
@@ -86,6 +90,86 @@ static struct se_device *fd_alloc_device(struct se_hba *hba, const char *name)
 	return &fd_dev->dev;
 }
 
+static bool fd_backing_file_is_configfs(struct file *file)
+{
+	return d_real(file_dentry(file), D_REAL_DATA)->d_sb->s_magic ==
+	       CONFIGFS_MAGIC;
+}
+
+static int fd_open_prot_file(struct file *file, int flags, umode_t mode,
+			     struct file **filep)
+{
+	struct dentry *backing_dentry = file_dentry(file);
+	struct path parent_path = {
+		.mnt = mntget(file->f_path.mnt),
+		.dentry = dget_parent(backing_dentry),
+	};
+	struct path prot_path = { .mnt = parent_path.mnt };
+	struct file *prot_file;
+	struct qstr prot_qname;
+	char *prot_name;
+	size_t prot_name_len;
+	int ret;
+
+	if (fd_backing_file_is_configfs(file)) {
+		ret = -EINVAL;
+		goto out_put_parent;
+	}
+
+	prot_name_len = backing_dentry->d_name.len + strlen(".protection");
+	if (prot_name_len > NAME_MAX) {
+		ret = -ENAMETOOLONG;
+		goto out_put_parent;
+	}
+
+	prot_name = kmalloc(prot_name_len + 1, GFP_KERNEL);
+	if (!prot_name) {
+		ret = -ENOMEM;
+		goto out_put_parent;
+	}
+
+	memcpy(prot_name, backing_dentry->d_name.name, backing_dentry->d_name.len);
+	memcpy(prot_name + backing_dentry->d_name.len, ".protection",
+	       strlen(".protection") + 1);
+	prot_qname = QSTR_INIT(prot_name, prot_name_len);
+
+	inode_lock_nested(d_inode(parent_path.dentry), I_MUTEX_PARENT);
+	prot_path.dentry = lookup_one_qstr_excl(&prot_qname, parent_path.dentry,
+						LOOKUP_CREATE);
+	if (IS_ERR(prot_path.dentry)) {
+		ret = PTR_ERR(prot_path.dentry);
+		goto out_unlock;
+	}
+
+	if (d_is_negative(prot_path.dentry)) {
+		ret = mnt_want_write_file(file);
+		if (ret)
+			goto out_dput;
+
+		prot_file = dentry_create(&prot_path, flags, mode, current_cred());
+		mnt_drop_write_file(file);
+	} else {
+		prot_file = dentry_open(&prot_path, flags & ~O_CREAT,
+					current_cred());
+	}
+	if (IS_ERR(prot_file)) {
+		ret = PTR_ERR(prot_file);
+		goto out_dput;
+	}
+
+	*filep = prot_file;
+	ret = 0;
+
+out_dput:
+	dput(prot_path.dentry);
+out_unlock:
+	inode_unlock(d_inode(parent_path.dentry));
+	kfree(prot_name);
+out_put_parent:
+	path_put(&parent_path);
+	return ret;
+}
+
 static bool fd_configure_unmap(struct se_device *dev)
 {
 	struct file *file = FD_DEV(dev)->fd_file;
@@ -827,7 +911,6 @@ static int fd_init_prot(struct se_device *dev)
 	struct file *prot_file, *file = fd_dev->fd_file;
 	struct inode *inode;
 	int ret, flags = O_RDWR | O_CREAT | O_LARGEFILE | O_DSYNC;
-	char buf[FD_MAX_DEV_PROT_NAME];
 
 	if (!file) {
 		pr_err("Unable to locate fd_dev->fd_file\n");
@@ -844,13 +927,16 @@ static int fd_init_prot(struct se_device *dev)
 	if (fd_dev->fbd_flags & FDBD_HAS_BUFFERED_IO_WCE)
 		flags &= ~O_DSYNC;
 
-	snprintf(buf, FD_MAX_DEV_PROT_NAME, "%s.protection",
-		 fd_dev->fd_dev_name);
-
-	prot_file = filp_open(buf, flags, 0600);
-	if (IS_ERR(prot_file)) {
-		pr_err("filp_open(%s) failed\n", buf);
-		ret = PTR_ERR(prot_file);
+	ret = fd_open_prot_file(file, flags, 0600, &prot_file);
+	if (ret) {
+		if (ret == -EINVAL)
+			pr_err("configfs-backed FILEIO backends cannot store PI metadata\n");
+		else if (ret == -ENAMETOOLONG)
+			pr_err("protection sidecar name is too long for FILEIO backend: %pd.protection\n",
+			       file_dentry(file));
+		else
+			pr_err("failed to open FILEIO protection sidecar for %pd: %d\n",
+			       file_dentry(file), ret);
 		return ret;
 	}
 	fd_dev->fd_prot_file = prot_file;
diff --git a/drivers/target/target_core_file.h b/drivers/target/target_core_file.h
index 929b1ecd544e..1b4c1b43827f 100644
--- a/drivers/target/target_core_file.h
+++ b/drivers/target/target_core_file.h
@@ -7,7 +7,6 @@
 #define FD_VERSION		"4.0"
 
 #define FD_MAX_DEV_NAME		256
-#define FD_MAX_DEV_PROT_NAME	FD_MAX_DEV_NAME + 16
 #define FD_DEVICE_QUEUE_DEPTH	32
 #define FD_MAX_DEVICE_QUEUE_DEPTH 128
 #define FD_BLOCKSIZE		512
-- 
2.34.1

Re: [PATCH v2] scsi: target: file: avoid recursive configfs open in fd_init_prot()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Double fetch of `d_name.len` allows concurrent rename to cause a heap buffer overflow and use-after-free
- [High] AB-BA deadlock between the directory inode lock (`i_rwsem`) and filesystem freeze protection (`sb_writers`)
- [High] Synthesizing a `struct path` using a bind mount's vfsmount and the underlying dcache parent creates an invalid VFS object state
- [High] Opening an existing protection sidecar file does not verify its file type, allowing an unprivileged user to panic the kernel
- [Medium] Usage of the unexported static function `lookup_one_qstr_excl()` causes build failures, and passing an uninitialized `qstr.hash` corrupts the dcache
- [Medium] Missing validation of the backing file's parent directory and hashed state after acquiring the parent inode lock
--

--- Patch [1]: [PATCH v2] scsi: target: file: avoid recursive configfs open in fd_init_prot() ---
commit dff9df1c1570ab279457bc3f5bedd541efae3c74
Author: Runyu Xiao <runyu.xiao@seu.edu.cn>
scsi: target: file: avoid recursive configfs open in fd_init_prot()

This commit addresses a potential recursive configfs open issue when creating
protection sidecar files. It introduces fd_open_prot_file() to lookup, and
either create or open the protection sidecar directly from the configured
backing file's parent directory, instead of using filp_open() which might
re-enter configfs.

> diff --git a/drivers/target/target_core_file.c b/drivers/target/target_core_file.c
> index 62ced9f5102ff..9872f8eda1ac4 100644
> --- a/drivers/target/target_core_file.c
> +++ b/drivers/target/target_core_file.c

[ ... ]

> @@ -86,6 +90,86 @@ static struct se_device *fd_alloc_device(struct se_hba *hba, const char *name)
>  	return &fd_dev->dev;
>  }
>  
> +static bool fd_backing_file_is_configfs(struct file *file)
> +{
> +	return d_real(file_dentry(file), D_REAL_DATA)->d_sb->s_magic ==
> +	       CONFIGFS_MAGIC;
> +}
> +
> +static int fd_open_prot_file(struct file *file, int flags, umode_t mode,
> +			     struct file **filep)
> +{
> +	struct dentry *backing_dentry = file_dentry(file);
> +	struct path parent_path = {
> +		.mnt = mntget(file->f_path.mnt),
> +		.dentry = dget_parent(backing_dentry),
> +	};

[Severity: High]
Does this create an invalid VFS object state if the backing file is a
file-level bind mount?

If the file was provided via a file-level bind mount, the vfsmount's root is
the file itself. Using dget_parent() escapes this root to the underlying parent
directory, and passing this disjoint path to dentry_open() or dentry_create()
could violate VFS invariants.

> +	struct path prot_path = { .mnt = parent_path.mnt };
> +	struct file *prot_file;
> +	struct qstr prot_qname;
> +	char *prot_name;
> +	size_t prot_name_len;
> +	int ret;
> +
> +	if (fd_backing_file_is_configfs(file)) {
> +		ret = -EINVAL;
> +		goto out_put_parent;
> +	}
> +
> +	prot_name_len = backing_dentry->d_name.len + strlen(".protection");
> +	if (prot_name_len > NAME_MAX) {
> +		ret = -ENAMETOOLONG;
> +		goto out_put_parent;
> +	}
> +
> +	prot_name = kmalloc(prot_name_len + 1, GFP_KERNEL);
> +	if (!prot_name) {
> +		ret = -ENOMEM;
> +		goto out_put_parent;
> +	}
> +
> +	memcpy(prot_name, backing_dentry->d_name.name, backing_dentry->d_name.len);

[Severity: High]
Is there a risk of a heap buffer overflow here?

The length of d_name is read without locks to allocate prot_name via kmalloc().

If an unprivileged user renames the backing file concurrently to a longer name,
couldn't the second fetch of d_name.len during memcpy() exceed the allocated
buffer size?

> +	memcpy(prot_name + backing_dentry->d_name.len, ".protection",
> +	       strlen(".protection") + 1);
> +	prot_qname = QSTR_INIT(prot_name, prot_name_len);
> +
> +	inode_lock_nested(d_inode(parent_path.dentry), I_MUTEX_PARENT);

[Severity: Medium]
Is there a missing check for concurrent renames or unlinks here?

The parent dentry was fetched locklessly via dget_parent(), but there is no
re-validation that backing_dentry->d_parent still matches parent_path.dentry,
or that the dentry is still hashed, after acquiring the inode lock.

> +	prot_path.dentry = lookup_one_qstr_excl(&prot_qname, parent_path.dentry,
> +						LOOKUP_CREATE);

[Severity: Medium]
Will this cause a build failure when compiled as a module?

lookup_one_qstr_excl() is a static function in fs/namei.c and is not exported
to modules.

Additionally, prot_qname is initialized above using QSTR_INIT() without
computing its hash. Does passing a qstr with a zero hash to low-level VFS
functions force dentries into bucket 0 and corrupt the dcache hash table?

> +	if (IS_ERR(prot_path.dentry)) {
> +		ret = PTR_ERR(prot_path.dentry);
> +		goto out_unlock;
> +	}
> +
> +	if (d_is_negative(prot_path.dentry)) {
> +		ret = mnt_want_write_file(file);

[Severity: High]
Does this invert the standard VFS lock ordering and risk an AB-BA deadlock?

The directory's i_rwsem is currently held via inode_lock_nested(), and
mnt_want_write_file() internally acquires sb_writers (read). The standard
order is sb_writers before i_rwsem, so this could deadlock against a
filesystem freeze which takes sb_writers exclusively.

> +		if (ret)
> +			goto out_dput;
> +
> +		prot_file = dentry_create(&prot_path, flags, mode, current_cred());
> +		mnt_drop_write_file(file);
> +	} else {
> +		prot_file = dentry_open(&prot_path, flags & ~O_CREAT,
> +					current_cred());

[Severity: High]
Is there a check missing to ensure the existing protection sidecar is a regular
file?

If an unprivileged user preemptively creates a symlink named with .protection,
opening it directly via dentry_open() triggers a WARN_ON in do_dentry_open()
because symlinks generally have a NULL i_fop. This can panic the kernel on
systems with panic_on_warn.

> +	}

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

[PATCH v3] scsi: target: file: avoid recursive configfs open in fd_init_prot()

[Runyu Xiao]

pi_prot_type_store() runs under the configfs item's frag_sem.  For
FILEIO devices it calls fd_init_prot(), which rebuilds a
"$FILE.protection" pathname from fd_dev_name and opens it with
filp_open().  If that pathname resolves inside configfs, the open path
can re-enter configfs while the store callback still holds frag_sem.

By the time fd_init_prot() runs, the backing file has already been
configured and pinned in fd_dev->fd_file, so avoid reopening a
user-controlled pathname.  Instead, derive the protection sidecar from
the opened backing file itself: reject configfs-backed backing files,
take freeze protection before locking the parent directory, snapshot the
stable backing dentry name under that lock, and look up the sibling
sidecar by that stable name.  Create the sidecar directly when it is
absent, but only reopen an existing sidecar if it is a regular file.

If the backing file is itself the root of a mounted file, there is no
in-mount sibling pathname for a ".protection" sidecar.  Reject that
case explicitly instead of reconstructing a pathname from fd_dev_name,
which would reintroduce the same configfs recursion hazard this fix is
trying to remove.

This keeps the PI sidecar tied to the opened backing file while closing
the rename, hash initialization, non-regular sidecar, and sb_writers
ordering issues in the earlier v2 draft.

Link: https://lore.kernel.org/r/20260818073539.1555605-1-runyu.xiao@seu.edu.cn
Fixes: 0f5e2ec46dd6 ("target/file: Add DIF protection init/format support")
Cc: stable@vger.kernel.org
Signed-off-by: Runyu Xiao <runyu.xiao@seu.edu.cn>

v3:
- take freeze protection before locking the parent directory
- snapshot the backing dentry name under the parent lock
- use lookup_one() with mnt_idmap() for hashed sibling lookup
- only reopen an existing PI sidecar if it is a regular file
- reject mounted-file-root backends instead of rebuilding a pathname
---
 drivers/target/target_core_file.c | 153 ++++++++++++++++++++++++++++--
 drivers/target/target_core_file.h |   1 -
 2 files changed, 145 insertions(+), 9 deletions(-)

diff --git a/drivers/target/target_core_file.c b/drivers/target/target_core_file.c
index 2d78ef74633c..a499545e6272 100644
--- a/drivers/target/target_core_file.c
+++ b/drivers/target/target_core_file.c
@@ -19,6 +19,10 @@
 #include <linux/module.h>
 #include <linux/vmalloc.h>
 #include <linux/falloc.h>
+#include <linux/dcache.h>
+#include <linux/limits.h>
+#include <linux/mount.h>
+#include <linux/namei.h>
 #include <linux/uio.h>
 #include <linux/scatterlist.h>
 #include <scsi/scsi_proto.h>
@@ -86,6 +90,131 @@ static struct se_device *fd_alloc_device(struct se_hba *hba, const char *name)
 	return &fd_dev->dev;
 }
 
+static bool fd_backing_file_is_configfs(struct file *file)
+{
+	return !strcmp(d_real(file_dentry(file), D_REAL_DATA)->d_sb->s_type->name,
+		       "configfs");
+}
+
+static int fd_open_prot_file(struct file *file, int flags, umode_t mode,
+			     struct file **filep)
+{
+	struct dentry *backing_dentry = file_dentry(file);
+	struct name_snapshot backing_name;
+	struct path parent_path;
+	struct path prot_path;
+	struct file *prot_file;
+	struct qstr prot_qname;
+	char *prot_name;
+	size_t prot_name_len;
+	int ret, retries = 0;
+
+	if (fd_backing_file_is_configfs(file))
+		return -EOPNOTSUPP;
+
+	/*
+	 * A regular file used as the root of a mount has no in-mount sibling
+	 * pathname for a ".protection" sidecar.  Reconstructing one from the
+	 * original string path would reopen the configfs recursion hole.
+	 */
+	if (backing_dentry == file->f_path.mnt->mnt_root)
+		return -EXDEV;
+
+retry:
+	parent_path.mnt = mntget(file->f_path.mnt);
+	parent_path.dentry = dget_parent(backing_dentry);
+	prot_path.mnt = parent_path.mnt;
+
+	ret = mnt_want_write_file(file);
+	if (ret)
+		goto out_put_parent;
+
+	inode_lock_nested(d_inode(parent_path.dentry), I_MUTEX_PARENT);
+	if (d_unlinked(backing_dentry) ||
+	    backing_dentry->d_parent != parent_path.dentry) {
+		ret = -ESTALE;
+		goto out_unlock;
+	}
+
+	take_dentry_name_snapshot(&backing_name, backing_dentry);
+	prot_name_len = backing_name.name.len + strlen(".protection");
+	if (prot_name_len > NAME_MAX) {
+		ret = -ENAMETOOLONG;
+		goto out_release_name;
+	}
+
+	prot_name = kmalloc(prot_name_len + 1, GFP_KERNEL);
+	if (!prot_name) {
+		ret = -ENOMEM;
+		goto out_release_name;
+	}
+
+	memcpy(prot_name, backing_name.name.name, backing_name.name.len);
+	memcpy(prot_name + backing_name.name.len, ".protection",
+	       strlen(".protection") + 1);
+	prot_qname = QSTR_LEN(prot_name, prot_name_len);
+
+	prot_path.dentry = lookup_one(mnt_idmap(parent_path.mnt), &prot_qname,
+				      parent_path.dentry);
+	if (IS_ERR(prot_path.dentry)) {
+		ret = PTR_ERR(prot_path.dentry);
+		goto out_free_name;
+	}
+
+	if (d_is_negative(prot_path.dentry)) {
+		prot_file = dentry_create(&prot_path, flags, mode, current_cred());
+	} else {
+		if (!d_is_reg(prot_path.dentry)) {
+			ret = -EINVAL;
+			goto out_dput;
+		}
+
+		inode_unlock(d_inode(parent_path.dentry));
+		mnt_drop_write_file(file);
+		release_dentry_name_snapshot(&backing_name);
+		kfree(prot_name);
+
+		prot_file = dentry_open(&prot_path, flags & ~O_CREAT,
+					current_cred());
+		if (IS_ERR(prot_file)) {
+			ret = PTR_ERR(prot_file);
+			goto out_dput_nolock;
+		}
+
+		*filep = prot_file;
+		ret = 0;
+		goto out_dput_nolock;
+	}
+
+	if (IS_ERR(prot_file)) {
+		ret = PTR_ERR(prot_file);
+		goto out_dput;
+	}
+
+	*filep = prot_file;
+	ret = 0;
+
+out_dput:
+	dput(prot_path.dentry);
+out_free_name:
+	kfree(prot_name);
+out_release_name:
+	release_dentry_name_snapshot(&backing_name);
+out_unlock:
+	inode_unlock(d_inode(parent_path.dentry));
+	mnt_drop_write_file(file);
+out_put_parent:
+	path_put(&parent_path);
+	if (ret == -ESTALE && !retries++)
+		goto retry;
+	return ret;
+
+out_dput_nolock:
+	dput(prot_path.dentry);
+	path_put(&parent_path);
+	return ret;
+}
+
 static bool fd_configure_unmap(struct se_device *dev)
 {
 	struct file *file = FD_DEV(dev)->fd_file;
@@ -827,7 +956,6 @@ static int fd_init_prot(struct se_device *dev)
 	struct file *prot_file, *file = fd_dev->fd_file;
 	struct inode *inode;
 	int ret, flags = O_RDWR | O_CREAT | O_LARGEFILE | O_DSYNC;
-	char buf[FD_MAX_DEV_PROT_NAME];
 
 	if (!file) {
 		pr_err("Unable to locate fd_dev->fd_file\n");
@@ -844,13 +972,22 @@ static int fd_init_prot(struct se_device *dev)
 	if (fd_dev->fbd_flags & FDBD_HAS_BUFFERED_IO_WCE)
 		flags &= ~O_DSYNC;
 
-	snprintf(buf, FD_MAX_DEV_PROT_NAME, "%s.protection",
-		 fd_dev->fd_dev_name);
-
-	prot_file = filp_open(buf, flags, 0600);
-	if (IS_ERR(prot_file)) {
-		pr_err("filp_open(%s) failed\n", buf);
-		ret = PTR_ERR(prot_file);
+	ret = fd_open_prot_file(file, flags, 0600, &prot_file);
+	if (ret) {
+		if (ret == -EOPNOTSUPP)
+			pr_err("FILEIO PI sidecars are not supported for configfs-backed backends\n");
+		else if (ret == -EXDEV)
+			pr_err("FILEIO PI sidecars are not supported when the backend file is the root of a mount\n");
+		else if (ret == -EINVAL)
+			pr_err("existing FILEIO PI sidecar must be a regular file\n");
+		else if (ret == -ENAMETOOLONG)
+			pr_err("FILEIO PI sidecar name is too long: %pd.protection\n",
+			       file_dentry(file));
+		else if (ret == -ESTALE)
+			pr_err("FILEIO backing file moved during PI sidecar setup\n");
+		else
+			pr_err("failed to open FILEIO PI sidecar for %pd: %d\n",
+			       file_dentry(file), ret);
 		return ret;
 	}
 	fd_dev->fd_prot_file = prot_file;
diff --git a/drivers/target/target_core_file.h b/drivers/target/target_core_file.h
index 929b1ecd544e..1b4c1b43827f 100644
--- a/drivers/target/target_core_file.h
+++ b/drivers/target/target_core_file.h
@@ -7,7 +7,6 @@
 #define FD_VERSION		"4.0"
 
 #define FD_MAX_DEV_NAME		256
-#define FD_MAX_DEV_PROT_NAME	FD_MAX_DEV_NAME + 16
 #define FD_DEVICE_QUEUE_DEPTH	32
 #define FD_MAX_DEVICE_QUEUE_DEPTH 128
 #define FD_BLOCKSIZE		512
-- 
2.34.1

Re: [PATCH v3] scsi: target: file: avoid recursive configfs open in fd_init_prot()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Reopening an existing PI sidecar file using `dentry_open()` bypasses `may_open()` and `inode_permission()` checks.
--

--- Patch [1]: [PATCH v3] scsi: target: file: avoid recursive configfs open in fd_init_prot() ---
commit 12b71238378c9751f15927b9f16e21baaa769408
Author: Runyu Xiao <runyu.xiao@seu.edu.cn>

scsi: target: file: avoid recursive configfs open in fd_init_prot()

This patch fixes a recursive configfs open hazard by avoiding the reopening
of a user-controlled pathname. Instead, it derives the protection sidecar
file from the opened backing file itself and opens it directly.

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

> diff --git a/drivers/target/target_core_file.c b/drivers/target/target_core_file.c
> index 62ced9f5102ff..f6c34e69dbfed 100644
> --- a/drivers/target/target_core_file.c
> +++ b/drivers/target/target_core_file.c
[ ... ]
> @@ -86,6 +90,131 @@ static struct se_device *fd_alloc_device(struct se_hba *hba, const char *name)
[ ... ]
> +static int fd_open_prot_file(struct file *file, int flags, umode_t mode,
> +			     struct file **filep)
> +{
[ ... ]
> +	if (d_is_negative(prot_path.dentry)) {
> +		prot_file = dentry_create(&prot_path, flags, mode, current_cred());
> +	} else {
> +		if (!d_is_reg(prot_path.dentry)) {
> +			ret = -EINVAL;
> +			goto out_dput;
> +		}
> +
> +		inode_unlock(d_inode(parent_path.dentry));
> +		mnt_drop_write_file(file);
> +		release_dentry_name_snapshot(&backing_name);
> +		kfree(prot_name);
> +
> +		prot_file = dentry_open(&prot_path, flags & ~O_CREAT,
> +					current_cred());

[Severity: High]
Does this dentry_open() call bypass standard permission checks?

When reopening an existing sidecar file, this goes straight to dentry_open()
without calling inode_permission() or may_open(). This appears to skip
access controls, including standard permissions, security hooks, and checks
for immutable or append-only file attributes.

If someone with configuration access sets up a backend and links an
immutable file (or one they lack write access to) at the sidecar path,
could they use this to overwrite the file since the permission gates
are bypassed?

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

[PATCH v4] scsi: target: file: avoid recursive configfs open in fd_init_prot()

[Runyu Xiao]

pi_prot_type_store() runs under the configfs item's frag_sem.  For
FILEIO devices it calls fd_init_prot(), which rebuilds a
"$FILE.protection" pathname from fd_dev_name and opens it with
filp_open().  If that pathname resolves inside configfs, the open path
can re-enter configfs while the store callback still holds frag_sem.

By the time fd_init_prot() runs, the backing file has already been
configured and pinned in fd_dev->fd_file, so avoid reopening a
user-controlled pathname.  Instead, derive the protection sidecar from
the opened backing file itself: reject configfs-backed backing files,
take freeze protection before locking the parent directory, snapshot the
stable backing dentry name under that lock, and look up the sibling
sidecar by that stable name.  Create the sidecar directly when it is
absent, but only reopen an existing sidecar if it is a regular file.

If the backing file is itself the root of a mounted file, there is no
in-mount sibling pathname for a ".protection" sidecar.  Reject that
case explicitly instead of reconstructing a pathname from fd_dev_name,
which would reintroduce the same configfs recursion hazard this fix is
trying to remove.

When reopening an existing sidecar, restore the same open-time access
checks that a pathname-based open would have enforced before calling
dentry_open().  This preserves inode_permission() / MAY_OPEN gating for
existing sidecars and continues to reject immutable, append-only, and
otherwise unauthorized files.

This keeps the PI sidecar tied to the opened backing file while closing
the rename, hash initialization, non-regular sidecar, sb_writers
ordering, and existing-sidecar permission-check issues in the earlier
drafts.

Link: https://lore.kernel.org/r/20260818091326.1582200-1-runyu.xiao@seu.edu.cn
Fixes: 0f5e2ec46dd6 ("target/file: Add DIF protection init/format support")
Cc: stable@vger.kernel.org
Signed-off-by: Runyu Xiao <runyu.xiao@seu.edu.cn>
---
v4:
- restore may_open()-equivalent permission checks before dentry_open()
- reject unauthorized existing PI sidecars via inode_permission()
- keep append-only and O_NOATIME open-time checks for existing sidecars

 drivers/target/target_core_file.c | 184 ++++++++++++++++++++++++++++--
 drivers/target/target_core_file.h |   1 -
 2 files changed, 176 insertions(+), 9 deletions(-)

diff --git a/drivers/target/target_core_file.c b/drivers/target/target_core_file.c
index 2d78ef74633c..3b4f270a913a 100644
--- a/drivers/target/target_core_file.c
+++ b/drivers/target/target_core_file.c
@@ -19,6 +19,10 @@
 #include <linux/module.h>
 #include <linux/vmalloc.h>
 #include <linux/falloc.h>
+#include <linux/dcache.h>
+#include <linux/limits.h>
+#include <linux/mount.h>
+#include <linux/namei.h>
 #include <linux/uio.h>
 #include <linux/scatterlist.h>
 #include <scsi/scsi_proto.h>
@@ -86,6 +90,162 @@ static struct se_device *fd_alloc_device(struct se_hba *hba, const char *name)
 	return &fd_dev->dev;
 }
 
+static bool fd_backing_file_is_configfs(struct file *file)
+{
+	return !strcmp(d_real(file_dentry(file), D_REAL_DATA)->d_sb->s_type->name,
+		       "configfs");
+}
+
+static int fd_may_open_existing_file(const struct path *path, int flags)
+{
+	struct inode *inode = d_inode(path->dentry);
+	struct mnt_idmap *idmap = mnt_idmap(path->mnt);
+	int acc_mode = ACC_MODE(flags);
+	int ret;
+
+	if (flags & O_APPEND)
+		acc_mode |= MAY_APPEND;
+
+	ret = inode_permission(idmap, inode, MAY_OPEN | acc_mode);
+	if (ret)
+		return ret;
+
+	if (IS_APPEND(inode)) {
+		if ((flags & O_ACCMODE) != O_RDONLY && !(flags & O_APPEND))
+			return -EPERM;
+		if (flags & O_TRUNC)
+			return -EPERM;
+	}
+
+	if (flags & O_NOATIME && !inode_owner_or_capable(idmap, inode))
+		return -EPERM;
+
+	return 0;
+}
+
+static int fd_open_prot_file(struct file *file, int flags, umode_t mode,
+			     struct file **filep)
+{
+	struct dentry *backing_dentry = file_dentry(file);
+	struct name_snapshot backing_name;
+	struct path parent_path;
+	struct path prot_path;
+	struct file *prot_file;
+	struct qstr prot_qname;
+	char *prot_name;
+	size_t prot_name_len;
+	int ret, retries = 0;
+
+	if (fd_backing_file_is_configfs(file))
+		return -EOPNOTSUPP;
+
+	/*
+	 * A regular file used as the root of a mount has no in-mount sibling
+	 * pathname for a ".protection" sidecar.  Reconstructing one from the
+	 * original string path would reopen the configfs recursion hole.
+	 */
+	if (backing_dentry == file->f_path.mnt->mnt_root)
+		return -EXDEV;
+
+retry:
+	parent_path.mnt = mntget(file->f_path.mnt);
+	parent_path.dentry = dget_parent(backing_dentry);
+	prot_path.mnt = parent_path.mnt;
+
+	ret = mnt_want_write_file(file);
+	if (ret)
+		goto out_put_parent;
+
+	inode_lock_nested(d_inode(parent_path.dentry), I_MUTEX_PARENT);
+	if (d_unlinked(backing_dentry) ||
+	    backing_dentry->d_parent != parent_path.dentry) {
+		ret = -ESTALE;
+		goto out_unlock;
+	}
+
+	take_dentry_name_snapshot(&backing_name, backing_dentry);
+	prot_name_len = backing_name.name.len + strlen(".protection");
+	if (prot_name_len > NAME_MAX) {
+		ret = -ENAMETOOLONG;
+		goto out_release_name;
+	}
+
+	prot_name = kmalloc(prot_name_len + 1, GFP_KERNEL);
+	if (!prot_name) {
+		ret = -ENOMEM;
+		goto out_release_name;
+	}
+
+	memcpy(prot_name, backing_name.name.name, backing_name.name.len);
+	memcpy(prot_name + backing_name.name.len, ".protection",
+	       strlen(".protection") + 1);
+	prot_qname = QSTR_LEN(prot_name, prot_name_len);
+
+	prot_path.dentry = lookup_one(mnt_idmap(parent_path.mnt), &prot_qname,
+				      parent_path.dentry);
+	if (IS_ERR(prot_path.dentry)) {
+		ret = PTR_ERR(prot_path.dentry);
+		goto out_free_name;
+	}
+
+	if (d_is_negative(prot_path.dentry)) {
+		prot_file = dentry_create(&prot_path, flags, mode, current_cred());
+	} else {
+		if (!d_is_reg(prot_path.dentry)) {
+			ret = -EINVAL;
+			goto out_dput;
+		}
+
+		ret = fd_may_open_existing_file(&prot_path, flags & ~O_CREAT);
+		if (ret)
+			goto out_dput;
+
+		inode_unlock(d_inode(parent_path.dentry));
+		mnt_drop_write_file(file);
+		release_dentry_name_snapshot(&backing_name);
+		kfree(prot_name);
+
+		prot_file = dentry_open(&prot_path, flags & ~O_CREAT,
+					current_cred());
+		if (IS_ERR(prot_file)) {
+			ret = PTR_ERR(prot_file);
+			goto out_dput_nolock;
+		}
+
+		*filep = prot_file;
+		ret = 0;
+		goto out_dput_nolock;
+	}
+
+	if (IS_ERR(prot_file)) {
+		ret = PTR_ERR(prot_file);
+		goto out_dput;
+	}
+
+	*filep = prot_file;
+	ret = 0;
+
+out_dput:
+	dput(prot_path.dentry);
+out_free_name:
+	kfree(prot_name);
+out_release_name:
+	release_dentry_name_snapshot(&backing_name);
+out_unlock:
+	inode_unlock(d_inode(parent_path.dentry));
+	mnt_drop_write_file(file);
+out_put_parent:
+	path_put(&parent_path);
+	if (ret == -ESTALE && !retries++)
+		goto retry;
+	return ret;
+
+out_dput_nolock:
+	dput(prot_path.dentry);
+	path_put(&parent_path);
+	return ret;
+}
+
 static bool fd_configure_unmap(struct se_device *dev)
 {
 	struct file *file = FD_DEV(dev)->fd_file;
@@ -827,7 +987,6 @@ static int fd_init_prot(struct se_device *dev)
 	struct file *prot_file, *file = fd_dev->fd_file;
 	struct inode *inode;
 	int ret, flags = O_RDWR | O_CREAT | O_LARGEFILE | O_DSYNC;
-	char buf[FD_MAX_DEV_PROT_NAME];
 
 	if (!file) {
 		pr_err("Unable to locate fd_dev->fd_file\n");
@@ -844,13 +1003,22 @@ static int fd_init_prot(struct se_device *dev)
 	if (fd_dev->fbd_flags & FDBD_HAS_BUFFERED_IO_WCE)
 		flags &= ~O_DSYNC;
 
-	snprintf(buf, FD_MAX_DEV_PROT_NAME, "%s.protection",
-		 fd_dev->fd_dev_name);
-
-	prot_file = filp_open(buf, flags, 0600);
-	if (IS_ERR(prot_file)) {
-		pr_err("filp_open(%s) failed\n", buf);
-		ret = PTR_ERR(prot_file);
+	ret = fd_open_prot_file(file, flags, 0600, &prot_file);
+	if (ret) {
+		if (ret == -EOPNOTSUPP)
+			pr_err("FILEIO PI sidecars are not supported for configfs-backed backends\n");
+		else if (ret == -EXDEV)
+			pr_err("FILEIO PI sidecars are not supported when the backend file is the root of a mount\n");
+		else if (ret == -EINVAL)
+			pr_err("existing FILEIO PI sidecar must be a regular file\n");
+		else if (ret == -ENAMETOOLONG)
+			pr_err("FILEIO PI sidecar name is too long: %pd.protection\n",
+			       file_dentry(file));
+		else if (ret == -ESTALE)
+			pr_err("FILEIO backing file moved during PI sidecar setup\n");
+		else
+			pr_err("failed to open FILEIO PI sidecar for %pd: %d\n",
+			       file_dentry(file), ret);
 		return ret;
 	}
 	fd_dev->fd_prot_file = prot_file;
diff --git a/drivers/target/target_core_file.h b/drivers/target/target_core_file.h
index 929b1ecd544e..1b4c1b43827f 100644
--- a/drivers/target/target_core_file.h
+++ b/drivers/target/target_core_file.h
@@ -7,7 +7,6 @@
 #define FD_VERSION		"4.0"
 
 #define FD_MAX_DEV_NAME		256
-#define FD_MAX_DEV_PROT_NAME	FD_MAX_DEV_NAME + 16
 #define FD_DEVICE_QUEUE_DEPTH	32
 #define FD_MAX_DEVICE_QUEUE_DEPTH 128
 #define FD_BLOCKSIZE		512
-- 
2.34.1