Re: [PATCH 6/9] debugfs: Implement debugfs_create_str()
On Sat, 27 Mar 2021 22:24:45 +
Al Viro wrote:
> On Fri, Mar 26, 2021 at 11:33:58AM +0100, Peter Zijlstra wrote:
>
> > +again:
> > + rcu_read_lock();
> > + str = rcu_dereference(*(char **)file->private_data);
> > + len = strlen(str) + 1;
> > +
> > + if (!copy || copy_len < len) {
> > + rcu_read_unlock();
> > + kfree(copy);
> > + copy = kmalloc(len + 1, GFP_KERNEL);
> > + if (!copy) {
> > + debugfs_file_put(dentry);
> > + return -ENOMEM;
> > + }
> > + copy_len = len;
> > + goto again;
> > + }
> > +
> > + strncpy(copy, str, len);
> > + copy[len] = '\n';
> > + copy[len+1] = '\0';
> > + rcu_read_unlock();
>
> *Ow*
>
> If the string can't change under you, what is RCU use about?
> And if it can, any use of string functions is asking for serious
> trouble; they are *not* guaranteed to be safe when any of the strings
> involved might be modified under them.
Just from looking at the above, RCU isn't protecting that the string
can change under you, but the pointer to file->private_data can.
str = rcu_dereference(*(char **)file->private_data);
That's just getting a pointer to the string. While under rcu, the value
of that string wont change nor will it be free. But file->private_data
might change, and it might free its old value, but will do so after a
RCU grace period (which is why the above has rcu_read_lock).
What the above looks like to me is a way to copy that string safely,
without worrying that it will be freed underneath you. But there's no
worry that it will change.
-- Steve
Re: [PATCH 6/9] debugfs: Implement debugfs_create_str()
On Fri, Mar 26, 2021 at 11:33:58AM +0100, Peter Zijlstra wrote:
> +again:
> + rcu_read_lock();
> + str = rcu_dereference(*(char **)file->private_data);
> + len = strlen(str) + 1;
> +
> + if (!copy || copy_len < len) {
> + rcu_read_unlock();
> + kfree(copy);
> + copy = kmalloc(len + 1, GFP_KERNEL);
> + if (!copy) {
> + debugfs_file_put(dentry);
> + return -ENOMEM;
> + }
> + copy_len = len;
> + goto again;
> + }
> +
> + strncpy(copy, str, len);
> + copy[len] = '\n';
> + copy[len+1] = '\0';
> + rcu_read_unlock();
*Ow*
If the string can't change under you, what is RCU use about?
And if it can, any use of string functions is asking for serious
trouble; they are *not* guaranteed to be safe when any of the strings
involved might be modified under them.
Re: [PATCH 6/9] debugfs: Implement debugfs_create_str()
On Fri, Mar 26, 2021 at 12:18:47PM +0100, Peter Zijlstra wrote:
> On Fri, Mar 26, 2021 at 12:05:07PM +0100, Greg KH wrote:
> > On Fri, Mar 26, 2021 at 11:33:58AM +0100, Peter Zijlstra wrote:
> > >
> > > Signed-off-by: Peter Zijlstra (Intel)
> >
> > No changelog text? :(
>
> Yeah, didn't really know what to say that the Subject didn't already do.
>
> > > +/**
> > > + * debugfs_create_str - create a debugfs file that is used to read and
> > > write 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 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.
> > > + *
> > > + * This function will return a pointer to a dentry if it succeeds. This
> > > + * pointer must be passed to the debugfs_remove() function when the file
> > > is
> > > + * to be removed (no automatic cleanup happens if your module is
> > > unloaded,
> > > + * you are responsible here.) If an error occurs, ERR_PTR(-ERROR) will
> > > be
> > > + * returned.
> > > + *
> > > + * NOTE: when writing is enabled it will replace the string, string
> > > lifetime is
> > > + * assumed to be RCU managed.
> > > + *
> > > + * If debugfs is not enabled in the kernel, the value ERR_PTR(-ENODEV)
> > > will
> > > + * be returned.
> > > + */
> > > +struct dentry *debugfs_create_str(const char *name, umode_t mode,
> > > +struct dentry *parent, char **value)
> >
> > Please have this return void, no need for me to have to clean up
> > afterward later on :)
>
> That would make it inconsistent with debugfs_create_{bool,blob}() from
> which I copiously 'borrowed'.
As mentioned on IRC, I am trying to get rid of the return value, those
are the only 2 holdouts given their current use in some of the cruftier
areas of the kernel at the moment :(
> Also, the return dentry might be useful with debugfs_create_symlink(),
> but you're right in that most other create_file wrappers return void.
Great, change that and limit the size of the string that can be written
and it looks good to me, thanks for adding this.
greg k-h
Re: [PATCH 6/9] debugfs: Implement debugfs_create_str()
On Fri, Mar 26, 2021 at 12:05:07PM +0100, Greg KH wrote:
> On Fri, Mar 26, 2021 at 11:33:58AM +0100, Peter Zijlstra wrote:
> >
> > Signed-off-by: Peter Zijlstra (Intel)
>
> No changelog text? :(
Yeah, didn't really know what to say that the Subject didn't already do.
> > +/**
> > + * debugfs_create_str - create a debugfs file that is used to read and
> > write 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 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.
> > + *
> > + * This function will return a pointer to a dentry if it succeeds. This
> > + * pointer must be passed to the debugfs_remove() function when the file is
> > + * to be removed (no automatic cleanup happens if your module is unloaded,
> > + * you are responsible here.) If an error occurs, ERR_PTR(-ERROR) will be
> > + * returned.
> > + *
> > + * NOTE: when writing is enabled it will replace the string, string
> > lifetime is
> > + * assumed to be RCU managed.
> > + *
> > + * If debugfs is not enabled in the kernel, the value ERR_PTR(-ENODEV) will
> > + * be returned.
> > + */
> > +struct dentry *debugfs_create_str(const char *name, umode_t mode,
> > + struct dentry *parent, char **value)
>
> Please have this return void, no need for me to have to clean up
> afterward later on :)
That would make it inconsistent with debugfs_create_{bool,blob}() from
which I copiously 'borrowed'.
Also, the return dentry might be useful with debugfs_create_symlink(),
but you're right in that most other create_file wrappers return void.
Re: [PATCH 6/9] debugfs: Implement debugfs_create_str()
On Fri, Mar 26, 2021 at 11:33:58AM +0100, Peter Zijlstra wrote: > > Signed-off-by: Peter Zijlstra (Intel) No changelog text? :( > +/** > + * debugfs_create_str - create a debugfs file that is used to read and write > 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 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. > + * > + * This function will return a pointer to a dentry if it succeeds. This > + * pointer must be passed to the debugfs_remove() function when the file is > + * to be removed (no automatic cleanup happens if your module is unloaded, > + * you are responsible here.) If an error occurs, ERR_PTR(-ERROR) will be > + * returned. > + * > + * NOTE: when writing is enabled it will replace the string, string lifetime > is > + * assumed to be RCU managed. > + * > + * If debugfs is not enabled in the kernel, the value ERR_PTR(-ENODEV) will > + * be returned. > + */ > +struct dentry *debugfs_create_str(const char *name, umode_t mode, > +struct dentry *parent, char **value) Please have this return void, no need for me to have to clean up afterward later on :) thanks, greg k-h
