[v3,1/3] debugfs: Add write support to debugfs_create_str()

Currently, debugfs_create_str() only supports reading strings from
debugfs. Add support for writing them as well.

Based on original implementation by Peter Zijlstra [0]. Write support
was present in the initial patch version, but dropped in v2 due to lack
of users. We have a user now, so reintroduce it.

[0] https://lore.kernel.org/all/YF3Hv5zXb%2F6lauzs@hirez.programming.kicks-ass.net/

Signed-off-by: Mike Tipton <quic_mdtipton@quicinc.com>
---
 fs/debugfs/file.c | 48 +++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 46 insertions(+), 2 deletions(-)

On Mon, Aug 07, 2023 at 07:29:12AM -0700, Mike Tipton wrote:
> Currently, debugfs_create_str() only supports reading strings from
> debugfs. Add support for writing them as well.
> 
> Based on original implementation by Peter Zijlstra [0]. Write support
> was present in the initial patch version, but dropped in v2 due to lack
> of users. We have a user now, so reintroduce it.
> 
> [0] https://lore.kernel.org/all/YF3Hv5zXb%2F6lauzs@hirez.programming.kicks-ass.net/
> 
> Signed-off-by: Mike Tipton <quic_mdtipton@quicinc.com>
> ---
>  fs/debugfs/file.c | 48 +++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 46 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
> index b7711888dd17..87b3753aa4b1 100644
> --- a/fs/debugfs/file.c
> +++ b/fs/debugfs/file.c
> @@ -904,8 +904,52 @@ EXPORT_SYMBOL_GPL(debugfs_create_str);
>  static ssize_t debugfs_write_file_str(struct file *file, const char __user *user_buf,
>  				      size_t count, loff_t *ppos)
>  {
> -	/* This is really only for read-only strings */
> -	return -EINVAL;
> +	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 **)file->private_data, new);
> +	synchronize_rcu();
> +	kfree(old);
> +
> +	debugfs_file_put(dentry);
> +	return count;
> +
> +error:
> +	kfree(new);
> +	debugfs_file_put(dentry);
> +	return r;
>  }

So you just added write support for ALL debugfs files that use the
string interface, what did you just allow to break?

I recommend just using your own debugfs file function instead, as this
could cause bad problems, right?  Are you sure that all string calls can
handle the variable be freed underneath it like this call will allow to
happen?

So I wouldn't recommend doing this, sorry.

greg k-h

Hi Greg,

Thanks for the comments!

On 12.08.23 13:40, Greg KH wrote:
> On Mon, Aug 07, 2023 at 07:29:12AM -0700, Mike Tipton wrote:
>> Currently, debugfs_create_str() only supports reading strings from
>> debugfs. Add support for writing them as well.
>>
>> Based on original implementation by Peter Zijlstra [0]. Write support
>> was present in the initial patch version, but dropped in v2 due to lack
>> of users. We have a user now, so reintroduce it.
>>
>> [0] https://lore.kernel.org/all/YF3Hv5zXb%2F6lauzs@hirez.programming.kicks-ass.net/
>>
>> Signed-off-by: Mike Tipton <quic_mdtipton@quicinc.com>
>> ---
>>   fs/debugfs/file.c | 48 +++++++++++++++++++++++++++++++++++++++++++++--
>>   1 file changed, 46 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
>> index b7711888dd17..87b3753aa4b1 100644
>> --- a/fs/debugfs/file.c
>> +++ b/fs/debugfs/file.c
>> @@ -904,8 +904,52 @@ EXPORT_SYMBOL_GPL(debugfs_create_str);
>>   static ssize_t debugfs_write_file_str(struct file *file, const char __user *user_buf,
>>   				      size_t count, loff_t *ppos)
>>   {
>> -	/* This is really only for read-only strings */
>> -	return -EINVAL;
>> +	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 **)file->private_data, new);
>> +	synchronize_rcu();
>> +	kfree(old);
>> +
>> +	debugfs_file_put(dentry);
>> +	return count;
>> +
>> +error:
>> +	kfree(new);
>> +	debugfs_file_put(dentry);
>> +	return r;
>>   }
> 
> So you just added write support for ALL debugfs files that use the
> string interface, what did you just allow to break?

Not really. According to the existing code, the write support for strings
is enabled only when the file is created with +w permissions. For read-only
files, we use fops_str_ro, which is the case for all existing string files:

$ git grep -w debugfs_create_str | egrep -v "fs/debugfs/file.c|include/linux/debugfs.h"
drivers/firmware/arm_scmi/driver.c:	debugfs_create_str("instance_name", 0400, top_dentry,
drivers/firmware/arm_scmi/driver.c:	debugfs_create_str("type", 0400, trans, (char **)&dbg->type);
drivers/opp/debugfs.c:	debugfs_create_str("of_name", S_IRUGO, d, (char **)&opp->of_name);

For fops_str_ro, the .write function is not implemented, so nothing should break?

> I recommend just using your own debugfs file function instead, as this
> could cause bad problems, right?

Agree, and that should be exactly what this patch does.

> Are you sure that all string calls can
> handle the variable be freed underneath it like this call will allow to
> happen?

Looks fine, at least for this patch-set.

Thanks,
Georgi

Hi Greg,

On 12.08.23 13:40, Greg KH wrote:
> On Mon, Aug 07, 2023 at 07:29:12AM -0700, Mike Tipton wrote:
>> Currently, debugfs_create_str() only supports reading strings from
>> debugfs. Add support for writing them as well.
>>
>> Based on original implementation by Peter Zijlstra [0]. Write support
>> was present in the initial patch version, but dropped in v2 due to lack
>> of users. We have a user now, so reintroduce it.
>>
>> [0] https://lore.kernel.org/all/YF3Hv5zXb%2F6lauzs@hirez.programming.kicks-ass.net/
>>
>> Signed-off-by: Mike Tipton <quic_mdtipton@quicinc.com>
>> ---
>>   fs/debugfs/file.c | 48 +++++++++++++++++++++++++++++++++++++++++++++--
>>   1 file changed, 46 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
>> index b7711888dd17..87b3753aa4b1 100644
>> --- a/fs/debugfs/file.c
>> +++ b/fs/debugfs/file.c
>> @@ -904,8 +904,52 @@ EXPORT_SYMBOL_GPL(debugfs_create_str);
>>   static ssize_t debugfs_write_file_str(struct file *file, const char __user *user_buf,
>>   				      size_t count, loff_t *ppos)
>>   {
>> -	/* This is really only for read-only strings */
>> -	return -EINVAL;
>> +	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 **)file->private_data, new);
>> +	synchronize_rcu();
>> +	kfree(old);
>> +
>> +	debugfs_file_put(dentry);
>> +	return count;
>> +
>> +error:
>> +	kfree(new);
>> +	debugfs_file_put(dentry);
>> +	return r;
>>   }
> 
> So you just added write support for ALL debugfs files that use the
> string interface, what did you just allow to break?

Not true. Write support is added only for debugfs string files that are
created with +w permissions. All existing files are created as read-only
and use the fops_str_ro ops.

> I recommend just using your own debugfs file function instead, as this
> could cause bad problems, right?  Are you sure that all string calls can
> handle the variable be freed underneath it like this call will allow to
> happen?
> 
> So I wouldn't recommend doing this, sorry.
> 

Maybe you missed the fact that the different file ops are already there
and are selected based on permissions:

> 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,
> };

...so this patch is doing exactly what you suggested? If you agree,
could you ack it again please?

Thanks,
Georgi

On Fri, Aug 18, 2023 at 01:05:57PM +0300, Georgi Djakov wrote:
> Hi Greg,
> 
> On 12.08.23 13:40, Greg KH wrote:
> > On Mon, Aug 07, 2023 at 07:29:12AM -0700, Mike Tipton wrote:
> > > Currently, debugfs_create_str() only supports reading strings from
> > > debugfs. Add support for writing them as well.
> > > 
> > > Based on original implementation by Peter Zijlstra [0]. Write support
> > > was present in the initial patch version, but dropped in v2 due to lack
> > > of users. We have a user now, so reintroduce it.
> > > 
> > > [0] https://lore.kernel.org/all/YF3Hv5zXb%2F6lauzs@hirez.programming.kicks-ass.net/
> > > 
> > > Signed-off-by: Mike Tipton <quic_mdtipton@quicinc.com>
> > > ---
> > >   fs/debugfs/file.c | 48 +++++++++++++++++++++++++++++++++++++++++++++--
> > >   1 file changed, 46 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
> > > index b7711888dd17..87b3753aa4b1 100644
> > > --- a/fs/debugfs/file.c
> > > +++ b/fs/debugfs/file.c
> > > @@ -904,8 +904,52 @@ EXPORT_SYMBOL_GPL(debugfs_create_str);
> > >   static ssize_t debugfs_write_file_str(struct file *file, const char __user *user_buf,
> > >   				      size_t count, loff_t *ppos)
> > >   {
> > > -	/* This is really only for read-only strings */
> > > -	return -EINVAL;
> > > +	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 **)file->private_data, new);
> > > +	synchronize_rcu();
> > > +	kfree(old);
> > > +
> > > +	debugfs_file_put(dentry);
> > > +	return count;
> > > +
> > > +error:
> > > +	kfree(new);
> > > +	debugfs_file_put(dentry);
> > > +	return r;
> > >   }
> > 
> > So you just added write support for ALL debugfs files that use the
> > string interface, what did you just allow to break?
> 
> Not true. Write support is added only for debugfs string files that are
> created with +w permissions. All existing files are created as read-only
> and use the fops_str_ro ops.
> 
> > I recommend just using your own debugfs file function instead, as this
> > could cause bad problems, right?  Are you sure that all string calls can
> > handle the variable be freed underneath it like this call will allow to
> > happen?
> > 
> > So I wouldn't recommend doing this, sorry.
> > 
> 
> Maybe you missed the fact that the different file ops are already there
> and are selected based on permissions:
> 
> > 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,
> > };
> 
> ...so this patch is doing exactly what you suggested? If you agree,
> could you ack it again please?

Yes, I did miss that, sorry, my appologies for dragging this out so
long:

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>