Messages in this thread | | | Date | Mon, 8 Feb 2016 08:17:29 -0800 | From | Greg Kroah-Hartman <> | Subject | Re: [PATCH v2 2/2] debugfs: prevent access to removed files' private data |
| |
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?
thanks,
greg k-h
| |