From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1764198AbcINRlw (ORCPT ); Wed, 14 Sep 2016 13:41:52 -0400 Received: from out03.mta.xmission.com ([166.70.13.233]:37654 "EHLO out03.mta.xmission.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753801AbcINRlu (ORCPT ); Wed, 14 Sep 2016 13:41:50 -0400 From: ebiederm@xmission.com (Eric W. Biederman) To: Ian Kent Cc: Andrew Morton , autofs mailing list , Kernel Mailing List , Al Viro , linux-fsdevel , Omar Sandoval References: <20160914061434.24714.490.stgit@pluto.themaw.net> <20160914061445.24714.68331.stgit@pluto.themaw.net> Date: Wed, 14 Sep 2016 12:28:12 -0500 In-Reply-To: <20160914061445.24714.68331.stgit@pluto.themaw.net> (Ian Kent's message of "Wed, 14 Sep 2016 14:14:45 +0800") Message-ID: <87zina9ys3.fsf@x220.int.ebiederm.org> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-XM-SPF: eid=1bkEBu-0005By-2z;;;mid=<87zina9ys3.fsf@x220.int.ebiederm.org>;;;hst=in02.mta.xmission.com;;;ip=97.119.97.64;;;frm=ebiederm@xmission.com;;;spf=neutral X-XM-AID: U2FsdGVkX1+0HiWGaA3pHYZwN5CLkXpbEzDGGnyK/XQ= X-SA-Exim-Connect-IP: 97.119.97.64 X-SA-Exim-Mail-From: ebiederm@xmission.com X-Spam-Report: * -1.0 ALL_TRUSTED Passed through trusted hosts only via SMTP * 1.5 XMNoVowels Alpha-numberic number with no vowels * 0.7 XMSubLong Long Subject * 0.0 TVD_RCVD_IP Message was received from an IP address * 0.0 T_TM2_M_HEADER_IN_MSG BODY: No description available. * 0.8 BAYES_50 BODY: Bayes spam probability is 40 to 60% * [score: 0.5000] * -0.0 DCC_CHECK_NEGATIVE Not listed in DCC * [sa07 1397; Body=1 Fuz1=1 Fuz2=1] * 0.0 T_TooManySym_01 4+ unique symbols in subject X-Spam-DCC: XMission; sa07 1397; Body=1 Fuz1=1 Fuz2=1 X-Spam-Combo: **;Ian Kent X-Spam-Relay-Country: X-Spam-Timing: total 455 ms - load_scoreonly_sql: 0.03 (0.0%), signal_user_changed: 2.9 (0.6%), b_tie_ro: 1.97 (0.4%), parse: 0.86 (0.2%), extract_message_metadata: 20 (4.4%), get_uri_detail_list: 3.6 (0.8%), tests_pri_-1000: 11 (2.4%), tests_pri_-950: 1.19 (0.3%), tests_pri_-900: 1.06 (0.2%), tests_pri_-400: 39 (8.5%), check_bayes: 38 (8.3%), b_tokenize: 13 (2.8%), b_tok_get_all: 13 (2.9%), b_comp_prob: 3.1 (0.7%), b_tok_touch_all: 4.4 (1.0%), b_finish: 0.78 (0.2%), tests_pri_0: 372 (81.8%), check_dkim_signature: 0.57 (0.1%), check_dkim_adsp: 2.8 (0.6%), tests_pri_500: 4.1 (0.9%), rewrite_mail: 0.00 (0.0%) Subject: Re: [PATCH 3/4] autofs - make mountpoint checks namespace aware X-Spam-Flag: No X-SA-Exim-Version: 4.2.1 (built Thu, 05 May 2016 13:38:54 -0600) X-SA-Exim-Scanned: Yes (on in02.mta.xmission.com) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Ian Kent writes: > If an automount mount is clone(2)ed into a file system that is > propagation private, when it later expires in the originating > namespace subsequent calls to autofs ->d_automount() for that > dentry in the original namespace will return ELOOP until the > mount is manually umounted in the cloned namespace. > > In the same way, if an autofs mount is triggered by automount(8) > running within a container the dentry will be seen as mounted in > the root init namespace and calls to ->d_automount() in that namespace > will return ELOOP until the mount is umounted within the container. > > Also, have_submounts() can return an incorect result when a mount > exists in a namespace other than the one being checked. Overall this appears to be a fairly reasonable set of changes. It does increase the expense when an actual mount point is encountered, but if these are the desired some increase in cost when a dentry is a mountpoint is unavoidable. May I ask the motiviation for this set of changes? Reading through the changes I don't grasp why we want to change the behavior of autofs. What problem is being solved? What are the benefits? Eric > Signed-off-by: Ian Kent > Cc: Al Viro > Cc: Eric W. Biederman > Cc: Omar Sandoval > --- > fs/autofs4/dev-ioctl.c | 2 +- > fs/autofs4/expire.c | 4 ++-- > fs/autofs4/root.c | 30 +++++++++++++++--------------- > fs/autofs4/waitq.c | 2 +- > 4 files changed, 19 insertions(+), 19 deletions(-) > > diff --git a/fs/autofs4/dev-ioctl.c b/fs/autofs4/dev-ioctl.c > index c7fcc74..0024e25 100644 > --- a/fs/autofs4/dev-ioctl.c > +++ b/fs/autofs4/dev-ioctl.c > @@ -564,7 +564,7 @@ static int autofs_dev_ioctl_ismountpoint(struct file *fp, > > devid = new_encode_dev(dev); > > - err = have_submounts(path.dentry); > + err = have_local_submounts(path.dentry); > > if (follow_down_one(&path)) > magic = path.dentry->d_sb->s_magic; > diff --git a/fs/autofs4/expire.c b/fs/autofs4/expire.c > index d8e6d42..7cc34ef 100644 > --- a/fs/autofs4/expire.c > +++ b/fs/autofs4/expire.c > @@ -236,7 +236,7 @@ static int autofs4_tree_busy(struct vfsmount *mnt, > * count for the autofs dentry. > * If the fs is busy update the expiry counter. > */ > - if (d_mountpoint(p)) { > + if (is_local_mountpoint(p)) { > if (autofs4_mount_busy(mnt, p)) { > top_ino->last_used = jiffies; > dput(p); > @@ -280,7 +280,7 @@ static struct dentry *autofs4_check_leaves(struct vfsmount *mnt, > while ((p = get_next_positive_dentry(p, parent))) { > pr_debug("dentry %p %pd\n", p, p); > > - if (d_mountpoint(p)) { > + if (is_local_mountpoint(p)) { > /* Can we umount this guy */ > if (autofs4_mount_busy(mnt, p)) > continue; > diff --git a/fs/autofs4/root.c b/fs/autofs4/root.c > index fa84bb8..4150ad6 100644 > --- a/fs/autofs4/root.c > +++ b/fs/autofs4/root.c > @@ -123,7 +123,7 @@ static int autofs4_dir_open(struct inode *inode, struct file *file) > * it. > */ > spin_lock(&sbi->lookup_lock); > - if (!d_mountpoint(dentry) && simple_empty(dentry)) { > + if (!is_local_mountpoint(dentry) && simple_empty(dentry)) { > spin_unlock(&sbi->lookup_lock); > return -ENOENT; > } > @@ -370,28 +370,28 @@ static struct vfsmount *autofs4_d_automount(struct path *path) > > /* > * If the dentry is a symlink it's equivalent to a directory > - * having d_mountpoint() true, so there's no need to call back > - * to the daemon. > + * having is_local_mountpoint() true, so there's no need to > + * call back to the daemon. > */ > if (d_really_is_positive(dentry) && d_is_symlink(dentry)) { > spin_unlock(&sbi->fs_lock); > goto done; > } > > - if (!d_mountpoint(dentry)) { > + if (!is_local_mountpoint(dentry)) { > /* > * It's possible that user space hasn't removed directories > * after umounting a rootless multi-mount, although it > - * should. For v5 have_submounts() is sufficient to handle > - * this because the leaves of the directory tree under the > - * mount never trigger mounts themselves (they have an autofs > - * trigger mount mounted on them). But v4 pseudo direct mounts > - * do need the leaves to trigger mounts. In this case we > - * have no choice but to use the list_empty() check and > - * require user space behave. > + * should. For v5 have_local_submounts() is sufficient to > + * handle this because the leaves of the directory tree under > + * the mount never trigger mounts themselves (they have an > + * autofs trigger mount mounted on them). But v4 pseudo > + * direct mounts do need the leaves to trigger mounts. In > + * this case we have no choice but to use the list_empty() > + * check and require user space behave. > */ > if (sbi->version > 4) { > - if (have_submounts(dentry)) { > + if (have_local_submounts(dentry)) { > spin_unlock(&sbi->fs_lock); > goto done; > } > @@ -431,7 +431,7 @@ static int autofs4_d_manage(struct dentry *dentry, bool rcu_walk) > > /* The daemon never waits. */ > if (autofs4_oz_mode(sbi)) { > - if (!d_mountpoint(dentry)) > + if (!is_local_mountpoint(dentry)) > return -EISDIR; > return 0; > } > @@ -460,7 +460,7 @@ static int autofs4_d_manage(struct dentry *dentry, bool rcu_walk) > > if (ino->flags & AUTOFS_INF_WANT_EXPIRE) > return 0; > - if (d_mountpoint(dentry)) > + if (is_local_mountpoint(dentry)) > return 0; > inode = d_inode_rcu(dentry); > if (inode && S_ISLNK(inode->i_mode)) > @@ -487,7 +487,7 @@ static int autofs4_d_manage(struct dentry *dentry, bool rcu_walk) > * we can avoid needless calls ->d_automount() and avoid > * an incorrect ELOOP error return. > */ > - if ((!d_mountpoint(dentry) && !simple_empty(dentry)) || > + if ((!is_local_mountpoint(dentry) && !simple_empty(dentry)) || > (d_really_is_positive(dentry) && d_is_symlink(dentry))) > status = -EISDIR; > } > diff --git a/fs/autofs4/waitq.c b/fs/autofs4/waitq.c > index 431fd7e..911f4d5 100644 > --- a/fs/autofs4/waitq.c > +++ b/fs/autofs4/waitq.c > @@ -333,7 +333,7 @@ static int validate_request(struct autofs_wait_queue **wait, > dentry = new; > } > } > - if (have_submounts(dentry)) > + if (have_local_submounts(dentry)) > valid = 0; > > if (new)