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@xxxxxxxxxxxxx>
> ---
>  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