Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1452424 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH] debugfs: Add proxy function for the mmap file operation Liviu Dudau <Liviu.Dudau@arm.com> - 2016-07-29 16:40 +0200
    Re: [PATCH] debugfs: Add proxy function for the mmap file operation Nicolai Stange <nicstange@gmail.com> - 2016-07-29 19:40 +0200
      Re: [PATCH] debugfs: Add proxy function for the mmap file operation Nicolai Stange <nicstange@gmail.com> - 2016-08-02 20:40 +0200
        Re: [PATCH] debugfs: Add proxy function for the mmap file operation Brian Starkey <brian.starkey@arm.com> - 2016-08-05 12:20 +0200
          Re: [PATCH] debugfs: Add proxy function for the mmap file operation Nicolai Stange <nicstange@gmail.com> - 2016-08-05 13:20 +0200

#1452424 — [PATCH] debugfs: Add proxy function for the mmap file operation

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.

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));
+
 FULL_PROXY_FUNC(unlocked_ioctl, long, filp,
 		PROTO(struct file *filp, unsigned int cmd, unsigned long arg),
 		ARGS(filp, cmd, arg));
@@ -224,6 +228,8 @@ static void __full_proxy_fops_init(struct file_operations *proxy_fops,
 		proxy_fops->write = full_proxy_write;
 	if (real_fops->poll)
 		proxy_fops->poll = full_proxy_poll;
+	if (real_fops->mmap)
+		proxy_fops->mmap = full_proxy_mmap;
 	if (real_fops->unlocked_ioctl)
 		proxy_fops->unlocked_ioctl = full_proxy_unlocked_ioctl;
 }
-- 
2.9.0

[toc] | [next] | [standalone]

#1452490

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

[toc] | [prev] | [next] | [standalone]

#1455379

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.


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

[toc] | [prev] | [next] | [standalone]

#1457001

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
>

[toc] | [prev] | [next] | [standalone]

#1457034

Brian Starkey <brian.starkey@arm.com> writes:

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

I personally think that having ->mmap() support in debugfs would be a
good thing to have in general and I expect there to be some further
demand in the future.

But I also think that it is a little bit fragile in the current state:
how many people actually run the Cocci scripts on their changes? AFAICT,
even the kbuild test robot doesn't do this. And after all, the Cocci
script I provided could very well miss some obfuscated writes to
vma->vm_ops: if they aren't done from ->mmap() themselves, but from some
helper function invoked therein, for example.

I would personally prefer a hand coded full_proxy_mmap() which WARN()s
if the proxied ->mmap() changes vma->vm_ops:
- this would add an extra safety net
- ->mmap() for debugfs files isn't performance critical
- and lastly, we're already doing something similar to this in
  open_proxy_open().

But in the end, it's not mine but Greg K-H's opinion that matters here...

Thanks,

Nicolai

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web