[PATCH v2 0/3] debugfs: make debugfs_create_str() read-only

[PATCH v2 0/3] debugfs: make debugfs_create_str() read-only

[Yichong Chen]

debugfs_create_str() currently supports writable files through a generic
string replacement helper.  Concurrent writers can race in that helper.

Instead of adding more locking to the generic debugfs string code, convert
the existing writable in-tree users to local file operations first, then make
debugfs_create_str() read-only and refuse modes with write permission bits.

Changes since v1:
- Follow Greg's suggestion to avoid adding locking to the generic
  debugfs_create_str() write path.
- Convert the existing writable in-tree users to local file operations.
- Make debugfs_create_str() read-only and refuse writable modes.
- Split the change into a 3-patch series.

Yichong Chen (3):
  interconnect: debugfs: replace writable string helper
  soundwire: debugfs: replace writable string helper
  debugfs: make debugfs_create_str() read-only

 drivers/interconnect/debugfs-client.c | 75 ++++++++++++++++++++-----
 drivers/soundwire/debugfs.c           | 56 ++++++++++++++++--
 fs/debugfs/file.c                     | 81 ++++-----------------------
 3 files changed, 124 insertions(+), 88 deletions(-)

-- 
2.51.0

[PATCH v2 1/3] interconnect: debugfs: replace writable string helper

[Yichong Chen]

debugfs_create_str() is being made read-only because its generic write path
is hard to make safe without adding more locking to the helper.

Convert the interconnect debugfs client src_node and dst_node entries to
local file operations before removing writable string support from
debugfs_create_str().  Protect the string replacement and path lookup with
the existing debugfs_lock.

Signed-off-by: Yichong Chen <chenyichong@uniontech.com>
---
 drivers/interconnect/debugfs-client.c | 75 ++++++++++++++++++++++-----
 1 file changed, 62 insertions(+), 13 deletions(-)

diff --git a/drivers/interconnect/debugfs-client.c b/drivers/interconnect/debugfs-client.c
index 08df9188ef94..b71c25fd9314 100644
--- a/drivers/interconnect/debugfs-client.c
+++ b/drivers/interconnect/debugfs-client.c
@@ -5,6 +5,7 @@
 #include <linux/debugfs.h>
 #include <linux/interconnect.h>
 #include <linux/platform_device.h>
+#include <linux/slab.h>
 
 #include "internal.h"
 
@@ -36,6 +37,59 @@ struct debugfs_path {
 	struct list_head list;
 };
 
+static ssize_t icc_node_read(struct file *file, char __user *user_buf,
+			     size_t count, loff_t *ppos)
+{
+	char **node = file->private_data;
+	char *copy;
+	size_t len;
+	ssize_t ret;
+
+	mutex_lock(&debugfs_lock);
+	copy = kstrdup(*node ?: "", GFP_KERNEL);
+	mutex_unlock(&debugfs_lock);
+	if (!copy)
+		return -ENOMEM;
+
+	len = strlen(copy);
+	copy[len++] = '\n';
+	ret = simple_read_from_buffer(user_buf, count, ppos, copy, len);
+	kfree(copy);
+	return ret;
+}
+
+static ssize_t icc_node_write(struct file *file, const char __user *user_buf,
+			      size_t count, loff_t *ppos)
+{
+	char **node = file->private_data;
+	char *old, *new;
+
+	if (*ppos)
+		return -EINVAL;
+	if (count + 1 > PAGE_SIZE)
+		return -E2BIG;
+
+	new = memdup_user_nul(user_buf, count);
+	if (IS_ERR(new))
+		return PTR_ERR(new);
+	strim(new);
+
+	mutex_lock(&debugfs_lock);
+	old = *node;
+	*node = new;
+	mutex_unlock(&debugfs_lock);
+
+	kfree(old);
+	return count;
+}
+
+static const struct file_operations icc_node_fops = {
+	.open = simple_open,
+	.read = icc_node_read,
+	.write = icc_node_write,
+	.llseek = default_llseek,
+};
+
 static struct icc_path *get_path(const char *src, const char *dst)
 {
 	struct debugfs_path *path;
@@ -56,24 +110,17 @@ static int icc_get_set(void *data, u64 val)
 
 	mutex_lock(&debugfs_lock);
 
-	rcu_read_lock();
-	src = rcu_dereference(src_node);
-	dst = rcu_dereference(dst_node);
-
 	/*
 	 * If we've already looked up a path, then use the existing one instead
 	 * of calling icc_get() again. This allows for updating previous BW
 	 * votes when "get" is written to multiple times for multiple paths.
 	 */
-	cur_path = get_path(src, dst);
-	if (cur_path) {
-		rcu_read_unlock();
+	cur_path = get_path(src_node, dst_node);
+	if (cur_path)
 		goto out;
-	}
 
-	src = kstrdup(src, GFP_ATOMIC);
-	dst = kstrdup(dst, GFP_ATOMIC);
-	rcu_read_unlock();
+	src = kstrdup(src_node, GFP_KERNEL);
+	dst = kstrdup(dst_node, GFP_KERNEL);
 
 	if (!src || !dst) {
 		ret = -ENOMEM;
@@ -160,8 +207,10 @@ int icc_debugfs_client_init(struct dentry *icc_dir)
 
 	client_dir = debugfs_create_dir("test_client", icc_dir);
 
-	debugfs_create_str("src_node", 0600, client_dir, &src_node);
-	debugfs_create_str("dst_node", 0600, client_dir, &dst_node);
+	debugfs_create_file("src_node", 0600, client_dir, &src_node,
+			    &icc_node_fops);
+	debugfs_create_file("dst_node", 0600, client_dir, &dst_node,
+			    &icc_node_fops);
 	debugfs_create_file("get", 0200, client_dir, NULL, &icc_get_fops);
 	debugfs_create_u32("avg_bw", 0600, client_dir, &avg_bw);
 	debugfs_create_u32("peak_bw", 0600, client_dir, &peak_bw);
-- 
2.51.0

[PATCH v2 2/3] soundwire: debugfs: replace writable string helper

[Yichong Chen]

debugfs_create_str() is being made read-only because its generic write path
is hard to make safe without adding more locking to the helper.

Convert the SoundWire firmware_file entry to a local write-only file
operation before removing writable string support from
debugfs_create_str().

Copy the firmware name before request_firmware() so later debugfs writes
cannot replace the string while it is being used.

Signed-off-by: Yichong Chen <chenyichong@uniontech.com>
---
 drivers/soundwire/debugfs.c | 56 ++++++++++++++++++++++++++++++++++---
 1 file changed, 52 insertions(+), 4 deletions(-)

diff --git a/drivers/soundwire/debugfs.c b/drivers/soundwire/debugfs.c
index 099eb84a548e..bba2b65a0668 100644
--- a/drivers/soundwire/debugfs.c
+++ b/drivers/soundwire/debugfs.c
@@ -145,6 +145,38 @@ static u32 start_addr;
 static size_t num_bytes;
 static u8 read_buffer[MAX_CMD_BYTES];
 static char *firmware_file;
+static DEFINE_MUTEX(firmware_file_lock);
+
+static ssize_t firmware_file_write(struct file *file,
+				   const char __user *user_buf,
+				   size_t count, loff_t *ppos)
+{
+	char *old, *new;
+
+	if (*ppos)
+		return -EINVAL;
+	if (count + 1 > PAGE_SIZE)
+		return -E2BIG;
+
+	new = memdup_user_nul(user_buf, count);
+	if (IS_ERR(new))
+		return PTR_ERR(new);
+	strim(new);
+
+	mutex_lock(&firmware_file_lock);
+	old = firmware_file;
+	firmware_file = new;
+	mutex_unlock(&firmware_file_lock);
+
+	kfree(old);
+	return count;
+}
+
+static const struct file_operations firmware_file_fops = {
+	.open = simple_open,
+	.write = firmware_file_write,
+	.llseek = default_llseek,
+};
 
 static int set_command(void *data, u64 value)
 {
@@ -246,6 +278,7 @@ static int cmd_go(void *data, u64 value)
 {
 	const struct firmware *fw = NULL;
 	struct sdw_slave *slave = data;
+	char *fw_name = NULL;
 	ktime_t start_t;
 	ktime_t finish_t;
 	int ret;
@@ -265,15 +298,24 @@ static int cmd_go(void *data, u64 value)
 	}
 
 	if (cmd == 0) {
-		ret = request_firmware(&fw, firmware_file, &slave->dev);
+		mutex_lock(&firmware_file_lock);
+		if (firmware_file)
+			fw_name = kstrdup(firmware_file, GFP_KERNEL);
+		mutex_unlock(&firmware_file_lock);
+		if (!fw_name) {
+			ret = -ENOMEM;
+			goto out;
+		}
+
+		ret = request_firmware(&fw, fw_name, &slave->dev);
 		if (ret < 0) {
-			dev_err(&slave->dev, "firmware %s not found\n", firmware_file);
+			dev_err(&slave->dev, "firmware %s not found\n", fw_name);
 			goto out;
 		}
 		if (fw->size < num_bytes) {
 			dev_err(&slave->dev,
 				"firmware %s: firmware size %zd, desired %zd\n",
-				firmware_file, fw->size, num_bytes);
+				fw_name, fw->size, num_bytes);
 			goto out;
 		}
 	}
@@ -306,6 +348,7 @@ static int cmd_go(void *data, u64 value)
 out:
 	if (fw)
 		release_firmware(fw);
+	kfree(fw_name);
 
 	pm_runtime_mark_last_busy(&slave->dev);
 	pm_runtime_put(&slave->dev);
@@ -358,7 +401,8 @@ void sdw_slave_debugfs_init(struct sdw_slave *slave)
 
 	debugfs_create_file("read_buffer", 0400, d, slave, &read_buffer_fops);
 	if (firmware_file)
-		debugfs_create_str("firmware_file", 0200, d, &firmware_file);
+		debugfs_create_file("firmware_file", 0200, d, NULL,
+				    &firmware_file_fops);
 
 	slave->debugfs = d;
 }
@@ -370,8 +414,10 @@ void sdw_slave_debugfs_exit(struct sdw_slave *slave)
 
 void sdw_debugfs_init(void)
 {
+	mutex_lock(&firmware_file_lock);
 	if (!firmware_file)
 		firmware_file = kstrdup("", GFP_KERNEL);
+	mutex_unlock(&firmware_file_lock);
 
 	sdw_debugfs_root = debugfs_create_dir("soundwire", NULL);
 }
@@ -379,6 +425,8 @@ void sdw_debugfs_init(void)
 void sdw_debugfs_exit(void)
 {
 	debugfs_remove_recursive(sdw_debugfs_root);
+	mutex_lock(&firmware_file_lock);
 	kfree(firmware_file);
 	firmware_file = NULL;
+	mutex_unlock(&firmware_file_lock);
 }
-- 
2.51.0

[PATCH v2 3/3] debugfs: make debugfs_create_str() read-only

[Yichong Chen]

debugfs_create_str() supports replacing the backing string from userspace.
Concurrent writers can race and free the same old string twice.

All writable in-tree users have been converted to local file operations.
Remove the generic write support from debugfs_create_str(), and refuse to
create a file when the caller passes write permission bits.

This makes unsupported writable use visible instead of silently creating a
file with different permissions.

Fixes: 86b5488121db ("debugfs: Add write support to debugfs_create_str()")
Signed-off-by: Yichong Chen <chenyichong@uniontech.com>
---
 fs/debugfs/file.c | 81 ++++++-----------------------------------------
 1 file changed, 10 insertions(+), 71 deletions(-)

diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
index 08de6652a4f3..f5abd067b260 100644
--- a/fs/debugfs/file.c
+++ b/fs/debugfs/file.c
@@ -1049,98 +1049,37 @@ ssize_t debugfs_read_file_str(struct file *file, char __user *user_buf,
 	return ret;
 }
 
-static ssize_t debugfs_write_file_str(struct file *file, const char __user *user_buf,
-				      size_t count, loff_t *ppos)
-{
-	struct dentry *dentry = F_DENTRY(file);
-	char *old, *new = NULL;
-	int pos = *ppos;
-	int r;
-
-	r = debugfs_file_get(dentry);
-	if (unlikely(r))
-		return r;
-
-	old = *(char **)file->private_data;
-
-	/* only allow strict concatenation */
-	r = -EINVAL;
-	if (pos && pos != strlen(old))
-		goto error;
-
-	r = -E2BIG;
-	if (pos + count + 1 > PAGE_SIZE)
-		goto error;
-
-	r = -ENOMEM;
-	new = kmalloc(pos + count + 1, GFP_KERNEL);
-	if (!new)
-		goto error;
-
-	if (pos)
-		memcpy(new, old, pos);
-
-	r = -EFAULT;
-	if (copy_from_user(new + pos, user_buf, count))
-		goto error;
-
-	new[pos + count] = '\0';
-	strim(new);
-
-	rcu_assign_pointer(*(char __rcu **)file->private_data, new);
-	synchronize_rcu();
-	kfree(old);
-
-	debugfs_file_put(dentry);
-	return count;
-
-error:
-	kfree(new);
-	debugfs_file_put(dentry);
-	return r;
-}
-
 static const struct file_operations fops_str = {
 	.read =		debugfs_read_file_str,
-	.write =	debugfs_write_file_str,
-	.open =		simple_open,
-	.llseek =	default_llseek,
-};
-
-static const struct file_operations fops_str_ro = {
-	.read =		debugfs_read_file_str,
-	.open =		simple_open,
-	.llseek =	default_llseek,
-};
-
-static const struct file_operations fops_str_wo = {
-	.write =	debugfs_write_file_str,
 	.open =		simple_open,
 	.llseek =	default_llseek,
 };
 
 /**
- * debugfs_create_str - create a debugfs file that is used to read and write a string value
+ * debugfs_create_str - create a debugfs file that is used to read a string value
  * @name: a pointer to a string containing the name of the file to create.
  * @mode: the permission that the file should have
  * @parent: a pointer to the parent dentry for this file.  This should be a
  *          directory dentry if set.  If this parameter is %NULL, then the
  *          file will be created in the root of the debugfs filesystem.
- * @value: a pointer to the variable that the file should read to and write
- *         from. This pointer and the string it points to must not be %NULL.
+ * @value: a pointer to the variable that the file should read from. This
+ *         pointer and the string it points to must not be %NULL.
  *
  * This function creates a file in debugfs with the given name that
- * contains the value of the variable @value.  If the @mode variable is so
- * set, it can be read from, and written to.
+ * contains the value of the variable @value.  The file can be read from.
+ * Writable files are not supported; if @mode contains write permission bits,
+ * no file is created.
  */
 void debugfs_create_str(const char *name, umode_t mode,
 			struct dentry *parent, char **value)
 {
 	if (WARN_ON(!value || !*value))
 		return;
+	if (WARN_ONCE(mode & 0222,
+		      "%s() does not support writable files\n", __func__))
+		return;
 
-	debugfs_create_mode_unsafe(name, mode, parent, value, &fops_str,
-				   &fops_str_ro, &fops_str_wo);
+	debugfs_create_file_unsafe(name, mode, parent, value, &fops_str);
 }
 EXPORT_SYMBOL_GPL(debugfs_create_str);
 
-- 
2.51.0

Re: [PATCH v2 1/3] interconnect: debugfs: replace writable string helper

[Greg KH]

On Thu, Aug 06, 2026 at 04:48:52PM +0800, Yichong Chen wrote:
> debugfs_create_str() is being made read-only because its generic write path
> is hard to make safe without adding more locking to the helper.
> 
> Convert the interconnect debugfs client src_node and dst_node entries to
> local file operations before removing writable string support from
> debugfs_create_str().  Protect the string replacement and path lookup with
> the existing debugfs_lock.
> 
> Signed-off-by: Yichong Chen <chenyichong@uniontech.com>
> ---
>  drivers/interconnect/debugfs-client.c | 75 ++++++++++++++++++++++-----
>  1 file changed, 62 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/interconnect/debugfs-client.c b/drivers/interconnect/debugfs-client.c
> index 08df9188ef94..b71c25fd9314 100644
> --- a/drivers/interconnect/debugfs-client.c
> +++ b/drivers/interconnect/debugfs-client.c
> @@ -5,6 +5,7 @@
>  #include <linux/debugfs.h>
>  #include <linux/interconnect.h>
>  #include <linux/platform_device.h>
> +#include <linux/slab.h>
>  
>  #include "internal.h"
>  
> @@ -36,6 +37,59 @@ struct debugfs_path {
>  	struct list_head list;
>  };
>  
> +static ssize_t icc_node_read(struct file *file, char __user *user_buf,
> +			     size_t count, loff_t *ppos)
> +{
> +	char **node = file->private_data;
> +	char *copy;
> +	size_t len;
> +	ssize_t ret;
> +
> +	mutex_lock(&debugfs_lock);
> +	copy = kstrdup(*node ?: "", GFP_KERNEL);
> +	mutex_unlock(&debugfs_lock);

scoped guard?

> +	if (!copy)
> +		return -ENOMEM;
> +
> +	len = strlen(copy);
> +	copy[len++] = '\n';
> +	ret = simple_read_from_buffer(user_buf, count, ppos, copy, len);
> +	kfree(copy);
> +	return ret;
> +}
> +
> +static ssize_t icc_node_write(struct file *file, const char __user *user_buf,
> +			      size_t count, loff_t *ppos)
> +{
> +	char **node = file->private_data;
> +	char *old, *new;
> +
> +	if (*ppos)
> +		return -EINVAL;
> +	if (count + 1 > PAGE_SIZE)
> +		return -E2BIG;
> +
> +	new = memdup_user_nul(user_buf, count);
> +	if (IS_ERR(new))
> +		return PTR_ERR(new);
> +	strim(new);
> +
> +	mutex_lock(&debugfs_lock);
> +	old = *node;
> +	*node = new;
> +	mutex_unlock(&debugfs_lock);

Same here.

> +
> +	kfree(old);
> +	return count;
> +}
> +
> +static const struct file_operations icc_node_fops = {
> +	.open = simple_open,
> +	.read = icc_node_read,
> +	.write = icc_node_write,
> +	.llseek = default_llseek,
> +};
> +
>  static struct icc_path *get_path(const char *src, const char *dst)
>  {
>  	struct debugfs_path *path;
> @@ -56,24 +110,17 @@ static int icc_get_set(void *data, u64 val)
>  
>  	mutex_lock(&debugfs_lock);
>  
> -	rcu_read_lock();
> -	src = rcu_dereference(src_node);
> -	dst = rcu_dereference(dst_node);
> -
>  	/*
>  	 * If we've already looked up a path, then use the existing one instead
>  	 * of calling icc_get() again. This allows for updating previous BW
>  	 * votes when "get" is written to multiple times for multiple paths.
>  	 */
> -	cur_path = get_path(src, dst);
> -	if (cur_path) {
> -		rcu_read_unlock();
> +	cur_path = get_path(src_node, dst_node);
> +	if (cur_path)
>  		goto out;
> -	}
>  
> -	src = kstrdup(src, GFP_ATOMIC);
> -	dst = kstrdup(dst, GFP_ATOMIC);
> -	rcu_read_unlock();
> +	src = kstrdup(src_node, GFP_KERNEL);
> +	dst = kstrdup(dst_node, GFP_KERNEL);

Why is GFP_KERNEL now ok, while GFP_ATOMIC wasn't?  Is this the rcu
stuff interacting somehow?

thanks,

greg k-h

Re: [PATCH v2 2/3] soundwire: debugfs: replace writable string helper

[Greg KH]

On Thu, Aug 06, 2026 at 04:48:53PM +0800, Yichong Chen wrote:
> debugfs_create_str() is being made read-only because its generic write path
> is hard to make safe without adding more locking to the helper.
> 
> Convert the SoundWire firmware_file entry to a local write-only file
> operation before removing writable string support from
> debugfs_create_str().
> 
> Copy the firmware name before request_firmware() so later debugfs writes
> cannot replace the string while it is being used.
> 
> Signed-off-by: Yichong Chen <chenyichong@uniontech.com>
> ---
>  drivers/soundwire/debugfs.c | 56 ++++++++++++++++++++++++++++++++++---
>  1 file changed, 52 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/soundwire/debugfs.c b/drivers/soundwire/debugfs.c
> index 099eb84a548e..bba2b65a0668 100644
> --- a/drivers/soundwire/debugfs.c
> +++ b/drivers/soundwire/debugfs.c
> @@ -145,6 +145,38 @@ static u32 start_addr;
>  static size_t num_bytes;
>  static u8 read_buffer[MAX_CMD_BYTES];
>  static char *firmware_file;
> +static DEFINE_MUTEX(firmware_file_lock);
> +
> +static ssize_t firmware_file_write(struct file *file,
> +				   const char __user *user_buf,
> +				   size_t count, loff_t *ppos)
> +{
> +	char *old, *new;
> +
> +	if (*ppos)
> +		return -EINVAL;
> +	if (count + 1 > PAGE_SIZE)
> +		return -E2BIG;
> +
> +	new = memdup_user_nul(user_buf, count);
> +	if (IS_ERR(new))
> +		return PTR_ERR(new);
> +	strim(new);
> +
> +	mutex_lock(&firmware_file_lock);
> +	old = firmware_file;
> +	firmware_file = new;
> +	mutex_unlock(&firmware_file_lock);

scoped lock?

> +
> +	kfree(old);
> +	return count;
> +}
> +
> +static const struct file_operations firmware_file_fops = {
> +	.open = simple_open,
> +	.write = firmware_file_write,
> +	.llseek = default_llseek,
> +};
>  
>  static int set_command(void *data, u64 value)
>  {
> @@ -246,6 +278,7 @@ static int cmd_go(void *data, u64 value)
>  {
>  	const struct firmware *fw = NULL;
>  	struct sdw_slave *slave = data;
> +	char *fw_name = NULL;
>  	ktime_t start_t;
>  	ktime_t finish_t;
>  	int ret;
> @@ -265,15 +298,24 @@ static int cmd_go(void *data, u64 value)
>  	}
>  
>  	if (cmd == 0) {
> -		ret = request_firmware(&fw, firmware_file, &slave->dev);
> +		mutex_lock(&firmware_file_lock);

Again, scoped lock?

> +		if (firmware_file)
> +			fw_name = kstrdup(firmware_file, GFP_KERNEL);
> +		mutex_unlock(&firmware_file_lock);
> +		if (!fw_name) {
> +			ret = -ENOMEM;
> +			goto out;
> +		}
> +
> +		ret = request_firmware(&fw, fw_name, &slave->dev);
>  		if (ret < 0) {
> -			dev_err(&slave->dev, "firmware %s not found\n", firmware_file);
> +			dev_err(&slave->dev, "firmware %s not found\n", fw_name);
>  			goto out;
>  		}
>  		if (fw->size < num_bytes) {
>  			dev_err(&slave->dev,
>  				"firmware %s: firmware size %zd, desired %zd\n",
> -				firmware_file, fw->size, num_bytes);
> +				fw_name, fw->size, num_bytes);
>  			goto out;
>  		}
>  	}
> @@ -306,6 +348,7 @@ static int cmd_go(void *data, u64 value)
>  out:
>  	if (fw)
>  		release_firmware(fw);
> +	kfree(fw_name);
>  
>  	pm_runtime_mark_last_busy(&slave->dev);
>  	pm_runtime_put(&slave->dev);
> @@ -358,7 +401,8 @@ void sdw_slave_debugfs_init(struct sdw_slave *slave)
>  
>  	debugfs_create_file("read_buffer", 0400, d, slave, &read_buffer_fops);
>  	if (firmware_file)
> -		debugfs_create_str("firmware_file", 0200, d, &firmware_file);
> +		debugfs_create_file("firmware_file", 0200, d, NULL,
> +				    &firmware_file_fops);
>  
>  	slave->debugfs = d;
>  }
> @@ -370,8 +414,10 @@ void sdw_slave_debugfs_exit(struct sdw_slave *slave)
>  
>  void sdw_debugfs_init(void)
>  {
> +	mutex_lock(&firmware_file_lock);
>  	if (!firmware_file)
>  		firmware_file = kstrdup("", GFP_KERNEL);
> +	mutex_unlock(&firmware_file_lock);

Same here.

>  
>  	sdw_debugfs_root = debugfs_create_dir("soundwire", NULL);
>  }
> @@ -379,6 +425,8 @@ void sdw_debugfs_init(void)
>  void sdw_debugfs_exit(void)
>  {
>  	debugfs_remove_recursive(sdw_debugfs_root);
> +	mutex_lock(&firmware_file_lock);

And here.

>  	kfree(firmware_file);
>  	firmware_file = NULL;

This doesn't need to get set to NULL, right?

thanks,

greg k-h

Re: [PATCH v2 3/3] debugfs: make debugfs_create_str() read-only

[Greg KH]

On Thu, Aug 06, 2026 at 04:48:54PM +0800, Yichong Chen wrote:
> debugfs_create_str() supports replacing the backing string from userspace.
> Concurrent writers can race and free the same old string twice.
> 
> All writable in-tree users have been converted to local file operations.
> Remove the generic write support from debugfs_create_str(), and refuse to
> create a file when the caller passes write permission bits.
> 
> This makes unsupported writable use visible instead of silently creating a
> file with different permissions.
> 
> Fixes: 86b5488121db ("debugfs: Add write support to debugfs_create_str()")
> Signed-off-by: Yichong Chen <chenyichong@uniontech.com>
> ---
>  fs/debugfs/file.c | 81 ++++++-----------------------------------------
>  1 file changed, 10 insertions(+), 71 deletions(-)
> 
> diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
> index 08de6652a4f3..f5abd067b260 100644
> --- a/fs/debugfs/file.c
> +++ b/fs/debugfs/file.c
> @@ -1049,98 +1049,37 @@ ssize_t debugfs_read_file_str(struct file *file, char __user *user_buf,
>  	return ret;
>  }
>  
> -static ssize_t debugfs_write_file_str(struct file *file, const char __user *user_buf,
> -				      size_t count, loff_t *ppos)
> -{
> -	struct dentry *dentry = F_DENTRY(file);
> -	char *old, *new = NULL;
> -	int pos = *ppos;
> -	int r;
> -
> -	r = debugfs_file_get(dentry);
> -	if (unlikely(r))
> -		return r;
> -
> -	old = *(char **)file->private_data;
> -
> -	/* only allow strict concatenation */
> -	r = -EINVAL;
> -	if (pos && pos != strlen(old))
> -		goto error;
> -
> -	r = -E2BIG;
> -	if (pos + count + 1 > PAGE_SIZE)
> -		goto error;
> -
> -	r = -ENOMEM;
> -	new = kmalloc(pos + count + 1, GFP_KERNEL);
> -	if (!new)
> -		goto error;
> -
> -	if (pos)
> -		memcpy(new, old, pos);
> -
> -	r = -EFAULT;
> -	if (copy_from_user(new + pos, user_buf, count))
> -		goto error;
> -
> -	new[pos + count] = '\0';
> -	strim(new);
> -
> -	rcu_assign_pointer(*(char __rcu **)file->private_data, new);
> -	synchronize_rcu();
> -	kfree(old);
> -
> -	debugfs_file_put(dentry);
> -	return count;
> -
> -error:
> -	kfree(new);
> -	debugfs_file_put(dentry);
> -	return r;
> -}
> -
>  static const struct file_operations fops_str = {
>  	.read =		debugfs_read_file_str,
> -	.write =	debugfs_write_file_str,
> -	.open =		simple_open,
> -	.llseek =	default_llseek,
> -};
> -
> -static const struct file_operations fops_str_ro = {
> -	.read =		debugfs_read_file_str,
> -	.open =		simple_open,
> -	.llseek =	default_llseek,
> -};
> -
> -static const struct file_operations fops_str_wo = {
> -	.write =	debugfs_write_file_str,
>  	.open =		simple_open,
>  	.llseek =	default_llseek,
>  };
>  
>  /**
> - * debugfs_create_str - create a debugfs file that is used to read and write a string value
> + * debugfs_create_str - create a debugfs file that is used to read a string value
>   * @name: a pointer to a string containing the name of the file to create.
>   * @mode: the permission that the file should have
>   * @parent: a pointer to the parent dentry for this file.  This should be a
>   *          directory dentry if set.  If this parameter is %NULL, then the
>   *          file will be created in the root of the debugfs filesystem.
> - * @value: a pointer to the variable that the file should read to and write
> - *         from. This pointer and the string it points to must not be %NULL.
> + * @value: a pointer to the variable that the file should read from. This
> + *         pointer and the string it points to must not be %NULL.
>   *
>   * This function creates a file in debugfs with the given name that
> - * contains the value of the variable @value.  If the @mode variable is so
> - * set, it can be read from, and written to.
> + * contains the value of the variable @value.  The file can be read from.
> + * Writable files are not supported; if @mode contains write permission bits,
> + * no file is created.
>   */
>  void debugfs_create_str(const char *name, umode_t mode,
>  			struct dentry *parent, char **value)
>  {
>  	if (WARN_ON(!value || !*value))
>  		return;
> +	if (WARN_ONCE(mode & 0222,
> +		      "%s() does not support writable files\n", __func__))

Why WARN_ONCE()?  multiple callers could be hitting this.

thanks,

greg k-h

Re: [PATCH v2 1/3] interconnect: debugfs: replace writable string helper

[Yichong Chen]

On Thu, Aug 06, 2026 at 11:01:07AM +0200, Greg KH wrote:
> On Thu, Aug 06, 2026 at 04:48:52PM +0800, Yichong Chen wrote:
> > +	mutex_lock(&debugfs_lock);
> > +	copy = kstrdup(*node ?: "", GFP_KERNEL);
> > +	mutex_unlock(&debugfs_lock);
> 
> scoped guard?

Yes, scoped guard would be cleaner here.  I can use it in the next version.

> > -	src = kstrdup(src, GFP_ATOMIC);
> > -	dst = kstrdup(dst, GFP_ATOMIC);
> > -	rcu_read_unlock();
> > +	src = kstrdup(src_node, GFP_KERNEL);
> > +	dst = kstrdup(dst_node, GFP_KERNEL);
> 
> Why is GFP_KERNEL now ok, while GFP_ATOMIC wasn't?  Is this the rcu
> stuff interacting somehow?

Yes.  The old code duplicated the strings while still inside the RCU
read-side critical section, so it had to use GFP_ATOMIC.

After this change, src_node and dst_node are protected by debugfs_lock
instead of RCU.  The duplication is done while holding that mutex, so the
allocation can sleep and GFP_KERNEL should be OK.

I will make this clearer in the changelog when sending the next version.

Thanks,
Yichong