[PATCH 0/2] vfs: ensure that dentries are revalidated on open (try #2)

[PATCH 0/2] vfs: ensure that dentries are revalidated on open (try #2)

[Jeff Layton]

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(-)

[PATCH 1/2] vfs: force reval of dentries for LAST_BIND symlinks on open

[Jeff Layton]

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 <jlayton@redhat.com>
---
 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

[PATCH 2/2] vfs: force reval on dentry of bind mounted files for LOOKUP_OPEN

[Jeff Layton]

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 <jlayton@redhat.com>
---
 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

Re: [PATCH 0/2] vfs: ensure that dentries are revalidated on open (try #2)

[Miklos Szeredi]

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

Re: [PATCH 0/2] vfs: ensure that dentries are revalidated on open (try #2)

[Miklos Szeredi]

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
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/2] vfs: ensure that dentries are revalidated on open (try #2)

[Trond Myklebust]

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

Re: [PATCH 0/2] vfs: ensure that dentries are revalidated on open (try #2)

[Miklos Szeredi]

On Wed, 11 Nov 2009, Trond Myklebust wrote:
> > 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?

It could for example happen that client bind mounts a regular file,
then the file is unlinked on the server.  Then the bind mounted dentry
is no longer valid.

Thanks,
Miklos

Re: [PATCH 0/2] vfs: ensure that dentries are revalidated on open (try #2)

[Miklos Szeredi]

On Wed, 11 Nov 2009, Trond Myklebust wrote:
> > 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?

It could for example happen that client bind mounts a regular file,
then the file is unlinked on the server.  Then the bind mounted dentry
is no longer valid.

Thanks,
Miklos
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/2] vfs: ensure that dentries are revalidated on open (try #2)

[Miklos Szeredi]

On Wed, 11 Nov 2009, Trond Myklebust wrote:
> > 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?

It could for example happen that client bind mounts a regular file,
then the file is unlinked on the server.  Then the bind mounted dentry
is no longer valid.

Thanks,
Miklos

Re: [PATCH 0/2] vfs: ensure that dentries are revalidated on open (try #2)

[Trond Myklebust]

On Wed, 2009-11-11 at 09:33 +0100, Miklos Szeredi wrote:
> On Wed, 11 Nov 2009, Trond Myklebust wrote:
> > Aren't both proc symlinks and bind mounts pretty much guaranteed to
> > point to a valid dentry?
> 
> It could for example happen that client bind mounts a regular file,
> then the file is unlinked on the server.  Then the bind mounted dentry
> is no longer valid.

The mountpoint dentry is still supposed to remain valid and accessible
on the client even if the object it points to on the server is invalid.
Otherwise you will not be able to unmount.

Cheers
  Trond

Re: [PATCH 0/2] vfs: ensure that dentries are revalidated on open (try #2)

[Jeff Layton]

On Wed, 11 Nov 2009 17:26:10 +0900
Trond Myklebust <trond.myklebust@fys.uio.no> wrote:

> 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...
> 

Yes, that would be a much cleaner fix. It's good to hear that you're
working on it. Another approach for fixing this in the NFSv4 code did
occur to me too:

We could have nfs4_intent_set_file call lookup_instantiate_filp with an
open function pointer that's different from nfs_file_open. That function
would basically do what nfs_file_open does now. We could then fix
nfs_file_open to do a stateful open when it gets a filp with no state.

Until you have the VFS open interface fixed that might be the safest
approach in case there are other ways to skip dentry revalidation that
we haven't identified yet.

> > 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...
> 

As Miklos pointed out, removing the file on the server would make even
those invalid...

A new fs_flag sounds like a reasonable idea. Assuming that I didn't
break the refcounting, my main worry with the set I have so far is that
it'll slow down path walking. A new fs_flag should help minimize that
for filesystems where this isn't an issue.

-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH 0/2] vfs: ensure that dentries are revalidated on open (try #2)

[Jeff Layton]

On Wed, 11 Nov 2009 17:26:10 +0900
Trond Myklebust <trond.myklebust-41N18TsMXrtuMpJDpNschA@public.gmane.org> wrote:

> 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...
> 

Yes, that would be a much cleaner fix. It's good to hear that you're
working on it. Another approach for fixing this in the NFSv4 code did
occur to me too:

We could have nfs4_intent_set_file call lookup_instantiate_filp with an
open function pointer that's different from nfs_file_open. That function
would basically do what nfs_file_open does now. We could then fix
nfs_file_open to do a stateful open when it gets a filp with no state.

Until you have the VFS open interface fixed that might be the safest
approach in case there are other ways to skip dentry revalidation that
we haven't identified yet.

> > 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...
> 

As Miklos pointed out, removing the file on the server would make even
those invalid...

A new fs_flag sounds like a reasonable idea. Assuming that I didn't
break the refcounting, my main worry with the set I have so far is that
it'll slow down path walking. A new fs_flag should help minimize that
for filesystems where this isn't an issue.

-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/2] vfs: ensure that dentries are revalidated on open (try #2)

[Jeff Layton]

On Wed, 11 Nov 2009 17:26:10 +0900
Trond Myklebust <trond.myklebust@fys.uio.no> wrote:

> 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...
> 

Yes, that would be a much cleaner fix. It's good to hear that you're
working on it. Another approach for fixing this in the NFSv4 code did
occur to me too:

We could have nfs4_intent_set_file call lookup_instantiate_filp with an
open function pointer that's different from nfs_file_open. That function
would basically do what nfs_file_open does now. We could then fix
nfs_file_open to do a stateful open when it gets a filp with no state.

Until you have the VFS open interface fixed that might be the safest
approach in case there are other ways to skip dentry revalidation that
we haven't identified yet.

> > 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...
> 

As Miklos pointed out, removing the file on the server would make even
those invalid...

A new fs_flag sounds like a reasonable idea. Assuming that I didn't
break the refcounting, my main worry with the set I have so far is that
it'll slow down path walking. A new fs_flag should help minimize that
for filesystems where this isn't an issue.

-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH 0/2] vfs: ensure that dentries are revalidated on open (try #2)

[Pavel Machek]

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

Re: [PATCH 0/2] vfs: ensure that dentries are revalidated on open (try #2)

[Pavel Machek]

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
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/2] vfs: ensure that dentries are revalidated on open (try #2)

[Jeff Layton]

On Wed, 18 Nov 2009 05:19:06 +0100
Pavel Machek <pavel@ucw.cz> 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 <jlayton@redhat.com>

Re: [PATCH 0/2] vfs: ensure that dentries are revalidated on open (try #2)

[Pavel Machek]

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?
> 
> Does it? Here's what I just did to check that:

Yes it does, see http://seclists.org/bugtraq/2009/Oct/179


> # 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?

/proc does not allow you to use /proc/XX/fd of unrelated users; it is
another mechanism disallowing access. (Plus, I did my experiments with
/proc/XX/fd, not /exe).
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 0/2] vfs: ensure that dentries are revalidated on open (try #2)

[Jeff Layton]

On Wed, 18 Nov 2009 18:38:17 +0100
Pavel Machek <pavel@ucw.cz> 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?
> > 
> > Does it? Here's what I just did to check that:
> 
> Yes it does, see http://seclists.org/bugtraq/2009/Oct/179
> 
> 
> > # 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?
> 
> /proc does not allow you to use /proc/XX/fd of unrelated users; it is
> another mechanism disallowing access. (Plus, I did my experiments with
> /proc/XX/fd, not /exe).
> 									Pavel

Thanks for the info. Took me a while to get through it but I read most
of the thread. I agree that it sounds like a very similar problem.

I'm beginning to wonder whether the right answer is to just make
these /proc symlinks behave more like normal symlinks. Get rid of
LAST_BIND and have follow_link turn the dentry into a path via d_path().

It's less efficient, but it means less special-casing in the path
walking code. I don't see /proc symlinks as being so performance
critical that we can't do it that way instead.

That still leaves the issue with bind mounted files not causing a
d_revalidate, but we can deal with that separately once the other issue
is resolved...

Thoughts?
-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH 0/2] vfs: ensure that dentries are revalidated on open (try #2)

[Pavel Machek]

Hi!

> > > Does it? Here's what I just did to check that:
> > 
> > Yes it does, see http://seclists.org/bugtraq/2009/Oct/179
...
> > /proc does not allow you to use /proc/XX/fd of unrelated users; it is
> > another mechanism disallowing access. (Plus, I did my experiments with
> > /proc/XX/fd, not /exe).

> Thanks for the info. Took me a while to get through it but I read most
> of the thread. I agree that it sounds like a very similar problem.
> 
> I'm beginning to wonder whether the right answer is to just make
> these /proc symlinks behave more like normal symlinks. Get rid of
> LAST_BIND and have follow_link turn the dentry into a path via
> d_path().

That would work for me.

> It's less efficient, but it means less special-casing in the path
> walking code. I don't see /proc symlinks as being so performance
> critical that we can't do it that way instead.

Current approach works with deleted files; without special-casing that
will stop. But I see it as a good thing: you should not have to chmod
000 before deleting a file.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html