Re: [PATCH 2/2] vfs: force reval on dentry of bind mounted files on FS_REVAL_DOT filesystems

[Jeff Layton <jlayton@redhat.com>]

On Wed, 02 Dec 2009 16:01:59 -0800
ebiederm@xmission.com (Eric W. Biederman) wrote:

> Jeff Layton <jlayton@redhat.com> writes:
> 
> > In the case of a bind mounted file, the path walking code will assume
> > that the cached dentry that was bind mounted is valid. This is a problem
> > problem for NFSv4 in a way that's similar to LAST_BIND symlinks.
> >
> > Fix this by revalidating the dentry if FS_FOLLOW_DOT is set and
> > __follow_mount returns true.
> >
> > Note that in the non-open codepath, we cannot return an error to the
> > lookup if the revalidation fails. Doing so will leave a bind mount in
> > a state such that we can't unmount it. In that case we'll just have to
> > settle for d_invalidating it (which should mostly turn out to be a
> > d_drop in this case) and returning success.
> 
> Looks reasonable to me.  I wonder a little if we care about follow_mount
> as well as __follow_mount.
> 

Good question.

It does sort of seem like it. There's also follow_down too...

OTOH, we have a reproducible testcases that the two patches in this
series fix. I'm not aware of anything that's technically "broken" by
the lack of revalidations in the other codepaths so I'm hesitant to add
them in unless and until we do.

> This seems reasonable to me.
> 
> Acked-by: "Eric W. Biederman" <ebiederm@xmission.com>
> 
> >
> > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > ---
> >  fs/namei.c |   11 ++++++++++-
> >  1 files changed, 10 insertions(+), 1 deletions(-)
> >
> > diff --git a/fs/namei.c b/fs/namei.c
> > index 339789e..0d55b6f 100644
> > --- a/fs/namei.c
> > +++ b/fs/namei.c
> > @@ -851,7 +851,13 @@ static int do_lookup(struct nameidata *nd, struct qstr *name,
> >  done:
> >  	path->mnt = mnt;
> >  	path->dentry = dentry;
> > -	__follow_mount(path);
> > +
> > +	/*
> > +	 * cannot return the error returned by force_reval_path as that can
> > +	 * make it impossible to unmount a bind mounted dentry if it's stale.
> > +	 */
> > +	if (__follow_mount(path))
> > +		force_reval_path(path, nd);
> >  	return 0;
> >  
> >  need_lookup:
> > @@ -1840,6 +1846,9 @@ do_last:
> >  		error = -ELOOP;
> >  		if (flag & O_NOFOLLOW)
> >  			goto exit_dput;
> > +		error = force_reval_path(&path, &nd);
> > +		if (error)
> > +			goto exit_dput;
> >  	}
> >  
> >  	error = -ENOENT;
> > -- 
> > 1.5.5.6


-- 
Jeff Layton <jlayton@redhat.com>