From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from out03.mta.xmission.com ([166.70.13.233]:54974 "EHLO out03.mta.xmission.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750719AbdADD5H (ORCPT ); Tue, 3 Jan 2017 22:57:07 -0500 From: ebiederm@xmission.com (Eric W. Biederman) To: Al Viro Cc: Krister Johansen , linux-fsdevel@vger.kernel.org References: <20161231041001.GA2448@templeofstupid.com> <20161231061729.GX1555@ZenIV.linux.org.uk> <874m1hdkyv.fsf@xmission.com> <20170103014806.GA1555@ZenIV.linux.org.uk> <87ful07ryd.fsf@xmission.com> <20170103040052.GB1555@ZenIV.linux.org.uk> Date: Wed, 04 Jan 2017 16:52:39 +1300 In-Reply-To: <20170103040052.GB1555@ZenIV.linux.org.uk> (Al Viro's message of "Tue, 3 Jan 2017 04:00:52 +0000") Message-ID: <87y3yr32ig.fsf@xmission.com> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [PATCH] Fix a race in put_mountpoint. Sender: linux-fsdevel-owner@vger.kernel.org List-ID: Al Viro writes: > On Tue, Jan 03, 2017 at 04:17:14PM +1300, Eric W. Biederman wrote: >> Al Viro writes: >> >> > On Tue, Jan 03, 2017 at 01:51:36PM +1300, Eric W. Biederman wrote: >> > >> >> The only significant thing I see is that you have not taken the >> >> mount_lock on the path where new_mountpoint adds the new struct >> >> mountpoint into the mountpoint hash table. >> > >> > Umm... Point, but I really don't like that bouncing mount_lock up >> > and down there. It's not going to cause any serious overhead, >> > but it just looks ugly... ;-/ >> > >> > Let me think for a while... >> >> The other possibility is to grab namespace_sem in mntput_no_expire >> around the call of umount_mnt. That is the only path where >> put_mountpoint can be called where we are not holding namespace_sem. >> That works in the small but I haven't traced the callers of mntput and >> mntput_no_expire yet to see if it works in practice. > > No, that's a really bad idea. Final mntput should _not_ happen under > namespace_lock, but I don't want grabbing it in that place. Agreed. That just makes the code harder to maintain later on. > How about this instead: I really don't like the logic inlined as my patch to kill shadow mounts needs to call acquire a mountpoint which may not already have been allocated as well. Beyond that we can make the logic simpler by causing d_set_mounted to fail if the flag is already set and syncrhonize on that. Which means we don't have to verify the ordering between mount_lock and rename_lock (from d_set_mounted) is not a problem, which makes backports easier to verify. Patch follows. Eric