Date:	Tue, 02 Aug 2016 19:31:36 +0200
From:	Nicolai Stange <nicstange@...il.com>
To:	Liviu Dudau <Liviu.Dudau@....com>
Cc:	Nicolai Stange <nicstange@...il.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Brian Starkey <Brian.Starkey@....com>,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] debugfs: Add proxy function for the mmap file operation

Nicolai Stange <nicstange@...il.com> writes:
> Liviu Dudau <Liviu.Dudau@....com> writes:
>
>> Add proxy function for the mmap file_operations hook under the
>> full_proxy_fops structure. This is useful for providing a custom
>> mmap routine in a driver's debugfs implementation.
>
> I guess you've got some specific use case for mmap() usage on some new
> debugfs file in mind?
>
> Currently, there exist only two mmap providers:
>   drivers/staging/android/sync_debug.c
>   kernel/kcov.c
>
> Both don't suffer from the lack of mmap support in the debugfs full proxy
> implementation because they don't use it -- their files never go away
> and thus, can be (and are) created via debugfs_create_file_unsafe().
>
> However, if you wish to have some mmapable debugfs file which *can* go
> away, introducing mmap support in the debugfs full proxy is perfectly
> valid. But please see below.

Assuming that you've got such a use case, please consider resending your
patch along with the Cocci script below (and the Coccinelle team CC'ed,
of course). If OTOH your mmapable debugfs files are never removed, just
drop this message and use debugfs_create_file_unsafe() instead.


>> diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
>> index 592059f..d87148a 100644
>> --- a/fs/debugfs/file.c
>> +++ b/fs/debugfs/file.c
>> @@ -168,6 +168,10 @@ FULL_PROXY_FUNC(write, ssize_t, filp,
>>  			loff_t *ppos),
>>  		ARGS(filp, buf, size, ppos));
>>  
>> +FULL_PROXY_FUNC(mmap, int, filp,
>> +		PROTO(struct file *filp, struct vm_area_struct *vma),
>> +		ARGS(filp, vma));
>> +
>
>
> While this protects the call to ->mmap() itself against file removal
> races, it doesn't protect anything possibly installed at vma->vm_ops
> from that.
>
> I'm fine with this as long as vma->vm_ops isn't set from ->mmap() ;).
> At the very least, we should probably provide a Coccinelle script for
> this. I'll try to put something together at the weekend or at the
> beginning of next week (if you aren't faster).

Here it is:

--8<---------------cut here---------------start------------->8---
>From e26c57f5a57875a4acffdab837497ca4e9e85672 Mon Sep 17 00:00:00 2001
From: Nicolai Stange <nicstange@...il.com>
Date: Tue, 2 Aug 2016 18:33:59 +0200
Subject: debugfs, coccinelle: check for ->vm_ops setting ->mmap()
 implementations

While debugfs files may provide their custom ->mmap() implementations now,
they must not set the vm_area_struct's ->vm_ops for the following reason:
its methods can be invoked at any time by the MM subsystem and thus, they
are subject to file removal races.

Further explanation: for the struct file_operations, this issue has been
resolved by installing some protecting proxies from the debugfs core.
However, we certainly don't want to do this for the vm_operations_struct:
first, there isn't any real demand currently and second, we would probably
have to SIGSEGV userspace under certain conditions (->fault() invoked on
stale file).

Thus, don't support custom ->vm_ops for debugfs files. Introduce a
Coccinelle script checking for this forbidden usage pattern: moan if a
struct file_operations with a ->mmap() writing to vma->vm_ops is handed
to debugfs_create_file().

Signed-off-by: Nicolai Stange <nicstange@...il.com>

diff --git a/scripts/coccinelle/api/debugfs/debugfs_mmap_vm_ops.cocci b/scripts/coccinelle/api/debugfs/debugfs_mmap_vm_ops.cocci
new file mode 100644
index 0000000..c53286b
--- /dev/null
+++ b/scripts/coccinelle/api/debugfs/debugfs_mmap_vm_ops.cocci
@@ -0,0 +1,66 @@
+/// Don't set vma->vm_ops from a debugfs file's ->mmap() implementation.
+///
+//# Rationale: While a debugfs file's struct file_operations is
+//# protected against file removal races through a proxy wrapper
+//# automatically provided by the debugfs core, anything installed at
+//# vma->vm_ops from ->mmap() isn't: the mm subsystem may and will
+//# invoke its members at any time.
+//
+// Copyright (C): 2016 Nicolai Stange
+// Options: --no-includes
+//
+
+virtual context
+virtual report
+virtual org
+
+@...upp_mmap_impl@
+identifier mmap_impl;
+identifier filp, vma;
+expression e;
+position p;
+@@
+
+int mmap_impl(struct file *filp, struct vm_area_struct *vma)
+{
+  ...
+  vma->vm_ops@p = e
+  ...
+}
+
+@...upp_fops@
+identifier fops;
+identifier unsupp_mmap_impl.mmap_impl;
+@@
+struct file_operations fops = {
+ .mmap = mmap_impl,
+};
+
+@...upp_fops_usage@
+expression name, mode, parent, data;
+identifier unsupp_fops.fops;
+@@
+debugfs_create_file(name, mode, parent, data, &fops)
+
+
+@...text_unsupp_mmap_impl depends on context && unsupp_fops_usage@
+identifier unsupp_mmap_impl.mmap_impl;
+identifier unsupp_mmap_impl.filp, unsupp_mmap_impl.vma;
+expression unsupp_mmap_impl.e;
+@@
+int mmap_impl(struct file *filp, struct vm_area_struct *vma)
+{
+  ...
+* vma->vm_ops = e
+  ...
+}
+
+@...ipt:python depends on org && unsupp_fops_usage@
+p << unsupp_mmap_impl.p;
+@@
+coccilib.org.print_todo(p[0], "a debugfs file's ->mmap() must not set ->vm_ops")
+
+@...ipt:python depends on report && unsupp_fops_usage@
+p << unsupp_mmap_impl.p;
+@@
+coccilib.report.print_report(p[0], "a debugfs file's ->mmap() must not set ->vm_ops")
-- 
2.9.2

--8<---------------cut here---------------end--------------->8---

Thanks,

Nicolai

Powered by blists - more mailing lists