[PATCH] btrfs: remove pointless debugfs interface

[PATCH] btrfs: remove pointless debugfs interface

[Eric Sandeen]

A /sys/kernel/debug/btrfs/test file was added nearly
two and a half years ago, but it serves no purpose;
it stores and returns a value, but nothing in the btrfs
code uses this value in any way.  There are no other btrfs
files in this debugfs dir.

This was brought to my attention because it is world-writable;
it is the only such file under /sys/kernel/debug, and without
knowledge of its purpose, some users were alarmed by this.

Another option would be to change the perms, but given that
there is no point to it as far as I can tell, it seems best
to simply remove it.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
index 4879656..8c3ffb9 100644
--- a/fs/btrfs/sysfs.c
+++ b/fs/btrfs/sysfs.c
@@ -24,7 +24,6 @@
 #include <linux/kobject.h>
 #include <linux/bug.h>
 #include <linux/genhd.h>
-#include <linux/debugfs.h>
 
 #include "ctree.h"
 #include "disk-io.h"
@@ -728,12 +727,6 @@ int btrfs_sysfs_add_device_link(struct btrfs_fs_devices *fs_devices,
 /* /sys/fs/btrfs/ entry */
 static struct kset *btrfs_kset;
 
-/* /sys/kernel/debug/btrfs */
-static struct dentry *btrfs_debugfs_root_dentry;
-
-/* Debugging tunables and exported data */
-u64 btrfs_debugfs_test;
-
 /*
  * Can be called by the device discovery thread.
  * And parent can be specified for seed device
@@ -827,19 +820,6 @@ void btrfs_sysfs_feature_update(struct btrfs_fs_info *fs_info,
 	ret = sysfs_create_group(fsid_kobj, &btrfs_feature_attr_group);
 }
 
-static int btrfs_init_debugfs(void)
-{
-#ifdef CONFIG_DEBUG_FS
-	btrfs_debugfs_root_dentry = debugfs_create_dir("btrfs", NULL);
-	if (!btrfs_debugfs_root_dentry)
-		return -ENOMEM;
-
-	debugfs_create_u64("test", S_IRUGO | S_IWUGO, btrfs_debugfs_root_dentry,
-			&btrfs_debugfs_test);
-#endif
-	return 0;
-}
-
 int btrfs_init_sysfs(void)
 {
 	int ret;
@@ -848,28 +828,19 @@ int btrfs_init_sysfs(void)
 	if (!btrfs_kset)
 		return -ENOMEM;
 
-	ret = btrfs_init_debugfs();
-	if (ret)
-		goto out1;
-
 	init_feature_attrs();
 	ret = sysfs_create_group(&btrfs_kset->kobj, &btrfs_feature_attr_group);
-	if (ret)
-		goto out2;
+	if (ret) {
+		kset_unregister(btrfs_kset);
+		return ret;
+	}
 
 	return 0;
-out2:
-	debugfs_remove_recursive(btrfs_debugfs_root_dentry);
-out1:
-	kset_unregister(btrfs_kset);
-
-	return ret;
 }
 
 void btrfs_exit_sysfs(void)
 {
 	sysfs_remove_group(&btrfs_kset->kobj, &btrfs_feature_attr_group);
 	kset_unregister(btrfs_kset);
-	debugfs_remove_recursive(btrfs_debugfs_root_dentry);
 }
 
diff --git a/fs/btrfs/sysfs.h b/fs/btrfs/sysfs.h
index d7da1a4..45bad52 100644
--- a/fs/btrfs/sysfs.h
+++ b/fs/btrfs/sysfs.h
@@ -1,11 +1,6 @@
 #ifndef _BTRFS_SYSFS_H_
 #define _BTRFS_SYSFS_H_
 
-/*
- * Data exported through sysfs
- */
-extern u64 btrfs_debugfs_test;
-
 enum btrfs_feature_set {
 	FEAT_COMPAT,
 	FEAT_COMPAT_RO,

Re: [PATCH] btrfs: remove pointless debugfs interface

[David Sterba]

On Wed, Aug 31, 2016 at 10:13:49AM -0500, Eric Sandeen wrote:
> A /sys/kernel/debug/btrfs/test file was added nearly
> two and a half years ago, but it serves no purpose;

It does. Introduced in 1bae30982bc86ab66d61ccb6e22792593b45d44d says
something about helping developers to easily export information from the
filesystem, to aid debugging. Writing the debugfs support code is not
obviously trivial, so it's idling in the source. Exporing a new value is
as easy as copy and update 3 lines of code. If you have no use for it,
fine.

> it stores and returns a value, but nothing in the btrfs
> code uses this value in any way.  There are no other btrfs
> files in this debugfs dir.
> 
> This was brought to my attention because it is world-writable;
> it is the only such file under /sys/kernel/debug, and without
> knowledge of its purpose, some users were alarmed by this.

So let's fix the permissions.

Re: [PATCH] btrfs: remove pointless debugfs interface

[Eric Sandeen]

On 8/31/16 2:08 PM, David Sterba wrote:
> On Wed, Aug 31, 2016 at 10:13:49AM -0500, Eric Sandeen wrote:
>> A /sys/kernel/debug/btrfs/test file was added nearly
>> two and a half years ago, but it serves no purpose;
> 
> It does. Introduced in 1bae30982bc86ab66d61ccb6e22792593b45d44d says
> something about helping developers to easily export information from the
> filesystem, to aid debugging. Writing the debugfs support code is not
> obviously trivial, so it's idling in the source. Exporing a new value is
> as easy as copy and update 3 lines of code. If you have no use for it,
> fine.

I had thought that Documentation/filesystems/debugfs.txt would suffice,
but if you keep stuff lying around in btrfs just in case somebody needs
to export a global variable in the future, I suppose that's cool too.  ;)

>> it stores and returns a value, but nothing in the btrfs
>> code uses this value in any way.  There are no other btrfs
>> files in this debugfs dir.
>>
>> This was brought to my attention because it is world-writable;
>> it is the only such file under /sys/kernel/debug, and without
>> knowledge of its purpose, some users were alarmed by this.
> 
> So let's fix the permissions.

*shrug* ok.

-Eric

Re: [PATCH] btrfs: remove pointless debugfs interface

[David Sterba]

On Wed, Aug 31, 2016 at 04:38:16PM -0500, Eric Sandeen wrote:
> On 8/31/16 2:08 PM, David Sterba wrote:
> > On Wed, Aug 31, 2016 at 10:13:49AM -0500, Eric Sandeen wrote:
> >> A /sys/kernel/debug/btrfs/test file was added nearly
> >> two and a half years ago, but it serves no purpose;
> > 
> > It does. Introduced in 1bae30982bc86ab66d61ccb6e22792593b45d44d says
> > something about helping developers to easily export information from the
> > filesystem, to aid debugging. Writing the debugfs support code is not
> > obviously trivial, so it's idling in the source. Exporing a new value is
> > as easy as copy and update 3 lines of code. If you have no use for it,
> > fine.
> 
> I had thought that Documentation/filesystems/debugfs.txt would suffice,
> but if you keep stuff lying around in btrfs just in case somebody needs
> to export a global variable in the future, I suppose that's cool too.  ;)

How much time would you spend coding it?  I guess more than a couple of
minutes, possibly more than once.  And not in the middle of debugging
something else.  There are more examples of code that has no apparent
user but is used for debugging.

Re: [PATCH] btrfs: remove pointless debugfs interface

[Jeff Mahoney]

[-- Attachment #1.1: Type: text/plain, Size: 1182 bytes --]

On 8/31/16 3:08 PM, David Sterba wrote:
> On Wed, Aug 31, 2016 at 10:13:49AM -0500, Eric Sandeen wrote:
>> A /sys/kernel/debug/btrfs/test file was added nearly
>> two and a half years ago, but it serves no purpose;
> 
> It does. Introduced in 1bae30982bc86ab66d61ccb6e22792593b45d44d says
> something about helping developers to easily export information from the
> filesystem, to aid debugging. Writing the debugfs support code is not
> obviously trivial, so it's idling in the source. Exporing a new value is
> as easy as copy and update 3 lines of code. If you have no use for it,
> fine.
> 
>> it stores and returns a value, but nothing in the btrfs
>> code uses this value in any way.  There are no other btrfs
>> files in this debugfs dir.
>>
>> This was brought to my attention because it is world-writable;
>> it is the only such file under /sys/kernel/debug, and without
>> knowledge of its purpose, some users were alarmed by this.
> 
> So let's fix the permissions.

Perhaps we can also just stick it behind a CONFIG option as well if the
intention is to keep it around for developer debugging purposes.

-Jeff

-- 
Jeff Mahoney
SUSE Labs


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 881 bytes --]

Re: [PATCH] btrfs: remove pointless debugfs interface

[David Sterba]

On Wed, Aug 31, 2016 at 06:36:34PM -0400, Jeff Mahoney wrote:
> On 8/31/16 3:08 PM, David Sterba wrote:
> > On Wed, Aug 31, 2016 at 10:13:49AM -0500, Eric Sandeen wrote:
> >> A /sys/kernel/debug/btrfs/test file was added nearly
> >> two and a half years ago, but it serves no purpose;
> > 
> > It does. Introduced in 1bae30982bc86ab66d61ccb6e22792593b45d44d says
> > something about helping developers to easily export information from the
> > filesystem, to aid debugging. Writing the debugfs support code is not
> > obviously trivial, so it's idling in the source. Exporing a new value is
> > as easy as copy and update 3 lines of code. If you have no use for it,
> > fine.
> > 
> >> it stores and returns a value, but nothing in the btrfs
> >> code uses this value in any way.  There are no other btrfs
> >> files in this debugfs dir.
> >>
> >> This was brought to my attention because it is world-writable;
> >> it is the only such file under /sys/kernel/debug, and without
> >> knowledge of its purpose, some users were alarmed by this.
> > 
> > So let's fix the permissions.
> 
> Perhaps we can also just stick it behind a CONFIG option as well if the
> intention is to keep it around for developer debugging purposes.

So let's hide creating the 'test' file under BTRFS_DEBUG at least.

[PATCH V2] btrfs: fix perms on demonstration debugfs interface

[Eric Sandeen]

btrfs provides a helpful demonstration of how to export
a global variable via debugfs; however, it is unique among
other debugfs files in that it is world-writable, which causes
some concern to people who are not familiar with its purpose.

Fix it so that it is only user-writable.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
index 4879656..fb84685 100644
--- a/fs/btrfs/sysfs.c
+++ b/fs/btrfs/sysfs.c
@@ -834,7 +834,7 @@ static int btrfs_init_debugfs(void)
 	if (!btrfs_debugfs_root_dentry)
 		return -ENOMEM;
 
-	debugfs_create_u64("test", S_IRUGO | S_IWUGO, btrfs_debugfs_root_dentry,
+	debugfs_create_u64("test", S_IRUGO | S_IWUSR, btrfs_debugfs_root_dentry,
 			&btrfs_debugfs_test);
 #endif
 	return 0;

Re: [PATCH V2] btrfs: fix perms on demonstration debugfs interface

[David Sterba]

On Wed, Aug 31, 2016 at 04:49:29PM -0500, Eric Sandeen wrote:
> btrfs provides a helpful demonstration of how to export
> a global variable via debugfs; however, it is unique among
> other debugfs files in that it is world-writable, which causes
> some concern to people who are not familiar with its purpose.
> 
> Fix it so that it is only user-writable.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

Reviewed-by: David Sterba <dsterba@suse.com>