From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751808AbdBMR5a (ORCPT ); Mon, 13 Feb 2017 12:57:30 -0500 Received: from muru.com ([72.249.23.125]:34960 "EHLO muru.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751007AbdBMR53 (ORCPT ); Mon, 13 Feb 2017 12:57:29 -0500 Date: Mon, 13 Feb 2017 09:57:25 -0800 From: Tony Lindgren To: Tejun Heo Cc: linux-kernel@vger.kernel.org, Greg Kroah-Hartman Subject: Re: [PATCH] kernfs: fix locking around kernfs_ops->release() callback Message-ID: <20170213175724.GH21809@atomide.com> References: <20170209003642.GY3897@atomide.com> <20170211031819.GC19050@mtj.duckdns.org> <20170211044855.GC3897@atomide.com> <20170211203302.GA25834@mtj.duckdns.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170211203302.GA25834@mtj.duckdns.org> User-Agent: Mutt/1.7.2 (2016-11-26) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org * Tejun Heo [170211 12:34]: > The release callback may be called from two places - file release > operation and kernfs open file draining. kernfs_open_file->mutex is > used to synchronize the two callsites. This unfortunately leads to > possible circular locking because of->mutex is used to protect the > usual kernfs operations which may use locking constructs which are > held while removing and thus draining kernfs files. > > @of->mutex is for synchronizing concurrent kernfs access operations > and all we need here is synchronization between the releaes and drain > paths. As the drain path has to grab kernfs_open_file_mutex anyway, > let's use the mutex to synchronize the release operation instead. > > Signed-off-by: Tejun Heo > Reported-by: Tony Lindgren > Fixes: 0e67db2f9fe9 ("kernfs: add kernfs_ops->open/release() callbacks") > --- > Hello, > > Tony, can you please verify that this resolves the lockdep warnings > that you've been seeing on linux-next? Yes thanks this fixes the issue I was seeing: Tested-by: Tony Lindgren > Greg, this is a fix for the kernfs patches which are being routed > through the cgroup tree. Once Tony confirms, I'll apply this patch on > top. > > Thanks! > > fs/kernfs/file.c | 19 ++++++++++++++----- > 1 file changed, 14 insertions(+), 5 deletions(-) > > --- a/fs/kernfs/file.c > +++ b/fs/kernfs/file.c > @@ -747,10 +747,15 @@ err_out: > static void kernfs_release_file(struct kernfs_node *kn, > struct kernfs_open_file *of) > { > - if (!(kn->flags & KERNFS_HAS_RELEASE)) > - return; > + /* > + * @of is guaranteed to have no other file operations in flight and > + * we just want to synchronize release and drain paths. > + * @kernfs_open_file_mutex is enough. @of->mutex can't be used > + * here because drain path may be called from places which can > + * cause circular dependency. > + */ > + lockdep_assert_held(&kernfs_open_file_mutex); > > - mutex_lock(&of->mutex); > if (!of->released) { > /* > * A file is never detached without being released and we > @@ -760,7 +765,6 @@ static void kernfs_release_file(struct k > kn->attr.ops->release(of); > of->released = true; > } > - mutex_unlock(&of->mutex); > } > > static int kernfs_fop_release(struct inode *inode, struct file *filp) > @@ -768,7 +772,12 @@ static int kernfs_fop_release(struct ino > struct kernfs_node *kn = filp->f_path.dentry->d_fsdata; > struct kernfs_open_file *of = kernfs_of(filp); > > - kernfs_release_file(kn, of); > + if (kn->flags & KERNFS_HAS_RELEASE) { > + mutex_lock(&kernfs_open_file_mutex); > + kernfs_release_file(kn, of); > + mutex_unlock(&kernfs_open_file_mutex); > + } > + > kernfs_put_open_node(kn, of); > seq_release(inode, filp); > kfree(of->prealloc_buf);