From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756022AbcBHRPF (ORCPT ); Mon, 8 Feb 2016 12:15:05 -0500 Received: from mail-wm0-f68.google.com ([74.125.82.68]:33940 "EHLO mail-wm0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754218AbcBHRPC (ORCPT ); Mon, 8 Feb 2016 12:15:02 -0500 From: Nicolai Stange To: Greg Kroah-Hartman Cc: Nicolai Stange , Alexander Viro , Jonathan Corbet , Jan Kara , "Paul E. McKenney" , Andrew Morton , linux-kernel@vger.kernel.org Subject: Re: [PATCH v2 2/2] debugfs: prevent access to removed files' private data References: <87fux3memd.fsf@gmail.com> <877fifmegw.fsf@gmail.com> <20160208161729.GA9814@kroah.com> Date: Mon, 08 Feb 2016 18:14:58 +0100 In-Reply-To: <20160208161729.GA9814@kroah.com> (Greg Kroah-Hartman's message of "Mon, 8 Feb 2016 08:17:29 -0800") Message-ID: <87y4avktt9.fsf@gmail.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Greg Kroah-Hartman 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