Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1721915 > unrolled thread

[PATCH net-next v2 02/10] net: dsa: debugfs: add tree

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH net-next v2 02/10] net: dsa: debugfs: add tree Vivien Didelot <vivien.didelot@savoirfairelinux.com> - 2017-08-28 21:30 +0200
    Re: [PATCH net-next v2 02/10] net: dsa: debugfs: add tree Vivien Didelot <vivien.didelot@savoirfairelinux.com> - 2017-09-08 16:30 +0200
      Re: [PATCH net-next v2 02/10] net: dsa: debugfs: add tree Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2017-09-08 16:50 +0200
    Re: [PATCH net-next v2 02/10] net: dsa: debugfs: add tree Vivien Didelot <vivien.didelot@savoirfairelinux.com> - 2017-09-08 17:10 +0200
      RE: [PATCH net-next v2 02/10] net: dsa: debugfs: add tree David Laight <David.Laight@ACULAB.COM> - 2017-09-08 17:10 +0200
      Re: [PATCH net-next v2 02/10] net: dsa: debugfs: add tree Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2017-09-08 17:30 +0200

#1721915 — [PATCH net-next v2 02/10] net: dsa: debugfs: add tree

This commit adds the boiler plate to create a DSA related debug
filesystem entry as well as a "tree" file, containing the tree index.

    # cat switch1/tree
    0

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
 net/dsa/debugfs.c | 107 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 107 insertions(+)

diff --git a/net/dsa/debugfs.c b/net/dsa/debugfs.c
index b6b5e5c97389..54e97e05a9d7 100644
--- a/net/dsa/debugfs.c
+++ b/net/dsa/debugfs.c
@@ -10,6 +10,7 @@
  */
 
 #include <linux/debugfs.h>
+#include <linux/seq_file.h>
 
 #include "dsa_priv.h"
 
@@ -19,6 +20,107 @@
 /* DSA module debugfs directory */
 static struct dentry *dsa_debugfs_dir;
 
+struct dsa_debugfs_ops {
+	int (*read)(struct dsa_switch *ds, int id, struct seq_file *seq);
+	int (*write)(struct dsa_switch *ds, int id, char *buf);
+};
+
+struct dsa_debugfs_priv {
+	const struct dsa_debugfs_ops *ops;
+	struct dsa_switch *ds;
+	int id;
+};
+
+static int dsa_debugfs_show(struct seq_file *seq, void *p)
+{
+	struct dsa_debugfs_priv *priv = seq->private;
+	struct dsa_switch *ds = priv->ds;
+
+	/* Somehow file mode is bypassed... Double check here */
+	if (!priv->ops->read)
+		return -EOPNOTSUPP;
+
+	return priv->ops->read(ds, priv->id, seq);
+}
+
+static ssize_t dsa_debugfs_write(struct file *file, const char __user *user_buf,
+				 size_t count, loff_t *ppos)
+{
+	struct seq_file *seq = file->private_data;
+	struct dsa_debugfs_priv *priv = seq->private;
+	struct dsa_switch *ds = priv->ds;
+	char buf[count + 1];
+	int err;
+
+	/* Somehow file mode is bypassed... Double check here */
+	if (!priv->ops->write)
+		return -EOPNOTSUPP;
+
+	if (copy_from_user(buf, user_buf, count))
+		return -EFAULT;
+
+	buf[count] = '\0';
+
+	err = priv->ops->write(ds, priv->id, buf);
+
+	return err ? err : count;
+}
+
+static int dsa_debugfs_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, dsa_debugfs_show, inode->i_private);
+}
+
+static const struct file_operations dsa_debugfs_fops = {
+	.open = dsa_debugfs_open,
+	.read = seq_read,
+	.write = dsa_debugfs_write,
+	.llseek = no_llseek,
+	.release = single_release,
+	.owner = THIS_MODULE,
+};
+
+static int dsa_debugfs_create_file(struct dsa_switch *ds, struct dentry *dir,
+				   char *name, int id,
+				   const struct dsa_debugfs_ops *ops)
+{
+	struct dsa_debugfs_priv *priv;
+	struct dentry *entry;
+	umode_t mode;
+
+	priv = devm_kzalloc(ds->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->ops = ops;
+	priv->ds = ds;
+	priv->id = id;
+
+	mode = 0;
+	if (ops->read)
+		mode |= 0444;
+	if (ops->write)
+		mode |= 0200;
+
+	entry = debugfs_create_file(name, mode, dir, priv, &dsa_debugfs_fops);
+	if (IS_ERR_OR_NULL(entry))
+		return -EFAULT;
+
+	return 0;
+}
+
+static int dsa_debugfs_tree_read(struct dsa_switch *ds, int id,
+				 struct seq_file *seq)
+{
+	seq_printf(seq, "%d\n", ds->dst->tree);
+
+	return 0;
+}
+
+static const struct dsa_debugfs_ops dsa_debugfs_tree_ops = {
+	.read = dsa_debugfs_tree_read,
+};
+
 static int dsa_debugfs_create_port(struct dsa_switch *ds, int port)
 {
 	struct dentry *dir;
@@ -48,6 +150,11 @@ static int dsa_debugfs_create_switch(struct dsa_switch *ds)
 	if (IS_ERR_OR_NULL(ds->debugfs_dir))
 		return -EFAULT;
 
+	err = dsa_debugfs_create_file(ds, ds->debugfs_dir, "tree", -1,
+				      &dsa_debugfs_tree_ops);
+	if (err)
+		return err;
+
 	for (i = 0; i < ds->num_ports; i++) {
 		if (ds->enabled_port_mask & BIT(i)) {
 			err = dsa_debugfs_create_port(ds, i);
-- 
2.14.1

[toc] | [next] | [standalone]

#1728971

Hi Greg,

Can I ask for a quick review of this patch as well? It's the one adding
the boilerplate for a single debugfs file, and I'm pretty sure it can be
reduced somehow.

Also more important, you will notice what seems to be a bug to me:
I can read or write a file even if I didn't mask the corresponding mode,
hence the double check in dsa_debugfs_show and dsa_debugfs_write.


Thanks,

        Vivien

[toc] | [prev] | [next] | [standalone]

#1728997

On Fri, Sep 08, 2017 at 10:18:27AM -0400, Vivien Didelot wrote:
> Hi Greg,
> 
> Can I ask for a quick review of this patch as well? It's the one adding
> the boilerplate for a single debugfs file, and I'm pretty sure it can be
> reduced somehow.

I don't see a patch here :(

> Also more important, you will notice what seems to be a bug to me:
> I can read or write a file even if I didn't mask the corresponding mode,
> hence the double check in dsa_debugfs_show and dsa_debugfs_write.

The mode can be changed by userspace, you shouldn't ever need to check
it in any debugfs calls, right?

thanks,

greg k-h

[toc] | [prev] | [next] | [standalone]

#1729013

Hi Greg,

You wrote:

> > Can I ask for a quick review of this patch as well? It's the one adding
> > the boilerplate for a single debugfs file, and I'm pretty sure it can be
> > reduced somehow.
> 
> I don't see a patch here :(

Oops, you weren't originally in Cc. Please find the patch below.

> > Also more important, you will notice what seems to be a bug to me:
> > I can read or write a file even if I didn't mask the corresponding mode
> > hence the double check in dsa_debugfs_show and dsa_debugfs_write.
> 
> The mode can be changed by userspace, you shouldn't ever need to check
> it in any debugfs calls, right?

Correct. But this happens even if the file mode isn't changed by
userspace in the meantime, which seemed weird to me. e.g. echo
redirected to a -r--r--r-- debugfs entry will call dsa_debugfs_write.


Thanks,

        Vivien


------ Beginning of the patch ------

This commit adds the boiler plate to create a DSA related debug
filesystem entry as well as a "tree" file, containing the tree index.

    # cat switch1/tree
    0

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
 net/dsa/debugfs.c | 107 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 107 insertions(+)

diff --git a/net/dsa/debugfs.c b/net/dsa/debugfs.c
index b6b5e5c97389..54e97e05a9d7 100644
--- a/net/dsa/debugfs.c
+++ b/net/dsa/debugfs.c
@@ -10,6 +10,7 @@
  */
 
 #include <linux/debugfs.h>
+#include <linux/seq_file.h>
 
 #include "dsa_priv.h"
 
@@ -19,6 +20,107 @@
 /* DSA module debugfs directory */
 static struct dentry *dsa_debugfs_dir;
 
+struct dsa_debugfs_ops {
+	int (*read)(struct dsa_switch *ds, int id, struct seq_file *seq);
+	int (*write)(struct dsa_switch *ds, int id, char *buf);
+};
+
+struct dsa_debugfs_priv {
+	const struct dsa_debugfs_ops *ops;
+	struct dsa_switch *ds;
+	int id;
+};
+
+static int dsa_debugfs_show(struct seq_file *seq, void *p)
+{
+	struct dsa_debugfs_priv *priv = seq->private;
+	struct dsa_switch *ds = priv->ds;
+
+	/* Somehow file mode is bypassed... Double check here */
+	if (!priv->ops->read)
+		return -EOPNOTSUPP;
+
+	return priv->ops->read(ds, priv->id, seq);
+}
+
+static ssize_t dsa_debugfs_write(struct file *file, const char __user *user_buf,
+				 size_t count, loff_t *ppos)
+{
+	struct seq_file *seq = file->private_data;
+	struct dsa_debugfs_priv *priv = seq->private;
+	struct dsa_switch *ds = priv->ds;
+	char buf[count + 1];
+	int err;
+
+	/* Somehow file mode is bypassed... Double check here */
+	if (!priv->ops->write)
+		return -EOPNOTSUPP;
+
+	if (copy_from_user(buf, user_buf, count))
+		return -EFAULT;
+
+	buf[count] = '\0';
+
+	err = priv->ops->write(ds, priv->id, buf);
+
+	return err ? err : count;
+}
+
+static int dsa_debugfs_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, dsa_debugfs_show, inode->i_private);
+}
+
+static const struct file_operations dsa_debugfs_fops = {
+	.open = dsa_debugfs_open,
+	.read = seq_read,
+	.write = dsa_debugfs_write,
+	.llseek = no_llseek,
+	.release = single_release,
+	.owner = THIS_MODULE,
+};
+
+static int dsa_debugfs_create_file(struct dsa_switch *ds, struct dentry *dir,
+				   char *name, int id,
+				   const struct dsa_debugfs_ops *ops)
+{
+	struct dsa_debugfs_priv *priv;
+	struct dentry *entry;
+	umode_t mode;
+
+	priv = devm_kzalloc(ds->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->ops = ops;
+	priv->ds = ds;
+	priv->id = id;
+
+	mode = 0;
+	if (ops->read)
+		mode |= 0444;
+	if (ops->write)
+		mode |= 0200;
+
+	entry = debugfs_create_file(name, mode, dir, priv, &dsa_debugfs_fops);
+	if (IS_ERR_OR_NULL(entry))
+		return -EFAULT;
+
+	return 0;
+}
+
+static int dsa_debugfs_tree_read(struct dsa_switch *ds, int id,
+				 struct seq_file *seq)
+{
+	seq_printf(seq, "%d\n", ds->dst->tree);
+
+	return 0;
+}
+
+static const struct dsa_debugfs_ops dsa_debugfs_tree_ops = {
+	.read = dsa_debugfs_tree_read,
+};
+
 static int dsa_debugfs_create_port(struct dsa_switch *ds, int port)
 {
 	struct dentry *dir;
@@ -48,6 +150,11 @@ static int dsa_debugfs_create_switch(struct dsa_switch *ds)
 	if (IS_ERR_OR_NULL(ds->debugfs_dir))
 		return -EFAULT;
 
+	err = dsa_debugfs_create_file(ds, ds->debugfs_dir, "tree", -1,
+				      &dsa_debugfs_tree_ops);
+	if (err)
+		return err;
+
 	for (i = 0; i < ds->num_ports; i++) {
 		if (ds->enabled_port_mask & BIT(i)) {
 			err = dsa_debugfs_create_port(ds, i);
-- 
2.14.1

[toc] | [prev] | [next] | [standalone]

#1729014

From: Vivien Didelot
> Sent: 08 September 2017 15:57
...
> > > Also more important, you will notice what seems to be a bug to me:
> > > I can read or write a file even if I didn't mask the corresponding mode
> > > hence the double check in dsa_debugfs_show and dsa_debugfs_write.
> >
> > The mode can be changed by userspace, you shouldn't ever need to check
> > it in any debugfs calls, right?
> 
> Correct. But this happens even if the file mode isn't changed by
> userspace in the meantime, which seemed weird to me. e.g. echo
> redirected to a -r--r--r-- debugfs entry will call dsa_debugfs_write.

root will be able to write using 'root' permissions, regardless of
the directory entry.

	David

[toc] | [prev] | [next] | [standalone]

#1729026

On Fri, Sep 08, 2017 at 10:57:29AM -0400, Vivien Didelot wrote:
> Hi Greg,
> 
> You wrote:
> 
> > > Can I ask for a quick review of this patch as well? It's the one adding
> > > the boilerplate for a single debugfs file, and I'm pretty sure it can be
> > > reduced somehow.
> > 
> > I don't see a patch here :(
> 
> Oops, you weren't originally in Cc. Please find the patch below.
> 
> > > Also more important, you will notice what seems to be a bug to me:
> > > I can read or write a file even if I didn't mask the corresponding mode
> > > hence the double check in dsa_debugfs_show and dsa_debugfs_write.
> > 
> > The mode can be changed by userspace, you shouldn't ever need to check
> > it in any debugfs calls, right?
> 
> Correct. But this happens even if the file mode isn't changed by
> userspace in the meantime, which seemed weird to me. e.g. echo
> redirected to a -r--r--r-- debugfs entry will call dsa_debugfs_write.
> 
> 
> Thanks,
> 
>         Vivien
> 
> 
> ------ Beginning of the patch ------
> 
> This commit adds the boiler plate to create a DSA related debug
> filesystem entry as well as a "tree" file, containing the tree index.
> 
>     # cat switch1/tree
>     0
> 
> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> ---
>  net/dsa/debugfs.c | 107 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 107 insertions(+)
> 
> diff --git a/net/dsa/debugfs.c b/net/dsa/debugfs.c
> index b6b5e5c97389..54e97e05a9d7 100644
> --- a/net/dsa/debugfs.c
> +++ b/net/dsa/debugfs.c
> @@ -10,6 +10,7 @@
>   */
>  
>  #include <linux/debugfs.h>
> +#include <linux/seq_file.h>
>  
>  #include "dsa_priv.h"
>  
> @@ -19,6 +20,107 @@
>  /* DSA module debugfs directory */
>  static struct dentry *dsa_debugfs_dir;
>  
> +struct dsa_debugfs_ops {
> +	int (*read)(struct dsa_switch *ds, int id, struct seq_file *seq);
> +	int (*write)(struct dsa_switch *ds, int id, char *buf);
> +};
> +
> +struct dsa_debugfs_priv {
> +	const struct dsa_debugfs_ops *ops;
> +	struct dsa_switch *ds;
> +	int id;
> +};
> +
> +static int dsa_debugfs_show(struct seq_file *seq, void *p)
> +{
> +	struct dsa_debugfs_priv *priv = seq->private;
> +	struct dsa_switch *ds = priv->ds;
> +
> +	/* Somehow file mode is bypassed... Double check here */

As was said, root can do this, change your comment, just delete it :)

> +	if (!priv->ops->read)
> +		return -EOPNOTSUPP;
> +
> +	return priv->ops->read(ds, priv->id, seq);
> +}
> +
> +static ssize_t dsa_debugfs_write(struct file *file, const char __user *user_buf,
> +				 size_t count, loff_t *ppos)
> +{
> +	struct seq_file *seq = file->private_data;
> +	struct dsa_debugfs_priv *priv = seq->private;
> +	struct dsa_switch *ds = priv->ds;
> +	char buf[count + 1];

Nice, userspace asks to write 100Gb, and boom, you just smashed the
stack!

Repeat after me:
	All input is evil.

Say it again.

Always remember it.

> +	int err;
> +
> +	/* Somehow file mode is bypassed... Double check here */
> +	if (!priv->ops->write)
> +		return -EOPNOTSUPP;
> +
> +	if (copy_from_user(buf, user_buf, count))
> +		return -EFAULT;
> +
> +	buf[count] = '\0';

Be careful here.

Use the kernel library functions instead of a "raw" copy_from/to_user()
calls, that is what they are there for (simple_read_to_buffer,
simple_write_to_buffer).

> +
> +	err = priv->ops->write(ds, priv->id, buf);
> +
> +	return err ? err : count;
> +}
> +
> +static int dsa_debugfs_open(struct inode *inode, struct file *file)
> +{
> +	return single_open(file, dsa_debugfs_show, inode->i_private);
> +}
> +
> +static const struct file_operations dsa_debugfs_fops = {
> +	.open = dsa_debugfs_open,
> +	.read = seq_read,
> +	.write = dsa_debugfs_write,
> +	.llseek = no_llseek,
> +	.release = single_release,
> +	.owner = THIS_MODULE,
> +};
> +
> +static int dsa_debugfs_create_file(struct dsa_switch *ds, struct dentry *dir,
> +				   char *name, int id,
> +				   const struct dsa_debugfs_ops *ops)
> +{
> +	struct dsa_debugfs_priv *priv;
> +	struct dentry *entry;
> +	umode_t mode;
> +
> +	priv = devm_kzalloc(ds->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->ops = ops;
> +	priv->ds = ds;
> +	priv->id = id;
> +
> +	mode = 0;
> +	if (ops->read)
> +		mode |= 0444;
> +	if (ops->write)
> +		mode |= 0200;
> +
> +	entry = debugfs_create_file(name, mode, dir, priv, &dsa_debugfs_fops);
> +	if (IS_ERR_OR_NULL(entry))
> +		return -EFAULT;

Again, you don't care, don't check!

thanks,

greg k-h

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web