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

[Greg KH <gregkh@linuxfoundation.org>]

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