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
[PATCH 6/9] debugfs: Implement debugfs_create_str()
Signed-off-by: Peter Zijlstra (Intel)
---
fs/debugfs/file.c | 144
include/linux/debugfs.h | 29 +
2 files changed, 173 insertions(+)
--- a/fs/debugfs/file.c
+++ b/fs/debugfs/file.c
@@ -865,6 +865,150 @@ struct dentry *debugfs_create_bool(const
}
EXPORT_SYMBOL_GPL(debugfs_create_bool);
+ssize_t debugfs_read_file_str(struct file *file, char __user *user_buf,
+ size_t count, loff_t *ppos)
+{
+ struct dentry *dentry = F_DENTRY(file);
+ char *str, *copy = NULL;
+ int copy_len, len;
+ ssize_t ret;
+
+ ret = debugfs_file_get(dentry);
+ if (unlikely(ret))
+ return ret;
+
+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();
+
+ debugfs_file_put(dentry);
+
+ ret = simple_read_from_buffer(user_buf, count, ppos, copy, len + 1);
+ kfree(copy);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(debugfs_read_file_str);
+
+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 concattenation */
+ r = -EINVAL;
+ if (pos && pos != strlen(old))
+ 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;
+}
+EXPORT_SYMBOL_GPL(debugfs_write_file_str);
+
+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
+ * @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)
+{
+ return debugfs_create_mode_unsafe(name, mode, parent, value, &fops_str,
+ &fops_str_ro, &fops_str_wo);
+}
+EXPORT_SYMBOL_GPL(debugfs_create_str);
+
static ssize_t read_file_blob(struct file *file, char __user *user_buf,
