Message-ID: <20160805101849.GA29995@e106950-lin.cambridge.arm.com>
Date:	Fri, 5 Aug 2016 11:18:49 +0100
From:	Brian Starkey <brian.starkey@....com>
To:	Nicolai Stange <nicstange@...il.com>
Cc:	Liviu Dudau <Liviu.Dudau@....com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] debugfs: Add proxy function for the mmap file operation

Hi Nicolai,

On Tue, Aug 02, 2016 at 07:31:36PM +0200, Nicolai Stange wrote:
>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.

So we do have an implementation using this, but it's likely we will
keep it out-of-tree (it's a stop-gap until we can get a non-debugfs
implementation of the functionality into mainline).

Do you think it's worth merging this (and your cocci script) anyway to
save someone else doing the same thing later?

Thanks,
Brian

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