Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
---
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,
size_t count, loff_t *ppos)
{
--- a/include/linux/debugfs.h
+++ b/include/linux/debugfs.h
@@ -128,6 +128,8 @@ void debugfs_create_atomic_t(const char
struct dentry *parent, atomic_t *value);
struct dentry *debugfs_create_bool(const char *name, umode_t mode,
struct dentry *parent, bool *value);
+struct dentry *debugfs_create_str(const char *name, umode_t mode,
+ struct dentry *parent, char **value);
struct dentry *debugfs_create_blob(const char *name, umode_t mode,
struct dentry *parent,
@@ -156,6 +158,12 @@ ssize_t debugfs_read_file_bool(struct fi
ssize_t debugfs_write_file_bool(struct file *file, const char __user *user_buf,
size_t count, loff_t *ppos);
+ssize_t debugfs_read_file_str(struct file *file, char __user *user_buf,
+ size_t count, loff_t *ppos);
+
+ssize_t debugfs_write_file_str(struct file *file, const char __user *user_buf,
+ size_t count, loff_t *ppos);
+
#else
#include <linux/err.h>
@@ -297,6 +305,13 @@ static inline struct dentry *debugfs_cre
return ERR_PTR(-ENODEV);
}
+static inline struct dentry *debugfs_create_str(const char *name, umode_t mode,
+ struct dentry *parent,
+ char **value)
+{
+ return ERR_PTR(-ENODEV);
+}
+
static inline struct dentry *debugfs_create_blob(const char *name, umode_t mode,
struct dentry *parent,
struct debugfs_blob_wrapper *blob)
@@ -347,6 +362,20 @@ static inline ssize_t debugfs_write_file
{
return -ENODEV;
}
+
+static inline ssize_t debugfs_read_file_str(struct file *file,
+ char __user *user_buf,
+ size_t count, loff_t *ppos)
+{
+ return -ENODEV;
+}
+
+static inline ssize_t debugfs_write_file_str(struct file *file,
+ const char __user *user_buf,
+ size_t count, loff_t *ppos)
+{
+ return -ENODEV;
+}
#endif
On Fri, Mar 26, 2021 at 11:33:58AM +0100, Peter Zijlstra wrote:
>
> Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
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
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) <[email protected]>
>
> 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.
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) <[email protected]>
> >
> > 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
On Fri, Mar 26, 2021 at 12:30:24PM +0100, Greg KH wrote:
> Great, change that and limit the size of the string that can be written
> and it looks good to me, thanks for adding this.
Here goes..
---
Subject: debugfs: Implement debugfs_create_str()
From: Peter Zijlstra <[email protected]>
Date: Thu Mar 25 10:53:55 CET 2021
Implement debugfs_create_str() to easily display names and such in
debugfs.
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
---
fs/debugfs/file.c | 148 ++++++++++++++++++++++++++++++++++++++++++++++++
include/linux/debugfs.h | 27 ++++++++
2 files changed, 175 insertions(+)
--- a/fs/debugfs/file.c
+++ b/fs/debugfs/file.c
@@ -865,6 +865,154 @@ 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 = -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;
+}
+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.
+ */
+void debugfs_create_str(const char *name, umode_t mode,
+ struct dentry *parent, char **value)
+{
+ 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,
size_t count, loff_t *ppos)
{
--- a/include/linux/debugfs.h
+++ b/include/linux/debugfs.h
@@ -128,6 +128,8 @@ void debugfs_create_atomic_t(const char
struct dentry *parent, atomic_t *value);
struct dentry *debugfs_create_bool(const char *name, umode_t mode,
struct dentry *parent, bool *value);
+void debugfs_create_str(const char *name, umode_t mode,
+ struct dentry *parent, char **value);
struct dentry *debugfs_create_blob(const char *name, umode_t mode,
struct dentry *parent,
@@ -156,6 +158,12 @@ ssize_t debugfs_read_file_bool(struct fi
ssize_t debugfs_write_file_bool(struct file *file, const char __user *user_buf,
size_t count, loff_t *ppos);
+ssize_t debugfs_read_file_str(struct file *file, char __user *user_buf,
+ size_t count, loff_t *ppos);
+
+ssize_t debugfs_write_file_str(struct file *file, const char __user *user_buf,
+ size_t count, loff_t *ppos);
+
#else
#include <linux/err.h>
@@ -297,6 +305,11 @@ static inline struct dentry *debugfs_cre
return ERR_PTR(-ENODEV);
}
+static inline void debugfs_create_str(const char *name, umode_t mode,
+ struct dentry *parent,
+ char **value)
+{ }
+
static inline struct dentry *debugfs_create_blob(const char *name, umode_t mode,
struct dentry *parent,
struct debugfs_blob_wrapper *blob)
@@ -347,6 +360,20 @@ static inline ssize_t debugfs_write_file
{
return -ENODEV;
}
+
+static inline ssize_t debugfs_read_file_str(struct file *file,
+ char __user *user_buf,
+ size_t count, loff_t *ppos)
+{
+ return -ENODEV;
+}
+
+static inline ssize_t debugfs_write_file_str(struct file *file,
+ const char __user *user_buf,
+ size_t count, loff_t *ppos)
+{
+ return -ENODEV;
+}
#endif
On Fri, Mar 26, 2021 at 12:38:39PM +0100, Peter Zijlstra wrote:
> On Fri, Mar 26, 2021 at 12:30:24PM +0100, Greg KH wrote:
> > Great, change that and limit the size of the string that can be written
> > and it looks good to me, thanks for adding this.
>
> Here goes..
Great, except one tiny thing:
> + * 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.
Nothing is returned anymore so the top and bottom paragraphs here no
longer apply.
Fix that up and feel free to add:
Reviewed-by: Greg Kroah-Hartman <[email protected]>
thanks,
greg k-h
On 26/03/2021 12.38, Peter Zijlstra wrote:
> Implement debugfs_create_str() to easily display names and such in
> debugfs.
Patches 7-9 don't seem to add any users of this. What's it for precisely?
> +
> +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();
As noted (accidentally off-list), this is broken. I think you want this
on top
- len = strlen(str) + 1;
+ len = strlen(str);
- strncpy(copy, str, len);
+ memcpy(copy, str, len);
copy[len] = '\n';
- copy[len+1] = '\0';
> +EXPORT_SYMBOL_GPL(debugfs_read_file_str);
Why?
> +
> +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;
Other than the synchronize_rcu() below, I don't see any rcu stuff in
here. What prevents one task from kfree'ing old while another computes
its strlen()? Or two tasks from getting the same value of old and both
kfree'ing the same pointer?
And what part of the kernel periodically looks at some string and
decides something needs to be done? Shouldn't a write imply some
notification or callback? I can see the usefulness of the read part to
expose some otherwise-maintained string, but what good does allowing
writes do?
Rasmus
On Fri, Mar 26, 2021 at 01:53:59PM +0100, Rasmus Villemoes wrote:
> On 26/03/2021 12.38, Peter Zijlstra wrote:
>
> > Implement debugfs_create_str() to easily display names and such in
> > debugfs.
>
> Patches 7-9 don't seem to add any users of this. What's it for precisely?
It's used in patch 7, look close :)
>
> > +
> > +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();
>
> As noted (accidentally off-list), this is broken. I think you want this
> on top
>
> - len = strlen(str) + 1;
> + len = strlen(str);
>
> - strncpy(copy, str, len);
> + memcpy(copy, str, len);
> copy[len] = '\n';
> - copy[len+1] = '\0';
>
> > +EXPORT_SYMBOL_GPL(debugfs_read_file_str);
>
> Why?
Because there is a user of it? Yes, it's not in a module, but to make
it easy, might as well export it now.
thanks,
greg k-h
On 26/03/2021 13.57, Greg KH wrote:
> On Fri, Mar 26, 2021 at 01:53:59PM +0100, Rasmus Villemoes wrote:
>> On 26/03/2021 12.38, Peter Zijlstra wrote:
>>
>>> Implement debugfs_create_str() to easily display names and such in
>>> debugfs.
>>
>> Patches 7-9 don't seem to add any users of this. What's it for precisely?
>
> It's used in patch 7, look close :)
Ah, macrology. But the write capability doesn't seem used, and I (still)
fail to see how that could be useful.
>>> +EXPORT_SYMBOL_GPL(debugfs_read_file_str);
>>
>> Why?
>
> Because there is a user of it? Yes, it's not in a module, but to make
> it easy, might as well export it now.
No, I was asking why the individual read (and write) methods would need
to be exported. They have even less reason then debugfs_create_str() -
which I also think shouldn't be exported until a modular user shows up.
Rasmus
On Fri, Mar 26, 2021 at 02:10:41PM +0100, Rasmus Villemoes wrote:
> On 26/03/2021 13.57, Greg KH wrote:
> > On Fri, Mar 26, 2021 at 01:53:59PM +0100, Rasmus Villemoes wrote:
> >> On 26/03/2021 12.38, Peter Zijlstra wrote:
> >>
> >>> Implement debugfs_create_str() to easily display names and such in
> >>> debugfs.
> >>
> >> Patches 7-9 don't seem to add any users of this. What's it for precisely?
> >
> > It's used in patch 7, look close :)
>
> Ah, macrology. But the write capability doesn't seem used, and I (still)
> fail to see how that could be useful.
Correct, it isn't used. But I didn't think it would be acceptible to
not implement the write side. OTOH, if Greg would be okay with it, I can
change it to:
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;
}
EXPORT_SYMBOL_GPL(debugfs_write_file_str);
That's certianly simpler.
On Fri, Mar 26, 2021 at 03:12:35PM +0100, Peter Zijlstra wrote:
> On Fri, Mar 26, 2021 at 02:10:41PM +0100, Rasmus Villemoes wrote:
> > On 26/03/2021 13.57, Greg KH wrote:
> > > On Fri, Mar 26, 2021 at 01:53:59PM +0100, Rasmus Villemoes wrote:
> > >> On 26/03/2021 12.38, Peter Zijlstra wrote:
> > >>
> > >>> Implement debugfs_create_str() to easily display names and such in
> > >>> debugfs.
> > >>
> > >> Patches 7-9 don't seem to add any users of this. What's it for precisely?
> > >
> > > It's used in patch 7, look close :)
> >
> > Ah, macrology. But the write capability doesn't seem used, and I (still)
> > fail to see how that could be useful.
>
> Correct, it isn't used. But I didn't think it would be acceptible to
> not implement the write side. OTOH, if Greg would be okay with it, I can
> change it to:
>
> 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;
> }
> EXPORT_SYMBOL_GPL(debugfs_write_file_str);
>
> That's certianly simpler.
Fine with me. Might as well make it static and not exported as well if
you are touching it :)
On Fri, Mar 26, 2021 at 01:53:59PM +0100, Rasmus Villemoes wrote:
> On 26/03/2021 12.38, 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();
>
> As noted (accidentally off-list), this is broken. I think you want this
> on top
>
> - len = strlen(str) + 1;
> + len = strlen(str);
kmalloc(len + 2, ...);
> - strncpy(copy, str, len);
> + memcpy(copy, str, len);
> copy[len] = '\n';
> - copy[len+1] = '\0';
I'll go with strscpy() I tihnk, something like:
len = strscpy(copy, str, len);
if (len < 0)
return len;
copy[len] = '\n';
copy[len + 1] = '\0';
> > +EXPORT_SYMBOL_GPL(debugfs_read_file_str);
>
> Why?
Copy-pasta from debugfs_*_bool(). This thing seems to export everything
and I figured I'd go along with that.
Subject: debugfs: Implement debugfs_create_str()
From: Peter Zijlstra <[email protected]>
Date: Thu Mar 25 10:53:55 CET 2021
Implement debugfs_create_str() to easily display names and such in
debugfs.
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
---
fs/debugfs/file.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++++
include/linux/debugfs.h | 17 ++++++++
2 files changed, 111 insertions(+)
--- a/fs/debugfs/file.c
+++ b/fs/debugfs/file.c
@@ -865,6 +865,100 @@ 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;
+
+ str = *(char **)file->private_data;
+ len = strlen(str) + 1;
+ copy = kmalloc(len + 1, GFP_KERNEL);
+ if (!copy) {
+ debugfs_file_put(dentry);
+ return -ENOMEM;
+ }
+
+ copy_len = strscpy(copy, str, len);
+ debugfs_file_put(dentry);
+ if (copy_len < 0) {
+ kfree(copy);
+ return copy_len;
+ }
+
+ copy[copy_len] = '\n';
+ copy[copy_len + 1] = '\0';
+
+ ret = simple_read_from_buffer(user_buf, count, ppos, copy, copy_len + 1);
+ kfree(copy);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(debugfs_read_file_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;
+}
+
+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.
+ *
+ * If debugfs is not enabled in the kernel, the value ERR_PTR(-ENODEV) will
+ * be returned.
+ */
+void debugfs_create_str(const char *name, umode_t mode,
+ struct dentry *parent, char **value)
+{
+ 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,
size_t count, loff_t *ppos)
{
--- a/include/linux/debugfs.h
+++ b/include/linux/debugfs.h
@@ -128,6 +128,8 @@ void debugfs_create_atomic_t(const char
struct dentry *parent, atomic_t *value);
struct dentry *debugfs_create_bool(const char *name, umode_t mode,
struct dentry *parent, bool *value);
+void debugfs_create_str(const char *name, umode_t mode,
+ struct dentry *parent, char **value);
struct dentry *debugfs_create_blob(const char *name, umode_t mode,
struct dentry *parent,
@@ -156,6 +158,9 @@ ssize_t debugfs_read_file_bool(struct fi
ssize_t debugfs_write_file_bool(struct file *file, const char __user *user_buf,
size_t count, loff_t *ppos);
+ssize_t debugfs_read_file_str(struct file *file, char __user *user_buf,
+ size_t count, loff_t *ppos);
+
#else
#include <linux/err.h>
@@ -297,6 +302,11 @@ static inline struct dentry *debugfs_cre
return ERR_PTR(-ENODEV);
}
+static inline void debugfs_create_str(const char *name, umode_t mode,
+ struct dentry *parent,
+ char **value)
+{ }
+
static inline struct dentry *debugfs_create_blob(const char *name, umode_t mode,
struct dentry *parent,
struct debugfs_blob_wrapper *blob)
@@ -347,6 +357,13 @@ static inline ssize_t debugfs_write_file
{
return -ENODEV;
}
+
+static inline ssize_t debugfs_read_file_str(struct file *file,
+ char __user *user_buf,
+ size_t count, loff_t *ppos)
+{
+ return -ENODEV;
+}
#endif
On 26/03/2021 15.22, Peter Zijlstra wrote:
> On Fri, Mar 26, 2021 at 01:53:59PM +0100, Rasmus Villemoes wrote:
>> On 26/03/2021 12.38, 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();
>>
>> As noted (accidentally off-list), this is broken. I think you want this
>> on top
>>
>> - len = strlen(str) + 1;
>> + len = strlen(str);
>
> kmalloc(len + 2, ...);
No, because nul-terminating the stuff you pass to
simple_read_from_buffer is pointless cargo-culting. Yeah, read_file_bool
does it, but that's just bogus.
>> - strncpy(copy, str, len);
>> + memcpy(copy, str, len);
>> copy[len] = '\n';
>> - copy[len+1] = '\0';
>
> I'll go with strscpy() I tihnk, something like:
>
> len = strscpy(copy, str, len);
> if (len < 0)
> return len;
To what end? The only way that could possibly return -EFOO is if the
nul-terminator in str vanished between the strlen() and here, and in
that case you have bigger problems.
> copy[len] = '\n';
> copy[len + 1] = '\0';
>
>>> +EXPORT_SYMBOL_GPL(debugfs_read_file_str);
>>
>> Why?
>
> Copy-pasta from debugfs_*_bool(). This thing seems to export everything
> and I figured I'd go along with that.
I thought the convention was not to export anything until the kernel
itself had a (modular) user. But I can't find that stated under
Documentation/ anywhere - but it does say "Every function that is
exported to loadable modules using
``EXPORT_SYMBOL`` or ``EXPORT_SYMBOL_GPL`` should have a kernel-doc
comment.". Anyway, the *_bool stuff doesn't seem to be something to be
copy-pasted.
Rasmus
On Fri, Mar 26, 2021 at 03:58:37PM +0100, Rasmus Villemoes wrote:
> > kmalloc(len + 2, ...);
>
> No, because nul-terminating the stuff you pass to
> simple_read_from_buffer is pointless cargo-culting. Yeah, read_file_bool
> does it, but that's just bogus.
Urgh, feel yuck to not have it zero terminated, but if you feel strongly
about it I suppose I can make it go away.
> > len = strscpy(copy, str, len);
> > if (len < 0)
> > return len;
>
> To what end? The only way that could possibly return -EFOO is if the
> nul-terminator in str vanished between the strlen() and here, and in
> that case you have bigger problems.
There are strings in the kernel which we rewrite in most ugly ways,
task_struct::comm comes to mind. Best be paranoid.
> > Copy-pasta from debugfs_*_bool(). This thing seems to export everything
> > and I figured I'd go along with that.
>
> I thought the convention was not to export anything until the kernel
> itself had a (modular) user.
That's generally how I feel too. But this really isn't my subsystem so I
more or less try to mimmick what I see done there.
On Fri, Mar 26, 2021 at 04:19:12PM +0100, Peter Zijlstra wrote:
> On Fri, Mar 26, 2021 at 03:58:37PM +0100, Rasmus Villemoes wrote:
> > > kmalloc(len + 2, ...);
> >
> > No, because nul-terminating the stuff you pass to
> > simple_read_from_buffer is pointless cargo-culting. Yeah, read_file_bool
> > does it, but that's just bogus.
>
> Urgh, feel yuck to not have it zero terminated, but if you feel strongly
> about it I suppose I can make it go away.
>
> > > len = strscpy(copy, str, len);
> > > if (len < 0)
> > > return len;
> >
> > To what end? The only way that could possibly return -EFOO is if the
> > nul-terminator in str vanished between the strlen() and here, and in
> > that case you have bigger problems.
>
> There are strings in the kernel which we rewrite in most ugly ways,
> task_struct::comm comes to mind. Best be paranoid.
>
> > > Copy-pasta from debugfs_*_bool(). This thing seems to export everything
> > > and I figured I'd go along with that.
> >
> > I thought the convention was not to export anything until the kernel
> > itself had a (modular) user.
>
> That's generally how I feel too. But this really isn't my subsystem so I
> more or less try to mimmick what I see done there.
I'll take some time next week and go through and remove any exports in
debugfs that are not actually needed for exports as it's been a while
since I last looked...
But that should not affect this change, it's fine to me as-is.
thanks,
greg k-h
On Fri, Mar 26, 2021 at 03:50:00PM +0100, Peter Zijlstra wrote:
> Subject: debugfs: Implement debugfs_create_str()
> From: Peter Zijlstra <[email protected]>
> Date: Thu Mar 25 10:53:55 CET 2021
>
> Implement debugfs_create_str() to easily display names and such in
> debugfs.
>
> Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
Reviewed-by: Greg Kroah-Hartman <[email protected]>
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.
On Sat, 27 Mar 2021 22:24:45 +0000
Al Viro <[email protected]> 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