Re: [PATCH v2 2/2] debugfs: prevent access to removed files' private data

[Nicolai Stange]

Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx> writes:

> On Mon, Feb 08, 2016 at 04:03:27PM +0100, Nicolai Stange wrote:
>> Upon return of debugfs_remove()/debugfs_remove_recursive(), it might
>> still be attempted to access associated private file data through
>> previously opened struct file objects. If that data has been freed by
>> the caller of debugfs_remove*() in the meanwhile, the reading/writing
>> process would either encounter a fault or, if the memory address in
>> question has been reassigned again, unrelated data structures could get
>> overwritten.
>> 
>> However, since debugfs files are seldomly removed, usually from module
>> exit handlers only, the impact is very low.
>> 
>> Since debugfs_remove() and debugfs_remove_recursive() are already
>> waiting for a SRCU grace period before returning to their callers,
>> enclosing the access to private file data from ->read() and ->write()
>> within a SRCU read-side critical section does the trick:
>> - Introduce the debugfs_file_use_data_start() and
>>   debugfs_file_use_data_finish() helpers which just enter and leave
>>   a SRCU read-side critical section. The former also reports whether the
>>   file is still alive, that is if d_delete() has _not_ been called on
>>   the corresponding dentry.
>> - Introduce the DEFINE_DEBUGFS_ATTRIBUTE() macro which is completely
>>   equivalent to the DEFINE_SIMPLE_ATTRIBUTE() macro except that
>>   ->read() and ->write are set to SRCU protecting wrappers around the
>>   original simple_read() and simple_write() helpers.
>> - Use that DEFINE_DEBUGFS_ATTRIBUTE() macro for all debugfs_create_*()
>>   attribute creation variants where appropriate.
>> - Manually introduce SRCU protection to the debugfs-predefined readers
>>   and writers not covered by the above DEFINE_SIMPLE_ATTRIBUTE()->
>>   DEFINE_DEBUGFS_ATTRIBUTE() replacement.
>> 
>> Finally, it should be worth to note that in the vast majority of cases
>> where debugfs users are handing in a "custom" struct file_operations
>> object to debugfs_create_file(), an attribute's associated data's
>> lifetime is bound to the one of the containing module and thus,
>> taking a reference on ->owner during file opening acts as a proxy here.
>> There is no need to do a mass replace of DEFINE_SIMPLE_ATTRIBUTE() to
>> DEFINE_DEBUGFS_ATTRIBUTE() outside of debugfs.
>> 
>> OTOH, new users of debugfs are encouraged to prefer the
>> DEFINE_DEBUGFS_ATTRIBUTE() macro over DEFINE_SIMPLE_ATTRIBUTE() and it,
>> as well as the needed read/write wrappers are made available globally.
>> For new users implementing their own readers and writers, the lifetime
>> management helpers debugfs_file_use_data_start() and
>> debugfs_file_use_data_finish() are exported.
>
> Nice job.  One more request... :)
>
> Can you show how you would convert a subsystem to use these new
> macros/calls to give a solid example of it in use outside of the debugfs
> core?

You mean in the form of a patch [3/3] for an arbitrary subsystem other
than debugfs? Or in the form of an update of
Documentation/filesystems/debugfs.txt?

In case you want to have a patch: for the DEFINE_DEBUGFS_ATTRIBUTE, I
could simply abuse
  drivers/staging/android/ion/ion.c
as it has got a DEFINE_SIMPLE_ATTRIBUTE debug_shrink_fops passed to
debugfs. In this particular case, it even looks like that this debugfs
file can be removed through ion_client_destroy() without any module
removal. Fixing this would be as easy as
s/DEFINE_SIMPLE_ATTRIBUTE/DEFINE_DEBUGFS_ATTRIBUTE/.

Regarding a use case with custom made file_operations whose
reader and writer are protected by the debugfs_file_use_data_*()
helpers, I'm a little bit at a loss with: ion.c has got its custom
'debug_heap_fops', but in this case, it would probably be more
appropriate to create a general debugfs_create_seqfile() centrally in
debugfs.

Thank you!

Nicolai