On Fri, 2008-07-18 at 14:52 -0400, Jeff Moyer wrote:
> Ian Kent <raven@xxxxxxxxxx> writes:
>
> > The selection of a dentry for expiration and the setting of the
> > AUTOFS_INF_EXPIRING flag isn't done atomically which can lead to
> > lookups walking into an expiring mount.
> >
> > What happens is that an expire is initiated by the daemon and
> > a dentry is selected for expire but, since there is no lock
> > held between the selection and setting of the expiring flag,
> > a process may find the flag clear and continue walking into
> > the mount tree at the same time the daemon attempts the expire
> > it.
> >
> > Signed-off-by: Ian Kent <raven@xxxxxxxxxx>
> >
> > ---
>
> [...]
> > static inline void autofs4_copy_atime(struct file *src, struct file *dst)
> > diff --git a/fs/autofs4/expire.c b/fs/autofs4/expire.c
> > index 894fee5..19f5bea 100644
> > --- a/fs/autofs4/expire.c
> > +++ b/fs/autofs4/expire.c
> > @@ -292,6 +292,8 @@ static struct dentry *autofs4_expire_indirect(struct super_block *sb,
> > struct list_head *next;
> > int do_now = how & AUTOFS_EXP_IMMEDIATE;
> > int exp_leaves = how & AUTOFS_EXP_LEAVES;
> > + struct autofs_info *ino;
> > + unsigned int ino_count;
> >
> > if (!root)
> > return NULL;
> > @@ -316,6 +318,9 @@ static struct dentry *autofs4_expire_indirect(struct super_block *sb,
> > dentry = dget(dentry);
> > spin_unlock(&dcache_lock);
> >
> > + spin_lock(&sbi->fs_lock);
> > + ino = autofs4_dentry_ino(dentry);
> > +
> > /*
> > * Case 1: (i) indirect mount or top level pseudo direct mount
> > * (autofs-4.1).
> > @@ -326,6 +331,11 @@ static struct dentry *autofs4_expire_indirect(struct super_block *sb,
> > DPRINTK("checking mountpoint %p %.*s",
> > dentry, (int)dentry->d_name.len, dentry->d_name.name);
> >
> > + /* Path walk currently on this dentry? */
> > + ino_count = atomic_read(&ino->count) + 2;
> > + if (atomic_read(&dentry->d_count) > ino_count)
> > + goto next;
> > +
>
> It would be nice to document the +2. The +1s (below) may be evident
> given that we did a dget above, but still might merit mention.
>
> > /* Can we umount this guy */
> > if (autofs4_mount_busy(mnt, dentry))
> > goto next;
> > @@ -343,23 +353,25 @@ static struct dentry *autofs4_expire_indirect(struct super_block *sb,
> >
> > /* Case 2: tree mount, expire iff entire tree is not busy */
> > if (!exp_leaves) {
> > - /* Lock the tree as we must expire as a whole */
> > - spin_lock(&sbi->fs_lock);
> > - if (!autofs4_tree_busy(mnt, dentry, timeout, do_now)) {
> > - struct autofs_info *inf = autofs4_dentry_ino(dentry);
> > + /* Path walk currently on this dentry? */
> > + ino_count = atomic_read(&ino->count) + 1;
> > + if (atomic_read(&dentry->d_count) > ino_count)
> > + goto next;
> >
> > - /* Set this flag early to catch sys_chdir and the like */
> > - inf->flags |= AUTOFS_INF_EXPIRING;
> > - spin_unlock(&sbi->fs_lock);
> > + if (!autofs4_tree_busy(mnt, dentry, timeout, do_now)) {
> > expired = dentry;
> > goto found;
> > }
> > - spin_unlock(&sbi->fs_lock);
> > /*
> > * Case 3: pseudo direct mount, expire individual leaves
> > * (autofs-4.1).
> > */
> > } else {
> > + /* Path walk currently on this dentry? */
> > + ino_count = atomic_read(&ino->count) + 1;
> > + if (atomic_read(&dentry->d_count) > ino_count)
> > + goto next;
> > +
> > expired = autofs4_check_leaves(mnt, dentry, timeout, do_now);
> > if (expired) {
> > dput(dentry);
The expired dentry may be a different dentry!
> > @@ -367,6 +379,7 @@ static struct dentry *autofs4_expire_indirect(struct super_block *sb,
> > }
> > }
> > next:
> > + spin_unlock(&sbi->fs_lock);
> > dput(dentry);
> > spin_lock(&dcache_lock);
> > next = next->next;
> > @@ -377,6 +390,9 @@ next:
> > found:
> > DPRINTK("returning %p %.*s",
> > expired, (int)expired->d_name.len, expired->d_name.name);
> > + ino = autofs4_dentry_ino(expired);
>
> If we get here, ino is already set to the autofs4_dentry_ino(expired),
> so this statement is redundant.
Almost but see above.
>
> [...]
> > diff --git a/fs/autofs4/root.c b/fs/autofs4/root.c
> > index 2944b28..2ed2a51 100644
> > --- a/fs/autofs4/root.c
> > +++ b/fs/autofs4/root.c
> > @@ -133,7 +133,10 @@ static int try_to_fill_dentry(struct dentry *dentry, int flags)
> > /* Block on any pending expiry here; invalidate the dentry
> > when expiration is done to trigger mount request with a new
> > dentry */
> > - if (ino && (ino->flags & AUTOFS_INF_EXPIRING)) {
> > + spin_lock(&sbi->fs_lock);
> > + if (ino->flags & AUTOFS_INF_EXPIRING) {
> > + spin_unlock(&sbi->fs_lock);
> > +
> > DPRINTK("waiting for expire %p name=%.*s",
> > dentry, dentry->d_name.len, dentry->d_name.name);
>
> This is okay, since we wait on the AUTOFS_INF_EXPIRING flag in
> validate_request. That check is done outside the lock, but I doubt
> there are issues with not seeing an update since you perform a schedule
> there.
>
> > @@ -149,8 +152,11 @@ static int try_to_fill_dentry(struct dentry *dentry, int flags)
> > status = d_invalidate(dentry);
> > if (status != -EBUSY)
> > return -EAGAIN;
> > - }
> >
> > + goto cont;
> > + }
> > + spin_unlock(&sbi->fs_lock);
> > +cont:
> > DPRINTK("dentry=%p %.*s ino=%p",
> > dentry, dentry->d_name.len, dentry->d_name.name, dentry->d_inode);
> >
> > @@ -229,15 +235,21 @@ static void *autofs4_follow_link(struct dentry *dentry, struct nameidata *nd)
> > goto done;
> >
> > /* If an expire request is pending wait for it. */
> > - if (ino && (ino->flags & AUTOFS_INF_EXPIRING)) {
> > + spin_lock(&sbi->fs_lock);
> > + if (ino->flags & AUTOFS_INF_EXPIRING) {
> > + spin_unlock(&sbi->fs_lock);
> > +
> > DPRINTK("waiting for active request %p name=%.*s",
> > dentry, dentry->d_name.len, dentry->d_name.name);
> >
> > status = autofs4_wait(sbi, dentry, NFY_NONE);
> >
> > DPRINTK("request done status=%d", status);
> > - }
> >
> > + goto cont;
> > + }
> > + spin_unlock(&sbi->fs_lock);
> > +cont:
>
> could've done an:
> } else
> spin_unlock(&sbi->fs_lock);
>
> But, whatever...
>
> Reviewed-by: Jeff Moyer <jmoyer@xxxxxxxxxx>
--
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
