All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jeff Layton <jlayton@redhat.com>
To: linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org,
	linux-nfs@vger.kernel.org
Cc: adobriyan@gmail.com, viro@ZenIV.linux.org.uk
Subject: [PATCH] proc: revalidate dentry returned by proc_pid_follow_link
Date: Fri,  6 Nov 2009 08:19:54 -0500	[thread overview]
Message-ID: <1257513594-31071-1-git-send-email-jlayton@redhat.com> (raw)

The procfs follow_link operation doesn't provide an actual pathname
string.  Instead, it returns a struct path in the nameidata and sets the
last_type field in the nameidata to LAST_BIND. This makes the open path
walking routine (do_filp_open) skip doing a lookup against the path
string and has it just trust the info in the struct path.

The problem here is that this makes that code shortcut any lookup or
revalidation of the dentry. In general, this isn't a problem -- in most
cases the dentry is known to be good. It is a problem however for NFSv4.
If this symlink is followed on an open operation no actual open call
occurs and the open state isn't properly established. This causes
problems when we later try to use this file descriptor for actual
operations.

This patch takes a minimalist approach to fixing this by making the
/proc/pid follow_link routine revalidate the dentry before returning it.
It seems to fix the reproducer I have for this, but I wonder whether it
might be better to do this at a higher level, maybe in __do_follow_link
in case there are other filesystems in the future that return a
last_type of LAST_BIND. Also, should this dentry be d_invalidated if
it d_revalidate returns 0?

Comments welcome.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/proc/base.c |   14 +++++++++++++-
 1 files changed, 13 insertions(+), 1 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 837469a..1bd994c 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -1343,7 +1343,7 @@ static int proc_exe_link(struct inode *inode, struct path *exe_path)
 static void *proc_pid_follow_link(struct dentry *dentry, struct nameidata *nd)
 {
 	struct inode *inode = dentry->d_inode;
-	int error = -EACCES;
+	int valid = 1, error = -EACCES;
 
 	/* We don't need a base pointer in the /proc filesystem */
 	path_put(&nd->path);
@@ -1354,6 +1354,18 @@ static void *proc_pid_follow_link(struct dentry *dentry, struct nameidata *nd)
 
 	error = PROC_I(inode)->op.proc_get_link(inode, &nd->path);
 	nd->last_type = LAST_BIND;
+
+	if (error)
+		goto out;
+
+	dentry = nd->path.dentry;
+	if (dentry->d_op && dentry->d_op->d_revalidate)
+		valid = dentry->d_op->d_revalidate(dentry, nd);
+
+	if (valid <= 0) {
+		path_put(&nd->path);
+		error = -ESTALE;
+	}
 out:
 	return ERR_PTR(error);
 }
-- 
1.5.5.6


WARNING: multiple messages have this Message-ID (diff)
From: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
To: linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-fsdevel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-nfs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: adobriyan-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org,
	viro-3bDd1+5oDREiFSDQTTA3OLVCufUGDwFn@public.gmane.org
Subject: [PATCH] proc: revalidate dentry returned by proc_pid_follow_link
Date: Fri,  6 Nov 2009 08:19:54 -0500	[thread overview]
Message-ID: <1257513594-31071-1-git-send-email-jlayton@redhat.com> (raw)

The procfs follow_link operation doesn't provide an actual pathname
string.  Instead, it returns a struct path in the nameidata and sets the
last_type field in the nameidata to LAST_BIND. This makes the open path
walking routine (do_filp_open) skip doing a lookup against the path
string and has it just trust the info in the struct path.

The problem here is that this makes that code shortcut any lookup or
revalidation of the dentry. In general, this isn't a problem -- in most
cases the dentry is known to be good. It is a problem however for NFSv4.
If this symlink is followed on an open operation no actual open call
occurs and the open state isn't properly established. This causes
problems when we later try to use this file descriptor for actual
operations.

This patch takes a minimalist approach to fixing this by making the
/proc/pid follow_link routine revalidate the dentry before returning it.
It seems to fix the reproducer I have for this, but I wonder whether it
might be better to do this at a higher level, maybe in __do_follow_link
in case there are other filesystems in the future that return a
last_type of LAST_BIND. Also, should this dentry be d_invalidated if
it d_revalidate returns 0?

Comments welcome.

Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 fs/proc/base.c |   14 +++++++++++++-
 1 files changed, 13 insertions(+), 1 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 837469a..1bd994c 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -1343,7 +1343,7 @@ static int proc_exe_link(struct inode *inode, struct path *exe_path)
 static void *proc_pid_follow_link(struct dentry *dentry, struct nameidata *nd)
 {
 	struct inode *inode = dentry->d_inode;
-	int error = -EACCES;
+	int valid = 1, error = -EACCES;
 
 	/* We don't need a base pointer in the /proc filesystem */
 	path_put(&nd->path);
@@ -1354,6 +1354,18 @@ static void *proc_pid_follow_link(struct dentry *dentry, struct nameidata *nd)
 
 	error = PROC_I(inode)->op.proc_get_link(inode, &nd->path);
 	nd->last_type = LAST_BIND;
+
+	if (error)
+		goto out;
+
+	dentry = nd->path.dentry;
+	if (dentry->d_op && dentry->d_op->d_revalidate)
+		valid = dentry->d_op->d_revalidate(dentry, nd);
+
+	if (valid <= 0) {
+		path_put(&nd->path);
+		error = -ESTALE;
+	}
 out:
 	return ERR_PTR(error);
 }
-- 
1.5.5.6

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

             reply	other threads:[~2009-11-06 13:19 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2009-11-06 13:19 Jeff Layton [this message]
2009-11-06 13:19 ` [PATCH] proc: revalidate dentry returned by proc_pid_follow_link Jeff Layton
2009-11-06 20:36 ` Jamie Lokier
2009-11-06 21:06   ` Jeff Layton
2009-11-06 21:06     ` Jeff Layton
2009-11-08 10:15     ` Eric W. Biederman
2009-11-09  1:12       ` Jeff Layton
2009-11-09  1:12         ` Jeff Layton
2009-11-09  1:12         ` Jeff Layton
2009-11-09  3:30         ` Eric W. Biederman
2009-11-09  3:30           ` Eric W. Biederman
2009-11-09  3:30           ` Eric W. Biederman
2009-11-09 13:11           ` Jeff Layton
2009-11-09 13:11             ` Jeff Layton
2009-11-09 13:11             ` Jeff Layton

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1257513594-31071-1-git-send-email-jlayton@redhat.com \
    --to=jlayton@redhat.com \
    --cc=adobriyan@gmail.com \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-nfs@vger.kernel.org \
    --cc=viro@ZenIV.linux.org.uk \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.