From mboxrd@z Thu Jan 1 00:00:00 1970 From: Omar Sandoval Subject: Re: Autofs and mount namespaces Date: Tue, 5 Jul 2016 11:47:54 -0700 Message-ID: <20160705184754.GA28931@vader.DHCP.thefacebook.com> References: <20160407181911.GA9018@vader> <1460076663.3135.37.camel@themaw.net> Mime-Version: 1.0 Return-path: DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=osandov-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=xZtJuQxAdgb16OcnU0IsdKZi2tsYr4mLp0rh0s68vEI=; b=C9G8mZdKPfP//HiEmb2BjTsinzw7slE3VXa4LjWXtSgpecmOn/2gLNa5re6aSIdxkH F9gXILNJz0RW1tGTwYkBO9olRNcksduCzZscyUQZJroqbyIqTIUe6aeXhA08aNurFZhI fY8rXR0U6l8zqL6/xMO9IXI+xkY7cHjUr3crUzjtQxV8lKeYrfHcaF4Il2beFczU4Oao dWz9Wz7/PlBRX9p+ZK5Rugp1v6e6gxQofbj5mZLL9k8AZNrUOBK4wIYncL8vCCo5Ueq4 g0x821m7H7OM08b+N8do5sjp6vu1ZMMOv7iZBFm/eb2lVHEQNDy6UFSymNOSpYaNFR0s Lrfg== Content-Disposition: inline In-Reply-To: <1460076663.3135.37.camel@themaw.net> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Ian Kent Cc: autofs@vger.kernel.org, linux-fsdevel@vger.kernel.org, kernel-team@fb.com On Fri, Apr 08, 2016 at 08:51:03AM +0800, Ian Kent wrote: [snip] > > I think this is a better approach (beware, not even compile tested so it > almost certainly doesn't work properly) and I'm not even sure that > exposing __is_local_mountpoint() is ok from a VFS POV but something > needs to be done: > > autofs4 - make mountpoint checks namespace aware > > From: Ian Kent > > If an automount mount is clone(2)ed into a file system that is > propagation private, when it later expires the mount subsequent > calls to autofs ->d_automount() for in that dentry in that > namespace will return ELOOP until the mount is manually umounted. > > In the same way, if an autofs mount is triggered by automount(8) > 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. > > Signed-off-by: Ian Kent > Cc: Al Viro > Cc: Eric W. Biederman > --- > fs/autofs4/expire.c | 4 ++-- > fs/autofs4/root.c | 14 +++++++------- > fs/dcache.c | 2 +- > fs/mount.h | 9 --------- > fs/namespace.c | 1 + > include/linux/mount.h | 9 +++++++++ > 6 files changed, 20 insertions(+), 19 deletions(-) > > diff --git a/fs/autofs4/expire.c b/fs/autofs4/expire.c > index 9510d8d..23b9701 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 7ab9239..9ba487a 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,15 +370,15 @@ 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 > @@ -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_EXPIRING | AUTOFS_INF_NO_RCU)) > 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/dcache.c b/fs/dcache.c > index 32ceae3..9c42dc9 100644 > --- a/fs/dcache.c > +++ b/fs/dcache.c > @@ -1271,7 +1271,7 @@ rename_retry: > static enum d_walk_ret check_mount(void *data, struct dentry *dentry) > { > int *ret = data; > - if (d_mountpoint(dentry)) { > + if (is_local_mountpoint(dentry)) { > *ret = 1; > return D_WALK_QUIT; > } > diff --git a/fs/mount.h b/fs/mount.h > index 14db05d..c25e6e8 100644 > --- a/fs/mount.h > +++ b/fs/mount.h > @@ -127,12 +127,3 @@ struct proc_mounts { > }; > > extern const struct seq_operations mounts_op; > - > -extern bool __is_local_mountpoint(struct dentry *dentry); > -static inline bool is_local_mountpoint(struct dentry *dentry) > -{ > - if (!d_mountpoint(dentry)) > - return false; > - > - return __is_local_mountpoint(dentry); > -} > diff --git a/fs/namespace.c b/fs/namespace.c > index 4fb1691..9e1bc00 100644 > --- a/fs/namespace.c > +++ b/fs/namespace.c > @@ -725,6 +725,7 @@ bool __is_local_mountpoint(struct dentry *dentry) > out: > return is_covered; > } > +EXPORT_SYMBOL(__is_local_mountpoint); > > static struct mountpoint *lookup_mountpoint(struct dentry *dentry) > { > diff --git a/include/linux/mount.h b/include/linux/mount.h > index f822c3c..7419c0c 100644 > --- a/include/linux/mount.h > +++ b/include/linux/mount.h > @@ -15,6 +15,7 @@ > #include > #include > #include > +#include > > struct super_block; > struct vfsmount; > @@ -95,4 +96,12 @@ extern void mark_mounts_for_expiry(struct list_head *mounts); > > extern dev_t name_to_dev_t(const char *name); > > +extern bool __is_local_mountpoint(struct dentry *dentry); > +static inline bool is_local_mountpoint(struct dentry *dentry) > +{ > + if (!d_mountpoint(dentry)) > + return false; > + > + return __is_local_mountpoint(dentry); > +} > #endif /* _LINUX_MOUNT_H */ Hi, Ian, Just wanted to check on the status of this fix. Is this still the approach you wanted to take/is there anything else you wanted to do with this? -- Omar