linux-kernel - Re: [PATCH 2/2] vfs: force reval on dentry of bind mounted files on FS_REVAL

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20091202202322.58e651d1@tlielax.poochiereds.net>
Date:	Wed, 2 Dec 2009 20:23:22 -0500
From:	Jeff Layton <jlayton@...hat.com>
To:	ebiederm@...ssion.com (Eric W. Biederman)
Cc:	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
	jamie@...reable.org, pavel@....cz, miklos@...redi.hu,
	viro@...IV.linux.org.uk, duaneg@...da.com
Subject: Re: [PATCH 2/2] vfs: force reval on dentry of bind mounted files on
 FS_REVAL_DOT filesystems

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

> Jeff Layton <jlayton@...hat.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@...ssion.com>
> 
> >
> > Signed-off-by: Jeff Layton <jlayton@...hat.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@...hat.com>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/