[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <87y4avktt9.fsf@gmail.com>
Date: Mon, 08 Feb 2016 18:14:58 +0100
From: Nicolai Stange <nicstange@...il.com>
To: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc: Nicolai Stange <nicstange@...il.com>,
Alexander Viro <viro@...iv.linux.org.uk>,
Jonathan Corbet <corbet@....net>, Jan Kara <jack@...e.com>,
"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
Andrew Morton <akpm@...ux-foundation.org>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] debugfs: prevent access to removed files' private data
Greg Kroah-Hartman <gregkh@...uxfoundation.org> 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
Powered by blists - more mailing lists