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

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

[Liviu Dudau]

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

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

[Nicolai Stange]

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

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

[Nicolai Stange]

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

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

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

[Nicolai Stange]

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

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

[Greg Kroah-Hartman]

On Fri, Aug 05, 2016 at 01:11:45PM +0200, Nicolai Stange wrote:
> 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.

Ugh, mmap in debugfs, that's funny.  And sad...

> 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().

Yes, that would be the best thing to do here.

thanks,

greg k-h

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

[Liviu Dudau]

On Wed, Aug 31, 2016 at 03:07:49PM +0200, Greg Kroah-Hartman wrote:
> On Fri, Aug 05, 2016 at 01:11:45PM +0200, Nicolai Stange wrote:
> > 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.
> 
> Ugh, mmap in debugfs, that's funny.  And sad...

Yeah.

While our need for the mmap-ing the debugfs entry is at best a temporary
option and a hack, I would be interested to know what alternatives could
be used to read a large amount of data that does not need the seq_operations
API? The out-of-tree proof-of-concept code that we have to interact with
a memory write engine needs to be able to access the output buffer from
userspace, but that output buffer is created by the kernel KMS driver.

> 
> > 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().
> 
> Yes, that would be the best thing to do here.

Thanks a lot for the feedback and specially to Nicolai for the provided Cocci script!
Sorry for not replying earlier, I went on a long holiday and just returned.

Best regards,
Liviu

> 
> thanks,
> 
> greg k-h
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

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

[Greg Kroah-Hartman]

On Wed, Aug 31, 2016 at 04:23:52PM +0100, Liviu Dudau wrote:
> On Wed, Aug 31, 2016 at 03:07:49PM +0200, Greg Kroah-Hartman wrote:
> > On Fri, Aug 05, 2016 at 01:11:45PM +0200, Nicolai Stange wrote:
> > > 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.
> > 
> > Ugh, mmap in debugfs, that's funny.  And sad...
> 
> Yeah.
> 
> While our need for the mmap-ing the debugfs entry is at best a temporary
> option and a hack, I would be interested to know what alternatives could
> be used to read a large amount of data that does not need the seq_operations
> API? The out-of-tree proof-of-concept code that we have to interact with
> a memory write engine needs to be able to access the output buffer from
> userspace, but that output buffer is created by the kernel KMS driver.

What type of debugging do you need this for?

A binary sysfs attribute also might work well for you, on the device
that you are talking to, but if not, yeah, mmap on debugfs will work
just fine, seems to be the best fit.

thanks,

greg k-h

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

[Liviu Dudau]

On Thu, Sep 01, 2016 at 08:19:33AM +0200, Greg Kroah-Hartman wrote:
> On Wed, Aug 31, 2016 at 04:23:52PM +0100, Liviu Dudau wrote:
> > On Wed, Aug 31, 2016 at 03:07:49PM +0200, Greg Kroah-Hartman wrote:
> > > On Fri, Aug 05, 2016 at 01:11:45PM +0200, Nicolai Stange wrote:
> > > > 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.
> > > 
> > > Ugh, mmap in debugfs, that's funny.  And sad...
> > 
> > Yeah.
> > 
> > While our need for the mmap-ing the debugfs entry is at best a temporary
> > option and a hack, I would be interested to know what alternatives could
> > be used to read a large amount of data that does not need the seq_operations
> > API? The out-of-tree proof-of-concept code that we have to interact with
> > a memory write engine needs to be able to access the output buffer from
> > userspace, but that output buffer is created by the kernel KMS driver.
> 
> What type of debugging do you need this for?

Taking snapshots of a composition scene using the KMS driver for Mali DP.

> 
> A binary sysfs attribute also might work well for you, on the device
> that you are talking to, but if not, yeah, mmap on debugfs will work
> just fine, seems to be the best fit.

We felt sysfs gives a whiff of official support for the feature, while in reality
is a stop gap until we work out the V4L2 functionality to do the same thing.

Best regards,
Liviu

> 
> thanks,
> 
> greg k-h
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

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

[Greg Kroah-Hartman]

On Thu, Sep 01, 2016 at 01:50:39PM +0100, Liviu Dudau wrote:
> On Thu, Sep 01, 2016 at 08:19:33AM +0200, Greg Kroah-Hartman wrote:
> > On Wed, Aug 31, 2016 at 04:23:52PM +0100, Liviu Dudau wrote:
> > > On Wed, Aug 31, 2016 at 03:07:49PM +0200, Greg Kroah-Hartman wrote:
> > > > On Fri, Aug 05, 2016 at 01:11:45PM +0200, Nicolai Stange wrote:
> > > > > 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.
> > > > 
> > > > Ugh, mmap in debugfs, that's funny.  And sad...
> > > 
> > > Yeah.
> > > 
> > > While our need for the mmap-ing the debugfs entry is at best a temporary
> > > option and a hack, I would be interested to know what alternatives could
> > > be used to read a large amount of data that does not need the seq_operations
> > > API? The out-of-tree proof-of-concept code that we have to interact with
> > > a memory write engine needs to be able to access the output buffer from
> > > userspace, but that output buffer is created by the kernel KMS driver.
> > 
> > What type of debugging do you need this for?
> 
> Taking snapshots of a composition scene using the KMS driver for Mali DP.

So it's not just debugging?  This is a "real" thing that code will rely
on?  If so, that's not good, don't ever use debugfs for that.

good luck!

greg k-h