inode: debugfs_create_dir uses mode permission from parent

PATCH Superseded Thomas Richter 2 participants 6 messages Apr 27, 2018 Driver core

Share & tools

Thomas Richter

author

2018-04-27 08:07 UTC #0 lore raw

PATCH 1/1 inode: debugfs_create_dir uses mode permission from parent

Currently function debugfs_create_dir() creates a new directory in the debugfs (usually mounted /sys/kernel/debug) with permission rwxr-xr-x. This is hard coded.

Change this to use the parent directory permission.

Fixes edac65eaf8d5c ("debugfs: take mode-dependent parts of debugfs_get_inode() into callers")

Signed-off-by Thomas Richter <[email protected]>

Cc Greg Kroah-Hartman <[email protected]>

---
 fs/debugfs/inode.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

fs/debugfs/inode.c +4 -1

index 13b01351dd1c..80618330d86a 100644
@@ -512,7 +512,10 @@ struct dentry *debugfs_create_dir(const char *name, struct dentry *parent)
 	if (unlikely(!inode))
 		return failed_creating(dentry);
 
-	inode->i_mode = S_IFDIR | S_IRWXU | S_IRUGO | S_IXUGO;
+	if(!parent)
+		parent = debugfs_mount->mnt_root;
+	inode->i_mode = S_IFDIR | (d_inode(parent)->i_mode
+				   & (S_IRWXU | S_IRWXG));
 	inode->i_op = &simple_dir_inode_operations;
 	inode->i_fop = &simple_dir_operations;

Signature · 1 line

2.14.3

Greg KH

maintainer

2018-04-27 08:27 UTC #1 lore raw

Quoted 19 lines · Thomas Richter

Currently function debugfs_create_dir() creates a new directory in the debugfs (usually mounted /sys/kernel/debug) with permission rwxr-xr-x. This is hard coded.
Change this to use the parent directory permission.
Fixes edac65eaf8d5c ("debugfs: take mode-dependent parts of debugfs_get_inode() into callers")
Signed-off-by Thomas Richter <[email protected]>
Cc Greg Kroah-Hartman <[email protected]>
---
 fs/debugfs/inode.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
fs/debugfs/inode.c +0 -0
index 13b01351dd1c..80618330d86a 100644
@@ -512,7 +512,10 @@ struct dentry *debugfs_create_dir(const char *name, struct dentry *parent)
 	if (unlikely(!inode))

fs/debugfs/inode.c struct dentry *debugfs_create_dir(const char *name, struct dentry *parent) · line 521

Quoted 10 lines

fs/debugfs/inode.c +4 -1

 		return failed_creating(dentry);
 
-	inode->i_mode = S_IFDIR | S_IRWXU | S_IRUGO | S_IXUGO;
+	if(!parent)
+		parent = debugfs_mount->mnt_root;
+	inode->i_mode = S_IFDIR | (d_inode(parent)->i_mode
+				   & (S_IRWXU | S_IRWXG));
 	inode->i_op = &simple_dir_inode_operations;
 	inode->i_fop = &simple_dir_operations;

This looks ok, but is it going to change the permissions of existing stuff in ways that might breaks things, right?

Have you done a before/after comparison?

thanks,

greg k-h

Thomas-Mich Richter

author

2018-04-27 09:14 UTC #2 lore raw

Quoted 22 lines · Greg KH

Quoted 21 lines · Thomas Richter
Currently function debugfs_create_dir() creates a new directory in the debugfs (usually mounted /sys/kernel/debug) with permission rwxr-xr-x. This is hard coded.
Change this to use the parent directory permission.
Fixes edac65eaf8d5c ("debugfs: take mode-dependent parts of debugfs_get_inode() into callers")
Signed-off-by Thomas Richter <[email protected]>
Cc Greg Kroah-Hartman <[email protected]>
---
 fs/debugfs/inode.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
fs/debugfs/inode.c +0 -0
index 13b01351dd1c..80618330d86a 100644
@@ -512,7 +512,10 @@ struct dentry *debugfs_create_dir(const char *name, struct dentry *parent)
 	if (unlikely(!inode))
 		return failed_creating(dentry);
 

Quoted 11 lines

fs/debugfs/inode.c struct dentry *debugfs_create_dir(const char *name, struct dentry *parent) · line 521
Quoted 8 lines
fs/debugfs/inode.c +4 -1
-	inode->i_mode = S_IFDIR | S_IRWXU | S_IRUGO | S_IXUGO;
+	if(!parent)
+		parent = debugfs_mount->mnt_root;
+	inode->i_mode = S_IFDIR | (d_inode(parent)->i_mode
+				   & (S_IRWXU | S_IRWXG));
 	inode->i_op = &simple_dir_inode_operations;
 	inode->i_fop = &simple_dir_operations;
 
This looks ok, but is it going to change the permissions of existing stuff in ways that might breaks things, right?

Right, but debugfs is usually mounted on /sys/kernel/debug with permissions rwx to root owner. It can be changed after the mount, of course. Unless this is done, the directory permissions for /sys/kernel/debug will stop any descend regardless of the subdirectory permissions.

Quoted 2 lines

Have you done a before/after comparison?

I have tested this patch on my Linux 4.17.0rc2 kernel on s390. That worked well, I have not tested other systems.

Signature · 5 lines

Thomas Richter, Dept 3303, IBM s390 Linux Development, Boeblingen, Germany
--
Vorsitzende des Aufsichtsrats: Martina Koederitz 
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen / Registergericht: Amtsgericht Stuttgart, HRB 243294

Greg KH

maintainer

2018-04-27 10:06 UTC #3 lore raw

Quoted 41 lines · Thomas-Mich Richter

Quoted 22 lines · Greg KH
Quoted 21 lines · Thomas Richter
Currently function debugfs_create_dir() creates a new directory in the debugfs (usually mounted /sys/kernel/debug) with permission rwxr-xr-x. This is hard coded.
Change this to use the parent directory permission.
Fixes edac65eaf8d5c ("debugfs: take mode-dependent parts of debugfs_get_inode() into callers")
Signed-off-by Thomas Richter <[email protected]>
Cc Greg Kroah-Hartman <[email protected]>
---
 fs/debugfs/inode.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
fs/debugfs/inode.c +0 -0
index 13b01351dd1c..80618330d86a 100644
@@ -512,7 +512,10 @@ struct dentry *debugfs_create_dir(const char *name, struct dentry *parent)
 	if (unlikely(!inode))
 		return failed_creating(dentry);
 
Quoted 11 lines
fs/debugfs/inode.c struct dentry *debugfs_create_dir(const char *name, struct dentry *parent) · line 521
Quoted 8 lines
fs/debugfs/inode.c +4 -1
-	inode->i_mode = S_IFDIR | S_IRWXU | S_IRUGO | S_IXUGO;
+	if(!parent)
+		parent = debugfs_mount->mnt_root;
+	inode->i_mode = S_IFDIR | (d_inode(parent)->i_mode
+				   & (S_IRWXU | S_IRWXG));
 	inode->i_op = &simple_dir_inode_operations;
 	inode->i_fop = &simple_dir_operations;
 
This looks ok, but is it going to change the permissions of existing stuff in ways that might breaks things, right?
Right, but debugfs is usually mounted on /sys/kernel/debug with permissions rwx to root owner. It can be changed after the mount, of course. Unless this is done, the directory permissions for /sys/kernel/debug will stop any descend regardless of the subdirectory permissions.
Quoted 1 line

Quoted 4 lines

Quoted 1 line
Have you done a before/after comparison?
I have tested this patch on my Linux 4.17.0rc2 kernel on s390. That worked well, I have not tested other systems.

What do you mean by "worked well"? What were the full tree differences between before and after? You should be able to get this by using:
tree -dp /sys/kernel/debug/
and then doing a diff on the two files.

thanks,

greg k-h

Thomas-Mich Richter

author

2018-04-27 11:30 UTC #4 lore raw

Quoted 47 lines · Greg KH

Quoted 41 lines · Thomas-Mich Richter
Quoted 22 lines · Greg KH
Quoted 21 lines · Thomas Richter
Currently function debugfs_create_dir() creates a new directory in the debugfs (usually mounted /sys/kernel/debug) with permission rwxr-xr-x. This is hard coded.
Change this to use the parent directory permission.
Fixes edac65eaf8d5c ("debugfs: take mode-dependent parts of debugfs_get_inode() into callers")
Signed-off-by Thomas Richter <[email protected]>
Cc Greg Kroah-Hartman <[email protected]>
---
 fs/debugfs/inode.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
fs/debugfs/inode.c +0 -0
index 13b01351dd1c..80618330d86a 100644
@@ -512,7 +512,10 @@ struct dentry *debugfs_create_dir(const char *name, struct dentry *parent)
 	if (unlikely(!inode))
 		return failed_creating(dentry);
 
Quoted 11 lines
fs/debugfs/inode.c struct dentry *debugfs_create_dir(const char *name, struct dentry *parent) · line 521
Quoted 8 lines
fs/debugfs/inode.c +4 -1
-	inode->i_mode = S_IFDIR | S_IRWXU | S_IRUGO | S_IXUGO;
+	if(!parent)
+		parent = debugfs_mount->mnt_root;
+	inode->i_mode = S_IFDIR | (d_inode(parent)->i_mode
+				   & (S_IRWXU | S_IRWXG));
 	inode->i_op = &simple_dir_inode_operations;
 	inode->i_fop = &simple_dir_operations;
 
This looks ok, but is it going to change the permissions of existing stuff in ways that might breaks things, right?
Right, but debugfs is usually mounted on /sys/kernel/debug with permissions rwx to root owner. It can be changed after the mount, of course. Unless this is done, the directory permissions for /sys/kernel/debug will stop any descend regardless of the subdirectory permissions.
Quoted 1 line
Quoted 4 lines
Quoted 1 line
Have you done a before/after comparison?
I have tested this patch on my Linux 4.17.0rc2 kernel on s390. That worked well, I have not tested other systems.

Quoted 9 lines

What do you mean by "worked well"? What were the full tree differences between before and after? You should be able to get this by using:
tree -dp /sys/kernel/debug/
and then doing a diff on the two files.
thanks,
greg k-h

Ok, this is the tree output

Before the patch:
root@s8360047 ~]# tree -dp -L 1 /sys/kernel/debug/ /sys/kernel/debug/

├── [drwxr-xr-x]  bdi
├── [drwxr-xr-x]  block
├── [drwxr-xr-x]  dasd
├── [drwxr-xr-x]  device_component
├── [drwxr-xr-x]  extfrag
├── [drwxr-xr-x]  hid
├── [drwxr-xr-x]  kprobes
├── [drwxr-xr-x]  kvm
├── [drwxr-xr-x]  memblock
├── [drwxr-xr-x]  pm_qos
├── [drwxr-xr-x]  qdio
├── [drwxr-xr-x]  s390
├── [drwxr-xr-x]  s390dbf
└── [drwx------]  tracing

14 directories

After the patch:
[root@s8360047 ~]# tree -dp -L 1 /sys/kernel/debug/ sys/kernel/debug/

├── [drwx------]  bdi
├── [drwx------]  block
├── [drwx------]  dasd
├── [drwx------]  device_component
├── [drwx------]  extfrag
├── [drwx------]  hid
├── [drwx------]  kprobes
├── [drwx------]  kvm
├── [drwx------]  memblock
├── [drwx------]  pm_qos
├── [drwx------]  qdio
├── [drwx------]  s390
├── [drwx------]  s390dbf
└── [drwx------]  tracing

14 directories
[root@s8360047 ~]#

I attached the diff of the full tree before and after the patch.

Signature · 5 lines

Thomas Richter, Dept 3303, IBM s390 Linux Development, Boeblingen, Germany
--
Vorsitzende des Aufsichtsrats: Martina Koederitz 
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen / Registergericht: Amtsgericht Stuttgart, HRB 243294

2,52c2,52

< ├── [drwxr-xr-x]  bdi
< │   ├── [drwxr-xr-x]  1:0
< │   ├── [drwxr-xr-x]  1:1
< │   ├── [drwxr-xr-x]  1:10
< │   ├── [drwxr-xr-x]  1:11
< │   ├── [drwxr-xr-x]  1:12
< │   ├── [drwxr-xr-x]  1:13
< │   ├── [drwxr-xr-x]  1:14
< │   ├── [drwxr-xr-x]  1:15
< │   ├── [drwxr-xr-x]  1:2
< │   ├── [drwxr-xr-x]  1:3
< │   ├── [drwxr-xr-x]  1:4
< │   ├── [drwxr-xr-x]  1:5
< │   ├── [drwxr-xr-x]  1:6
< │   ├── [drwxr-xr-x]  1:7
< │   ├── [drwxr-xr-x]  1:8
< │   ├── [drwxr-xr-x]  1:9
< │   └── [drwxr-xr-x]  94:0
< ├── [drwxr-xr-x]  block
< ├── [drwxr-xr-x]  dasd
< │   ├── [drwxr-xr-x]  0.0.e18a
< │   ├── [drwxr-xr-x]  dasda
< │   └── [drwxr-xr-x]  global
< ├── [drwxr-xr-x]  device_component
< ├── [drwxr-xr-x]  extfrag
< ├── [drwxr-xr-x]  hid
< ├── [drwxr-xr-x]  kprobes
< ├── [drwxr-xr-x]  kvm
< ├── [drwxr-xr-x]  memblock
< ├── [drwxr-xr-x]  pm_qos
< ├── [drwxr-xr-x]  qdio
< │   └── [drwxr-xr-x]  0.0.f5f2
< ├── [drwxr-xr-x]  s390
< │   └── [drwxr-xr-x]  stsi
< ├── [drwxr-xr-x]  s390dbf
< │   ├── [drwxr-xr-x]  0.0.e18a
< │   ├── [drwxr-xr-x]  cio_crw
< │   ├── [drwxr-xr-x]  cio_msg
< │   ├── [drwxr-xr-x]  cio_trace
< │   ├── [drwxr-xr-x]  dasd
< │   ├── [drwxr-xr-x]  kvm-trace
< │   ├── [drwxr-xr-x]  lgr
< │   ├── [drwxr-xr-x]  qdio_0.0.f5f2
< │   ├── [drwxr-xr-x]  qdio_error
< │   ├── [drwxr-xr-x]  qdio_setup
< │   ├── [drwxr-xr-x]  qeth_card_0.0.f5f0
< │   ├── [drwxr-xr-x]  qeth_control
< │   ├── [drwxr-xr-x]  qeth_msg
< │   ├── [drwxr-xr-x]  qeth_setup
< │   ├── [drwxr-xr-x]  vmcp
< │   └── [drwxr-xr-x]  vmur

---

Quoted 51 lines

├── [drwx------]  bdi
│   ├── [drwx------]  1:0
│   ├── [drwx------]  1:1
│   ├── [drwx------]  1:10
│   ├── [drwx------]  1:11
│   ├── [drwx------]  1:12
│   ├── [drwx------]  1:13
│   ├── [drwx------]  1:14
│   ├── [drwx------]  1:15
│   ├── [drwx------]  1:2
│   ├── [drwx------]  1:3
│   ├── [drwx------]  1:4
│   ├── [drwx------]  1:5
│   ├── [drwx------]  1:6
│   ├── [drwx------]  1:7
│   ├── [drwx------]  1:8
│   ├── [drwx------]  1:9
│   └── [drwx------]  94:0
├── [drwx------]  block
├── [drwx------]  dasd
│   ├── [drwx------]  0.0.e18a
│   ├── [drwx------]  dasda
│   └── [drwx------]  global
├── [drwx------]  device_component
├── [drwx------]  extfrag
├── [drwx------]  hid
├── [drwx------]  kprobes
├── [drwx------]  kvm
├── [drwx------]  memblock
├── [drwx------]  pm_qos
├── [drwx------]  qdio
│   └── [drwx------]  0.0.f5f2
├── [drwx------]  s390
│   └── [drwx------]  stsi
├── [drwx------]  s390dbf
│   ├── [drwx------]  0.0.e18a
│   ├── [drwx------]  cio_crw
│   ├── [drwx------]  cio_msg
│   ├── [drwx------]  cio_trace
│   ├── [drwx------]  dasd
│   ├── [drwx------]  kvm-trace
│   ├── [drwx------]  lgr
│   ├── [drwx------]  qdio_0.0.f5f2
│   ├── [drwx------]  qdio_error
│   ├── [drwx------]  qdio_setup
│   ├── [drwx------]  qeth_card_0.0.f5f0
│   ├── [drwx------]  qeth_control
│   ├── [drwx------]  qeth_msg
│   ├── [drwx------]  qeth_setup
│   ├── [drwx------]  vmcp
│   └── [drwx------]  vmur

Greg KH

maintainer

2018-04-27 11:47 UTC #5 lore raw

Quoted 98 lines · Thomas-Mich Richter

Quoted 47 lines · Greg KH
Quoted 41 lines · Thomas-Mich Richter
Quoted 22 lines · Greg KH
Quoted 21 lines · Thomas Richter
Currently function debugfs_create_dir() creates a new directory in the debugfs (usually mounted /sys/kernel/debug) with permission rwxr-xr-x. This is hard coded.
Change this to use the parent directory permission.
Fixes edac65eaf8d5c ("debugfs: take mode-dependent parts of debugfs_get_inode() into callers")
Signed-off-by Thomas Richter <[email protected]>
Cc Greg Kroah-Hartman <[email protected]>
---
 fs/debugfs/inode.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
fs/debugfs/inode.c +0 -0
index 13b01351dd1c..80618330d86a 100644
@@ -512,7 +512,10 @@ struct dentry *debugfs_create_dir(const char *name, struct dentry *parent)
 	if (unlikely(!inode))
 		return failed_creating(dentry);
 
Quoted 11 lines
fs/debugfs/inode.c struct dentry *debugfs_create_dir(const char *name, struct dentry *parent) · line 521
Quoted 8 lines
fs/debugfs/inode.c +4 -1
-	inode->i_mode = S_IFDIR | S_IRWXU | S_IRUGO | S_IXUGO;
+	if(!parent)
+		parent = debugfs_mount->mnt_root;
+	inode->i_mode = S_IFDIR | (d_inode(parent)->i_mode
+				   & (S_IRWXU | S_IRWXG));
 	inode->i_op = &simple_dir_inode_operations;
 	inode->i_fop = &simple_dir_operations;
 
This looks ok, but is it going to change the permissions of existing stuff in ways that might breaks things, right?
Right, but debugfs is usually mounted on /sys/kernel/debug with permissions rwx to root owner. It can be changed after the mount, of course. Unless this is done, the directory permissions for /sys/kernel/debug will stop any descend regardless of the subdirectory permissions.
Quoted 1 line
Quoted 4 lines
Quoted 1 line
Have you done a before/after comparison?
I have tested this patch on my Linux 4.17.0rc2 kernel on s390. That worked well, I have not tested other systems.
Quoted 9 lines
What do you mean by "worked well"? What were the full tree differences between before and after? You should be able to get this by using:
tree -dp /sys/kernel/debug/
and then doing a diff on the two files.
thanks,
greg k-h
Ok, this is the tree output
Before the patch:
root@s8360047 ~]# tree -dp -L 1 /sys/kernel/debug/ /sys/kernel/debug/
├── [drwxr-xr-x]  bdi
├── [drwxr-xr-x]  block
├── [drwxr-xr-x]  dasd
├── [drwxr-xr-x]  device_component
├── [drwxr-xr-x]  extfrag
├── [drwxr-xr-x]  hid
├── [drwxr-xr-x]  kprobes
├── [drwxr-xr-x]  kvm
├── [drwxr-xr-x]  memblock
├── [drwxr-xr-x]  pm_qos
├── [drwxr-xr-x]  qdio
├── [drwxr-xr-x]  s390
├── [drwxr-xr-x]  s390dbf
└── [drwx------]  tracing
14 directories
After the patch:
[root@s8360047 ~]# tree -dp -L 1 /sys/kernel/debug/ sys/kernel/debug/
├── [drwx------]  bdi
├── [drwx------]  block
├── [drwx------]  dasd
├── [drwx------]  device_component
├── [drwx------]  extfrag
├── [drwx------]  hid
├── [drwx------]  kprobes
├── [drwx------]  kvm
├── [drwx------]  memblock
├── [drwx------]  pm_qos
├── [drwx------]  qdio
├── [drwx------]  s390
├── [drwx------]  s390dbf
└── [drwx------]  tracing

Quoted 4 lines

14 directories
[root@s8360047 ~]#
I attached the diff of the full tree before and after the patch.

"diff -u" is your friend, this isn't the 1990's anymore :)

Anyway, why just look at the root directory here? Your patch changes more than just that, right?

Also, always run checkpatch.pl on your patches before a grumpy maintainer tells you to run checkpatch.pl...

thanks,

greg k-h