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

[Liviu Dudau <Liviu.Dudau@arm.com>]

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!  /
  ---------------
    ¯\_(ツ)_/¯