[PATCH 0/2] debugfs: support read-only and write-only bool types

[PATCH 0/2] debugfs: support read-only and write-only bool types

[Richard Fitzgerald]

Honour the read/write access bits for bool types.
Regmap is declaring two of its read/write bools as read-only so
I have also done a patch for that.

Richard Fitzgerald (2):
  debugfs: support read-only and write-only bool types
  regmap: Fix permissions on debugfs cache controls

 drivers/base/regmap/regmap-debugfs.c |   12 +++++-----
 fs/debugfs/file.c                    |   35 ++++++++++++++++++++++++++++++++++
 2 files changed, 41 insertions(+), 6 deletions(-)

-- 
1.7.2.5

[PATCH 1/2] debugfs: support read-only and write-only bool types

[Richard Fitzgerald]

The various integer functions support read-only and write-only
versions so extend this to bool types.

Signed-off-by: Richard Fitzgerald <rf@opensource.wolfsonmicro.com>
---
 fs/debugfs/file.c |   35 +++++++++++++++++++++++++++++++++++
 1 files changed, 35 insertions(+), 0 deletions(-)

diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
index 830a7e7..7dc3f8a 100644
--- a/fs/debugfs/file.c
+++ b/fs/debugfs/file.c
@@ -43,6 +43,18 @@ const struct file_operations debugfs_file_operations = {
 	.llseek =	noop_llseek,
 };
 
+static ssize_t debugs_read_wo_file(struct file *file, char __user *buf,
+				   size_t count, loff_t *ppos)
+{
+	return -EACCES;
+}
+
+static ssize_t debugs_write_ro_file(struct file *file, const char __user *buf,
+				    size_t count, loff_t *ppos)
+{
+	return -EACCES;
+}
+
 static void *debugfs_follow_link(struct dentry *dentry, struct nameidata *nd)
 {
 	nd_set_link(nd, d_inode(dentry)->i_private);
@@ -488,6 +500,20 @@ static const struct file_operations fops_bool = {
 	.llseek =	default_llseek,
 };
 
+static const struct file_operations fops_bool_ro = {
+	.read =		read_file_bool,
+	.write =	debugs_write_ro_file,
+	.open =		simple_open,
+	.llseek =	default_llseek,
+};
+
+static const struct file_operations fops_bool_wo = {
+	.read =		debugs_read_wo_file,
+	.write =	write_file_bool,
+	.open =		simple_open,
+	.llseek =	default_llseek,
+};
+
 /**
  * debugfs_create_bool - create a debugfs file that is used to read and write a boolean value
  * @name: a pointer to a string containing the name of the file to create.
@@ -515,6 +541,15 @@ static const struct file_operations fops_bool = {
 struct dentry *debugfs_create_bool(const char *name, umode_t mode,
 				   struct dentry *parent, u32 *value)
 {
+	/* if there are no write bits set, make read only */
+	if (!(mode & S_IWUGO))
+		return debugfs_create_file(name, mode, parent, value,
+					   &fops_bool_ro);
+	/* if there are no read bits set, make write only */
+	if (!(mode & S_IRUGO))
+		return debugfs_create_file(name, mode, parent, value,
+					   &fops_bool_wo);
+
 	return debugfs_create_file(name, mode, parent, value, &fops_bool);
 }
 EXPORT_SYMBOL_GPL(debugfs_create_bool);
-- 
1.7.2.5

Re: [PATCH 1/2] debugfs: support read-only and write-only bool types

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 434 bytes --]

On Fri, Jun 05, 2015 at 03:34:45PM +0100, Richard Fitzgerald wrote:

> +static ssize_t debugs_read_wo_file(struct file *file, char __user *buf,
> +				   size_t count, loff_t *ppos)
> +{
> +	return -EACCES;
> +}

Perhaps I'm missing something but shouldn't we be able to just omit
empty functions like this (this seems to be what the other ro/wo debugfs
defintions do otherwise these would already be there and it's what I'd
expect)?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 1/2] debugfs: support read-only and write-only bool types

[Greg KH]

On Fri, Jun 05, 2015 at 05:42:55PM +0100, Mark Brown wrote:
> On Fri, Jun 05, 2015 at 03:34:45PM +0100, Richard Fitzgerald wrote:
> 
> > +static ssize_t debugs_read_wo_file(struct file *file, char __user *buf,
> > +				   size_t count, loff_t *ppos)
> > +{
> > +	return -EACCES;
> > +}
> 
> Perhaps I'm missing something but shouldn't we be able to just omit
> empty functions like this (this seems to be what the other ro/wo debugfs
> defintions do otherwise these would already be there and it's what I'd
> expect)?


Yes you should.

[PATCH 2/2] regmap: Fix permissions on debugfs cache controls

[Richard Fitzgerald]

The cache_only and cache_bypass debugfs entries are expected
to be writable by user-side but are set to read-only permissions.
This is only working accidentally because debugfs doesn't currently
honour the read-only bit.

Signed-off-by: Richard Fitzgerald <rf@opensource.wolfsonmicro.com>
---
 drivers/base/regmap/regmap-debugfs.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/base/regmap/regmap-debugfs.c b/drivers/base/regmap/regmap-debugfs.c
index 5799a0b..a48579e 100644
--- a/drivers/base/regmap/regmap-debugfs.c
+++ b/drivers/base/regmap/regmap-debugfs.c
@@ -530,12 +530,12 @@ void regmap_debugfs_init(struct regmap *map, const char *name)
 	}
 
 	if (map->cache_type) {
-		debugfs_create_bool("cache_only", 0400, map->debugfs,
-				    &map->cache_only);
-		debugfs_create_bool("cache_dirty", 0400, map->debugfs,
-				    &map->cache_dirty);
-		debugfs_create_bool("cache_bypass", 0400, map->debugfs,
-				    &map->cache_bypass);
+		debugfs_create_bool("cache_only", S_IRUGO | S_IWUSR,
+				    map->debugfs, &map->cache_only);
+		debugfs_create_bool("cache_dirty", S_IRUGO,
+				    map->debugfs, &map->cache_dirty);
+		debugfs_create_bool("cache_bypass", S_IRUGO | S_IWUSR,
+				    map->debugfs, &map->cache_bypass);
 	}
 
 	next = rb_first(&map->range_tree);
-- 
1.7.2.5

Re: [PATCH 2/2] regmap: Fix permissions on debugfs cache controls

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 872 bytes --]

On Fri, Jun 05, 2015 at 03:34:46PM +0100, Richard Fitzgerald wrote:
> The cache_only and cache_bypass debugfs entries are expected
> to be writable by user-side but are set to read-only permissions.

Your expectation is not my expectation here :)   The permissions here
are quite deliberate.

> This is only working accidentally because debugfs doesn't currently
> honour the read-only bit.

Honestly it wasn't supposed to be working at all.  We can have a
discussion about if it makes sense for it to work, that's not a totally
unreasonable thing though I'd really want to taint the kernel if anyone
actually does it (particularly for cache only) since it seems even more
likely to interact poorly with drivers than random register writes.

We'll also want to sync the cache when transitioning from cache only to
normal operation I think, or provide a way of doing that.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 2/2] regmap: Fix permissions on debugfs cache controls

[Richard Fitzgerald]

On Fri, Jun 05, 2015 at 04:25:37PM +0100, Mark Brown wrote:
> On Fri, Jun 05, 2015 at 03:34:46PM +0100, Richard Fitzgerald wrote:
> > The cache_only and cache_bypass debugfs entries are expected
> > to be writable by user-side but are set to read-only permissions.
> 
> Your expectation is not my expectation here :)   The permissions here
> are quite deliberate.
> 
> > This is only working accidentally because debugfs doesn't currently
> > honour the read-only bit.
> 
> Honestly it wasn't supposed to be working at all.  We can have a
> discussion about if it makes sense for it to work, that's not a totally
> unreasonable thing though I'd really want to taint the kernel if anyone
> actually does it (particularly for cache only) since it seems even more
> likely to interact poorly with drivers than random register writes.
> 
> We'll also want to sync the cache when transitioning from cache only to
> normal operation I think, or provide a way of doing that.

We use writability of these all the time for all sorts of debugging so
it would be bad for us if it actually stopped being writeable.

Our expectations are that you're on your own if you fiddle with the
cache settings via debugfs. Other people's might be different. But that's
the current behaviour so if anyone is currently using the accidental
writability this patch will preserve that behaviour (broken or not).
And if they aren't using it, it doesn't matter.

I think it's preferable to avoid changing the behaviour of regmap
as a side effect of improving debugfs and worry later, separately,
about whether to improve the way regmap handles this.

Re: [PATCH 2/2] regmap: Fix permissions on debugfs cache controls

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1689 bytes --]

On Fri, Jun 05, 2015 at 04:53:54PM +0100, Richard Fitzgerald wrote:
> On Fri, Jun 05, 2015 at 04:25:37PM +0100, Mark Brown wrote:

> > Honestly it wasn't supposed to be working at all.  We can have a
> > discussion about if it makes sense for it to work, that's not a totally
> > unreasonable thing though I'd really want to taint the kernel if anyone
> > actually does it (particularly for cache only) since it seems even more
> > likely to interact poorly with drivers than random register writes.

> > We'll also want to sync the cache when transitioning from cache only to
> > normal operation I think, or provide a way of doing that.

> We use writability of these all the time for all sorts of debugging so
> it would be bad for us if it actually stopped being writeable.

Sure, like I say it's not totally unreasonable.

> Our expectations are that you're on your own if you fiddle with the
> cache settings via debugfs. Other people's might be different. But that's
> the current behaviour so if anyone is currently using the accidental
> writability this patch will preserve that behaviour (broken or not).
> And if they aren't using it, it doesn't matter.

This is why we want it to taint - it doesn't stop people doing anything,
it just means that if they report bugs (potentially in something totally
unrelated) then any oopses or whatever will say someone was doing
something behind the back of the kernel which might've broken the world.

> I think it's preferable to avoid changing the behaviour of regmap
> as a side effect of improving debugfs and worry later, separately,
> about whether to improve the way regmap handles this.

The current behaviour is a bug in regmap.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]