linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Al Viro <viro@ZenIV.linux.org.uk>
To: linux-fsdevel@vger.kernel.org
Cc: Alexey Dobriyan <adobriyan@gmail.com>
Subject: [PATCH 1/5] procfs: get rid of ancient BS in pid_revalidate() uses
Date: Sat, 26 May 2018 01:04:06 +0100	[thread overview]
Message-ID: <20180526000410.10804-1-viro@ZenIV.linux.org.uk> (raw)
In-Reply-To: <20180526000302.GK30522@ZenIV.linux.org.uk>

From: Al Viro <viro@zeniv.linux.org.uk>

First of all, calling pid_revalidate() in the end of <pid>/* lookups
is *not* about closing any kind of races; that used to be true once
upon a time, but these days those comments are actively misleading.
Especially since pid_revalidate() doesn't even do d_drop() on
failure anymore.  It doesn't matter, anyway, since once
pid_revalidate() starts returning false, ->d_delete() of those
dentries starts saying "don't keep"; they won't get stuck in
dcache any longer than they are pinned.

These calls cannot be just removed, though - the side effect of
pid_revalidate() (updating i_uid/i_gid/etc.) is what we are calling
it for here.

Let's separate the "update ownership" into a new helper (pid_update_inode())
and use it, both in lookups and in pid_revalidate() itself.

The comments in pid_revalidate() are also out of date - they refer to
the time when pid_revalidate() used to call d_drop() directly...

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/proc/base.c       | 55 +++++++++++++++++++++++-----------------------------
 fs/proc/internal.h   |  2 +-
 fs/proc/namespaces.c |  9 +++------
 3 files changed, 28 insertions(+), 38 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index eafa39a3a88c..6e0875505898 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -1803,15 +1803,22 @@ int pid_getattr(const struct path *path, struct kstat *stat,
 /* dentry stuff */
 
 /*
- *	Exceptional case: normally we are not allowed to unhash a busy
- * directory. In this case, however, we can do it - no aliasing problems
- * due to the way we treat inodes.
- *
+ * Set <pid>/... inode ownership (can change due to setuid(), etc.)
+ */
+void pid_update_inode(struct task_struct *task, struct inode *inode)
+{
+	task_dump_owner(task, inode->i_mode, &inode->i_uid, &inode->i_gid);
+
+	inode->i_mode &= ~(S_ISUID | S_ISGID);
+	security_task_to_inode(task, inode);
+}
+
+/*
  * Rewrite the inode's ownerships here because the owning task may have
  * performed a setuid(), etc.
  *
  */
-int pid_revalidate(struct dentry *dentry, unsigned int flags)
+static int pid_revalidate(struct dentry *dentry, unsigned int flags)
 {
 	struct inode *inode;
 	struct task_struct *task;
@@ -1823,10 +1830,7 @@ int pid_revalidate(struct dentry *dentry, unsigned int flags)
 	task = get_proc_task(inode);
 
 	if (task) {
-		task_dump_owner(task, inode->i_mode, &inode->i_uid, &inode->i_gid);
-
-		inode->i_mode &= ~(S_ISUID | S_ISGID);
-		security_task_to_inode(task, inode);
+		pid_update_inode(task, inode);
 		put_task_struct(task);
 		return 1;
 	}
@@ -2438,7 +2442,7 @@ static int proc_pident_instantiate(struct inode *dir,
 
 	inode = proc_pid_make_inode(dir->i_sb, task, p->mode);
 	if (!inode)
-		goto out;
+		return -ENOENT;
 
 	ei = PROC_I(inode);
 	if (S_ISDIR(inode->i_mode))
@@ -2448,13 +2452,10 @@ static int proc_pident_instantiate(struct inode *dir,
 	if (p->fop)
 		inode->i_fop = p->fop;
 	ei->op = p->op;
+	pid_update_inode(task, inode);
 	d_set_d_op(dentry, &pid_dentry_operations);
 	d_add(dentry, inode);
-	/* Close the race of the process dying before we return the dentry */
-	if (pid_revalidate(dentry, 0))
-		return 0;
-out:
-	return -ENOENT;
+	return 0;
 }
 
 static struct dentry *proc_pident_lookup(struct inode *dir, 
@@ -3140,22 +3141,18 @@ static int proc_pid_instantiate(struct inode *dir,
 
 	inode = proc_pid_make_inode(dir->i_sb, task, S_IFDIR | S_IRUGO | S_IXUGO);
 	if (!inode)
-		goto out;
+		return -ENOENT;
 
 	inode->i_op = &proc_tgid_base_inode_operations;
 	inode->i_fop = &proc_tgid_base_operations;
 	inode->i_flags|=S_IMMUTABLE;
 
 	set_nlink(inode, nlink_tgid);
+	pid_update_inode(task, inode);
 
 	d_set_d_op(dentry, &pid_dentry_operations);
-
 	d_add(dentry, inode);
-	/* Close the race of the process dying before we return the dentry */
-	if (pid_revalidate(dentry, 0))
-		return 0;
-out:
-	return -ENOENT;
+	return 0;
 }
 
 struct dentry *proc_pid_lookup(struct inode *dir, struct dentry * dentry, unsigned int flags)
@@ -3434,23 +3431,19 @@ static int proc_task_instantiate(struct inode *dir,
 {
 	struct inode *inode;
 	inode = proc_pid_make_inode(dir->i_sb, task, S_IFDIR | S_IRUGO | S_IXUGO);
-
 	if (!inode)
-		goto out;
+		return -ENOENT;
+
 	inode->i_op = &proc_tid_base_inode_operations;
 	inode->i_fop = &proc_tid_base_operations;
-	inode->i_flags|=S_IMMUTABLE;
+	inode->i_flags |= S_IMMUTABLE;
 
 	set_nlink(inode, nlink_tid);
+	pid_update_inode(task, inode);
 
 	d_set_d_op(dentry, &pid_dentry_operations);
-
 	d_add(dentry, inode);
-	/* Close the race of the process dying before we return the dentry */
-	if (pid_revalidate(dentry, 0))
-		return 0;
-out:
-	return -ENOENT;
+	return 0;
 }
 
 static struct dentry *proc_task_lookup(struct inode *dir, struct dentry * dentry, unsigned int flags)
diff --git a/fs/proc/internal.h b/fs/proc/internal.h
index 0f1692e63cb6..04a455b9ae69 100644
--- a/fs/proc/internal.h
+++ b/fs/proc/internal.h
@@ -147,7 +147,7 @@ extern const struct dentry_operations pid_dentry_operations;
 extern int pid_getattr(const struct path *, struct kstat *, u32, unsigned int);
 extern int proc_setattr(struct dentry *, struct iattr *);
 extern struct inode *proc_pid_make_inode(struct super_block *, struct task_struct *, umode_t);
-extern int pid_revalidate(struct dentry *, unsigned int);
+extern void pid_update_inode(struct task_struct *, struct inode *);
 extern int pid_delete_dentry(const struct dentry *);
 extern int proc_pid_readdir(struct file *, struct dir_context *);
 extern struct dentry *proc_pid_lookup(struct inode *, struct dentry *, unsigned int);
diff --git a/fs/proc/namespaces.c b/fs/proc/namespaces.c
index 59b17e509f46..ad1adce6541d 100644
--- a/fs/proc/namespaces.c
+++ b/fs/proc/namespaces.c
@@ -96,19 +96,16 @@ static int proc_ns_instantiate(struct inode *dir,
 
 	inode = proc_pid_make_inode(dir->i_sb, task, S_IFLNK | S_IRWXUGO);
 	if (!inode)
-		goto out;
+		return -ENOENT;
 
 	ei = PROC_I(inode);
 	inode->i_op = &proc_ns_link_inode_operations;
 	ei->ns_ops = ns_ops;
+	pid_update_inode(task, inode);
 
 	d_set_d_op(dentry, &pid_dentry_operations);
 	d_add(dentry, inode);
-	/* Close the race of the process dying before we return the dentry */
-	if (pid_revalidate(dentry, 0))
-		return 0;
-out:
-	return -ENOENT;
+	return 0;
 }
 
 static int proc_ns_dir_readdir(struct file *file, struct dir_context *ctx)
-- 
2.11.0

  reply	other threads:[~2018-05-26  0:04 UTC|newest]

Thread overview: 46+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-05-13 21:26 [RFC][PATCHES] reducing d_add() use, part 1 Al Viro
2018-05-13 21:30 ` [PATCH 01/15] bfs_lookup(): use d_splice_alias() Al Viro
2018-05-13 21:30   ` [PATCH 02/15] bfs_find_entry: pass name/len as qstr pointer Al Viro
2018-05-13 21:30   ` [PATCH 03/15] bfs_add_entry: " Al Viro
2018-05-13 21:30   ` [PATCH 04/15] cramfs_lookup(): use d_splice_alias() Al Viro
2018-05-13 21:30   ` [PATCH 05/15] freevxfs_lookup(): " Al Viro
2018-05-14  8:41     ` Christoph Hellwig
2018-05-14 15:26       ` Al Viro
2018-05-13 21:30   ` [PATCH 06/15] minix_lookup: " Al Viro
2018-05-13 21:30   ` [PATCH 07/15] qnx4_lookup: " Al Viro
2018-05-14 10:48     ` Anders Larsen
2018-05-13 21:30   ` [PATCH 08/15] sysv_lookup: " Al Viro
2018-05-14  8:41     ` Christoph Hellwig
2018-05-13 21:30   ` [PATCH 09/15] ubifs_lookup: " Al Viro
2018-05-14  6:48     ` Richard Weinberger
2018-05-13 21:30   ` [PATCH 10/15] qnx6_lookup: switch to d_splice_alias() Al Viro
2018-05-13 21:30   ` [PATCH 11/15] romfs_lookup: hash negative lookups, use d_splice_alias() Al Viro
2018-05-13 21:30   ` [PATCH 12/15] adfs_lookup_byname: .. *is* taken care of in fs/namei.c Al Viro
2018-05-13 21:30   ` [PATCH 13/15] adfs_lookup: do not fail with ENOENT on negatives, use d_splice_alias() Al Viro
2018-05-13 21:30   ` [PATCH 14/15] xfs_vn_lookup: simplify a bit Al Viro
2018-05-14  8:41     ` Christoph Hellwig
2018-05-13 21:30   ` [PATCH 15/15] openpromfs: switch to d_splice_alias() Al Viro
2018-05-16  9:45   ` [PATCH 01/15] bfs_lookup(): use d_splice_alias() Tigran Aivazian
2018-05-25 23:53 ` [RFC][PATCHES] reducing d_add() use, part 2 Al Viro
2018-05-25 23:54   ` [PATCH 01/10] openpromfs: switch to d_splice_alias() Al Viro
2018-05-25 23:54     ` [PATCH 02/10] orangefs_lookup: simplify Al Viro
2018-05-31 18:23       ` Mike Marshall
2018-05-25 23:54     ` [PATCH 03/10] omfs_lookup(): report IO errors, use d_splice_alias() Al Viro
2018-05-25 23:54     ` [PATCH 04/10] hfs: " Al Viro
2018-05-25 23:54     ` [PATCH 05/10] hfs: don't allow mounting over .../rsrc Al Viro
2018-05-25 23:54     ` [PATCH 06/10] hfsplus: switch to d_splice_alias() Al Viro
2018-05-25 23:54     ` [PATCH 07/10] ncp_lookup(): use d_splice_alias() Al Viro
2018-05-25 23:54     ` [PATCH 08/10] 9p: unify paths in v9fs_vfs_lookup() Al Viro
2018-05-25 23:54     ` [PATCH 09/10] cifs_lookup(): cifs_get_inode_...() never returns 0 with *inode left NULL Al Viro
2018-05-25 23:54     ` [PATCH 10/10] cifs_lookup(): switch to d_splice_alias() Al Viro
2018-05-26  0:03   ` [RFC][PATCHES] reducing d_add() use, part 3 (procfs) Al Viro
2018-05-26  0:04     ` Al Viro [this message]
2018-05-26  0:04       ` [PATCH 2/5] proc_lookupfd_common(): don't bother with instantiate unless the file is open Al Viro
2018-05-26  0:04       ` [PATCH 3/5] don't bother with tid_fd_revalidate() in lookups Al Viro
2018-05-26  0:04       ` [PATCH 4/5] procfs: switch instantiate_t to d_splice_alias() Al Viro
2018-05-26  0:04       ` [PATCH 5/5] switch the rest of procfs lookups " Al Viro
2018-05-31  8:28       ` [PATCH 1/5] procfs: get rid of ancient BS in pid_revalidate() uses Alexey Dobriyan
2018-05-26 13:07     ` [RFC][PATCHES] reducing d_add() use, part 3 (procfs) Alexey Dobriyan
2018-05-26 13:56       ` Alexey Dobriyan
2018-05-26 18:20         ` Al Viro
2018-05-26 18:38           ` Matthew Wilcox

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=20180526000410.10804-1-viro@ZenIV.linux.org.uk \
    --to=viro@zeniv.linux.org.uk \
    --cc=adobriyan@gmail.com \
    --cc=linux-fsdevel@vger.kernel.org \
    /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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).