kernel/kcov: unproxify debugfs file's fops

PATCH In review Nicolai Stange 3 participants 3 messages May 24, 2016 – Jun 14, 2016 KCOV

Share & tools

Nicolai Stange

author

2016-05-24 12:05 UTC #0 lore raw

PATCH v2 1/1 kernel/kcov: unproxify debugfs file's fops

Since commit 49d200deaa68 ("debugfs: prevent access to removed files' private data"), a debugfs file's file_operations methods get proxied through lifetime aware wrappers.

However, only a certain subset of the file_operations members is supported by debugfs and ->mmap isn't among them -- it appears to be NULL from the VFS layer's perspective.

This behaviour breaks the /sys/kernel/debug/kcov file introduced concurrently with commit 5c9a8750a640 ("kernel: add kcov code coverage").

Since that file never gets removed, there is no file removal race and thus, a lifetime checking proxy isn't needed.

Avoid the proxying for /sys/kernel/debug/kcov by creating it via debugfs_create_file_unsafe() rather than debugfs_create_file().

Fixes 49d200deaa68 ("debugfs: prevent access to removed files' private

                      data")

Fixes 5c9a8750a640 ("kernel: add kcov code coverage")

Signed-off-by Nicolai Stange <[email protected]>

---
The v1 thread can be found at
http://lkml.kernel.org/g/[email protected]

Changes to v1:
- Following the suggestion of Kees Cook, a comment explaining why the use of debugfs_create_file_unsafe() is actually safe there has been added.

This issue has been debugged and reported by
Sasha Levin <[email protected]>:
http://lkml.kernel.org/g/[email protected]

Applicable to linux-next 20160524.
In particular, it depends on
- c64688081490 ("debugfs: add support for self-protecting attribute file

                  fops")

- 5c9a8750a640 ("kernel: add kcov code coverage")

 kernel/kcov.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

kernel/kcov.c +6 -1

index a02f2dd..8d44b3f 100644
@@ -264,7 +264,12 @@ static const struct file_operations kcov_fops = {
 
 static int __init kcov_init(void)
 {
-	if (!debugfs_create_file("kcov", 0600, NULL, NULL, &kcov_fops)) {
+	/*
+	 * The kcov debugfs file won't ever get removed and thus,
+	 * there is no need to protect it against removal races. The
+	 * use of debugfs_create_file_unsafe() is actually safe here.
+	 */
+	if (!debugfs_create_file_unsafe("kcov", 0600, NULL, NULL, &kcov_fops)) {
 		pr_err("failed to create kcov in debugfs\n");
 		return -ENOMEM;
 	}

Signature · 1 line

2.8.2

Greg Kroah-Hartman

2016-05-24 14:39 UTC #1 lore raw

Quoted 50 lines · Nicolai Stange

Since commit 49d200deaa68 ("debugfs: prevent access to removed files' private data"), a debugfs file's file_operations methods get proxied through lifetime aware wrappers.
However, only a certain subset of the file_operations members is supported by debugfs and ->mmap isn't among them -- it appears to be NULL from the VFS layer's perspective.
This behaviour breaks the /sys/kernel/debug/kcov file introduced concurrently with commit 5c9a8750a640 ("kernel: add kcov code coverage").
Since that file never gets removed, there is no file removal race and thus, a lifetime checking proxy isn't needed.
Avoid the proxying for /sys/kernel/debug/kcov by creating it via debugfs_create_file_unsafe() rather than debugfs_create_file().
Fixes 49d200deaa68 ("debugfs: prevent access to removed files' private
                      data")
Fixes 5c9a8750a640 ("kernel: add kcov code coverage")
Signed-off-by Nicolai Stange <[email protected]>
---
The v1 thread can be found at
http://lkml.kernel.org/g/[email protected]
Changes to v1:
- Following the suggestion of Kees Cook, a comment explaining why the use of debugfs_create_file_unsafe() is actually safe there has been added.
This issue has been debugged and reported by
Sasha Levin <[email protected]>:
http://lkml.kernel.org/g/[email protected]
Applicable to linux-next 20160524.
In particular, it depends on
- c64688081490 ("debugfs: add support for self-protecting attribute file
                  fops")
- 5c9a8750a640 ("kernel: add kcov code coverage")
 kernel/kcov.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
kernel/kcov.c +0 -0
index a02f2dd..8d44b3f 100644
@@ -264,7 +264,12 @@ static const struct file_operations kcov_fops = {
 
 static int __init kcov_init(void)
 {

kernel/kcov.c static const struct file_operations kcov_fops = { · line 275

Quoted 10 lines

kernel/kcov.c +6 -1

-	if (!debugfs_create_file("kcov", 0600, NULL, NULL, &kcov_fops)) {
+	/*
+	 * The kcov debugfs file won't ever get removed and thus,
+	 * there is no need to protect it against removal races. The
+	 * use of debugfs_create_file_unsafe() is actually safe here.
+	 */
+	if (!debugfs_create_file_unsafe("kcov", 0600, NULL, NULL, &kcov_fops)) {
 		pr_err("failed to create kcov in debugfs\n");
 		return -ENOMEM;
 	}

Thanks, I'll queue this up after 4.7-rc1 is out.

greg k-h

Sasha Levin

2016-06-14 21:19 UTC #2 lore raw

Quoted 49 lines · Greg Kroah-Hartman

Quoted 48 lines · Nicolai Stange
Quoted 48 lines
Since commit 49d200deaa68 ("debugfs: prevent access to removed files' private data"), a debugfs file's file_operations methods get proxied through lifetime aware wrappers.
However, only a certain subset of the file_operations members is supported by debugfs and ->mmap isn't among them -- it appears to be NULL from the VFS layer's perspective.
This behaviour breaks the /sys/kernel/debug/kcov file introduced concurrently with commit 5c9a8750a640 ("kernel: add kcov code coverage").
Since that file never gets removed, there is no file removal race and thus, a lifetime checking proxy isn't needed.
Avoid the proxying for /sys/kernel/debug/kcov by creating it via debugfs_create_file_unsafe() rather than debugfs_create_file().
Fixes 49d200deaa68 ("debugfs: prevent access to removed files' private
                      data")
Fixes 5c9a8750a640 ("kernel: add kcov code coverage")
Signed-off-by Nicolai Stange <[email protected]>
---
The v1 thread can be found at
http://lkml.kernel.org/g/[email protected]
Changes to v1:
- Following the suggestion of Kees Cook, a comment explaining why the use of debugfs_create_file_unsafe() is actually safe there has been added.
This issue has been debugged and reported by
Sasha Levin <[email protected]>:
http://lkml.kernel.org/g/[email protected]
Applicable to linux-next 20160524.
In particular, it depends on
- c64688081490 ("debugfs: add support for self-protecting attribute file
                  fops")
- 5c9a8750a640 ("kernel: add kcov code coverage")
 kernel/kcov.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
kernel/kcov.c +0 -0
index a02f2dd..8d44b3f 100644
@@ -264,7 +264,12 @@ static const struct file_operations kcov_fops = {
 

Quoted 13 lines

Quoted 4 lines

kernel/kcov.c +1 -1

 static int __init kcov_init(void)
 {
-	if (!debugfs_create_file("kcov", 0600, NULL, NULL, &kcov_fops)) {
+	/*

Quoted 8 lines

kernel/kcov.c static const struct file_operations kcov_fops = { · line 275

Quoted 8 lines

kernel/kcov.c +5 -0

+	 * The kcov debugfs file won't ever get removed and thus,
+	 * there is no need to protect it against removal races. The
+	 * use of debugfs_create_file_unsafe() is actually safe here.
+	 */
+	if (!debugfs_create_file_unsafe("kcov", 0600, NULL, NULL, &kcov_fops)) {
 		pr_err("failed to create kcov in debugfs\n");
 		return -ENOMEM;
 	}

Thanks, I'll queue this up after 4.7-rc1 is out.

Hey Greg,

Just wanted to remind you about this one.

Thanks,
Sasha