Re: [PATCH] debugfs: Add proxy function for the mmap file operation

[Brian Starkey]

Hi Nicolai,

On Tue, Aug 02, 2016 at 07:31:36PM +0200, Nicolai Stange wrote:
> Nicolai Stange <nicstange@xxxxxxxxx> writes:
> > Liviu Dudau <Liviu.Dudau@xxxxxxx> 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@xxxxxxxxx>
> 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@xxxxxxxxx>
>
> 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