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

[Greg KH <gregkh@linuxfoundation.org>]

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