From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753488AbcEROtM (ORCPT ); Wed, 18 May 2016 10:49:12 -0400 Received: from aserp1040.oracle.com ([141.146.126.69]:46962 "EHLO aserp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753137AbcEROtK (ORCPT ); Wed, 18 May 2016 10:49:10 -0400 Subject: Re: [PATCH v6 2/8] debugfs: prevent access to removed files' private data To: Nicolai Stange , Greg Kroah-Hartman References: <1458652280-19785-1-git-send-email-nicstange@gmail.com> <1458652280-19785-3-git-send-email-nicstange@gmail.com> Cc: Rasmus Villemoes , "Paul E. McKenney" , Alexander Viro , Jonathan Corbet , Jan Kara , Andrew Morton , Julia Lawall , Gilles Muller , Nicolas Palix , Michal Marek , linux-kernel@vger.kernel.org, cocci@systeme.lip6.fr From: Sasha Levin Message-ID: <573C80C8.6090307@oracle.com> Date: Wed, 18 May 2016 10:48:40 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.6.0 MIME-Version: 1.0 In-Reply-To: <1458652280-19785-3-git-send-email-nicstange@gmail.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-Source-IP: userv0021.oracle.com [156.151.31.71] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 03/22/2016 09:11 AM, 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. > > Currently, there are ~1000 call sites of debugfs_create_file() spread > throughout the whole tree and touching all of those struct file_operations > in order to make them file removal aware by means of checking the result of > debugfs_use_file_start() from within their methods is unfeasible. > > Instead, wrap the struct file_operations by a lifetime managing proxy at > file open: > - In debugfs_create_file(), the original fops handed in has got stashed > away in ->d_fsdata already. > - In debugfs_create_file(), install a proxy file_operations factory, > debugfs_full_proxy_file_operations, at ->i_fop. > > This proxy factory has got an ->open() method only. It carries out some > lifetime checks and if successful, dynamically allocates and sets up a new > struct file_operations proxy at ->f_op. Afterwards, it forwards to the > ->open() of the original struct file_operations in ->d_fsdata, if any. > > The dynamically set up proxy at ->f_op has got a lifetime managing wrapper > set for each of the methods defined in the original struct file_operations > in ->d_fsdata. > > Its ->release()er frees the proxy again and forwards to the original > ->release(), if any. > > In order not to mislead the VFS layer, it is strictly necessary to leave > those fields blank in the proxy that have been NULL in the original > struct file_operations also, i.e. aren't supported. This is why there is a > need for dynamically allocated proxies. The choice made not to allocate a > proxy instance for every dentry at file creation, but for every > struct file object instantiated thereof is justified by the expected usage > pattern of debugfs, namely that in general very few files get opened more > than once at a time. > > The wrapper methods set in the struct file_operations implement lifetime > managing by means of the SRCU protection facilities already in place for > debugfs: > They set up a SRCU read side critical section and check whether the dentry > is still alive by means of debugfs_use_file_start(). If so, they forward > the call to the original struct file_operation stored in ->d_fsdata, still > under the protection of the SRCU read side critical section. > This SRCU read side critical section prevents any pending debugfs_remove() > and friends to return to their callers. Since a file's private data must > only be freed after the return of debugfs_remove(), the ongoing proxied > call is guarded against any file removal race. > > If, on the other hand, the initial call to debugfs_use_file_start() detects > that the dentry is dead, the wrapper simply returns -EIO and does not > forward the call. Note that the ->poll() wrapper is special in that its > signature does not allow for the return of arbitrary -EXXX values and thus, > POLLHUP is returned here. > > In order not to pollute debugfs with wrapper definitions that aren't ever > needed, I chose not to define a wrapper for every struct file_operations > method possible. Instead, a wrapper is defined only for the subset of > methods which are actually set by any debugfs users. > Currently, these are: > > ->llseek() > ->read() > ->write() > ->unlocked_ioctl() > ->poll() > > The ->release() wrapper is special in that it does not protect the original > ->release() in any way from dead files in order not to leak resources. > Thus, any ->release() handed to debugfs must implement file lifetime > management manually, if needed. > For only 33 out of a total of 434 releasers handed in to debugfs, it could > not be verified immediately whether they access data structures that might > have been freed upon a debugfs_remove() return in the meanwhile. > > Export debugfs_use_file_start() and debugfs_use_file_finish() in order to > allow any ->release() to manually implement file lifetime management. > > For a set of common cases of struct file_operations implemented by the > debugfs_core itself, future patches will incorporate file lifetime > management directly within those in order to allow for their unproxied > operation. Rename the original, non-proxying "debugfs_create_file()" to > "debugfs_create_file_unsafe()" and keep it for future internal use by > debugfs itself. Factor out code common to both into the new > __debugfs_create_file(). > > Signed-off-by: Nicolai Stange Hey, I'm seeing a hang on boot which was bisected (twice) to this commit. I don't see any lockup messages, but everything just seems to stop. Thanks, Sasha