[PATCH] debugfs: serialize debugfs_create_str() writers

[PATCH] debugfs: serialize debugfs_create_str() writers

[Yichong Chen]

debugfs_write_file_str() replaces the string pointer backing a
debugfs_create_str() file and frees the old string after
synchronize_rcu().

Concurrent writers can observe the same old pointer before either
replacement is published. They can then both replace the pointer and
both free the same old string, which KASAN reports as a double-free.

Serialize writers with a mutex so only one writer can replace and free
the old string at a time. Also make readers use rcu_read_lock() and
rcu_dereference(), matching the existing RCU grace period before the old
string is freed.

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

diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
index 08de6652a4f3..566f9ce976b0 100644
--- a/fs/debugfs/file.c
+++ b/fs/debugfs/file.c
@@ -18,6 +18,7 @@
 #include <linux/slab.h>
 #include <linux/atomic.h>
 #include <linux/device.h>
+#include <linux/mutex.h>
 #include <linux/pm_runtime.h>
 #include <linux/poll.h>
 #include <linux/security.h>
@@ -1014,6 +1015,8 @@ void debugfs_create_bool(const char *name, umode_t mode, struct dentry *parent,
 }
 EXPORT_SYMBOL_GPL(debugfs_create_bool);
 
+static DEFINE_MUTEX(debugfs_str_write_mutex);
+
 ssize_t debugfs_read_file_str(struct file *file, char __user *user_buf,
 			      size_t count, loff_t *ppos)
 {
@@ -1026,15 +1029,18 @@ ssize_t debugfs_read_file_str(struct file *file, char __user *user_buf,
 	if (unlikely(ret))
 		return ret;
 
-	str = *(char **)file->private_data;
+	rcu_read_lock();
+	str = rcu_dereference(*(char __rcu **)file->private_data);
 	len = strlen(str) + 1;
-	copy = kmalloc(len, GFP_KERNEL);
+	copy = kmalloc(len, GFP_ATOMIC);
 	if (!copy) {
+		rcu_read_unlock();
 		debugfs_file_put(dentry);
 		return -ENOMEM;
 	}
 
 	copy_len = strscpy(copy, str, len);
+	rcu_read_unlock();
 	debugfs_file_put(dentry);
 	if (copy_len < 0) {
 		kfree(copy);
@@ -1061,7 +1067,9 @@ static ssize_t debugfs_write_file_str(struct file *file, const char __user *user
 	if (unlikely(r))
 		return r;
 
-	old = *(char **)file->private_data;
+	mutex_lock(&debugfs_str_write_mutex);
+	old = rcu_dereference_protected(*(char __rcu **)file->private_data,
+					lockdep_is_held(&debugfs_str_write_mutex));
 
 	/* only allow strict concatenation */
 	r = -EINVAL;
@@ -1091,11 +1099,13 @@ static ssize_t debugfs_write_file_str(struct file *file, const char __user *user
 	synchronize_rcu();
 	kfree(old);
 
+	mutex_unlock(&debugfs_str_write_mutex);
 	debugfs_file_put(dentry);
 	return count;
 
 error:
 	kfree(new);
+	mutex_unlock(&debugfs_str_write_mutex);
 	debugfs_file_put(dentry);
 	return r;
 }
-- 
2.51.0

Re: [PATCH] debugfs: serialize debugfs_create_str() writers

[Greg KH]

On Mon, Aug 03, 2026 at 02:09:20PM +0800, Yichong Chen wrote:
> debugfs_write_file_str() replaces the string pointer backing a
> debugfs_create_str() file and frees the old string after
> synchronize_rcu().
> 
> Concurrent writers can observe the same old pointer before either
> replacement is published. They can then both replace the pointer and
> both free the same old string, which KASAN reports as a double-free.
> 
> Serialize writers with a mutex so only one writer can replace and free
> the old string at a time. Also make readers use rcu_read_lock() and
> rcu_dereference(), matching the existing RCU grace period before the old
> string is freed.
> 
> Fixes: 86b5488121db ("debugfs: Add write support to debugfs_create_str()")
> Signed-off-by: Yichong Chen <chenyichong@uniontech.com>
> ---
>  fs/debugfs/file.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
> index 08de6652a4f3..566f9ce976b0 100644
> --- a/fs/debugfs/file.c
> +++ b/fs/debugfs/file.c
> @@ -18,6 +18,7 @@
>  #include <linux/slab.h>
>  #include <linux/atomic.h>
>  #include <linux/device.h>
> +#include <linux/mutex.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/poll.h>
>  #include <linux/security.h>
> @@ -1014,6 +1015,8 @@ void debugfs_create_bool(const char *name, umode_t mode, struct dentry *parent,
>  }
>  EXPORT_SYMBOL_GPL(debugfs_create_bool);
>  
> +static DEFINE_MUTEX(debugfs_str_write_mutex);

Ouch, you are doing to serialize _all_ debugfs strings on one lock?

> +
>  ssize_t debugfs_read_file_str(struct file *file, char __user *user_buf,
>  			      size_t count, loff_t *ppos)
>  {
> @@ -1026,15 +1029,18 @@ ssize_t debugfs_read_file_str(struct file *file, char __user *user_buf,
>  	if (unlikely(ret))
>  		return ret;
>  
> -	str = *(char **)file->private_data;
> +	rcu_read_lock();
> +	str = rcu_dereference(*(char __rcu **)file->private_data);
>  	len = strlen(str) + 1;
> -	copy = kmalloc(len, GFP_KERNEL);
> +	copy = kmalloc(len, GFP_ATOMIC);

This feels wrong :(

>  	if (!copy) {
> +		rcu_read_unlock();
>  		debugfs_file_put(dentry);
>  		return -ENOMEM;
>  	}
>  
>  	copy_len = strscpy(copy, str, len);
> +	rcu_read_unlock();

Wait, why rcu if you have a lock?

>  	debugfs_file_put(dentry);
>  	if (copy_len < 0) {
>  		kfree(copy);
> @@ -1061,7 +1067,9 @@ static ssize_t debugfs_write_file_str(struct file *file, const char __user *user
>  	if (unlikely(r))
>  		return r;
>  
> -	old = *(char **)file->private_data;
> +	mutex_lock(&debugfs_str_write_mutex);

guard() is nicer.

My larger question is, what code is broken because of this?  What
debugfs string replacements are happening?  I hate the string debugfs
code as it has had lots of issues like this over the years so maybe we
should just drop it and force users to "roll their own" implementation
that would be much simpler without the rcu/locking mess at all?

thanks,

greg k-h

Re: [PATCH] debugfs: serialize debugfs_create_str() writers

[Yichong Chen]

On Mon, Aug 03, 2026 at 08:19:05AM +0200, Greg KH wrote:
> Ouch, you are doing to serialize _all_ debugfs strings on one lock?
>
> This feels wrong :(
>
> Wait, why rcu if you have a lock?
>
> guard() is nicer.
>
> My larger question is, what code is broken because of this?  What
> debugfs string replacements are happening?  I hate the string debugfs
> code as it has had lots of issues like this over the years so maybe we
> should just drop it and force users to "roll their own" implementation
> that would be much simpler without the rcu/locking mess at all?

Thanks for the review.

The patch was trying to fix a double-free.  Two concurrent writes to the same
debugfs_create_str() file can both observe the same old pointer before
either replacement is published, and then both free that old string.  I
reproduced this with KASAN and got:

  BUG: KASAN: double-free in debugfs_write_file_str()

I agree with your comments that the current fix is not a good direction.  A
global mutex serializes unrelated debugfs string files, and mixing that with
the RCU read side makes the helper more complicated.

The directly writable in-tree users I found are:

  drivers/interconnect/debugfs-client.c:
    /sys/kernel/debug/interconnect/test_client/src_node, mode 0600
    /sys/kernel/debug/interconnect/test_client/dst_node, mode 0600

  drivers/soundwire/debugfs.c:
    firmware_file, mode 0200

So the write path is reachable through in-tree debugfs users, although I
do not know whether anyone relies on concurrent writes to these files in
practice.

I can rework this in a few ways:

  1. make the locking per-file instead of global, if we want to keep the
     generic writable string helper;

  2. drop the write support from debugfs_create_str() and convert the
     writable users to their own small file operations;

  3. take another direction if you have a preferred approach.

Dropping write support would avoid keeping this locking in the generic
helper, but debugfs_create_str() is exported, so that would also change
behavior for any out-of-tree users relying on writable strings.

Please let me know which direction you prefer.  If none of these options
looks worthwhile for debugfs, I am also fine with dropping this patch.

Thanks,
Yichong

Re: [PATCH] debugfs: serialize debugfs_create_str() writers

[Greg KH]

On Mon, Aug 03, 2026 at 04:34:01PM +0800, Yichong Chen wrote:
> On Mon, Aug 03, 2026 at 08:19:05AM +0200, Greg KH wrote:
> > Ouch, you are doing to serialize _all_ debugfs strings on one lock?
> >
> > This feels wrong :(
> >
> > Wait, why rcu if you have a lock?
> >
> > guard() is nicer.
> >
> > My larger question is, what code is broken because of this?  What
> > debugfs string replacements are happening?  I hate the string debugfs
> > code as it has had lots of issues like this over the years so maybe we
> > should just drop it and force users to "roll their own" implementation
> > that would be much simpler without the rcu/locking mess at all?
> 
> Thanks for the review.
> 
> The patch was trying to fix a double-free.  Two concurrent writes to the same
> debugfs_create_str() file can both observe the same old pointer before
> either replacement is published, and then both free that old string.  I
> reproduced this with KASAN and got:
> 
>   BUG: KASAN: double-free in debugfs_write_file_str()
> 
> I agree with your comments that the current fix is not a good direction.  A
> global mutex serializes unrelated debugfs string files, and mixing that with
> the RCU read side makes the helper more complicated.
> 
> The directly writable in-tree users I found are:
> 
>   drivers/interconnect/debugfs-client.c:
>     /sys/kernel/debug/interconnect/test_client/src_node, mode 0600
>     /sys/kernel/debug/interconnect/test_client/dst_node, mode 0600
> 
>   drivers/soundwire/debugfs.c:
>     firmware_file, mode 0200
> 
> So the write path is reachable through in-tree debugfs users, although I
> do not know whether anyone relies on concurrent writes to these files in
> practice.
> 
> I can rework this in a few ways:
> 
>   1. make the locking per-file instead of global, if we want to keep the
>      generic writable string helper;
> 
>   2. drop the write support from debugfs_create_str() and convert the
>      writable users to their own small file operations;

If we only have 3 writable string users, I would suggest we do this, and
just leave the debugfs string functionality for read-only files as that
is the MUCH simpler case.

If we have more, well, let's see how many more, and just what exactly a
conversion to "small file operation" would entail.  Try it for the
soundwire file above and let's see what the diff looks like.

>   3. take another direction if you have a preferred approach.
> 
> Dropping write support would avoid keeping this locking in the generic
> helper, but debugfs_create_str() is exported, so that would also change
> behavior for any out-of-tree users relying on writable strings.

For obvious reasons, we do not care about out-of-tree users for any
in-kernel api, as that way would be insanity. :)

thanks,

greg k-h

Re: [PATCH] debugfs: serialize debugfs_create_str() writers

[Yichong Chen]

Hi Greg,

I tried the direction you suggested and converted the SoundWire
firmware_file debugfs entry away from debugfs_create_str().

The draft diff below does two things:

  1. debugfs_create_str() becomes read-only only.  It drops write
     permission bits from the requested mode.  If the caller passed only
     write bits, it creates an owner-readable file instead of a 0000 file.

  2. drivers/soundwire/debugfs.c uses debugfs_create_file() with a small
     local write-only file operation for firmware_file.

The SoundWire command path copies firmware_file under a mutex before using
it for request_firmware(), so a later debugfs write can replace the global
string without invalidating the name being used by the command.

I tested the generic debugfs_create_str() mode handling with a small test
module:

  0444 -> 0444, readable, write fails
  0600 -> 0400, readable, write fails
  0200 -> 0400, readable, write fails

I do not have SoundWire hardware in my test VM, so I could only build that
part.

Does this match the direction you had in mind?  If so, I can finish the
conversion for the interconnect writable string users as well and send a
proper v2.

Thanks,
Yichong

diff --git a/drivers/soundwire/debugfs.c b/drivers/soundwire/debugfs.c
index 099eb84a548e..9aa881689fb9 100644
--- a/drivers/soundwire/debugfs.c
+++ b/drivers/soundwire/debugfs.c
@@ -5,6 +5,7 @@
 #include <linux/device.h>
 #include <linux/debugfs.h>
 #include <linux/firmware.h>
+#include <linux/mutex.h>
 #include <linux/pm_runtime.h>
 #include <linux/slab.h>
 #include <linux/soundwire/sdw.h>
@@ -145,6 +146,7 @@ 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 int set_command(void *data, u64 value)
 {
@@ -246,6 +248,7 @@ static int cmd_go(void *data, u64 value)
 {
 	const struct firmware *fw = NULL;
 	struct sdw_slave *slave = data;
+	char *fw_name __free(kfree) = NULL;
 	ktime_t start_t;
 	ktime_t finish_t;
 	int ret;
@@ -265,15 +268,23 @@ static int cmd_go(void *data, u64 value)
 	}
 
 	if (cmd == 0) {
-		ret = request_firmware(&fw, firmware_file, &slave->dev);
+		mutex_lock(&firmware_file_lock);
+		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;
 		}
 	}
@@ -315,6 +326,37 @@ static int cmd_go(void *data, u64 value)
 DEFINE_DEBUGFS_ATTRIBUTE(cmd_go_fops, NULL,
 			 cmd_go, "%llu\n");
 
+static ssize_t firmware_file_write(struct file *file,
+				   const char __user *user_buf,
+				   size_t count, loff_t *ppos)
+{
+	char *new, *old;
+
+	if (*ppos)
+		return -EINVAL;
+	if (count > PAGE_SIZE - 1)
+		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,
+};
+
 #define MAX_LINE_LEN 128
 
 static int read_buffer_show(struct seq_file *s_file, void *data)
@@ -358,7 +400,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;
 }
@@ -379,6 +422,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);
 }
diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
index 08de6652a4f3..4ce768539b4f 100644
--- a/fs/debugfs/file.c
+++ b/fs/debugfs/file.c
@@ -1049,89 +1049,26 @@ 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.  Write permission bits in
+ * @mode are ignored.  If this leaves no read permission bits, the file is
+ * created owner-readable.
  */
 void debugfs_create_str(const char *name, umode_t mode,
 			struct dentry *parent, char **value)
@@ -1139,8 +1076,11 @@ void debugfs_create_str(const char *name, umode_t mode,
 	if (WARN_ON(!value || !*value))
 		return;
 
-	debugfs_create_mode_unsafe(name, mode, parent, value, &fops_str,
-				   &fops_str_ro, &fops_str_wo);
+	mode &= ~S_IWUGO;
+	if (!(mode & S_IRUGO))
+		mode |= S_IRUSR;
+
+	debugfs_create_file_unsafe(name, mode, parent, value, &fops_str_ro);
 }
 EXPORT_SYMBOL_GPL(debugfs_create_str);
 

Re: [PATCH] debugfs: serialize debugfs_create_str() writers

[Greg KH]

On Tue, Aug 04, 2026 at 01:50:56PM +0800, Yichong Chen wrote:
> Hi Greg,
> 
> I tried the direction you suggested and converted the SoundWire
> firmware_file debugfs entry away from debugfs_create_str().
> 
> The draft diff below does two things:
> 
>   1. debugfs_create_str() becomes read-only only.  It drops write
>      permission bits from the requested mode.  If the caller passed only
>      write bits, it creates an owner-readable file instead of a 0000 file.

If the caller passes write bits, the function should fail to create the
file at all, to make it easy to determine what just went wrong :)

>   2. drivers/soundwire/debugfs.c uses debugfs_create_file() with a small
>      local write-only file operation for firmware_file.

Great!

> The SoundWire command path copies firmware_file under a mutex before using
> it for request_firmware(), so a later debugfs write can replace the global
> string without invalidating the name being used by the command.

As this is debugging only, and root only, and loading firmware files
which better only be done by a developer on their own, this should be
fine and not really an issue at all (i.e. if it breaks, they get to keep
the pieces of their broken system...)

> I tested the generic debugfs_create_str() mode handling with a small test
> module:
> 
>   0444 -> 0444, readable, write fails
>   0600 -> 0400, readable, write fails
>   0200 -> 0400, readable, write fails
> 
> I do not have SoundWire hardware in my test VM, so I could only build that
> part.
> 
> Does this match the direction you had in mind?  If so, I can finish the
> conversion for the interconnect writable string users as well and send a
> proper v2.

Sure, but it should be a patch series, first convert the existing write
string usages to local copies, and then change the debugfs code to
refuse to handle writing strings.

thanks,

greg k-h