On Thu, Jan 13, 2011 at 09:54:05PM +0000, David Howells wrote:
> @@ -1038,12 +1147,14 @@ static int do_lookup(struct nameidata *nd, struct qstr *name,
> - __follow_mount_rcu(nd, path, inode);
> + if (unlikely(!__follow_mount_rcu(nd, path, inode, false)) &&
> + nameidata_drop_rcu(nd))
> + return -ECHILD;
Gotcha. That's what'd been causing fun with autofs. Look: we run into
automount. __follow_mount_rcu() fails. nd->path *AND* path contain
vfsmounts/dentries we hadn't grabbed references to. nameidata_drop_rcu()
brings nd->path to normal. Good for it, but path is *still* not grabbed.
And LOOKUP_RCU is gone, so when the caller feeds it to path_to_nameidata()
the fun things start happening. We drop nd->dentry, for starters. And
replace it with ungrabbed dentry we just got from dcache. Better yet,
if automount dentry had been sitting on top of mountpoint (e.g. it was
a direct autofs mountpoint), we'll drop nd->mnt and replace it with
vfsmount we hadn't grabbed. Then we'll proceed to treat both as if they'd
been grabbed, eventually doing path_put() on the suckers.
vfsmount will stay around, since it's mounted and thus mntput() optimizations
will assume we are not dropping the last reference and not free the sucker.
dentry, OTOH, will be freed after a few time through that. At which point
it gets reused and we are left with complete crap. E.g. with vfsmount
mounted on that mountpoint, its ->mnt_root pointing to dentry from completely
unrelated fs. Keep walking into that and you'll get _that_ dentry freed
under whoever might be using it, etc.
Fix is trivial: if __follow_mount_rcu() succeeds (the normal case) just
return 0. Otherwise, try nameidata_drop_rcu(); if it fails, we bugger off.
So far it's equivalent to your variant, but if nameidata_drop_rcu() succeeds,
we fall through to non-RCU case instead of just returning 0.
I've dropped the following into for-next; autofs seems to be working fine
with it. I'll fold that into your first patch in #untested.
AFS issue is completely separate story; that one is mine and it's old.
Just that now it became more visible; earlier it only messed the expiry
logics (by screwing ->mnt_ghosts accounting). I've a half-finished fix
for that, but it needs more beating (and will be stable@ fodder, BTW).
Later...
commit c737c31d6f1ba4eb3f1aec523741ed49d5f6a22a
Author: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
Date: Sat Jan 15 18:10:31 2011 -0500
[foldme] fix do_lookup() handling of automount in RCU case
Signed-off-by: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
diff --git a/fs/namei.c b/fs/namei.c
index 9367fea..8f7b41a 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -1277,24 +1277,25 @@ static int do_lookup(struct nameidata *nd, struct qstr *name,
done2:
path->mnt = mnt;
path->dentry = dentry;
- if (unlikely(!__follow_mount_rcu(nd, path, inode, false)) &&
- nameidata_drop_rcu(nd))
+ if (likely(__follow_mount_rcu(nd, path, inode, false)))
+ return 0;
+ if (nameidata_drop_rcu(nd))
return -ECHILD;
- } else {
- dentry = __d_lookup(parent, name);
- if (!dentry)
- goto need_lookup;
+ /* fallthru */
+ }
+ dentry = __d_lookup(parent, name);
+ if (!dentry)
+ goto need_lookup;
found:
- if (dentry->d_flags & DCACHE_OP_REVALIDATE)
- goto need_revalidate;
+ if (dentry->d_flags & DCACHE_OP_REVALIDATE)
+ goto need_revalidate;
done:
- path->mnt = mnt;
- path->dentry = dentry;
- err = follow_managed(path, nd->flags);
- if (unlikely(err < 0))
- return err;
- *inode = path->dentry->d_inode;
- }
+ path->mnt = mnt;
+ path->dentry = dentry;
+ err = follow_managed(path, nd->flags);
+ if (unlikely(err < 0))
+ return err;
+ *inode = path->dentry->d_inode;
return 0;
need_lookup:
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
