This is the second attempt to fix this problem. The first one attempted
to fix this in procfs, but Eric Biederman pointed out that file bind
mounts have a similar problem. This set attempts to fix the issue at a
higher level, in the generic VFS layer.
In certain situations, when it knows that they are valid, the path
walking code will skip revalidating dentries that it finds in the cache.
This causes problems with filesystems such as NFSv4 and CIFS that depend
on the d_revalidate routine to do opens during lookup.
A simple way to demonstrate this problem is by having a program open a
file that sits on NFSv4 via a procfs symlink or file bind mount, and
then try to set a fcntl read lock on the file. The lock operation will
return -ENOLCK because the open file has no NFSv4 state attached.
This set fixes this problem by adding a new routine to force a
revalidation of the dentry in these situations when they're being done
in order to open a file.
This fixes my testcase, and I haven't seen any other adverse affects on
it. I am however, far from certain that I'm not breaking the refcounting
in the situation where open_reval_path returns an error. I'd appreciate
someone giving me some sanity checks there. Also, have I missed any
places that need to force a revalidate like this?
Comments welcome...
Jeff Layton (2):
vfs: force reval of dentries for LAST_BIND symlinks on open
vfs: force reval on dentry of bind mounted files for LOOKUP_OPEN
fs/namei.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++--
1 files changed, 48 insertions(+), 2 deletions(-)
On Tue, 10 Nov 2009, Jeff Layton wrote:
> This is the second attempt to fix this problem. The first one attempted
> to fix this in procfs, but Eric Biederman pointed out that file bind
> mounts have a similar problem. This set attempts to fix the issue at a
> higher level, in the generic VFS layer.
I suspect the correct fix would be to clean up the open API so that
NFSv4 doesn't have to hack its stateful open routine into the
->lookup() and ->d_revalidate() methods.
Having said that, doing revalidation for proc symlinks and bind mounts
(and not just for opens) might make sense. This is something similar
to FS_REVAL_DOT, so perhaps make it conditional on this flag (or a
new, appropriately named one).
Thanks,
Miklos
On Wed, 2009-11-11 at 08:57 +0100, Miklos Szeredi wrote:
> On Tue, 10 Nov 2009, Jeff Layton wrote:
> > This is the second attempt to fix this problem. The first one attempted
> > to fix this in procfs, but Eric Biederman pointed out that file bind
> > mounts have a similar problem. This set attempts to fix the issue at a
> > higher level, in the generic VFS layer.
>
> I suspect the correct fix would be to clean up the open API so that
> NFSv4 doesn't have to hack its stateful open routine into the
> ->lookup() and ->d_revalidate() methods.
I've been working on that. I hope to have patches soon...
> Having said that, doing revalidation for proc symlinks and bind mounts
> (and not just for opens) might make sense. This is something similar
> to FS_REVAL_DOT, so perhaps make it conditional on this flag (or a
> new, appropriately named one).
Aren't both proc symlinks and bind mounts pretty much guaranteed to
point to a valid dentry? Once we fix the open case, I can't see that we
need to do much more. Networked filesystems may want to revalidate the
inode attributes, but not the dentry itself...
Cheers
Trond
Hi!
> This is the second attempt to fix this problem. The first one attempted
> to fix this in procfs, but Eric Biederman pointed out that file bind
> mounts have a similar problem. This set attempts to fix the issue at a
> higher level, in the generic VFS layer.
>
> In certain situations, when it knows that they are valid, the path
> walking code will skip revalidating dentries that it finds in the cache.
> This causes problems with filesystems such as NFSv4 and CIFS that depend
> on the d_revalidate routine to do opens during lookup.
...and it allows bypassing directory permissions. Could we fix both
here?
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Wed, 18 Nov 2009 05:19:06 +0100
Pavel Machek <[email protected]> wrote:
> Hi!
>
> > This is the second attempt to fix this problem. The first one attempted
> > to fix this in procfs, but Eric Biederman pointed out that file bind
> > mounts have a similar problem. This set attempts to fix the issue at a
> > higher level, in the generic VFS layer.
> >
> > In certain situations, when it knows that they are valid, the path
> > walking code will skip revalidating dentries that it finds in the cache.
> > This causes problems with filesystems such as NFSv4 and CIFS that depend
> > on the d_revalidate routine to do opens during lookup.
>
> ...and it allows bypassing directory permissions. Could we fix both
> here?
> Pavel
Does it? Here's what I just did to check that:
# cp /bin/sleep /root/sleep
# ls -l /root /root/sleep
dr-xr-x---. 19 root root 4096 2009-11-18 07:20 /root
-rwxr-xr-x. 1 root root 29152 2009-11-18 07:20 /root/sleep
# /root/sleep 600
...then as unprivileged user:
$ ps -ef | grep sleep
(find pid of sleep program that root is running)
$ /proc/5258/exe 600
bash: /proc/5258/exe: Permission denied
...it looks like directory permissions are respected here. Did I
misunderstand what you're concerned about?
--
Jeff Layton <[email protected]>
procfs-style symlinks return a last_type of LAST_BIND without an actual
path string. This causes __follow_link to skip calling __vfs_follow_link
and so the dentry isn't revalidated.
This is a problem when the link target sits on NFSv4 as it depends on
the VFS to revalidate the dentry before using it on an open call. Ensure
that this occurs by forcing a revalidation of the target dentry of
LAST_BIND symlinks.
Signed-off-by: Jeff Layton <[email protected]>
---
fs/namei.c | 41 +++++++++++++++++++++++++++++++++++++++++
1 files changed, 41 insertions(+), 0 deletions(-)
diff --git a/fs/namei.c b/fs/namei.c
index 3374917..5c8ef80 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -414,6 +414,45 @@ do_revalidate(struct dentry *dentry, struct nameidata *nd)
}
/*
+ * open_reval_path - force revalidation of a dentry for file opens
+ *
+ * in some situations the path walking code will trust dentries without
+ * revalidating them. This causes problems for filesystems that depend on
+ * d_revalidate to handle the actual file open (e.g. NFSv4). When LOOKUP_OPEN
+ * is set, force a revalidation if the dentry appears to be valid and a
+ * d_revalidate routine exists.
+ *
+ * Returns 0 if the revalidation was successful. If the revalidation fails,
+ * either return the error returned by d_revalidate or -ESTALE if the
+ * revalidation indicates an invalid dentry. On error, references to the dentry
+ * and vfsmount in the path are put.
+ */
+static int
+open_reval_path(struct path *path, struct nameidata *nd)
+{
+ struct dentry *dentry = path->dentry;
+
+ /* only bother with this for opens */
+ if (!(nd->flags & LOOKUP_OPEN))
+ return 0;
+
+ /* no reval routine, just return */
+ if (!dentry->d_op || !dentry->d_op->d_revalidate)
+ return 0;
+
+ dentry = do_revalidate(dentry, nd);
+ if (dentry && !IS_ERR(dentry))
+ return 0;
+
+ mntput(path->mnt);
+
+ if (!dentry)
+ return -ESTALE;
+
+ return PTR_ERR(dentry);
+}
+
+/*
* Internal lookup() using the new generic dcache.
* SMP-safe
*/
@@ -641,6 +680,8 @@ static __always_inline int __do_follow_link(struct path *path, struct nameidata
error = 0;
if (s)
error = __vfs_follow_link(nd, s);
+ else if (nd->last_type == LAST_BIND)
+ error = open_reval_path(&nd->path, nd);
if (dentry->d_inode->i_op->put_link)
dentry->d_inode->i_op->put_link(dentry, nd, cookie);
}
--
1.5.5.6
In the case of a bind mounted file, the path walking code will assume
that a cached dentry is valid and doesn't revalidate it. This is a
problem for NFSv4 in a way that's similar to LAST_BIND symlinks.
Fix this by revalidating the dentry if LOOKUP_OPEN is set and
__follow_mount returns true.
Signed-off-by: Jeff Layton <[email protected]>
---
fs/namei.c | 9 +++++++--
1 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/fs/namei.c b/fs/namei.c
index 5c8ef80..b7c9747 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -839,6 +839,7 @@ static int do_lookup(struct nameidata *nd, struct qstr *name,
{
struct vfsmount *mnt = nd->path.mnt;
struct dentry *dentry = __d_lookup(nd->path.dentry, name);
+ int error = 0;
if (!dentry)
goto need_lookup;
@@ -847,8 +848,9 @@ static int do_lookup(struct nameidata *nd, struct qstr *name,
done:
path->mnt = mnt;
path->dentry = dentry;
- __follow_mount(path);
- return 0;
+ if (__follow_mount(path))
+ error = open_reval_path(path, nd);
+ return error;
need_lookup:
dentry = real_lookup(nd->path.dentry, name, nd);
@@ -1836,6 +1838,9 @@ do_last:
error = -ELOOP;
if (flag & O_NOFOLLOW)
goto exit_dput;
+ error = open_reval_path(&path, &nd);
+ if (error)
+ goto exit;
}
error = -ENOENT;
--
1.5.5.6