Messages in this thread | | | Date | Fri, 5 Aug 2016 11:18:49 +0100 | From | Brian Starkey <> | 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@gmail.com> writes: >> Liviu Dudau <Liviu.Dudau@arm.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@gmail.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@gmail.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 >+ >+@unsupp_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 >+ ... >+} >+ >+@unsupp_fops@ >+identifier fops; >+identifier unsupp_mmap_impl.mmap_impl; >+@@ >+struct file_operations fops = { >+ .mmap = mmap_impl, >+}; >+ >+@unsupp_fops_usage@ >+expression name, mode, parent, data; >+identifier unsupp_fops.fops; >+@@ >+debugfs_create_file(name, mode, parent, data, &fops) >+ >+ >+@context_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 >+ ... >+} >+ >+@script: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") >+ >+@script: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 >
| |