From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755500AbcBHGjr (ORCPT ); Mon, 8 Feb 2016 01:39:47 -0500 Received: from mail.linuxfoundation.org ([140.211.169.12]:43823 "EHLO mail.linuxfoundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751301AbcBHGjq (ORCPT ); Mon, 8 Feb 2016 01:39:46 -0500 Date: Sun, 7 Feb 2016 22:39:45 -0800 From: Greg Kroah-Hartman To: Roman Pen Cc: linux-kernel@vger.kernel.org Subject: Re: [RFC PATCH 3/3] debugfs: make __debugfs_remove wait for dentry release Message-ID: <20160208063945.GC12507@kroah.com> References: <1449751634-7887-1-git-send-email-r.peniaev@gmail.com> <1449751634-7887-4-git-send-email-r.peniaev@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1449751634-7887-4-git-send-email-r.peniaev@gmail.com> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Dec 10, 2015 at 01:47:14PM +0100, Roman Pen wrote: > __debugfs_remove does not wait for dentry release, thus dentry can still be > alive and file operations can still be invoked after the function returns. > > >From debugfs point of view this behaviour is definitely ok, but that can be > critical for users of debugfs and lead to usage-after-free: file operations > can be called after dentry is considered as removed. > > Simple grep over the sources shows that dynamic debugfs file creation and > removal is exactly the case, and common usage is the following: > > create_dev(): > dev = kmalloc(); > dev->debugfs_dentry = debugfs_create_file("my_dev", , dev, dev_fops); > ^^^ > !! pointer is passed to file > !! operations as private data > > remove_dev(dev): > debugfs_remove(dev->debugfs_dentry); > kfree(dev); > ^^^ > !! memory is freed, but fops->open/read/write > !! can still be called and lead to usage-after-free > > Here is quick grep output of the case described above: > > *** drivers/block/pktcdvd.c: > pkt_debugfs_dev_remove[489] debugfs_remove(pd->dfs_f_info); > pkt_debugfs_dev_remove[490] debugfs_remove(pd->dfs_d_root); > > *** drivers/char/virtio_console.c: > unplug_port[1595] debugfs_remove(port->debugfs_file); > > *** drivers/crypto/qat/qat_common/adf_cfg.c: > adf_cfg_dev_remove[187] debugfs_remove(dev_cfg_data->debug); > > *** drivers/gpu/drm/drm_debugfs.c: > drm_debugfs_remove_files[203] debugfs_remove(tmp->dent); > > .... and more and more and more ... > > In my grep output each third line is exactly this case: people expect that > debugfs_remove() is a barrier and file operations won't be invoked after it > (same behaviour as kobject_del(),kobject_put() tuple). > > So in this patch debugfs_remove() waits for completion of final dentry release > callback. > > BUT! I am not sure that nobody tries to remove the dentry from it's own file > operation (dentry suicide). And if so - deadlock will happen. > > Probably, dentry_remove_self() should be implemented for such cases, which is > similar to sysfs_remove_file_self(). But for now I do not want to add new > function which can be useless in the nearest future. > > Signed-off-by: Roman Pen > Cc: Greg Kroah-Hartman > Cc: linux-kernel@vger.kernel.org > --- > fs/debugfs/inode.c | 26 +++++++++++++++++++++++++- > 1 file changed, 25 insertions(+), 1 deletion(-) Without a real user, I don't want to take this, as it's "odd". Also this is debugfs, you shouldn't be using this for any "real" code, it's only for debugging. If you want this for a real api, use configfs. thanks, greg k-h