diff mbox series

[3/5] debugfs: Update debugfs_create_str() kerneldoc to warn about pointer race

Message ID 20230516160753.32317-4-rf@opensource.cirrus.com
State New
Headers
Series debugfs: Fixes and improvements to debugfs_create_str() |

Commit Message

Richard Fitzgerald May 16, 2023, 4:07 p.m. UTC 
  Add a warning to the debugfs_create_str() kerneldoc that the char * pointer
value must not change after the function returns, because of a race with
debugfs_read_file_str() accessing the pointer.

The only safe case is a change from NULL to non-NULL because in that case
debugfs_read_file_str() will see either the NULL or the valid pointer.

Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
---
 fs/debugfs/file.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

Comments

Greg KH May 16, 2023, 4:35 p.m. UTC | #1 
On Tue, May 16, 2023 at 05:07:51PM +0100, Richard Fitzgerald wrote:
> Add a warning to the debugfs_create_str() kerneldoc that the char * pointer
> value must not change after the function returns, because of a race with
> debugfs_read_file_str() accessing the pointer.
> 
> The only safe case is a change from NULL to non-NULL because in that case
> debugfs_read_file_str() will see either the NULL or the valid pointer.
> 
> Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
> ---
>  fs/debugfs/file.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
> index 0c039a3d9a42..77794871f26d 100644
> --- a/fs/debugfs/file.c
> +++ b/fs/debugfs/file.c
> @@ -938,11 +938,18 @@ static const struct file_operations fops_str_wo = {
>   *          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.
> + *         from. The char* pointer must not change, except from NULL to
> + *         non-NULL.

This feels odd.  Why wouldn't you want to change the string value?  Or
why would you?

And why is this one-way transition ok?

Given that this is only used internally, why is it exported?

thanks,

greg k-h
Richard Fitzgerald May 16, 2023, 5:50 p.m. UTC | #2 
On 16/5/23 17:35, Greg KH wrote:
> On Tue, May 16, 2023 at 05:07:51PM +0100, Richard Fitzgerald wrote:
>> Add a warning to the debugfs_create_str() kerneldoc that the char * pointer
>> value must not change after the function returns, because of a race with
>> debugfs_read_file_str() accessing the pointer.
>>
>> The only safe case is a change from NULL to non-NULL because in that case
>> debugfs_read_file_str() will see either the NULL or the valid pointer.
>>
>> Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
>> ---
>>   fs/debugfs/file.c | 9 ++++++++-
>>   1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
>> index 0c039a3d9a42..77794871f26d 100644
>> --- a/fs/debugfs/file.c
>> +++ b/fs/debugfs/file.c
>> @@ -938,11 +938,18 @@ static const struct file_operations fops_str_wo = {
>>    *          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.
>> + *         from. The char* pointer must not change, except from NULL to
>> + *         non-NULL.
> 
> This feels odd.  Why wouldn't you want to change the string value?  Or
> why would you?

Well, if you _would_ want to change the string value, then the
implementation of debugfs_create_str() is certainly broken and could
only be fixed by involving a shared mutex to protect use of the pointer.

> 
> And why is this one-way transition ok?
> 

This one case happens to be safe because it either sees NULL (which it
handles) or a valid pointer (which is ok). It will not result in using a
stale pointer. This wasn't a deliberate design intent but happens to be
safe, and easily maintainable behavior.

A transition from valid->NULL or old->new isn't safe because the
read function could get the old pointer but racing with that is the
change to the pointer, and so the debugfs code could try to use a
stale pointer.

> Given that this is only used internally, why is it exported?
> 

It isn't only used internally. I found 3 drivers that use it.
But there are no uses internal to debugfs.

I didn't write debugfs_create_str(), I only tried to use it and made
an attempt to fix some problems.

Given the limitations of the basic implementation of
debugfs_create_str() and its file reading function (the lack of
protection against the pointer changing) perhaps drop this chain? Don't
bother fixing it, instead deprecate it for being unsafe?

> thanks,
> 
> greg k-h
Greg KH May 17, 2023, 6:18 a.m. UTC | #3 
On Tue, May 16, 2023 at 06:50:16PM +0100, Richard Fitzgerald wrote:
> On 16/5/23 17:35, Greg KH wrote:
> > On Tue, May 16, 2023 at 05:07:51PM +0100, Richard Fitzgerald wrote:
> > > Add a warning to the debugfs_create_str() kerneldoc that the char * pointer
> > > value must not change after the function returns, because of a race with
> > > debugfs_read_file_str() accessing the pointer.
> > > 
> > > The only safe case is a change from NULL to non-NULL because in that case
> > > debugfs_read_file_str() will see either the NULL or the valid pointer.
> > > 
> > > Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
> > > ---
> > >   fs/debugfs/file.c | 9 ++++++++-
> > >   1 file changed, 8 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
> > > index 0c039a3d9a42..77794871f26d 100644
> > > --- a/fs/debugfs/file.c
> > > +++ b/fs/debugfs/file.c
> > > @@ -938,11 +938,18 @@ static const struct file_operations fops_str_wo = {
> > >    *          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.
> > > + *         from. The char* pointer must not change, except from NULL to
> > > + *         non-NULL.
> > 
> > This feels odd.  Why wouldn't you want to change the string value?  Or
> > why would you?
> 
> Well, if you _would_ want to change the string value, then the
> implementation of debugfs_create_str() is certainly broken and could
> only be fixed by involving a shared mutex to protect use of the pointer.

Agreed.  So let's just say "never change the pointer" and leave it at
that?

> > And why is this one-way transition ok?
> > 
> 
> This one case happens to be safe because it either sees NULL (which it
> handles) or a valid pointer (which is ok). It will not result in using a
> stale pointer. This wasn't a deliberate design intent but happens to be
> safe, and easily maintainable behavior.
> 
> A transition from valid->NULL or old->new isn't safe because the
> read function could get the old pointer but racing with that is the
> change to the pointer, and so the debugfs code could try to use a
> stale pointer.
> 
> > Given that this is only used internally, why is it exported?
> > 
> 
> It isn't only used internally. I found 3 drivers that use it.
> But there are no uses internal to debugfs.

Oops, I missed the other users (arm_scmi and opp), so let's leave it.

> I didn't write debugfs_create_str(), I only tried to use it and made
> an attempt to fix some problems.
> 
> Given the limitations of the basic implementation of
> debugfs_create_str() and its file reading function (the lack of
> protection against the pointer changing) perhaps drop this chain? Don't
> bother fixing it, instead deprecate it for being unsafe?

We don't "deprecate" things, that never works.  We either fix them, or
rip them out :)

thanks,

greg k-h
diff mbox series

Patch

diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
index 0c039a3d9a42..77794871f26d 100644
--- a/fs/debugfs/file.c
+++ b/fs/debugfs/file.c
@@ -938,11 +938,18 @@  static const struct file_operations fops_str_wo = {
  *          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.
+ *         from. The char* pointer must not change, except from NULL to
+ *         non-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.
+ *
+ * The char* pointed to by @value must not change after calling this
+ * function EXCEPT that it may change from NULL to non-NULL. This is to
+ * prevent the file read from accessing a stale pointer. A change from
+ * NULL to non-NULL is the only safe change, because the read will
+ * instantaneously see either NULL or the valid pointer.
  */
 void debugfs_create_str(const char *name, umode_t mode,
 			struct dentry *parent, char **value)