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

[Nicolai Stange <nicstange@gmail.com>]

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.


> Cc: Nicolai Stange <nicstange@gmail.com>
> Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
> ---
>  fs/debugfs/file.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> 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).

Another option would be to add a check in the wrapping ->mmap() whether
the vma->vm_ops has been set from the wrapped ->mmap().
Greg, do you think such a runtime check would be a good thing to have?

Btw, it would certainly be possible to even support a custom vma->vm_ops
by proxying this one, too. However, we probably would have to SIGSEGV
userspace if ->fault() was called on a stale debugfs file. And since
nobody has asked for this feature yet, I don't think that it should be
implemented now.

Thanks,

Nicolai