| Message ID | 1372684830-13272-1-git-send-email-jlayton@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, 1 Jul 2013 09:20:30 -0400 Jeff Layton <jlayton@redhat.com> wrote: > Christopher reported a regression where he was unable to unmount a NFS > filesystem where the root had gone stale. The problem is that > d_revalidate handles the root of the filesystem differently from other > dentries, but d_weak_revalidate does not. We could simply fix this by > making d_weak_revalidate return success on IS_ROOT dentries, but there > are cases where we do want to revalidate the root of the fs. > > A umount is really a special case. We generally aren't interested in > anything but the dentry and vfsmount that's attached at that point. If > the inode turns out to be stale we just don't care since the intent is > to stop using it anyway. > > Try to handle this situation better by treating umount as a special > case in the lookup code. Have it resolve the parent using normal > means, and then do a lookup of the final dentry without revalidating > it. In most cases, the final lookup will come out of the dcache, but > the case where there's a trailing symlink or !LAST_NORM entry on the > end complicates things a bit. > > Reported-by: Christopher T Vogan <cvogan@us.ibm.com> > Signed-off-by: Jeff Layton <jlayton@redhat.com> Thanks for this Jeff. It certainly looks credible to me. There is a lot of code copied from the "user_path_at" path which is a shame, but probably better that putting in lots of "is this an unmount" tests which would slow done the common case. On balance, I like it. Thanks, NeilBrown
On Tue, 2013-07-02 at 11:42 +1000, NeilBrown wrote: > On Mon, 1 Jul 2013 09:20:30 -0400 Jeff Layton <jlayton@redhat.com> wrote: > > > Christopher reported a regression where he was unable to unmount a NFS > > filesystem where the root had gone stale. The problem is that > > d_revalidate handles the root of the filesystem differently from other > > dentries, but d_weak_revalidate does not. We could simply fix this by > > making d_weak_revalidate return success on IS_ROOT dentries, but there > > are cases where we do want to revalidate the root of the fs. > > > > A umount is really a special case. We generally aren't interested in > > anything but the dentry and vfsmount that's attached at that point. If > > the inode turns out to be stale we just don't care since the intent is > > to stop using it anyway. > > > > Try to handle this situation better by treating umount as a special > > case in the lookup code. Have it resolve the parent using normal > > means, and then do a lookup of the final dentry without revalidating > > it. In most cases, the final lookup will come out of the dcache, but > > the case where there's a trailing symlink or !LAST_NORM entry on the > > end complicates things a bit. > > > > Reported-by: Christopher T Vogan <cvogan@us.ibm.com> > > Signed-off-by: Jeff Layton <jlayton@redhat.com> > > Thanks for this Jeff. It certainly looks credible to me. > > There is a lot of code copied from the "user_path_at" path which is a shame, > but probably better that putting in lots of "is this an unmount" tests which > would slow done the common case. > > On balance, I like it. > > Thanks, > NeilBrown > (cc'ing Christopher as I mistakenly left him off the original mail. I'll make sure to cc him on any respins...) Thanks for looking. Yeah it is a lot of code to handle one case. So, while this does seem to work, I'm still not 100% sold on this approach... I had assumed that we would sometimes want to revalidate IS_ROOT dentries in other codepaths. Now that I think about it though, I'm having a hard time coming up with any situations where that's necessary. We'll never want to invalidate such a dentry, so does that ever make sense? If it doesn't, we could just replace this patch with a test for IS_ROOT(dentry) in nfs_weak_revalidate, and call it a day. I tested a patch like that earlier and it also worked around the problem. Also, it bothers me a little that this patch stops revalidating anything once it hits the last component, even if it's a symlink and we know we'll have to chase it down. It may make sense to check for d_mountpoint in some cases and revalidate the dentry if it's true. Thoughts?
On Tue, 2013-07-02 at 06:34 -0400, Jeffrey Layton wrote: > On Tue, 2013-07-02 at 11:42 +1000, NeilBrown wrote: > > On Mon, 1 Jul 2013 09:20:30 -0400 Jeff Layton <jlayton@redhat.com> wrote: > > > > > Christopher reported a regression where he was unable to unmount a NFS > > > filesystem where the root had gone stale. The problem is that > > > d_revalidate handles the root of the filesystem differently from other > > > dentries, but d_weak_revalidate does not. We could simply fix this by > > > making d_weak_revalidate return success on IS_ROOT dentries, but there > > > are cases where we do want to revalidate the root of the fs. > > > > > > A umount is really a special case. We generally aren't interested in > > > anything but the dentry and vfsmount that's attached at that point. If > > > the inode turns out to be stale we just don't care since the intent is > > > to stop using it anyway. > > > > > > Try to handle this situation better by treating umount as a special > > > case in the lookup code. Have it resolve the parent using normal > > > means, and then do a lookup of the final dentry without revalidating > > > it. In most cases, the final lookup will come out of the dcache, but > > > the case where there's a trailing symlink or !LAST_NORM entry on the > > > end complicates things a bit. > > > > > > Reported-by: Christopher T Vogan <cvogan@us.ibm.com> > > > Signed-off-by: Jeff Layton <jlayton@redhat.com> > > > > Thanks for this Jeff. It certainly looks credible to me. > > > > There is a lot of code copied from the "user_path_at" path which is a shame, > > but probably better that putting in lots of "is this an unmount" tests which > > would slow done the common case. > > > > On balance, I like it. > > > > Thanks, > > NeilBrown > > > > (cc'ing Christopher as I mistakenly left him off the original mail. I'll > make sure to cc him on any respins...) > > Thanks for looking. Yeah it is a lot of code to handle one case. So, > while this does seem to work, I'm still not 100% sold on this > approach... > > I had assumed that we would sometimes want to revalidate IS_ROOT > dentries in other codepaths. Now that I think about it though, I'm > having a hard time coming up with any situations where that's > necessary. We'll never want to invalidate such a dentry, so does that > ever make sense? > > If it doesn't, we could just replace this patch with a test for > IS_ROOT(dentry) in nfs_weak_revalidate, and call it a day. I tested a > patch like that earlier and it also worked around the problem. > > Also, it bothers me a little that this patch stops revalidating anything > once it hits the last component, even if it's a symlink and we know > we'll have to chase it down. It may make sense to check for d_mountpoint > in some cases and revalidate the dentry if it's true. Erm... "and revalidate the dentry if it's _false_" ...I should have had my coffee before replying.
On Tue, 02 Jul 2013 06:34:38 -0400 Jeffrey Layton <jlayton@redhat.com> wrote: > On Tue, 2013-07-02 at 11:42 +1000, NeilBrown wrote: > > On Mon, 1 Jul 2013 09:20:30 -0400 Jeff Layton <jlayton@redhat.com> wrote: > > > > > Christopher reported a regression where he was unable to unmount a NFS > > > filesystem where the root had gone stale. The problem is that > > > d_revalidate handles the root of the filesystem differently from other > > > dentries, but d_weak_revalidate does not. We could simply fix this by > > > making d_weak_revalidate return success on IS_ROOT dentries, but there > > > are cases where we do want to revalidate the root of the fs. > > > > > > A umount is really a special case. We generally aren't interested in > > > anything but the dentry and vfsmount that's attached at that point. If > > > the inode turns out to be stale we just don't care since the intent is > > > to stop using it anyway. > > > > > > Try to handle this situation better by treating umount as a special > > > case in the lookup code. Have it resolve the parent using normal > > > means, and then do a lookup of the final dentry without revalidating > > > it. In most cases, the final lookup will come out of the dcache, but > > > the case where there's a trailing symlink or !LAST_NORM entry on the > > > end complicates things a bit. > > > > > > Reported-by: Christopher T Vogan <cvogan@us.ibm.com> > > > Signed-off-by: Jeff Layton <jlayton@redhat.com> > > > > Thanks for this Jeff. It certainly looks credible to me. > > > > There is a lot of code copied from the "user_path_at" path which is a shame, > > but probably better that putting in lots of "is this an unmount" tests which > > would slow done the common case. > > > > On balance, I like it. > > > > Thanks, > > NeilBrown > > > > (cc'ing Christopher as I mistakenly left him off the original mail. I'll > make sure to cc him on any respins...) > > Thanks for looking. Yeah it is a lot of code to handle one case. So, > while this does seem to work, I'm still not 100% sold on this > approach... > > I had assumed that we would sometimes want to revalidate IS_ROOT > dentries in other codepaths. Now that I think about it though, I'm > having a hard time coming up with any situations where that's > necessary. We'll never want to invalidate such a dentry, so does that > ever make sense? What does "revalidate" mean exactly here? I think this is d_revalidate, so it is validating the entry in the dcache, so it is "validating that the name still leads to the inode". Is that correct? In that case as IS_ROOT has no name it is hard to imagine that it needs to be revalidated. However I don't think a mount point is always IS_ROOT. With NFSv4 (an possibly the other NFS versions as well), if I mount server:/some/path /mnt then the NFS client will internally "mount" server:/, walk down to find "/some/path", and then bind that to /mnt. In that case I would argue that it is really the inode found at "/some/path", rather than the textual name "/some/path" which is being bound. So when stepping from one filesystem, to the next over a mount point, it is pointless to try to revalidate the dentry because it is really the inode that we want. However it could be that "revalidate" means "check that this inode still exists, and is still the same sort of object (directory or file)". If that is what we mean by "revalidate", then "umount" really is a special case as everything else would want to know that the object is still then, but umount wouldn't. Thinking a bit more: I see "umount" as a bit like "lstat". With "lstat", if the last component is a symlink we don't follow it, we just return it. With "umount" if the last component is a mountpoint we really don't want to follow it but rather return the underlying dentry. Once "umount" does the lookup and gets the dentry that something was mounted on to, it can carefully attach whatever is mounted there without ever "touching" it. (of couse if there are a stack of things mounted it need to only detach the last). Do those thoughts help at all? Or just add confusion? NeilBrown > > If it doesn't, we could just replace this patch with a test for > IS_ROOT(dentry) in nfs_weak_revalidate, and call it a day. I tested a > patch like that earlier and it also worked around the problem. > > Also, it bothers me a little that this patch stops revalidating anything > once it hits the last component, even if it's a symlink and we know > we'll have to chase it down. It may make sense to check for d_mountpoint > in some cases and revalidate the dentry if it's true. > > Thoughts?
On Wed, 2013-07-03 at 10:47 +1000, NeilBrown wrote: > On Tue, 02 Jul 2013 06:34:38 -0400 Jeffrey Layton <jlayton@redhat.com> wrote: > > > On Tue, 2013-07-02 at 11:42 +1000, NeilBrown wrote: > > > On Mon, 1 Jul 2013 09:20:30 -0400 Jeff Layton <jlayton@redhat.com> wrote: > > > > > > > Christopher reported a regression where he was unable to unmount a NFS > > > > filesystem where the root had gone stale. The problem is that > > > > d_revalidate handles the root of the filesystem differently from other > > > > dentries, but d_weak_revalidate does not. We could simply fix this by > > > > making d_weak_revalidate return success on IS_ROOT dentries, but there > > > > are cases where we do want to revalidate the root of the fs. > > > > > > > > A umount is really a special case. We generally aren't interested in > > > > anything but the dentry and vfsmount that's attached at that point. If > > > > the inode turns out to be stale we just don't care since the intent is > > > > to stop using it anyway. > > > > > > > > Try to handle this situation better by treating umount as a special > > > > case in the lookup code. Have it resolve the parent using normal > > > > means, and then do a lookup of the final dentry without revalidating > > > > it. In most cases, the final lookup will come out of the dcache, but > > > > the case where there's a trailing symlink or !LAST_NORM entry on the > > > > end complicates things a bit. > > > > > > > > Reported-by: Christopher T Vogan <cvogan@us.ibm.com> > > > > Signed-off-by: Jeff Layton <jlayton@redhat.com> > > > > > > Thanks for this Jeff. It certainly looks credible to me. > > > > > > There is a lot of code copied from the "user_path_at" path which is a shame, > > > but probably better that putting in lots of "is this an unmount" tests which > > > would slow done the common case. > > > > > > On balance, I like it. > > > > > > Thanks, > > > NeilBrown > > > > > > > (cc'ing Christopher as I mistakenly left him off the original mail. I'll > > make sure to cc him on any respins...) > > > > Thanks for looking. Yeah it is a lot of code to handle one case. So, > > while this does seem to work, I'm still not 100% sold on this > > approach... > > > > I had assumed that we would sometimes want to revalidate IS_ROOT > > dentries in other codepaths. Now that I think about it though, I'm > > having a hard time coming up with any situations where that's > > necessary. We'll never want to invalidate such a dentry, so does that > > ever make sense? > > What does "revalidate" mean exactly here? I think this is d_revalidate, so > it is validating the entry in the dcache, so it is "validating that the name > still leads to the inode". Is that correct? > > In that case as IS_ROOT has no name it is hard to imagine that it needs to be > revalidated. > However I don't think a mount point is always IS_ROOT. > With NFSv4 (an possibly the other NFS versions as well), if I > mount server:/some/path /mnt > then the NFS client will internally "mount" server:/, walk down to find > "/some/path", and then bind that to /mnt. > > In that case I would argue that it is really the inode found at "/some/path", > rather than the textual name "/some/path" which is being bound. > So when stepping from one filesystem, to the next over a mount point, it is > pointless to try to revalidate the dentry because it is really the inode that > we want. > > However it could be that "revalidate" means "check that this inode still > exists, and is still the same sort of object (directory or file)". If that > is what we mean by "revalidate", then "umount" really is a special case as > everything else would want to know that the object is still then, but umount > wouldn't. > > Thinking a bit more: I see "umount" as a bit like "lstat". With "lstat", if > the last component is a symlink we don't follow it, we just return it. > With "umount" if the last component is a mountpoint we really don't want to > follow it but rather return the underlying dentry. > > Once "umount" does the lookup and gets the dentry that something was mounted > on to, it can carefully attach whatever is mounted there without ever > "touching" it. (of couse if there are a stack of things mounted it need to > only detach the last). > > > > Do those thoughts help at all? Or just add confusion? > > NeilBrown > > Thanks for helping me sort this out in my head. It's a very confusing topic... ;) This is somewhat related to the discussion when we added the d_weak_revalidate op. When LOOKUP_JUMPED is set, then we know that we arrived at this dentry via some other means than a lookup in the parent dir. Thus, the dentry name isn't relevant, but the inode is and we only want to revalidate the inode, not the dentry. The commit message on the patch that originally added FS_REVAL_DOT says: For most filesystems this is OK, but it the case of the stateless NFS, this means that it circumvents path staleness detection, and the attribute+data cache revalidation code on such common commands as opendir("."). So I think the real question is: Should we treat the root of a vfsmount differently when revalidating its inode via d_weak_revalidate. Since it's possible for such an inode to go stale, I think that we do want to indicate that when someone does opendir() on it, even if it's the root of the fs. So, umount really does need to be a special case, and we ought not rely on d_weak_revalidate ops to try and get that right. So yeah, I guess this patch (or something like it) is what's needed. > > > > If it doesn't, we could just replace this patch with a test for > > IS_ROOT(dentry) in nfs_weak_revalidate, and call it a day. I tested a > > patch like that earlier and it also worked around the problem. > > > > Also, it bothers me a little that this patch stops revalidating anything > > once it hits the last component, even if it's a symlink and we know > > we'll have to chase it down. It may make sense to check for d_mountpoint > > in some cases and revalidate the dentry if it's true. > > On the above question, I think I'm inclined to not worry about it too much until/unless someone complains. Symlink chasing on the last component of a umount doesn't seem like it's common practice, particularly since util-linux's /bin/umount does a realpath() on the argument before calling umount() anyway.
diff --git a/fs/namei.c b/fs/namei.c index 9ed9361..47a7d69 100644 --- a/fs/namei.c +++ b/fs/namei.c @@ -2185,6 +2185,188 @@ user_path_parent(int dfd, const char __user *path, struct nameidata *nd, return s; } +/** + * umount_lookup_last - look up last component for umount + * @nd: pathwalk nameidata - currently pointing at parent directory of "last" + * @path: pointer to container for result + * + * This is a special lookup_last function just for umount. In this case, we + * need to resolve the path without doing any revalidation. + * + * The nameidata should be the result of doing a LOOKUP_PARENT pathwalk. Since + * mountpoints are always pinned in the dcache, their ancestors are too. Thus, + * in almost all cases, this lookup will be served out of the dcache. The only + * cases where it won't are if nd->last refers to a symlink or the path is + * bogus and it doesn't exist. + * + * Returns: + * -error: if there was an error during lookup. This includes -ENOENT if the + * lookup found a negative dentry. The nd->path reference will also be + * put in this case. + * + * 0: if we successfully resolved nd->path and found it to not to be a + * symlink that needs to be followed. "path" will also be populated. + * The nd->path reference will also be put. + * + * 1: if we successfully resolved nd->last and found it to be a symlink + * that needs to be followed. "path" will be populated with the path + * to the link, and nd->path will *not* be put. + */ +static int +umount_lookup_last(struct nameidata *nd, struct path *path) +{ + int error = 0; + struct dentry *dentry; + struct dentry *dir = nd->path.dentry; + + if (unlikely(nd->flags & LOOKUP_RCU)) { + WARN_ON_ONCE(1); + error = -ECHILD; + goto error_check; + } + + nd->flags &= ~LOOKUP_PARENT; + + if (unlikely(nd->last_type != LAST_NORM)) { + error = handle_dots(nd, nd->last_type); + if (!error) + dentry = dget(nd->path.dentry); + goto error_check; + } + + mutex_lock(&dir->d_inode->i_mutex); + dentry = d_lookup(dir, &nd->last); + if (!dentry) { + /* + * No cached dentry. Mounted dentries are pinned in the cache, + * so that means that this dentry is probably a symlink or the + * path doesn't actually point to a mounted dentry. + */ + dentry = d_alloc(dir, &nd->last); + if (!dentry) { + error = -ENOMEM; + } else { + dentry = lookup_real(dir->d_inode, dentry, nd->flags); + if (IS_ERR(dentry)) + error = PTR_ERR(dentry); + } + } + mutex_unlock(&dir->d_inode->i_mutex); + +error_check: + if (!error) { + if (!dentry->d_inode) { + error = -ENOENT; + dput(dentry); + } else { + path->dentry = dentry; + path->mnt = mntget(nd->path.mnt); + if (should_follow_link(dentry->d_inode, + nd->flags & LOOKUP_FOLLOW)) + return 1; + follow_mount(path); + } + } + terminate_walk(nd); + return error; +} + +/** + * path_umountat - look up a path to be umounted + * @dfd: directory file descriptor to start walk from + * @name: full pathname to walk + * @flags: lookup flags + * @nd: pathwalk nameidata + * + * Look up the given name, but don't attempt to revalidate the last component. + * Returns 0 and "path" will be valid on success; Retuns error otherwise. + */ +static int +path_umountat(int dfd, const char *name, struct path *path, unsigned int flags) +{ + struct file *base = NULL; + struct nameidata nd; + int err; + + err = path_init(dfd, name, flags | LOOKUP_PARENT, &nd, &base); + if (unlikely(err)) + return err; + + current->total_link_count = 0; + err = link_path_walk(name, &nd); + if (err) + goto out; + + /* If we're in rcuwalk, drop out of it to handle last component */ + if (nd.flags & LOOKUP_RCU) { + err = unlazy_walk(&nd, NULL); + if (err) { + terminate_walk(&nd); + goto out; + } + } + + err = umount_lookup_last(&nd, path); + while (err > 0) { + void *cookie; + struct path link = *path; + err = may_follow_link(&link, &nd); + if (unlikely(err)) + break; + nd.flags |= LOOKUP_PARENT; + err = follow_link(&link, &nd, &cookie); + if (err) + break; + err = umount_lookup_last(&nd, path); + put_link(&nd, &link, cookie); + } +out: + if (base) + fput(base); + + if (nd.root.mnt && !(nd.flags & LOOKUP_ROOT)) + path_put(&nd.root); + + return err; +} + +/** + * user_path_umountat - lookup a path from userland in order to umount it + * @dfd: directory file descriptor + * @name: pathname from userland + * @flags: lookup flags + * @path: pointer to container to hold result + * + * A umount is a special case for path walking. We're not actually interested + * in the inode in this situation, and ESTALE errors can be a problem. We + * simply want track down the dentry and vfsmount attached at the mountpoint + * and avoid revalidating the last component. + * + * Returns 0 and populates "path" on success. + */ +int +user_path_umountat(int dfd, const char __user *name, unsigned int flags, + struct path *path) +{ + struct filename *s = getname(name); + int error; + + if (IS_ERR(s)) + return PTR_ERR(s); + + error = path_umountat(dfd, s->name, path, flags | LOOKUP_RCU); + if (unlikely(error == -ECHILD)) + error = path_umountat(dfd, s->name, path, flags); + if (unlikely(error == -ESTALE)) + error = path_umountat(dfd, s->name, path, flags | LOOKUP_REVAL); + + if (likely(!error)) + audit_inode(s, path->dentry, 0); + + putname(s); + return error; +} + /* * It's inline, so penalty for filesystems that don't use sticky bit is * minimal. diff --git a/fs/namespace.c b/fs/namespace.c index 7b1ca9b..5d2676a 100644 --- a/fs/namespace.c +++ b/fs/namespace.c @@ -1318,7 +1318,7 @@ SYSCALL_DEFINE2(umount, char __user *, name, int, flags) if (!(flags & UMOUNT_NOFOLLOW)) lookup_flags |= LOOKUP_FOLLOW; - retval = user_path_at(AT_FDCWD, name, lookup_flags, &path); + retval = user_path_umountat(AT_FDCWD, name, lookup_flags, &path); if (retval) goto out; mnt = real_mount(path.mnt); diff --git a/include/linux/namei.h b/include/linux/namei.h index 5a5ff57..cd09751 100644 --- a/include/linux/namei.h +++ b/include/linux/namei.h @@ -58,6 +58,7 @@ enum {LAST_NORM, LAST_ROOT, LAST_DOT, LAST_DOTDOT, LAST_BIND}; extern int user_path_at(int, const char __user *, unsigned, struct path *); extern int user_path_at_empty(int, const char __user *, unsigned, struct path *, int *empty); +extern int user_path_umountat(int, const char __user *, unsigned int, struct path *); #define user_path(name, path) user_path_at(AT_FDCWD, name, LOOKUP_FOLLOW, path) #define user_lpath(name, path) user_path_at(AT_FDCWD, name, 0, path)
Christopher reported a regression where he was unable to unmount a NFS filesystem where the root had gone stale. The problem is that d_revalidate handles the root of the filesystem differently from other dentries, but d_weak_revalidate does not. We could simply fix this by making d_weak_revalidate return success on IS_ROOT dentries, but there are cases where we do want to revalidate the root of the fs. A umount is really a special case. We generally aren't interested in anything but the dentry and vfsmount that's attached at that point. If the inode turns out to be stale we just don't care since the intent is to stop using it anyway. Try to handle this situation better by treating umount as a special case in the lookup code. Have it resolve the parent using normal means, and then do a lookup of the final dentry without revalidating it. In most cases, the final lookup will come out of the dcache, but the case where there's a trailing symlink or !LAST_NORM entry on the end complicates things a bit. Reported-by: Christopher T Vogan <cvogan@us.ibm.com> Signed-off-by: Jeff Layton <jlayton@redhat.com> --- fs/namei.c | 182 ++++++++++++++++++++++++++++++++++++++++++++++++++ fs/namespace.c | 2 +- include/linux/namei.h | 1 + 3 files changed, 184 insertions(+), 1 deletion(-)