Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755349AbdDNSnU (ORCPT ); Fri, 14 Apr 2017 14:43:20 -0400 Received: from mail-wr0-f172.google.com ([209.85.128.172]:34558 "EHLO mail-wr0-f172.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754542AbdDNSnO (ORCPT ); Fri, 14 Apr 2017 14:43:14 -0400 MIME-Version: 1.0 In-Reply-To: <20170414182241.14659-1-martin@omnibond.com> References: <20170414182241.14659-1-martin@omnibond.com> From: Mike Marshall Date: Fri, 14 Apr 2017 14:43:07 -0400 Message-ID: Subject: Re: [PATCH] orangefs: free superblock when mount fails To: Martin Brandenburg Cc: Linus Torvalds , Al Viro , linux-fsdevel , LKML , stable@vger.kernel.org Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5906 Lines: 138 ACK. I tried to mount orangefs with a nonsense option and got: [96967.205842] ================================================ [96967.206439] [ BUG: lock held when returning to user space! ] [96967.207046] 4.10.0-00008-g554ce8b #2 Not tainted [96967.207531] ------------------------------------------------ [96967.208130] mount/6371 is leaving the kernel with locks still held! [96967.208797] 1 lock held by mount/6371: [96967.209211] #0: (&type->s_umount_key#52/1){+.+.+.}, at: [] sget_userns+0x2d2/0x510 and then I typed sync and it wedged... After applying Martin's patch, the nonsense mount option merely caused the mount to fail without sickening the kernel... -Mike On Fri, Apr 14, 2017 at 2:22 PM, Martin Brandenburg wrote: > Otherwise lockdep says: > > [ 1337.483798] ================================================ > [ 1337.483999] [ BUG: lock held when returning to user space! ] > [ 1337.484252] 4.11.0-rc6 #19 Not tainted > [ 1337.484423] ------------------------------------------------ > [ 1337.484626] mount/14766 is leaving the kernel with locks still held! > [ 1337.484841] 1 lock held by mount/14766: > [ 1337.485017] #0: (&type->s_umount_key#33/1){+.+.+.}, at: [] sget_userns+0x2af/0x520 > > Caught by xfstests generic/413 which tried to mount with the unsupported > mount option dax. Then xfstests generic/422 ran sync which deadlocks. > > Signed-off-by: Martin Brandenburg > Cc: stable@vger.kernel.org > --- > fs/orangefs/devorangefs-req.c | 9 +++++++-- > fs/orangefs/orangefs-kernel.h | 1 + > fs/orangefs/super.c | 23 ++++++++++++++++------- > 3 files changed, 24 insertions(+), 9 deletions(-) > > diff --git a/fs/orangefs/devorangefs-req.c b/fs/orangefs/devorangefs-req.c > index c4ab6fdf17a0..e1534c9bab16 100644 > --- a/fs/orangefs/devorangefs-req.c > +++ b/fs/orangefs/devorangefs-req.c > @@ -208,14 +208,19 @@ static ssize_t orangefs_devreq_read(struct file *file, > continue; > /* > * Skip ops whose filesystem we don't know about unless > - * it is being mounted. > + * it is being mounted or unmounted. It is possible for > + * a filesystem we don't know about to be unmounted if > + * it fails to mount in the kernel after userspace has > + * been sent the mount request. > */ > /* XXX: is there a better way to detect this? */ > } else if (ret == -1 && > !(op->upcall.type == > ORANGEFS_VFS_OP_FS_MOUNT || > op->upcall.type == > - ORANGEFS_VFS_OP_GETATTR)) { > + ORANGEFS_VFS_OP_GETATTR || > + op->upcall.type == > + ORANGEFS_VFS_OP_FS_UMOUNT)) { > gossip_debug(GOSSIP_DEV_DEBUG, > "orangefs: skipping op tag %llu %s\n", > llu(op->tag), get_opname_string(op)); > diff --git a/fs/orangefs/orangefs-kernel.h b/fs/orangefs/orangefs-kernel.h > index 5e48a0be9761..8afac46fcc87 100644 > --- a/fs/orangefs/orangefs-kernel.h > +++ b/fs/orangefs/orangefs-kernel.h > @@ -249,6 +249,7 @@ struct orangefs_sb_info_s { > char devname[ORANGEFS_MAX_SERVER_ADDR_LEN]; > struct super_block *sb; > int mount_pending; > + int no_list; > struct list_head list; > }; > > diff --git a/fs/orangefs/super.c b/fs/orangefs/super.c > index cd261c8de53a..629d8c917fa6 100644 > --- a/fs/orangefs/super.c > +++ b/fs/orangefs/super.c > @@ -493,7 +493,7 @@ struct dentry *orangefs_mount(struct file_system_type *fst, > > if (ret) { > d = ERR_PTR(ret); > - goto free_op; > + goto free_sb_and_op; > } > > /* > @@ -519,6 +519,9 @@ struct dentry *orangefs_mount(struct file_system_type *fst, > spin_unlock(&orangefs_superblocks_lock); > op_release(new_op); > > + /* Must be removed from the list now. */ > + ORANGEFS_SB(sb)->no_list = 0; > + > if (orangefs_userspace_version >= 20906) { > new_op = op_alloc(ORANGEFS_VFS_OP_FEATURES); > if (!new_op) > @@ -533,6 +536,10 @@ struct dentry *orangefs_mount(struct file_system_type *fst, > > return dget(sb->s_root); > > +free_sb_and_op: > + /* Will call orangefs_kill_sb with sb not in list. */ > + ORANGEFS_SB(sb)->no_list = 1; > + deactivate_locked_super(sb); > free_op: > gossip_err("orangefs_mount: mount request failed with %d\n", ret); > if (ret == -EINVAL) { > @@ -558,12 +565,14 @@ void orangefs_kill_sb(struct super_block *sb) > */ > orangefs_unmount_sb(sb); > > - /* remove the sb from our list of orangefs specific sb's */ > - > - spin_lock(&orangefs_superblocks_lock); > - __list_del_entry(&ORANGEFS_SB(sb)->list); /* not list_del_init */ > - ORANGEFS_SB(sb)->list.prev = NULL; > - spin_unlock(&orangefs_superblocks_lock); > + if (!ORANGEFS_SB(sb)->no_list) { > + /* remove the sb from our list of orangefs specific sb's */ > + spin_lock(&orangefs_superblocks_lock); > + /* not list_del_init */ > + __list_del_entry(&ORANGEFS_SB(sb)->list); > + ORANGEFS_SB(sb)->list.prev = NULL; > + spin_unlock(&orangefs_superblocks_lock); > + } > > /* > * make sure that ORANGEFS_DEV_REMOUNT_ALL loop that might've seen us > -- > 2.11.0 >