debugfs: support read-only and write-only bool types

PATCH In review Richard Fitzgerald 3 participants 8 messages Jun 5, 2015 – Jun 8, 2015

Subsystems (2)

Driver core Register map abstraction

lore

Share & tools

lore

Richard Fitzgerald

author

2015-06-05 14:37 UTC #0 lore raw

cover debugfs: support read-only and write-only bool types

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(-)

Signature · 1 line

1.7.2.5

Richard Fitzgerald

author

2015-06-05 14:37 UTC #1 lore raw

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

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

Signed-off-by Richard Fitzgerald <[email protected]>

---
 fs/debugfs/file.c |   35 +++++++++++++++++++++++++++++++++++
 1 files changed, 35 insertions(+), 0 deletions(-)

fs/debugfs/file.c +35 -0

index 830a7e7..7dc3f8a 100644
@@ -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);

Signature · 1 line

1.7.2.5

Mark Brown

maintainer

2015-06-05 16:43 UTC #2 lore raw

fs/debugfs/file.c const struct file_operations debugfs_file_operations = { · line 50

Quoted 5 lines · Richard Fitzgerald

fs/debugfs/file.c +5 -0

+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)?

Greg KH

maintainer

2015-06-08 20:57 UTC #3 lore raw

Quoted 12 lines · Mark Brown

fs/debugfs/file.c const struct file_operations debugfs_file_operations = { · line 50
Quoted 5 lines · Richard Fitzgerald
fs/debugfs/file.c +5 -0
+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.

Richard Fitzgerald

author

2015-06-05 14:37 UTC #4 lore raw

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

In reply to 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 <[email protected]>

---
 drivers/base/regmap/regmap-debugfs.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

drivers/base/regmap/regmap-debugfs.c +6 -6

index 5799a0b..a48579e 100644
@@ -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);

Signature · 1 line

1.7.2.5

Mark Brown

maintainer

2015-06-05 15:25 UTC #5 lore raw

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

Quoted 2 lines · 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.

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

Quoted 2 lines

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.

Richard Fitzgerald

author

2015-06-05 15:54 UTC #6 lore raw

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

Quoted 10 lines · Mark Brown

Quoted 2 lines · 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.
Your expectation is not my expectation here :) The permissions here
are quite deliberate.
Quoted 2 lines
This is only working accidentally because debugfs doesn't currently honour the read-only bit.

Quoted 8 lines

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.

Mark Brown

maintainer

2015-06-05 16:54 UTC #7 lore raw

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

Quoted 8 lines · Richard Fitzgerald

Quoted 5 lines · Mark Brown
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.

Quoted 5 lines

Quoted 2 lines
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.

Quoted 5 lines

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.

Quoted 3 lines

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.