linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RFC][PATCHES] reducing d_add() use, part 1
@ 2018-05-13 21:26 Al Viro
  2018-05-13 21:30 ` [PATCH 01/15] bfs_lookup(): use d_splice_alias() Al Viro
  2018-05-25 23:53 ` [RFC][PATCHES] reducing d_add() use, part 2 Al Viro
  0 siblings, 2 replies; 46+ messages in thread
From: Al Viro @ 2018-05-13 21:26 UTC (permalink / raw)
  To: linux-fsdevel

	A lot of d_add() uses are in ->lookup() instances;
those really should be d_splice_alias() - that's mandatory
for anything exported (and we'd seen people not bothering
to convert when adding exports/open-by-fhandle support) *and*
it's not costlier than d_add() anyway, in all cases when
d_add() wouldn't have caused instant breakage.

	What's more, d_splice_alias() makes for simpler life
in callers - it does the right thing when given ERR_PTR(),
which simplifies the logics in quite a few ->lookup() instances.

	There are trickier cases, but a bunch of those call
sites are completely straightforward.  See followups, or
vfs.git#work.lookup

^ permalink raw reply	[flat|nested] 46+ messages in thread

* [PATCH 01/15] bfs_lookup(): use d_splice_alias()
  2018-05-13 21:26 [RFC][PATCHES] reducing d_add() use, part 1 Al Viro
@ 2018-05-13 21:30 ` Al Viro
  2018-05-13 21:30   ` [PATCH 02/15] bfs_find_entry: pass name/len as qstr pointer Al Viro
                     ` (14 more replies)
  2018-05-25 23:53 ` [RFC][PATCHES] reducing d_add() use, part 2 Al Viro
  1 sibling, 15 replies; 46+ messages in thread
From: Al Viro @ 2018-05-13 21:30 UTC (permalink / raw)
  To: linux-fsdevel; +Cc: Tigran A. Aivazian

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

code is actually simpler that way.

Cc: "Tigran A. Aivazian" <aivazian.tigran@gmail.com>
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/bfs/dir.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/fs/bfs/dir.c b/fs/bfs/dir.c
index ee832ca5f734..facf9614a381 100644
--- a/fs/bfs/dir.c
+++ b/fs/bfs/dir.c
@@ -141,14 +141,9 @@ static struct dentry *bfs_lookup(struct inode *dir, struct dentry *dentry,
 		unsigned long ino = (unsigned long)le16_to_cpu(de->ino);
 		brelse(bh);
 		inode = bfs_iget(dir->i_sb, ino);
-		if (IS_ERR(inode)) {
-			mutex_unlock(&info->bfs_lock);
-			return ERR_CAST(inode);
-		}
 	}
 	mutex_unlock(&info->bfs_lock);
-	d_add(dentry, inode);
-	return NULL;
+	return d_splice_alias(inode, dentry);
 }
 
 static int bfs_link(struct dentry *old, struct inode *dir,
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 46+ messages in thread

* [PATCH 02/15] bfs_find_entry: pass name/len as qstr pointer
  2018-05-13 21:30 ` [PATCH 01/15] bfs_lookup(): use d_splice_alias() Al Viro
@ 2018-05-13 21:30   ` Al Viro
  2018-05-13 21:30   ` [PATCH 03/15] bfs_add_entry: " Al Viro
                     ` (13 subsequent siblings)
  14 siblings, 0 replies; 46+ messages in thread
From: Al Viro @ 2018-05-13 21:30 UTC (permalink / raw)
  To: linux-fsdevel; +Cc: Tigran A. Aivazian

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

all callers feed something->name/something->len anyway

Cc: "Tigran A. Aivazian" <aivazian.tigran@gmail.com>
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/bfs/dir.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/fs/bfs/dir.c b/fs/bfs/dir.c
index facf9614a381..528c69746f7d 100644
--- a/fs/bfs/dir.c
+++ b/fs/bfs/dir.c
@@ -24,7 +24,7 @@
 static int bfs_add_entry(struct inode *dir, const unsigned char *name,
 						int namelen, int ino);
 static struct buffer_head *bfs_find_entry(struct inode *dir,
-				const unsigned char *name, int namelen,
+				const struct qstr *child,
 				struct bfs_dirent **res_dir);
 
 static int bfs_readdir(struct file *f, struct dir_context *ctx)
@@ -136,7 +136,7 @@ static struct dentry *bfs_lookup(struct inode *dir, struct dentry *dentry,
 		return ERR_PTR(-ENAMETOOLONG);
 
 	mutex_lock(&info->bfs_lock);
-	bh = bfs_find_entry(dir, dentry->d_name.name, dentry->d_name.len, &de);
+	bh = bfs_find_entry(dir, &dentry->d_name, &de);
 	if (bh) {
 		unsigned long ino = (unsigned long)le16_to_cpu(de->ino);
 		brelse(bh);
@@ -178,7 +178,7 @@ static int bfs_unlink(struct inode *dir, struct dentry *dentry)
 	struct bfs_sb_info *info = BFS_SB(inode->i_sb);
 
 	mutex_lock(&info->bfs_lock);
-	bh = bfs_find_entry(dir, dentry->d_name.name, dentry->d_name.len, &de);
+	bh = bfs_find_entry(dir, &dentry->d_name, &de);
 	if (!bh || (le16_to_cpu(de->ino) != inode->i_ino))
 		goto out_brelse;
 
@@ -223,18 +223,14 @@ static int bfs_rename(struct inode *old_dir, struct dentry *old_dentry,
 	info = BFS_SB(old_inode->i_sb);
 
 	mutex_lock(&info->bfs_lock);
-	old_bh = bfs_find_entry(old_dir, 
-				old_dentry->d_name.name, 
-				old_dentry->d_name.len, &old_de);
+	old_bh = bfs_find_entry(old_dir, &old_dentry->d_name, &old_de);
 
 	if (!old_bh || (le16_to_cpu(old_de->ino) != old_inode->i_ino))
 		goto end_rename;
 
 	error = -EPERM;
 	new_inode = d_inode(new_dentry);
-	new_bh = bfs_find_entry(new_dir, 
-				new_dentry->d_name.name, 
-				new_dentry->d_name.len, &new_de);
+	new_bh = bfs_find_entry(new_dir, &new_dentry->d_name, &new_de);
 
 	if (new_bh && !new_inode) {
 		brelse(new_bh);
@@ -327,12 +323,14 @@ static inline int bfs_namecmp(int len, const unsigned char *name,
 }
 
 static struct buffer_head *bfs_find_entry(struct inode *dir,
-			const unsigned char *name, int namelen,
+			const struct qstr *child,
 			struct bfs_dirent **res_dir)
 {
 	unsigned long block = 0, offset = 0;
 	struct buffer_head *bh = NULL;
 	struct bfs_dirent *de;
+	const unsigned char *name = child->name;
+	int namelen = child->len;
 
 	*res_dir = NULL;
 	if (namelen > BFS_NAMELEN)
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 46+ messages in thread

* [PATCH 03/15] bfs_add_entry: pass name/len as qstr pointer
  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   ` Al Viro
  2018-05-13 21:30   ` [PATCH 04/15] cramfs_lookup(): use d_splice_alias() Al Viro
                     ` (12 subsequent siblings)
  14 siblings, 0 replies; 46+ messages in thread
From: Al Viro @ 2018-05-13 21:30 UTC (permalink / raw)
  To: linux-fsdevel; +Cc: Tigran A. Aivazian

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

same story as with bfs_find_entry()

Cc: "Tigran A. Aivazian" <aivazian.tigran@gmail.com>
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/bfs/dir.c | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/fs/bfs/dir.c b/fs/bfs/dir.c
index 528c69746f7d..f32f21c3bbc7 100644
--- a/fs/bfs/dir.c
+++ b/fs/bfs/dir.c
@@ -21,8 +21,7 @@
 #define dprintf(x...)
 #endif
 
-static int bfs_add_entry(struct inode *dir, const unsigned char *name,
-						int namelen, int ino);
+static int bfs_add_entry(struct inode *dir, const struct qstr *child, int ino);
 static struct buffer_head *bfs_find_entry(struct inode *dir,
 				const struct qstr *child,
 				struct bfs_dirent **res_dir);
@@ -111,8 +110,7 @@ static int bfs_create(struct inode *dir, struct dentry *dentry, umode_t mode,
         mark_inode_dirty(inode);
 	bfs_dump_imap("create", s);
 
-	err = bfs_add_entry(dir, dentry->d_name.name, dentry->d_name.len,
-							inode->i_ino);
+	err = bfs_add_entry(dir, &dentry->d_name, inode->i_ino);
 	if (err) {
 		inode_dec_link_count(inode);
 		mutex_unlock(&info->bfs_lock);
@@ -154,8 +152,7 @@ static int bfs_link(struct dentry *old, struct inode *dir,
 	int err;
 
 	mutex_lock(&info->bfs_lock);
-	err = bfs_add_entry(dir, new->d_name.name, new->d_name.len,
-							inode->i_ino);
+	err = bfs_add_entry(dir, &new->d_name, inode->i_ino);
 	if (err) {
 		mutex_unlock(&info->bfs_lock);
 		return err;
@@ -237,9 +234,7 @@ static int bfs_rename(struct inode *old_dir, struct dentry *old_dentry,
 		new_bh = NULL;
 	}
 	if (!new_bh) {
-		error = bfs_add_entry(new_dir, 
-					new_dentry->d_name.name,
-					new_dentry->d_name.len,
+		error = bfs_add_entry(new_dir, &new_dentry->d_name,
 					old_inode->i_ino);
 		if (error)
 			goto end_rename;
@@ -269,9 +264,10 @@ const struct inode_operations bfs_dir_inops = {
 	.rename			= bfs_rename,
 };
 
-static int bfs_add_entry(struct inode *dir, const unsigned char *name,
-							int namelen, int ino)
+static int bfs_add_entry(struct inode *dir, const struct qstr *child, int ino)
 {
+	const unsigned char *name = child->name;
+	int namelen = child->len;
 	struct buffer_head *bh;
 	struct bfs_dirent *de;
 	int block, sblock, eblock, off, pos;
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 46+ messages in thread

* [PATCH 04/15] cramfs_lookup(): use d_splice_alias()
  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   ` Al Viro
  2018-05-13 21:30   ` [PATCH 05/15] freevxfs_lookup(): " Al Viro
                     ` (11 subsequent siblings)
  14 siblings, 0 replies; 46+ messages in thread
From: Al Viro @ 2018-05-13 21:30 UTC (permalink / raw)
  To: linux-fsdevel; +Cc: Nicolas Pitre

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

simpler code that way, actually

Cc: Nicolas Pitre <nico@linaro.org>
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/cramfs/inode.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/fs/cramfs/inode.c b/fs/cramfs/inode.c
index 017b0ab19bc4..4756e9daa0b2 100644
--- a/fs/cramfs/inode.c
+++ b/fs/cramfs/inode.c
@@ -808,10 +808,7 @@ static struct dentry *cramfs_lookup(struct inode *dir, struct dentry *dentry, un
 	}
 out:
 	mutex_unlock(&read_mutex);
-	if (IS_ERR(inode))
-		return ERR_CAST(inode);
-	d_add(dentry, inode);
-	return NULL;
+	return d_splice_alias(inode, dentry);
 }
 
 static int cramfs_readpage(struct file *file, struct page *page)
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 46+ messages in thread

* [PATCH 05/15] freevxfs_lookup(): use d_splice_alias()
  2018-05-13 21:30 ` [PATCH 01/15] bfs_lookup(): use d_splice_alias() Al Viro
                     ` (2 preceding siblings ...)
  2018-05-13 21:30   ` [PATCH 04/15] cramfs_lookup(): use d_splice_alias() Al Viro
@ 2018-05-13 21:30   ` Al Viro
  2018-05-14  8:41     ` Christoph Hellwig
  2018-05-13 21:30   ` [PATCH 06/15] minix_lookup: " Al Viro
                     ` (10 subsequent siblings)
  14 siblings, 1 reply; 46+ messages in thread
From: Al Viro @ 2018-05-13 21:30 UTC (permalink / raw)
  To: linux-fsdevel; +Cc: Christoph Hellwig

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

Cc: Christoph Hellwig <hch@lst.de>
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/freevxfs/vxfs_lookup.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/fs/freevxfs/vxfs_lookup.c b/fs/freevxfs/vxfs_lookup.c
index ce4785fd81c6..a51425634f65 100644
--- a/fs/freevxfs/vxfs_lookup.c
+++ b/fs/freevxfs/vxfs_lookup.c
@@ -193,13 +193,9 @@ vxfs_lookup(struct inode *dip, struct dentry *dp, unsigned int flags)
 		return ERR_PTR(-ENAMETOOLONG);
 				 
 	ino = vxfs_inode_by_name(dip, dp);
-	if (ino) {
+	if (ino)
 		ip = vxfs_iget(dip->i_sb, ino);
-		if (IS_ERR(ip))
-			return ERR_CAST(ip);
-	}
-	d_add(dp, ip);
-	return NULL;
+	return d_splice_alias(ip, dp);
 }
 
 /**
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 46+ messages in thread

* [PATCH 06/15] minix_lookup: use d_splice_alias()
  2018-05-13 21:30 ` [PATCH 01/15] bfs_lookup(): use d_splice_alias() Al Viro
                     ` (3 preceding siblings ...)
  2018-05-13 21:30   ` [PATCH 05/15] freevxfs_lookup(): " Al Viro
@ 2018-05-13 21:30   ` Al Viro
  2018-05-13 21:30   ` [PATCH 07/15] qnx4_lookup: " Al Viro
                     ` (9 subsequent siblings)
  14 siblings, 0 replies; 46+ messages in thread
From: Al Viro @ 2018-05-13 21:30 UTC (permalink / raw)
  To: linux-fsdevel

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

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/minix/namei.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/fs/minix/namei.c b/fs/minix/namei.c
index ccf0f00030bf..1a6084d2b02e 100644
--- a/fs/minix/namei.c
+++ b/fs/minix/namei.c
@@ -28,13 +28,9 @@ static struct dentry *minix_lookup(struct inode * dir, struct dentry *dentry, un
 		return ERR_PTR(-ENAMETOOLONG);
 
 	ino = minix_inode_by_name(dentry);
-	if (ino) {
+	if (ino)
 		inode = minix_iget(dir->i_sb, ino);
-		if (IS_ERR(inode))
-			return ERR_CAST(inode);
-	}
-	d_add(dentry, inode);
-	return NULL;
+	return d_splice_alias(inode, dentry);
 }
 
 static int minix_mknod(struct inode * dir, struct dentry *dentry, umode_t mode, dev_t rdev)
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 46+ messages in thread

* [PATCH 07/15] qnx4_lookup: use d_splice_alias()
  2018-05-13 21:30 ` [PATCH 01/15] bfs_lookup(): use d_splice_alias() Al Viro
                     ` (4 preceding siblings ...)
  2018-05-13 21:30   ` [PATCH 06/15] minix_lookup: " Al Viro
@ 2018-05-13 21:30   ` Al Viro
  2018-05-14 10:48     ` Anders Larsen
  2018-05-13 21:30   ` [PATCH 08/15] sysv_lookup: " Al Viro
                     ` (8 subsequent siblings)
  14 siblings, 1 reply; 46+ messages in thread
From: Al Viro @ 2018-05-13 21:30 UTC (permalink / raw)
  To: linux-fsdevel; +Cc: Anders Larsen

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

Cc: Anders Larsen <al@alarsen.net>
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/qnx4/namei.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/fs/qnx4/namei.c b/fs/qnx4/namei.c
index eca27878079d..8d72221735d7 100644
--- a/fs/qnx4/namei.c
+++ b/fs/qnx4/namei.c
@@ -114,13 +114,9 @@ struct dentry * qnx4_lookup(struct inode *dir, struct dentry *dentry, unsigned i
 	brelse(bh);
 
 	foundinode = qnx4_iget(dir->i_sb, ino);
-	if (IS_ERR(foundinode)) {
+	if (IS_ERR(foundinode))
 		QNX4DEBUG((KERN_ERR "qnx4: lookup->iget -> error %ld\n",
 			   PTR_ERR(foundinode)));
-		return ERR_CAST(foundinode);
-	}
 out:
-	d_add(dentry, foundinode);
-
-	return NULL;
+	return d_splice_alias(foundinode, dentry);
 }
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 46+ messages in thread

* [PATCH 08/15] sysv_lookup: use d_splice_alias()
  2018-05-13 21:30 ` [PATCH 01/15] bfs_lookup(): use d_splice_alias() Al Viro
                     ` (5 preceding siblings ...)
  2018-05-13 21:30   ` [PATCH 07/15] qnx4_lookup: " Al Viro
@ 2018-05-13 21:30   ` Al Viro
  2018-05-14  8:41     ` Christoph Hellwig
  2018-05-13 21:30   ` [PATCH 09/15] ubifs_lookup: " Al Viro
                     ` (7 subsequent siblings)
  14 siblings, 1 reply; 46+ messages in thread
From: Al Viro @ 2018-05-13 21:30 UTC (permalink / raw)
  To: linux-fsdevel; +Cc: Christoph Hellwig

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

Cc: Christoph Hellwig <hch@lst.de>
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/sysv/namei.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/fs/sysv/namei.c b/fs/sysv/namei.c
index 250b0755b908..4d5d20491ffd 100644
--- a/fs/sysv/namei.c
+++ b/fs/sysv/namei.c
@@ -51,14 +51,9 @@ static struct dentry *sysv_lookup(struct inode * dir, struct dentry * dentry, un
 	if (dentry->d_name.len > SYSV_NAMELEN)
 		return ERR_PTR(-ENAMETOOLONG);
 	ino = sysv_inode_by_name(dentry);
-
-	if (ino) {
+	if (ino)
 		inode = sysv_iget(dir->i_sb, ino);
-		if (IS_ERR(inode))
-			return ERR_CAST(inode);
-	}
-	d_add(dentry, inode);
-	return NULL;
+	return d_splice_alias(inode, dentry);
 }
 
 static int sysv_mknod(struct inode * dir, struct dentry * dentry, umode_t mode, dev_t rdev)
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 46+ messages in thread

* [PATCH 09/15] ubifs_lookup: use d_splice_alias()
  2018-05-13 21:30 ` [PATCH 01/15] bfs_lookup(): use d_splice_alias() Al Viro
                     ` (6 preceding siblings ...)
  2018-05-13 21:30   ` [PATCH 08/15] sysv_lookup: " Al Viro
@ 2018-05-13 21:30   ` 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
                     ` (6 subsequent siblings)
  14 siblings, 1 reply; 46+ messages in thread
From: Al Viro @ 2018-05-13 21:30 UTC (permalink / raw)
  To: linux-fsdevel; +Cc: Richard Weinberger

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

Cc: Richard Weinberger <richard@nod.at>
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/ubifs/dir.c | 43 +++++++++++++++----------------------------
 1 file changed, 15 insertions(+), 28 deletions(-)

diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c
index 9d7fb88e172e..4e267cc21c77 100644
--- a/fs/ubifs/dir.c
+++ b/fs/ubifs/dir.c
@@ -214,7 +214,7 @@ static struct dentry *ubifs_lookup(struct inode *dir, struct dentry *dentry,
 	int err;
 	union ubifs_key key;
 	struct inode *inode = NULL;
-	struct ubifs_dent_node *dent;
+	struct ubifs_dent_node *dent = NULL;
 	struct ubifs_info *c = dir->i_sb->s_fs_info;
 	struct fscrypt_name nm;
 
@@ -229,14 +229,14 @@ static struct dentry *ubifs_lookup(struct inode *dir, struct dentry *dentry,
 		return ERR_PTR(err);
 
 	if (fname_len(&nm) > UBIFS_MAX_NLEN) {
-		err = -ENAMETOOLONG;
-		goto out_fname;
+		inode = ERR_PTR(-ENAMETOOLONG);
+		goto done;
 	}
 
 	dent = kmalloc(UBIFS_MAX_DENT_NODE_SZ, GFP_NOFS);
 	if (!dent) {
-		err = -ENOMEM;
-		goto out_fname;
+		inode = ERR_PTR(-ENOMEM);
+		goto done;
 	}
 
 	if (nm.hash) {
@@ -250,16 +250,16 @@ static struct dentry *ubifs_lookup(struct inode *dir, struct dentry *dentry,
 	}
 
 	if (err) {
-		if (err == -ENOENT) {
+		if (err == -ENOENT)
 			dbg_gen("not found");
-			goto done;
-		}
-		goto out_dent;
+		else
+			inode = ERR_PTR(err);
+		goto done;
 	}
 
 	if (dbg_check_name(c, dent, &nm)) {
-		err = -EINVAL;
-		goto out_dent;
+		inode = ERR_PTR(-EINVAL);
+		goto done;
 	}
 
 	inode = ubifs_iget(dir->i_sb, le64_to_cpu(dent->inum));
@@ -272,7 +272,7 @@ static struct dentry *ubifs_lookup(struct inode *dir, struct dentry *dentry,
 		ubifs_err(c, "dead directory entry '%pd', error %d",
 			  dentry, err);
 		ubifs_ro_mode(c, err);
-		goto out_dent;
+		goto done;
 	}
 
 	if (ubifs_crypt_is_encrypted(dir) &&
@@ -280,27 +280,14 @@ static struct dentry *ubifs_lookup(struct inode *dir, struct dentry *dentry,
 	    !fscrypt_has_permitted_context(dir, inode)) {
 		ubifs_warn(c, "Inconsistent encryption contexts: %lu/%lu",
 			   dir->i_ino, inode->i_ino);
-		err = -EPERM;
-		goto out_inode;
+		iput(inode);
+		inode = ERR_PTR(-EPERM);
 	}
 
 done:
 	kfree(dent);
 	fscrypt_free_filename(&nm);
-	/*
-	 * Note, d_splice_alias() would be required instead if we supported
-	 * NFS.
-	 */
-	d_add(dentry, inode);
-	return NULL;
-
-out_inode:
-	iput(inode);
-out_dent:
-	kfree(dent);
-out_fname:
-	fscrypt_free_filename(&nm);
-	return ERR_PTR(err);
+	return d_splice_alias(inode, dentry);
 }
 
 static int ubifs_create(struct inode *dir, struct dentry *dentry, umode_t mode,
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 46+ messages in thread

* [PATCH 10/15] qnx6_lookup: switch to d_splice_alias()
  2018-05-13 21:30 ` [PATCH 01/15] bfs_lookup(): use d_splice_alias() Al Viro
                     ` (7 preceding siblings ...)
  2018-05-13 21:30   ` [PATCH 09/15] ubifs_lookup: " Al Viro
@ 2018-05-13 21:30   ` Al Viro
  2018-05-13 21:30   ` [PATCH 11/15] romfs_lookup: hash negative lookups, use d_splice_alias() Al Viro
                     ` (5 subsequent siblings)
  14 siblings, 0 replies; 46+ messages in thread
From: Al Viro @ 2018-05-13 21:30 UTC (permalink / raw)
  To: linux-fsdevel

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

... and hash negative lookups

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/qnx6/namei.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/fs/qnx6/namei.c b/fs/qnx6/namei.c
index 72c2770830be..e2e98e653b8d 100644
--- a/fs/qnx6/namei.c
+++ b/fs/qnx6/namei.c
@@ -29,15 +29,11 @@ struct dentry *qnx6_lookup(struct inode *dir, struct dentry *dentry,
 	if (ino) {
 		foundinode = qnx6_iget(dir->i_sb, ino);
 		qnx6_put_page(page);
-		if (IS_ERR(foundinode)) {
+		if (IS_ERR(foundinode))
 			pr_debug("lookup->iget ->  error %ld\n",
 				 PTR_ERR(foundinode));
-			return ERR_CAST(foundinode);
-		}
 	} else {
 		pr_debug("%s(): not found %s\n", __func__, name);
-		return NULL;
 	}
-	d_add(dentry, foundinode);
-	return NULL;
+	return d_splice_alias(foundinode, dentry);
 }
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 46+ messages in thread

* [PATCH 11/15] romfs_lookup: hash negative lookups, use d_splice_alias()
  2018-05-13 21:30 ` [PATCH 01/15] bfs_lookup(): use d_splice_alias() Al Viro
                     ` (8 preceding siblings ...)
  2018-05-13 21:30   ` [PATCH 10/15] qnx6_lookup: switch to d_splice_alias() Al Viro
@ 2018-05-13 21:30   ` Al Viro
  2018-05-13 21:30   ` [PATCH 12/15] adfs_lookup_byname: .. *is* taken care of in fs/namei.c Al Viro
                     ` (4 subsequent siblings)
  14 siblings, 0 replies; 46+ messages in thread
From: Al Viro @ 2018-05-13 21:30 UTC (permalink / raw)
  To: linux-fsdevel

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

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/romfs/super.c | 36 +++++++++---------------------------
 1 file changed, 9 insertions(+), 27 deletions(-)

diff --git a/fs/romfs/super.c b/fs/romfs/super.c
index 8f06fd1f3d69..6ccb51993a76 100644
--- a/fs/romfs/super.c
+++ b/fs/romfs/super.c
@@ -213,7 +213,7 @@ static struct dentry *romfs_lookup(struct inode *dir, struct dentry *dentry,
 				   unsigned int flags)
 {
 	unsigned long offset, maxoff;
-	struct inode *inode;
+	struct inode *inode = NULL;
 	struct romfs_inode ri;
 	const char *name;		/* got from dentry */
 	int len, ret;
@@ -233,7 +233,7 @@ static struct dentry *romfs_lookup(struct inode *dir, struct dentry *dentry,
 
 	for (;;) {
 		if (!offset || offset >= maxoff)
-			goto out0;
+			break;
 
 		ret = romfs_dev_read(dir->i_sb, offset, &ri, sizeof(ri));
 		if (ret < 0)
@@ -244,37 +244,19 @@ static struct dentry *romfs_lookup(struct inode *dir, struct dentry *dentry,
 				       len);
 		if (ret < 0)
 			goto error;
-		if (ret == 1)
+		if (ret == 1) {
+			/* Hard link handling */
+			if ((be32_to_cpu(ri.next) & ROMFH_TYPE) == ROMFH_HRD)
+				offset = be32_to_cpu(ri.spec) & ROMFH_MASK;
+			inode = romfs_iget(dir->i_sb, offset);
 			break;
+		}
 
 		/* next entry */
 		offset = be32_to_cpu(ri.next) & ROMFH_MASK;
 	}
 
-	/* Hard link handling */
-	if ((be32_to_cpu(ri.next) & ROMFH_TYPE) == ROMFH_HRD)
-		offset = be32_to_cpu(ri.spec) & ROMFH_MASK;
-
-	inode = romfs_iget(dir->i_sb, offset);
-	if (IS_ERR(inode)) {
-		ret = PTR_ERR(inode);
-		goto error;
-	}
-	goto outi;
-
-	/*
-	 * it's a bit funky, _lookup needs to return an error code
-	 * (negative) or a NULL, both as a dentry.  ENOENT should not
-	 * be returned, instead we need to create a negative dentry by
-	 * d_add(dentry, NULL); and return 0 as no error.
-	 * (Although as I see, it only matters on writable file
-	 * systems).
-	 */
-out0:
-	inode = NULL;
-outi:
-	d_add(dentry, inode);
-	ret = 0;
+	return d_splice_alias(inode, dentry);
 error:
 	return ERR_PTR(ret);
 }
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 46+ messages in thread

* [PATCH 12/15] adfs_lookup_byname: .. *is* taken care of in fs/namei.c
  2018-05-13 21:30 ` [PATCH 01/15] bfs_lookup(): use d_splice_alias() Al Viro
                     ` (9 preceding siblings ...)
  2018-05-13 21:30   ` [PATCH 11/15] romfs_lookup: hash negative lookups, use d_splice_alias() Al Viro
@ 2018-05-13 21:30   ` 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
                     ` (3 subsequent siblings)
  14 siblings, 0 replies; 46+ messages in thread
From: Al Viro @ 2018-05-13 21:30 UTC (permalink / raw)
  To: linux-fsdevel; +Cc: Russell King

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

Cc: Russell King <linux@armlinux.org.uk>
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/adfs/dir.c | 14 --------------
 1 file changed, 14 deletions(-)

diff --git a/fs/adfs/dir.c b/fs/adfs/dir.c
index 29444c83da48..77a2d6ca3c60 100644
--- a/fs/adfs/dir.c
+++ b/fs/adfs/dir.c
@@ -146,20 +146,6 @@ adfs_dir_lookup_byname(struct inode *inode, const struct qstr *name, struct obje
 
 	obj->parent_id = inode->i_ino;
 
-	/*
-	 * '.' is handled by reserved_lookup() in fs/namei.c
-	 */
-	if (name->len == 2 && name->name[0] == '.' && name->name[1] == '.') {
-		/*
-		 * Currently unable to fill in the rest of 'obj',
-		 * but this is better than nothing.  We need to
-		 * ascend one level to find it's parent.
-		 */
-		obj->name_len = 0;
-		obj->file_id  = obj->parent_id;
-		goto free_out;
-	}
-
 	read_lock(&adfs_dir_lock);
 
 	ret = ops->setpos(&dir, 0);
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 46+ messages in thread

* [PATCH 13/15] adfs_lookup: do not fail with ENOENT on negatives, use d_splice_alias()
  2018-05-13 21:30 ` [PATCH 01/15] bfs_lookup(): use d_splice_alias() Al Viro
                     ` (10 preceding siblings ...)
  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   ` Al Viro
  2018-05-13 21:30   ` [PATCH 14/15] xfs_vn_lookup: simplify a bit Al Viro
                     ` (2 subsequent siblings)
  14 siblings, 0 replies; 46+ messages in thread
From: Al Viro @ 2018-05-13 21:30 UTC (permalink / raw)
  To: linux-fsdevel; +Cc: Russell King

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

Cc: Russell King <linux@armlinux.org.uk>
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/adfs/dir.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/adfs/dir.c b/fs/adfs/dir.c
index 77a2d6ca3c60..e18eff854e1a 100644
--- a/fs/adfs/dir.c
+++ b/fs/adfs/dir.c
@@ -252,17 +252,17 @@ adfs_lookup(struct inode *dir, struct dentry *dentry, unsigned int flags)
 
 	error = adfs_dir_lookup_byname(dir, &dentry->d_name, &obj);
 	if (error == 0) {
-		error = -EACCES;
 		/*
 		 * This only returns NULL if get_empty_inode
 		 * fails.
 		 */
 		inode = adfs_iget(dir->i_sb, &obj);
-		if (inode)
-			error = 0;
+		if (!inode)
+			inode = ERR_PTR(-EACCES);
+	} else if (error != -ENOENT) {
+		inode = ERR_PTR(error);
 	}
-	d_add(dentry, inode);
-	return ERR_PTR(error);
+	return d_splice_alias(inode, dentry);
 }
 
 /*
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 46+ messages in thread

* [PATCH 14/15] xfs_vn_lookup: simplify a bit
  2018-05-13 21:30 ` [PATCH 01/15] bfs_lookup(): use d_splice_alias() Al Viro
                     ` (11 preceding siblings ...)
  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   ` 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
  14 siblings, 1 reply; 46+ messages in thread
From: Al Viro @ 2018-05-13 21:30 UTC (permalink / raw)
  To: linux-fsdevel; +Cc: Darrick J . Wong

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

have all post-xfs_lookup() branches converge on d_splice_alias()

Cc: Darrick J. Wong <darrick.wong@oracle.com>
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/xfs/xfs_iops.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index a3ed3c811dfa..df42e4cb4dc4 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -260,6 +260,7 @@ xfs_vn_lookup(
 	struct dentry	*dentry,
 	unsigned int flags)
 {
+	struct inode *inode;
 	struct xfs_inode *cip;
 	struct xfs_name	name;
 	int		error;
@@ -269,14 +270,13 @@ xfs_vn_lookup(
 
 	xfs_dentry_to_name(&name, dentry);
 	error = xfs_lookup(XFS_I(dir), &name, &cip, NULL);
-	if (unlikely(error)) {
-		if (unlikely(error != -ENOENT))
-			return ERR_PTR(error);
-		d_add(dentry, NULL);
-		return NULL;
-	}
-
-	return d_splice_alias(VFS_I(cip), dentry);
+	if (likely(!error))
+		inode = VFS_I(cip);
+	else if (likely(error == -ENOENT))
+		inode = NULL;
+	else
+		inode = ERR_PTR(error);
+	return d_splice_alias(inode, dentry);
 }
 
 STATIC struct dentry *
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 46+ messages in thread

* [PATCH 15/15] openpromfs: switch to d_splice_alias()
  2018-05-13 21:30 ` [PATCH 01/15] bfs_lookup(): use d_splice_alias() Al Viro
                     ` (12 preceding siblings ...)
  2018-05-13 21:30   ` [PATCH 14/15] xfs_vn_lookup: simplify a bit Al Viro
@ 2018-05-13 21:30   ` Al Viro
  2018-05-16  9:45   ` [PATCH 01/15] bfs_lookup(): use d_splice_alias() Tigran Aivazian
  14 siblings, 0 replies; 46+ messages in thread
From: Al Viro @ 2018-05-13 21:30 UTC (permalink / raw)
  To: linux-fsdevel

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

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/openpromfs/inode.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/openpromfs/inode.c b/fs/openpromfs/inode.c
index 2200662a9bf1..607092f367ad 100644
--- a/fs/openpromfs/inode.c
+++ b/fs/openpromfs/inode.c
@@ -256,8 +256,7 @@ static struct dentry *openpromfs_lookup(struct inode *dir, struct dentry *dentry
 		break;
 	}
 
-	d_add(dentry, inode);
-	return NULL;
+	return d_splice_alias(inode, dentry);
 }
 
 static int openpromfs_readdir(struct file *file, struct dir_context *ctx)
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 46+ messages in thread

* Re: [PATCH 09/15] ubifs_lookup: use d_splice_alias()
  2018-05-13 21:30   ` [PATCH 09/15] ubifs_lookup: " Al Viro
@ 2018-05-14  6:48     ` Richard Weinberger
  0 siblings, 0 replies; 46+ messages in thread
From: Richard Weinberger @ 2018-05-14  6:48 UTC (permalink / raw)
  To: Al Viro; +Cc: linux-fsdevel, Linux MTD

Am Sonntag, 13. Mai 2018, 23:30:11 CEST schrieb Al Viro:
> From: Al Viro <viro@zeniv.linux.org.uk>
> 
> Cc: Richard Weinberger <richard@nod.at>
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---
>  fs/ubifs/dir.c | 43 +++++++++++++++----------------------------
>  1 file changed, 15 insertions(+), 28 deletions(-)
> 
> diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c
> index 9d7fb88e172e..4e267cc21c77 100644
> --- a/fs/ubifs/dir.c
> +++ b/fs/ubifs/dir.c
> @@ -214,7 +214,7 @@ static struct dentry *ubifs_lookup(struct inode *dir, struct dentry *dentry,
>  	int err;
>  	union ubifs_key key;
>  	struct inode *inode = NULL;
> -	struct ubifs_dent_node *dent;
> +	struct ubifs_dent_node *dent = NULL;
>  	struct ubifs_info *c = dir->i_sb->s_fs_info;
>  	struct fscrypt_name nm;
>  
> @@ -229,14 +229,14 @@ static struct dentry *ubifs_lookup(struct inode *dir, struct dentry *dentry,
>  		return ERR_PTR(err);
>  
>  	if (fname_len(&nm) > UBIFS_MAX_NLEN) {
> -		err = -ENAMETOOLONG;
> -		goto out_fname;
> +		inode = ERR_PTR(-ENAMETOOLONG);
> +		goto done;
>  	}
>  
>  	dent = kmalloc(UBIFS_MAX_DENT_NODE_SZ, GFP_NOFS);
>  	if (!dent) {
> -		err = -ENOMEM;
> -		goto out_fname;
> +		inode = ERR_PTR(-ENOMEM);
> +		goto done;
>  	}
>  
>  	if (nm.hash) {
> @@ -250,16 +250,16 @@ static struct dentry *ubifs_lookup(struct inode *dir, struct dentry *dentry,
>  	}
>  
>  	if (err) {
> -		if (err == -ENOENT) {
> +		if (err == -ENOENT)
>  			dbg_gen("not found");
> -			goto done;
> -		}
> -		goto out_dent;
> +		else
> +			inode = ERR_PTR(err);
> +		goto done;
>  	}
>  
>  	if (dbg_check_name(c, dent, &nm)) {
> -		err = -EINVAL;
> -		goto out_dent;
> +		inode = ERR_PTR(-EINVAL);
> +		goto done;
>  	}
>  
>  	inode = ubifs_iget(dir->i_sb, le64_to_cpu(dent->inum));
> @@ -272,7 +272,7 @@ static struct dentry *ubifs_lookup(struct inode *dir, struct dentry *dentry,
>  		ubifs_err(c, "dead directory entry '%pd', error %d",
>  			  dentry, err);
>  		ubifs_ro_mode(c, err);
> -		goto out_dent;
> +		goto done;
>  	}
>  
>  	if (ubifs_crypt_is_encrypted(dir) &&
> @@ -280,27 +280,14 @@ static struct dentry *ubifs_lookup(struct inode *dir, struct dentry *dentry,
>  	    !fscrypt_has_permitted_context(dir, inode)) {
>  		ubifs_warn(c, "Inconsistent encryption contexts: %lu/%lu",
>  			   dir->i_ino, inode->i_ino);
> -		err = -EPERM;
> -		goto out_inode;
> +		iput(inode);
> +		inode = ERR_PTR(-EPERM);
>  	}
>  
>  done:
>  	kfree(dent);
>  	fscrypt_free_filename(&nm);
> -	/*
> -	 * Note, d_splice_alias() would be required instead if we supported
> -	 * NFS.
> -	 */
> -	d_add(dentry, inode);
> -	return NULL;
> -
> -out_inode:
> -	iput(inode);
> -out_dent:
> -	kfree(dent);
> -out_fname:
> -	fscrypt_free_filename(&nm);
> -	return ERR_PTR(err);
> +	return d_splice_alias(inode, dentry);
>  }
>  
>  static int ubifs_create(struct inode *dir, struct dentry *dentry, umode_t mode,
> 

Acked-by: Richard Weinberger <richard@nod.at>

Thanks,
//richard

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH 05/15] freevxfs_lookup(): use d_splice_alias()
  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
  0 siblings, 1 reply; 46+ messages in thread
From: Christoph Hellwig @ 2018-05-14  8:41 UTC (permalink / raw)
  To: Al Viro; +Cc: linux-fsdevel, Christoph Hellwig

On Sun, May 13, 2018 at 10:30:07PM +0100, Al Viro wrote:
> From: Al Viro <viro@zeniv.linux.org.uk>
> 
> Cc: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

It looks a little cleaner, so this is fine, but is there also any
other reason?  A little changelog would be nice.

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH 08/15] sysv_lookup: use d_splice_alias()
  2018-05-13 21:30   ` [PATCH 08/15] sysv_lookup: " Al Viro
@ 2018-05-14  8:41     ` Christoph Hellwig
  0 siblings, 0 replies; 46+ messages in thread
From: Christoph Hellwig @ 2018-05-14  8:41 UTC (permalink / raw)
  To: Al Viro; +Cc: linux-fsdevel, Christoph Hellwig

Same here, looks sane, but a little changelog would be nice.

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH 14/15] xfs_vn_lookup: simplify a bit
  2018-05-13 21:30   ` [PATCH 14/15] xfs_vn_lookup: simplify a bit Al Viro
@ 2018-05-14  8:41     ` Christoph Hellwig
  0 siblings, 0 replies; 46+ messages in thread
From: Christoph Hellwig @ 2018-05-14  8:41 UTC (permalink / raw)
  To: Al Viro; +Cc: linux-fsdevel, Darrick J . Wong

On Sun, May 13, 2018 at 10:30:16PM +0100, Al Viro wrote:
> From: Al Viro <viro@zeniv.linux.org.uk>
> 
> have all post-xfs_lookup() branches converge on d_splice_alias()

I think this would be even better if you'd switch xfs_lookup so that it
returns a struct inode instead or ERR_PTR instead of an int and a
struct xfs_inode in the argument first.  That is what all callers want
anyway.

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH 07/15] qnx4_lookup: use d_splice_alias()
  2018-05-13 21:30   ` [PATCH 07/15] qnx4_lookup: " Al Viro
@ 2018-05-14 10:48     ` Anders Larsen
  0 siblings, 0 replies; 46+ messages in thread
From: Anders Larsen @ 2018-05-14 10:48 UTC (permalink / raw)
  To: Al Viro; +Cc: linux-fsdevel

On Sunday, 13 May 2018 22:30 Al Viro wrote:
> From: Al Viro <viro@zeniv.linux.org.uk>
> 
> Cc: Anders Larsen <al@alarsen.net>
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---
>  fs/qnx4/namei.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)

Acked-by: Anders Larsen <al@alarsen.net>

> diff --git a/fs/qnx4/namei.c b/fs/qnx4/namei.c
> index eca27878079d..8d72221735d7 100644
> --- a/fs/qnx4/namei.c
> +++ b/fs/qnx4/namei.c
> @@ -114,13 +114,9 @@ struct dentry * qnx4_lookup(struct inode *dir, struct
> dentry *dentry, unsigned i brelse(bh);
> 
>  	foundinode = qnx4_iget(dir->i_sb, ino);
> -	if (IS_ERR(foundinode)) {
> +	if (IS_ERR(foundinode))
>  		QNX4DEBUG((KERN_ERR "qnx4: lookup->iget -> error %ld\n",
>  			   PTR_ERR(foundinode)));
> -		return ERR_CAST(foundinode);
> -	}
>  out:
> -	d_add(dentry, foundinode);
> -
> -	return NULL;
> +	return d_splice_alias(foundinode, dentry);
>  }

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH 05/15] freevxfs_lookup(): use d_splice_alias()
  2018-05-14  8:41     ` Christoph Hellwig
@ 2018-05-14 15:26       ` Al Viro
  0 siblings, 0 replies; 46+ messages in thread
From: Al Viro @ 2018-05-14 15:26 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: linux-fsdevel

On Mon, May 14, 2018 at 10:41:02AM +0200, Christoph Hellwig wrote:
> On Sun, May 13, 2018 at 10:30:07PM +0100, Al Viro wrote:
> > From: Al Viro <viro@zeniv.linux.org.uk>
> > 
> > Cc: Christoph Hellwig <hch@lst.de>
> > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> 
> It looks a little cleaner, so this is fine, but is there also any
> other reason?  A little changelog would be nice.

The same changelog would be duplicated in a bunch of commits in that
series and I'm not sure how much is too much.  Let me put it that
way:
	* d_splice_alias() has better calling conventions for use
in ->lookup() - it tends to reduce the number of branches nicely.
	* d_add() in ->lookup() is no-go for anything exportable;
we did conversion for exported stuff back then, and it's documented
in Documentation/filesystems/nfs/Exporting, but people fail to RTFM
when converting more filesystems to exportability.  Preempting that
kind of bugs is a good idea.
	* the fewer stray d_add() we have sitting around, the better -
each needs a proof that this particular invocation won't end up with
multiple aliases of a directory inode.  d_splice_alias() is *NOT*
a universal replacement, but in a lot of ->lookup() instances we can
use it sanely; it can't be done quite blindly (the stuff that hangs
something off the dentry needs a careful look), but most of the time
it's fine - on any filesystem that doesn't play with ->d_fsdata, for
starters.  And in all cases such replacement in ->lookup() is
"it's no worse than before" - it's just that some cases need more
than just that conversion before they can be made exportable (see
e.g. 9p for example of that kind of extra work;
        res = d_splice_alias(inode, dentry);
        if (!res)
                v9fs_fid_add(dentry, fid);
        else if (!IS_ERR(res))
                v9fs_fid_add(res, fid);
        else
                p9_client_clunk(fid);
in there, when we need to decide which dentry to slap the fid onto)

The bunch in this series is the trivial part of conversions.  Next
group is where a bit more attention is needed and the last is procfs
nest of horrors.

I'm honestly not sure how much of the above makes sense as a boilerplate
to go into all those commits.  Suggestions?

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH 01/15] bfs_lookup(): use d_splice_alias()
  2018-05-13 21:30 ` [PATCH 01/15] bfs_lookup(): use d_splice_alias() Al Viro
                     ` (13 preceding siblings ...)
  2018-05-13 21:30   ` [PATCH 15/15] openpromfs: switch to d_splice_alias() Al Viro
@ 2018-05-16  9:45   ` Tigran Aivazian
  14 siblings, 0 replies; 46+ messages in thread
From: Tigran Aivazian @ 2018-05-16  9:45 UTC (permalink / raw)
  To: Al Viro; +Cc: linux-fsdevel

Hi Al,

Thank you. This feels a bit like a "time machine", i.e. being
instantly transferred to the almost forgotten distant past for the joy
of seeing familiar faces. Well, past, present or future --- Al Viro is
always right (especially as far as filesystems' code is concerned) and
I safely defer to you --- if you say it is better, then it most
certainly is, dear friend. And to me it looks better too.

Kind regards,
Tigran

On 13 May 2018 at 22:30, Al Viro <viro@zeniv.linux.org.uk> wrote:
> From: Al Viro <viro@zeniv.linux.org.uk>
>
> code is actually simpler that way.
>
> Cc: "Tigran A. Aivazian" <aivazian.tigran@gmail.com>
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---
>  fs/bfs/dir.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/fs/bfs/dir.c b/fs/bfs/dir.c
> index ee832ca5f734..facf9614a381 100644
> --- a/fs/bfs/dir.c
> +++ b/fs/bfs/dir.c
> @@ -141,14 +141,9 @@ static struct dentry *bfs_lookup(struct inode *dir, struct dentry *dentry,
>                 unsigned long ino = (unsigned long)le16_to_cpu(de->ino);
>                 brelse(bh);
>                 inode = bfs_iget(dir->i_sb, ino);
> -               if (IS_ERR(inode)) {
> -                       mutex_unlock(&info->bfs_lock);
> -                       return ERR_CAST(inode);
> -               }
>         }
>         mutex_unlock(&info->bfs_lock);
> -       d_add(dentry, inode);
> -       return NULL;
> +       return d_splice_alias(inode, dentry);
>  }
>
>  static int bfs_link(struct dentry *old, struct inode *dir,
> --
> 2.11.0
>

^ permalink raw reply	[flat|nested] 46+ messages in thread

* [RFC][PATCHES] reducing d_add() use, part 2
  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-25 23:53 ` Al Viro
  2018-05-25 23:54   ` [PATCH 01/10] openpromfs: switch to d_splice_alias() Al Viro
  2018-05-26  0:03   ` [RFC][PATCHES] reducing d_add() use, part 3 (procfs) Al Viro
  1 sibling, 2 replies; 46+ messages in thread
From: Al Viro @ 2018-05-25 23:53 UTC (permalink / raw)
  To: linux-fsdevel

On Sun, May 13, 2018 at 10:26:12PM +0100, Al Viro wrote:
> 	A lot of d_add() uses are in ->lookup() instances;
> those really should be d_splice_alias() - that's mandatory
> for anything exported (and we'd seen people not bothering
> to convert when adding exports/open-by-fhandle support) *and*
> it's not costlier than d_add() anyway, in all cases when
> d_add() wouldn't have caused instant breakage.
> 
> 	What's more, d_splice_alias() makes for simpler life
> in callers - it does the right thing when given ERR_PTR(),
> which simplifies the logics in quite a few ->lookup() instances.
> 
> 	There are trickier cases, but a bunch of those call
> sites are completely straightforward.  See followups, or
> vfs.git#work.lookup

Continuation of the series - more simple cases:
      openpromfs: switch to d_splice_alias()
      orangefs_lookup: simplify
      omfs_lookup(): report IO errors, use d_splice_alias()
      hfs: use d_splice_alias()
      hfs: don't allow mounting over .../rsrc
      hfsplus: switch to d_splice_alias()
      ncp_lookup(): use d_splice_alias()
      9p: unify paths in v9fs_vfs_lookup()
      cifs_lookup(): cifs_get_inode_...() never returns 0 with *inode left NULL
      cifs_lookup(): switch to d_splice_alias()

^ permalink raw reply	[flat|nested] 46+ messages in thread

* [PATCH 01/10] openpromfs: switch to d_splice_alias()
  2018-05-25 23:53 ` [RFC][PATCHES] reducing d_add() use, part 2 Al Viro
@ 2018-05-25 23:54   ` Al Viro
  2018-05-25 23:54     ` [PATCH 02/10] orangefs_lookup: simplify Al Viro
                       ` (8 more replies)
  2018-05-26  0:03   ` [RFC][PATCHES] reducing d_add() use, part 3 (procfs) Al Viro
  1 sibling, 9 replies; 46+ messages in thread
From: Al Viro @ 2018-05-25 23:54 UTC (permalink / raw)
  To: linux-fsdevel

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

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/openpromfs/inode.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/openpromfs/inode.c b/fs/openpromfs/inode.c
index 2200662a9bf1..607092f367ad 100644
--- a/fs/openpromfs/inode.c
+++ b/fs/openpromfs/inode.c
@@ -256,8 +256,7 @@ static struct dentry *openpromfs_lookup(struct inode *dir, struct dentry *dentry
 		break;
 	}
 
-	d_add(dentry, inode);
-	return NULL;
+	return d_splice_alias(inode, dentry);
 }
 
 static int openpromfs_readdir(struct file *file, struct dir_context *ctx)
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 46+ messages in thread

* [PATCH 02/10] orangefs_lookup: simplify
  2018-05-25 23:54   ` [PATCH 01/10] openpromfs: switch to d_splice_alias() Al Viro
@ 2018-05-25 23:54     ` 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
                       ` (7 subsequent siblings)
  8 siblings, 1 reply; 46+ messages in thread
From: Al Viro @ 2018-05-25 23:54 UTC (permalink / raw)
  To: linux-fsdevel

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

d_splice_alias() can handle NULL and ERR_PTR() for inode just fine...

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/orangefs/namei.c | 64 +++++++----------------------------------------------
 1 file changed, 8 insertions(+), 56 deletions(-)

diff --git a/fs/orangefs/namei.c b/fs/orangefs/namei.c
index 1b5707c44c3f..365cd73d9109 100644
--- a/fs/orangefs/namei.c
+++ b/fs/orangefs/namei.c
@@ -110,7 +110,6 @@ static struct dentry *orangefs_lookup(struct inode *dir, struct dentry *dentry,
 	struct orangefs_inode_s *parent = ORANGEFS_I(dir);
 	struct orangefs_kernel_op_s *new_op;
 	struct inode *inode;
-	struct dentry *res;
 	int ret = -EINVAL;
 
 	/*
@@ -158,65 +157,18 @@ static struct dentry *orangefs_lookup(struct inode *dir, struct dentry *dentry,
 		     new_op->downcall.resp.lookup.refn.fs_id,
 		     ret);
 
-	if (ret < 0) {
-		if (ret == -ENOENT) {
-			/*
-			 * if no inode was found, add a negative dentry to
-			 * dcache anyway; if we don't, we don't hold expected
-			 * lookup semantics and we most noticeably break
-			 * during directory renames.
-			 *
-			 * however, if the operation failed or exited, do not
-			 * add the dentry (e.g. in the case that a touch is
-			 * issued on a file that already exists that was
-			 * interrupted during this lookup -- no need to add
-			 * another negative dentry for an existing file)
-			 */
-
-			gossip_debug(GOSSIP_NAME_DEBUG,
-				     "orangefs_lookup: Adding *negative* dentry "
-				     "%p for %pd\n",
-				     dentry,
-				     dentry);
-
-			d_add(dentry, NULL);
-			res = NULL;
-			goto out;
-		}
-
+	if (ret >= 0) {
+		orangefs_set_timeout(dentry);
+		inode = orangefs_iget(dir->i_sb, &new_op->downcall.resp.lookup.refn);
+	} else if (ret == -ENOENT) {
+		inode = NULL;
+	} else {
 		/* must be a non-recoverable error */
-		res = ERR_PTR(ret);
-		goto out;
-	}
-
-	orangefs_set_timeout(dentry);
-
-	inode = orangefs_iget(dir->i_sb, &new_op->downcall.resp.lookup.refn);
-	if (IS_ERR(inode)) {
-		gossip_debug(GOSSIP_NAME_DEBUG,
-			"error %ld from iget\n", PTR_ERR(inode));
-		res = ERR_CAST(inode);
-		goto out;
+		inode = ERR_PTR(ret);
 	}
 
-	gossip_debug(GOSSIP_NAME_DEBUG,
-		     "%s:%s:%d "
-		     "Found good inode [%lu] with count [%d]\n",
-		     __FILE__,
-		     __func__,
-		     __LINE__,
-		     inode->i_ino,
-		     (int)atomic_read(&inode->i_count));
-
-	/* update dentry/inode pair into dcache */
-	res = d_splice_alias(inode, dentry);
-
-	gossip_debug(GOSSIP_NAME_DEBUG,
-		     "Lookup success (inode ct = %d)\n",
-		     (int)atomic_read(&inode->i_count));
-out:
 	op_release(new_op);
-	return res;
+	return d_splice_alias(inode, dentry);
 }
 
 /* return 0 on success; non-zero otherwise */
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 46+ messages in thread

* [PATCH 03/10] omfs_lookup(): report IO errors, use d_splice_alias()
  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-25 23:54     ` Al Viro
  2018-05-25 23:54     ` [PATCH 04/10] hfs: " Al Viro
                       ` (6 subsequent siblings)
  8 siblings, 0 replies; 46+ messages in thread
From: Al Viro @ 2018-05-25 23:54 UTC (permalink / raw)
  To: linux-fsdevel

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

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/omfs/dir.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/fs/omfs/dir.c b/fs/omfs/dir.c
index b7146526afff..4bee3a72b9f3 100644
--- a/fs/omfs/dir.c
+++ b/fs/omfs/dir.c
@@ -305,11 +305,10 @@ static struct dentry *omfs_lookup(struct inode *dir, struct dentry *dentry,
 		ino_t ino = be64_to_cpu(oi->i_head.h_self);
 		brelse(bh);
 		inode = omfs_iget(dir->i_sb, ino);
-		if (IS_ERR(inode))
-			return ERR_CAST(inode);
+	} else if (bh != ERR_PTR(-ENOENT)) {
+		inode = ERR_CAST(bh);
 	}
-	d_add(dentry, inode);
-	return NULL;
+	return d_splice_alias(inode, dentry);
 }
 
 /* sanity check block's self pointer */
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 46+ messages in thread

* [PATCH 04/10] hfs: use d_splice_alias()
  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-25 23:54     ` [PATCH 03/10] omfs_lookup(): report IO errors, use d_splice_alias() Al Viro
@ 2018-05-25 23:54     ` Al Viro
  2018-05-25 23:54     ` [PATCH 05/10] hfs: don't allow mounting over .../rsrc Al Viro
                       ` (5 subsequent siblings)
  8 siblings, 0 replies; 46+ messages in thread
From: Al Viro @ 2018-05-25 23:54 UTC (permalink / raw)
  To: linux-fsdevel

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

code is simpler that way

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/hfs/dir.c   | 20 +++++++-------------
 fs/hfs/inode.c |  3 +--
 2 files changed, 8 insertions(+), 15 deletions(-)

diff --git a/fs/hfs/dir.c b/fs/hfs/dir.c
index 75b254280ff6..3bf2ae0e467c 100644
--- a/fs/hfs/dir.c
+++ b/fs/hfs/dir.c
@@ -31,21 +31,15 @@ static struct dentry *hfs_lookup(struct inode *dir, struct dentry *dentry,
 	hfs_cat_build_key(dir->i_sb, fd.search_key, dir->i_ino, &dentry->d_name);
 	res = hfs_brec_read(&fd, &rec, sizeof(rec));
 	if (res) {
-		hfs_find_exit(&fd);
-		if (res == -ENOENT) {
-			/* No such entry */
-			inode = NULL;
-			goto done;
-		}
-		return ERR_PTR(res);
+		if (res != -ENOENT)
+			inode = ERR_PTR(res);
+	} else {
+		inode = hfs_iget(dir->i_sb, &fd.search_key->cat, &rec);
+		if (!inode)
+			inode = ERR_PTR(-EACCES);
 	}
-	inode = hfs_iget(dir->i_sb, &fd.search_key->cat, &rec);
 	hfs_find_exit(&fd);
-	if (!inode)
-		return ERR_PTR(-EACCES);
-done:
-	d_add(dentry, inode);
-	return NULL;
+	return d_splice_alias(inode, dentry);
 }
 
 /*
diff --git a/fs/hfs/inode.c b/fs/hfs/inode.c
index 2538b49cc349..0612fa367bd1 100644
--- a/fs/hfs/inode.c
+++ b/fs/hfs/inode.c
@@ -544,8 +544,7 @@ static struct dentry *hfs_file_lookup(struct inode *dir, struct dentry *dentry,
 	hlist_add_fake(&inode->i_hash);
 	mark_inode_dirty(inode);
 out:
-	d_add(dentry, inode);
-	return NULL;
+	return d_splice_alias(inode, dentry);
 }
 
 void hfs_evict_inode(struct inode *inode)
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 46+ messages in thread

* [PATCH 05/10] hfs: don't allow mounting over .../rsrc
  2018-05-25 23:54   ` [PATCH 01/10] openpromfs: switch to d_splice_alias() Al Viro
                       ` (2 preceding siblings ...)
  2018-05-25 23:54     ` [PATCH 04/10] hfs: " Al Viro
@ 2018-05-25 23:54     ` Al Viro
  2018-05-25 23:54     ` [PATCH 06/10] hfsplus: switch to d_splice_alias() Al Viro
                       ` (4 subsequent siblings)
  8 siblings, 0 replies; 46+ messages in thread
From: Al Viro @ 2018-05-25 23:54 UTC (permalink / raw)
  To: linux-fsdevel

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

That's one case when unlink() destroys a subtree, thanks to "resource
fork" idiocy.  We might forcibly evict that shit on unlink(2), but
for now let's just disallow overmounting; as it is, anything that
plays games with those would leak mounts.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/hfs/inode.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/hfs/inode.c b/fs/hfs/inode.c
index 0612fa367bd1..b3309b83371a 100644
--- a/fs/hfs/inode.c
+++ b/fs/hfs/inode.c
@@ -543,6 +543,7 @@ static struct dentry *hfs_file_lookup(struct inode *dir, struct dentry *dentry,
 	igrab(dir);
 	hlist_add_fake(&inode->i_hash);
 	mark_inode_dirty(inode);
+	dont_mount(dentry);
 out:
 	return d_splice_alias(inode, dentry);
 }
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 46+ messages in thread

* [PATCH 06/10] hfsplus: switch to d_splice_alias()
  2018-05-25 23:54   ` [PATCH 01/10] openpromfs: switch to d_splice_alias() Al Viro
                       ` (3 preceding siblings ...)
  2018-05-25 23:54     ` [PATCH 05/10] hfs: don't allow mounting over .../rsrc Al Viro
@ 2018-05-25 23:54     ` Al Viro
  2018-05-25 23:54     ` [PATCH 07/10] ncp_lookup(): use d_splice_alias() Al Viro
                       ` (3 subsequent siblings)
  8 siblings, 0 replies; 46+ messages in thread
From: Al Viro @ 2018-05-25 23:54 UTC (permalink / raw)
  To: linux-fsdevel

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

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/hfsplus/dir.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/hfsplus/dir.c b/fs/hfsplus/dir.c
index 15e06fb552da..b5254378f011 100644
--- a/fs/hfsplus/dir.c
+++ b/fs/hfsplus/dir.c
@@ -122,8 +122,7 @@ static struct dentry *hfsplus_lookup(struct inode *dir, struct dentry *dentry,
 	if (S_ISREG(inode->i_mode))
 		HFSPLUS_I(inode)->linkid = linkid;
 out:
-	d_add(dentry, inode);
-	return NULL;
+	return d_splice_alias(inode, dentry);
 fail:
 	hfs_find_exit(&fd);
 	return ERR_PTR(err);
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 46+ messages in thread

* [PATCH 07/10] ncp_lookup(): use d_splice_alias()
  2018-05-25 23:54   ` [PATCH 01/10] openpromfs: switch to d_splice_alias() Al Viro
                       ` (4 preceding siblings ...)
  2018-05-25 23:54     ` [PATCH 06/10] hfsplus: switch to d_splice_alias() Al Viro
@ 2018-05-25 23:54     ` Al Viro
  2018-05-25 23:54     ` [PATCH 08/10] 9p: unify paths in v9fs_vfs_lookup() Al Viro
                       ` (2 subsequent siblings)
  8 siblings, 0 replies; 46+ messages in thread
From: Al Viro @ 2018-05-25 23:54 UTC (permalink / raw)
  To: linux-fsdevel

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

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 drivers/staging/ncpfs/dir.c | 42 +++++++++++++++---------------------------
 1 file changed, 15 insertions(+), 27 deletions(-)

diff --git a/drivers/staging/ncpfs/dir.c b/drivers/staging/ncpfs/dir.c
index 0c57c5c5d40a..072bcb12898f 100644
--- a/drivers/staging/ncpfs/dir.c
+++ b/drivers/staging/ncpfs/dir.c
@@ -823,12 +823,11 @@ static struct dentry *ncp_lookup(struct inode *dir, struct dentry *dentry, unsig
 	struct ncp_server *server = NCP_SERVER(dir);
 	struct inode *inode = NULL;
 	struct ncp_entry_info finfo;
-	int error, res, len;
+	int res, len;
 	__u8 __name[NCP_MAXPATHLEN + 1];
 
-	error = -EIO;
 	if (!ncp_conn_valid(server))
-		goto finished;
+		return ERR_PTR(-EIO);
 
 	ncp_vdbg("server lookup for %pd2\n", dentry);
 
@@ -847,31 +846,20 @@ static struct dentry *ncp_lookup(struct inode *dir, struct dentry *dentry, unsig
 			res = ncp_obtain_info(server, dir, __name, &(finfo.i));
 	}
 	ncp_vdbg("looked for %pd2, res=%d\n", dentry, res);
-	/*
-	 * If we didn't find an entry, make a negative dentry.
-	 */
-	if (res)
-		goto add_entry;
-
-	/*
-	 * Create an inode for the entry.
-	 */
-	finfo.opened = 0;
-	finfo.ino = iunique(dir->i_sb, 2);
-	finfo.volume = finfo.i.volNumber;
-	error = -EACCES;
-	inode = ncp_iget(dir->i_sb, &finfo);
-
-	if (inode) {
-		ncp_new_dentry(dentry);
-add_entry:
-		d_add(dentry, inode);
-		error = 0;
+	if (!res) {
+		/*
+		 * Entry found; create an inode for it.
+		 */
+		finfo.opened = 0;
+		finfo.ino = iunique(dir->i_sb, 2);
+		finfo.volume = finfo.i.volNumber;
+		inode = ncp_iget(dir->i_sb, &finfo);
+		if (unlikely(!inode))
+			inode = ERR_PTR(-EACCES);
+		else
+			ncp_new_dentry(dentry);
 	}
-
-finished:
-	ncp_vdbg("result=%d\n", error);
-	return ERR_PTR(error);
+	return d_splice_alias(inode, dentry);
 }
 
 /*
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 46+ messages in thread

* [PATCH 08/10] 9p: unify paths in v9fs_vfs_lookup()
  2018-05-25 23:54   ` [PATCH 01/10] openpromfs: switch to d_splice_alias() Al Viro
                       ` (5 preceding siblings ...)
  2018-05-25 23:54     ` [PATCH 07/10] ncp_lookup(): use d_splice_alias() Al Viro
@ 2018-05-25 23:54     ` 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
  8 siblings, 0 replies; 46+ messages in thread
From: Al Viro @ 2018-05-25 23:54 UTC (permalink / raw)
  To: linux-fsdevel

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

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/9p/vfs_inode.c | 35 +++++++++++++++--------------------
 1 file changed, 15 insertions(+), 20 deletions(-)

diff --git a/fs/9p/vfs_inode.c b/fs/9p/vfs_inode.c
index 9ee534159cc6..42e102e2e74a 100644
--- a/fs/9p/vfs_inode.c
+++ b/fs/9p/vfs_inode.c
@@ -823,28 +823,21 @@ struct dentry *v9fs_vfs_lookup(struct inode *dir, struct dentry *dentry,
 	if (IS_ERR(dfid))
 		return ERR_CAST(dfid);
 
-	name = dentry->d_name.name;
-	fid = p9_client_walk(dfid, 1, &name, 1);
-	if (IS_ERR(fid)) {
-		if (fid == ERR_PTR(-ENOENT)) {
-			d_add(dentry, NULL);
-			return NULL;
-		}
-		return ERR_CAST(fid);
-	}
 	/*
 	 * Make sure we don't use a wrong inode due to parallel
 	 * unlink. For cached mode create calls request for new
 	 * inode. But with cache disabled, lookup should do this.
 	 */
-	if (v9ses->cache == CACHE_LOOSE || v9ses->cache == CACHE_FSCACHE)
+	name = dentry->d_name.name;
+	fid = p9_client_walk(dfid, 1, &name, 1);
+	if (fid == ERR_PTR(-ENOENT))
+		inode = NULL;
+	else if (IS_ERR(fid))
+		inode = ERR_CAST(fid);
+	else if (v9ses->cache == CACHE_LOOSE || v9ses->cache == CACHE_FSCACHE)
 		inode = v9fs_get_inode_from_fid(v9ses, fid, dir->i_sb);
 	else
 		inode = v9fs_get_new_inode_from_fid(v9ses, fid, dir->i_sb);
-	if (IS_ERR(inode)) {
-		p9_client_clunk(fid);
-		return ERR_CAST(inode);
-	}
 	/*
 	 * If we had a rename on the server and a parallel lookup
 	 * for the new name, then make sure we instantiate with
@@ -853,12 +846,14 @@ struct dentry *v9fs_vfs_lookup(struct inode *dir, struct dentry *dentry,
 	 * k/b.
 	 */
 	res = d_splice_alias(inode, dentry);
-	if (!res)
-		v9fs_fid_add(dentry, fid);
-	else if (!IS_ERR(res))
-		v9fs_fid_add(res, fid);
-	else
-		p9_client_clunk(fid);
+	if (!IS_ERR(fid)) {
+		if (!res)
+			v9fs_fid_add(dentry, fid);
+		else if (!IS_ERR(res))
+			v9fs_fid_add(res, fid);
+		else
+			p9_client_clunk(fid);
+	}
 	return res;
 }
 
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 46+ messages in thread

* [PATCH 09/10] cifs_lookup(): cifs_get_inode_...() never returns 0 with *inode left NULL
  2018-05-25 23:54   ` [PATCH 01/10] openpromfs: switch to d_splice_alias() Al Viro
                       ` (6 preceding siblings ...)
  2018-05-25 23:54     ` [PATCH 08/10] 9p: unify paths in v9fs_vfs_lookup() Al Viro
@ 2018-05-25 23:54     ` Al Viro
  2018-05-25 23:54     ` [PATCH 10/10] cifs_lookup(): switch to d_splice_alias() Al Viro
  8 siblings, 0 replies; 46+ messages in thread
From: Al Viro @ 2018-05-25 23:54 UTC (permalink / raw)
  To: linux-fsdevel

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

not since 2004...

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/cifs/dir.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
index 81ba6e0d88d8..ecbf36c459ea 100644
--- a/fs/cifs/dir.c
+++ b/fs/cifs/dir.c
@@ -812,7 +812,7 @@ cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry,
 				parent_dir_inode->i_sb, xid, NULL);
 	}
 
-	if ((rc == 0) && (newInode != NULL)) {
+	if (rc == 0) {
 		d_add(direntry, newInode);
 		/* since paths are not looked up by component - the parent
 		   directories are presumed to be good here */
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 46+ messages in thread

* [PATCH 10/10] cifs_lookup(): switch to d_splice_alias()
  2018-05-25 23:54   ` [PATCH 01/10] openpromfs: switch to d_splice_alias() Al Viro
                       ` (7 preceding siblings ...)
  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     ` Al Viro
  8 siblings, 0 replies; 46+ messages in thread
From: Al Viro @ 2018-05-25 23:54 UTC (permalink / raw)
  To: linux-fsdevel

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

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/cifs/dir.c | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
index ecbf36c459ea..751b8c9998ca 100644
--- a/fs/cifs/dir.c
+++ b/fs/cifs/dir.c
@@ -779,21 +779,25 @@ cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry,
 	tlink = cifs_sb_tlink(cifs_sb);
 	if (IS_ERR(tlink)) {
 		free_xid(xid);
-		return (struct dentry *)tlink;
+		return ERR_CAST(tlink);
 	}
 	pTcon = tlink_tcon(tlink);
 
 	rc = check_name(direntry, pTcon);
-	if (rc)
-		goto lookup_out;
+	if (unlikely(rc)) {
+		cifs_put_tlink(tlink);
+		free_xid(xid);
+		return ERR_PTR(rc);
+	}
 
 	/* can not grab the rename sem here since it would
 	deadlock in the cases (beginning of sys_rename itself)
 	in which we already have the sb rename sem */
 	full_path = build_path_from_dentry(direntry);
 	if (full_path == NULL) {
-		rc = -ENOMEM;
-		goto lookup_out;
+		cifs_put_tlink(tlink);
+		free_xid(xid);
+		return ERR_PTR(-ENOMEM);
 	}
 
 	if (d_really_is_positive(direntry)) {
@@ -813,28 +817,24 @@ cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry,
 	}
 
 	if (rc == 0) {
-		d_add(direntry, newInode);
 		/* since paths are not looked up by component - the parent
 		   directories are presumed to be good here */
 		renew_parental_timestamps(direntry);
-
 	} else if (rc == -ENOENT) {
-		rc = 0;
 		cifs_set_time(direntry, jiffies);
-		d_add(direntry, NULL);
-	/*	if it was once a directory (but how can we tell?) we could do
-		shrink_dcache_parent(direntry); */
-	} else if (rc != -EACCES) {
-		cifs_dbg(FYI, "Unexpected lookup error %d\n", rc);
-		/* We special case check for Access Denied - since that
-		is a common return code */
+		newInode = NULL;
+	} else {
+		if (rc != -EACCES) {
+			cifs_dbg(FYI, "Unexpected lookup error %d\n", rc);
+			/* We special case check for Access Denied - since that
+			is a common return code */
+		}
+		newInode = ERR_PTR(rc);
 	}
-
-lookup_out:
 	kfree(full_path);
 	cifs_put_tlink(tlink);
 	free_xid(xid);
-	return ERR_PTR(rc);
+	return d_splice_alias(newInode, direntry);
 }
 
 static int
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 46+ messages in thread

* [RFC][PATCHES] reducing d_add() use, part 3 (procfs)
  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-26  0:03   ` Al Viro
  2018-05-26  0:04     ` [PATCH 1/5] procfs: get rid of ancient BS in pid_revalidate() uses Al Viro
  2018-05-26 13:07     ` [RFC][PATCHES] reducing d_add() use, part 3 (procfs) Alexey Dobriyan
  1 sibling, 2 replies; 46+ messages in thread
From: Al Viro @ 2018-05-26  0:03 UTC (permalink / raw)
  To: linux-fsdevel; +Cc: Alexey Dobriyan

On Sat, May 26, 2018 at 12:53:26AM +0100, Al Viro wrote:
> Continuation of the series - more simple cases:
>       openpromfs: switch to d_splice_alias()
>       orangefs_lookup: simplify
>       omfs_lookup(): report IO errors, use d_splice_alias()
>       hfs: use d_splice_alias()
>       hfs: don't allow mounting over .../rsrc
>       hfsplus: switch to d_splice_alias()
>       ncp_lookup(): use d_splice_alias()
>       9p: unify paths in v9fs_vfs_lookup()
>       cifs_lookup(): cifs_get_inode_...() never returns 0 with *inode left NULL
>       cifs_lookup(): switch to d_splice_alias()

... and a lot more convoluted ones - procfs side.  There we have a bloody
misleading games around pid_revalidate() and friends; we do *NOT* need those
for any kind of race protection anymore (and hadn't for years).  However,
these calls cannot be simply dropped - some of their side effects (namely,
setting ownership and permissions) are needed.  Separating those into
common helpers used by revalidate and by lookups allows to untangle that
mess.

I would really like an ACK/NAK from Alexey on that part...

^ permalink raw reply	[flat|nested] 46+ messages in thread

* [PATCH 1/5] procfs: get rid of ancient BS in pid_revalidate() uses
  2018-05-26  0:03   ` [RFC][PATCHES] reducing d_add() use, part 3 (procfs) Al Viro
@ 2018-05-26  0:04     ` Al Viro
  2018-05-26  0:04       ` [PATCH 2/5] proc_lookupfd_common(): don't bother with instantiate unless the file is open Al Viro
                         ` (4 more replies)
  2018-05-26 13:07     ` [RFC][PATCHES] reducing d_add() use, part 3 (procfs) Alexey Dobriyan
  1 sibling, 5 replies; 46+ messages in thread
From: Al Viro @ 2018-05-26  0:04 UTC (permalink / raw)
  To: linux-fsdevel; +Cc: Alexey Dobriyan

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

^ permalink raw reply related	[flat|nested] 46+ messages in thread

* [PATCH 2/5] proc_lookupfd_common(): don't bother with instantiate unless the file is open
  2018-05-26  0:04     ` [PATCH 1/5] procfs: get rid of ancient BS in pid_revalidate() uses Al Viro
@ 2018-05-26  0:04       ` Al Viro
  2018-05-26  0:04       ` [PATCH 3/5] don't bother with tid_fd_revalidate() in lookups Al Viro
                         ` (3 subsequent siblings)
  4 siblings, 0 replies; 46+ messages in thread
From: Al Viro @ 2018-05-26  0:04 UTC (permalink / raw)
  To: linux-fsdevel; +Cc: Alexey Dobriyan

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

... and take the "check if file is open, pick ->f_mode" into a helper;
tid_fd_revalidate() can use it.

The next patch will get rid of tid_fd_revalidate() calls in instantiate
callbacks.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/proc/fd.c | 63 ++++++++++++++++++++++++++++++++----------------------------
 1 file changed, 34 insertions(+), 29 deletions(-)

diff --git a/fs/proc/fd.c b/fs/proc/fd.c
index 6b80cd1e419a..d38845ecc408 100644
--- a/fs/proc/fd.c
+++ b/fs/proc/fd.c
@@ -81,12 +81,29 @@ static const struct file_operations proc_fdinfo_file_operations = {
 	.release	= single_release,
 };
 
+static bool tid_fd_mode(struct task_struct *task, unsigned fd, fmode_t *mode)
+{
+	struct files_struct *files = get_files_struct(task);
+	struct file *file;
+
+	if (!files)
+		return false;
+
+	rcu_read_lock();
+	file = fcheck_files(files, fd);
+	if (file)
+		*mode = file->f_mode;
+	rcu_read_unlock();
+	put_files_struct(files);
+	return !!file;
+}
+
 static int tid_fd_revalidate(struct dentry *dentry, unsigned int flags)
 {
-	struct files_struct *files;
 	struct task_struct *task;
 	struct inode *inode;
 	unsigned int fd;
+	fmode_t f_mode;
 
 	if (flags & LOOKUP_RCU)
 		return -ECHILD;
@@ -96,35 +113,20 @@ static int tid_fd_revalidate(struct dentry *dentry, unsigned int flags)
 	fd = proc_fd(inode);
 
 	if (task) {
-		files = get_files_struct(task);
-		if (files) {
-			struct file *file;
-
-			rcu_read_lock();
-			file = fcheck_files(files, fd);
-			if (file) {
-				unsigned f_mode = file->f_mode;
-
-				rcu_read_unlock();
-				put_files_struct(files);
-
-				task_dump_owner(task, 0, &inode->i_uid, &inode->i_gid);
-
-				if (S_ISLNK(inode->i_mode)) {
-					unsigned i_mode = S_IFLNK;
-					if (f_mode & FMODE_READ)
-						i_mode |= S_IRUSR | S_IXUSR;
-					if (f_mode & FMODE_WRITE)
-						i_mode |= S_IWUSR | S_IXUSR;
-					inode->i_mode = i_mode;
-				}
-
-				security_task_to_inode(task, inode);
-				put_task_struct(task);
-				return 1;
+		if (tid_fd_mode(task, fd, &f_mode)) {
+			task_dump_owner(task, 0, &inode->i_uid, &inode->i_gid);
+
+			if (S_ISLNK(inode->i_mode)) {
+				unsigned i_mode = S_IFLNK;
+				if (f_mode & FMODE_READ)
+					i_mode |= S_IRUSR | S_IXUSR;
+				if (f_mode & FMODE_WRITE)
+					i_mode |= S_IWUSR | S_IXUSR;
+				inode->i_mode = i_mode;
 			}
-			rcu_read_unlock();
-			put_files_struct(files);
+			security_task_to_inode(task, inode);
+			put_task_struct(task);
+			return 1;
 		}
 		put_task_struct(task);
 	}
@@ -203,11 +205,14 @@ static struct dentry *proc_lookupfd_common(struct inode *dir,
 	struct task_struct *task = get_proc_task(dir);
 	int result = -ENOENT;
 	unsigned fd = name_to_int(&dentry->d_name);
+	fmode_t f_mode;
 
 	if (!task)
 		goto out_no_task;
 	if (fd == ~0U)
 		goto out;
+	if (!tid_fd_mode(task, fd, &f_mode))
+		goto out;
 
 	result = instantiate(dir, dentry, task, (void *)(unsigned long)fd);
 out:
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 46+ messages in thread

* [PATCH 3/5] don't bother with tid_fd_revalidate() in lookups
  2018-05-26  0:04     ` [PATCH 1/5] procfs: get rid of ancient BS in pid_revalidate() uses Al Viro
  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       ` Al Viro
  2018-05-26  0:04       ` [PATCH 4/5] procfs: switch instantiate_t to d_splice_alias() Al Viro
                         ` (2 subsequent siblings)
  4 siblings, 0 replies; 46+ messages in thread
From: Al Viro @ 2018-05-26  0:04 UTC (permalink / raw)
  To: linux-fsdevel; +Cc: Alexey Dobriyan

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

what we want it for is actually updating inode metadata;
take _that_ into a separate helper and use it.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/proc/fd.c | 81 +++++++++++++++++++++++++++++++++---------------------------
 1 file changed, 44 insertions(+), 37 deletions(-)

diff --git a/fs/proc/fd.c b/fs/proc/fd.c
index d38845ecc408..694faeacf42f 100644
--- a/fs/proc/fd.c
+++ b/fs/proc/fd.c
@@ -98,12 +98,27 @@ static bool tid_fd_mode(struct task_struct *task, unsigned fd, fmode_t *mode)
 	return !!file;
 }
 
+static void tid_fd_update_inode(struct task_struct *task, struct inode *inode,
+				fmode_t f_mode)
+{
+	task_dump_owner(task, 0, &inode->i_uid, &inode->i_gid);
+
+	if (S_ISLNK(inode->i_mode)) {
+		unsigned i_mode = S_IFLNK;
+		if (f_mode & FMODE_READ)
+			i_mode |= S_IRUSR | S_IXUSR;
+		if (f_mode & FMODE_WRITE)
+			i_mode |= S_IWUSR | S_IXUSR;
+		inode->i_mode = i_mode;
+	}
+	security_task_to_inode(task, inode);
+}
+
 static int tid_fd_revalidate(struct dentry *dentry, unsigned int flags)
 {
 	struct task_struct *task;
 	struct inode *inode;
 	unsigned int fd;
-	fmode_t f_mode;
 
 	if (flags & LOOKUP_RCU)
 		return -ECHILD;
@@ -113,18 +128,9 @@ static int tid_fd_revalidate(struct dentry *dentry, unsigned int flags)
 	fd = proc_fd(inode);
 
 	if (task) {
+		fmode_t f_mode;
 		if (tid_fd_mode(task, fd, &f_mode)) {
-			task_dump_owner(task, 0, &inode->i_uid, &inode->i_gid);
-
-			if (S_ISLNK(inode->i_mode)) {
-				unsigned i_mode = S_IFLNK;
-				if (f_mode & FMODE_READ)
-					i_mode |= S_IRUSR | S_IXUSR;
-				if (f_mode & FMODE_WRITE)
-					i_mode |= S_IWUSR | S_IXUSR;
-				inode->i_mode = i_mode;
-			}
-			security_task_to_inode(task, inode);
+			tid_fd_update_inode(task, inode, f_mode);
 			put_task_struct(task);
 			return 1;
 		}
@@ -168,34 +174,35 @@ static int proc_fd_link(struct dentry *dentry, struct path *path)
 	return ret;
 }
 
+struct fd_data {
+	fmode_t mode;
+	unsigned fd;
+};
+
 static int
 proc_fd_instantiate(struct inode *dir, struct dentry *dentry,
 		    struct task_struct *task, const void *ptr)
 {
-	unsigned fd = (unsigned long)ptr;
+	const struct fd_data *data = ptr;
 	struct proc_inode *ei;
 	struct inode *inode;
 
 	inode = proc_pid_make_inode(dir->i_sb, task, S_IFLNK);
 	if (!inode)
-		goto out;
+		return -ENOENT;
 
 	ei = PROC_I(inode);
-	ei->fd = fd;
+	ei->fd = data->fd;
 
 	inode->i_op = &proc_pid_link_inode_operations;
 	inode->i_size = 64;
 
 	ei->op.proc_get_link = proc_fd_link;
+	tid_fd_update_inode(task, inode, data->mode);
 
 	d_set_d_op(dentry, &tid_fd_dentry_operations);
 	d_add(dentry, inode);
-
-	/* Close the race of the process dying before we return the dentry */
-	if (tid_fd_revalidate(dentry, 0))
-		return 0;
- out:
-	return -ENOENT;
+	return 0;
 }
 
 static struct dentry *proc_lookupfd_common(struct inode *dir,
@@ -204,17 +211,16 @@ static struct dentry *proc_lookupfd_common(struct inode *dir,
 {
 	struct task_struct *task = get_proc_task(dir);
 	int result = -ENOENT;
-	unsigned fd = name_to_int(&dentry->d_name);
-	fmode_t f_mode;
+	struct fd_data data = {.fd = name_to_int(&dentry->d_name)};
 
 	if (!task)
 		goto out_no_task;
-	if (fd == ~0U)
+	if (data.fd == ~0U)
 		goto out;
-	if (!tid_fd_mode(task, fd, &f_mode))
+	if (!tid_fd_mode(task, data.fd, &data.mode))
 		goto out;
 
-	result = instantiate(dir, dentry, task, (void *)(unsigned long)fd);
+	result = instantiate(dir, dentry, task, &data);
 out:
 	put_task_struct(task);
 out_no_task:
@@ -241,17 +247,22 @@ static int proc_readfd_common(struct file *file, struct dir_context *ctx,
 	for (fd = ctx->pos - 2;
 	     fd < files_fdtable(files)->max_fds;
 	     fd++, ctx->pos++) {
+		struct file *file;
+		struct fd_data data;
 		char name[10 + 1];
 		int len;
 
-		if (!fcheck_files(files, fd))
+		file = fcheck_files(files, fd);
+		if (!file)
 			continue;
+		data.mode = file->f_mode;
 		rcu_read_unlock();
+		data.fd = fd;
 
 		len = snprintf(name, sizeof(name), "%u", fd);
 		if (!proc_fill_cache(file, ctx,
 				     name, len, instantiate, p,
-				     (void *)(unsigned long)fd))
+				     &data))
 			goto out_fd_loop;
 		cond_resched();
 		rcu_read_lock();
@@ -313,27 +324,23 @@ static int
 proc_fdinfo_instantiate(struct inode *dir, struct dentry *dentry,
 			struct task_struct *task, const void *ptr)
 {
-	unsigned fd = (unsigned long)ptr;
+	const struct fd_data *data = ptr;
 	struct proc_inode *ei;
 	struct inode *inode;
 
 	inode = proc_pid_make_inode(dir->i_sb, task, S_IFREG | S_IRUSR);
 	if (!inode)
-		goto out;
+		return -ENOENT;
 
 	ei = PROC_I(inode);
-	ei->fd = fd;
+	ei->fd = data->fd;
 
 	inode->i_fop = &proc_fdinfo_file_operations;
+	tid_fd_update_inode(task, inode, 0);
 
 	d_set_d_op(dentry, &tid_fd_dentry_operations);
 	d_add(dentry, inode);
-
-	/* Close the race of the process dying before we return the dentry */
-	if (tid_fd_revalidate(dentry, 0))
-		return 0;
- out:
-	return -ENOENT;
+	return 0;
 }
 
 static struct dentry *
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 46+ messages in thread

* [PATCH 4/5] procfs: switch instantiate_t to d_splice_alias()
  2018-05-26  0:04     ` [PATCH 1/5] procfs: get rid of ancient BS in pid_revalidate() uses Al Viro
  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       ` 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
  4 siblings, 0 replies; 46+ messages in thread
From: Al Viro @ 2018-05-26  0:04 UTC (permalink / raw)
  To: linux-fsdevel; +Cc: Alexey Dobriyan

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

... and get rid of pointless struct inode *dir argument of those,
while we are at it.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/proc/base.c       | 93 ++++++++++++++++++++++++----------------------------
 fs/proc/fd.c         | 30 ++++++++---------
 fs/proc/internal.h   |  2 +-
 fs/proc/namespaces.c | 19 +++++------
 4 files changed, 65 insertions(+), 79 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 6e0875505898..de22c2002b38 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -1878,8 +1878,8 @@ bool proc_fill_cache(struct file *file, struct dir_context *ctx,
 	struct dentry *child, *dir = file->f_path.dentry;
 	struct qstr qname = QSTR_INIT(name, len);
 	struct inode *inode;
-	unsigned type;
-	ino_t ino;
+	unsigned type = DT_UNKNOWN;
+	ino_t ino = 1;
 
 	child = d_hash_and_lookup(dir, &qname);
 	if (!child) {
@@ -1888,22 +1888,23 @@ bool proc_fill_cache(struct file *file, struct dir_context *ctx,
 		if (IS_ERR(child))
 			goto end_instantiate;
 		if (d_in_lookup(child)) {
-			int err = instantiate(d_inode(dir), child, task, ptr);
+			struct dentry *res;
+			res = instantiate(child, task, ptr);
 			d_lookup_done(child);
-			if (err < 0) {
-				dput(child);
+			if (IS_ERR(res))
 				goto end_instantiate;
+			if (unlikely(res)) {
+				dput(child);
+				child = res;
 			}
 		}
 	}
 	inode = d_inode(child);
 	ino = inode->i_ino;
 	type = inode->i_mode >> 12;
+end_instantiate:
 	dput(child);
 	return dir_emit(ctx, name, len, ino, type);
-
-end_instantiate:
-	return dir_emit(ctx, name, len, 1, DT_UNKNOWN);
 }
 
 /*
@@ -2065,19 +2066,19 @@ static const struct inode_operations proc_map_files_link_inode_operations = {
 	.setattr	= proc_setattr,
 };
 
-static int
-proc_map_files_instantiate(struct inode *dir, struct dentry *dentry,
+static struct dentry *
+proc_map_files_instantiate(struct dentry *dentry,
 			   struct task_struct *task, const void *ptr)
 {
 	fmode_t mode = (fmode_t)(unsigned long)ptr;
 	struct proc_inode *ei;
 	struct inode *inode;
 
-	inode = proc_pid_make_inode(dir->i_sb, task, S_IFLNK |
+	inode = proc_pid_make_inode(dentry->d_sb, task, S_IFLNK |
 				    ((mode & FMODE_READ ) ? S_IRUSR : 0) |
 				    ((mode & FMODE_WRITE) ? S_IWUSR : 0));
 	if (!inode)
-		return -ENOENT;
+		return ERR_PTR(-ENOENT);
 
 	ei = PROC_I(inode);
 	ei->op.proc_get_link = map_files_get_link;
@@ -2086,9 +2087,7 @@ proc_map_files_instantiate(struct inode *dir, struct dentry *dentry,
 	inode->i_size = 64;
 
 	d_set_d_op(dentry, &tid_map_files_dentry_operations);
-	d_add(dentry, inode);
-
-	return 0;
+	return d_splice_alias(inode, dentry);
 }
 
 static struct dentry *proc_map_files_lookup(struct inode *dir,
@@ -2097,19 +2096,19 @@ static struct dentry *proc_map_files_lookup(struct inode *dir,
 	unsigned long vm_start, vm_end;
 	struct vm_area_struct *vma;
 	struct task_struct *task;
-	int result;
+	struct dentry *result;
 	struct mm_struct *mm;
 
-	result = -ENOENT;
+	result = ERR_PTR(-ENOENT);
 	task = get_proc_task(dir);
 	if (!task)
 		goto out;
 
-	result = -EACCES;
+	result = ERR_PTR(-EACCES);
 	if (!ptrace_may_access(task, PTRACE_MODE_READ_FSCREDS))
 		goto out_put_task;
 
-	result = -ENOENT;
+	result = ERR_PTR(-ENOENT);
 	if (dname_to_vma_addr(dentry, &vm_start, &vm_end))
 		goto out_put_task;
 
@@ -2123,7 +2122,7 @@ static struct dentry *proc_map_files_lookup(struct inode *dir,
 		goto out_no_vma;
 
 	if (vma->vm_file)
-		result = proc_map_files_instantiate(dir, dentry, task,
+		result = proc_map_files_instantiate(dentry, task,
 				(void *)(unsigned long)vma->vm_file->f_mode);
 
 out_no_vma:
@@ -2132,7 +2131,7 @@ static struct dentry *proc_map_files_lookup(struct inode *dir,
 out_put_task:
 	put_task_struct(task);
 out:
-	return ERR_PTR(result);
+	return result;
 }
 
 static const struct inode_operations proc_map_files_inode_operations = {
@@ -2433,16 +2432,16 @@ static const struct file_operations proc_pid_set_timerslack_ns_operations = {
 	.release	= single_release,
 };
 
-static int proc_pident_instantiate(struct inode *dir,
-	struct dentry *dentry, struct task_struct *task, const void *ptr)
+static struct dentry *proc_pident_instantiate(struct dentry *dentry,
+	struct task_struct *task, const void *ptr)
 {
 	const struct pid_entry *p = ptr;
 	struct inode *inode;
 	struct proc_inode *ei;
 
-	inode = proc_pid_make_inode(dir->i_sb, task, p->mode);
+	inode = proc_pid_make_inode(dentry->d_sb, task, p->mode);
 	if (!inode)
-		return -ENOENT;
+		return ERR_PTR(-ENOENT);
 
 	ei = PROC_I(inode);
 	if (S_ISDIR(inode->i_mode))
@@ -2454,8 +2453,7 @@ static int proc_pident_instantiate(struct inode *dir,
 	ei->op = p->op;
 	pid_update_inode(task, inode);
 	d_set_d_op(dentry, &pid_dentry_operations);
-	d_add(dentry, inode);
-	return 0;
+	return d_splice_alias(inode, dentry);
 }
 
 static struct dentry *proc_pident_lookup(struct inode *dir, 
@@ -2463,11 +2461,9 @@ static struct dentry *proc_pident_lookup(struct inode *dir,
 					 const struct pid_entry *ents,
 					 unsigned int nents)
 {
-	int error;
 	struct task_struct *task = get_proc_task(dir);
 	const struct pid_entry *p, *last;
-
-	error = -ENOENT;
+	struct dentry *res = ERR_PTR(-ENOENT);
 
 	if (!task)
 		goto out_no_task;
@@ -2486,11 +2482,11 @@ static struct dentry *proc_pident_lookup(struct inode *dir,
 	if (p >= last)
 		goto out;
 
-	error = proc_pident_instantiate(dir, dentry, task, p);
+	res = proc_pident_instantiate(dentry, task, p);
 out:
 	put_task_struct(task);
 out_no_task:
-	return ERR_PTR(error);
+	return res;
 }
 
 static int proc_pident_readdir(struct file *file, struct dir_context *ctx,
@@ -3133,15 +3129,14 @@ void proc_flush_task(struct task_struct *task)
 	}
 }
 
-static int proc_pid_instantiate(struct inode *dir,
-				   struct dentry * dentry,
+static struct dentry *proc_pid_instantiate(struct dentry * dentry,
 				   struct task_struct *task, const void *ptr)
 {
 	struct inode *inode;
 
-	inode = proc_pid_make_inode(dir->i_sb, task, S_IFDIR | S_IRUGO | S_IXUGO);
+	inode = proc_pid_make_inode(dentry->d_sb, task, S_IFDIR | S_IRUGO | S_IXUGO);
 	if (!inode)
-		return -ENOENT;
+		return ERR_PTR(-ENOENT);
 
 	inode->i_op = &proc_tgid_base_inode_operations;
 	inode->i_fop = &proc_tgid_base_operations;
@@ -3151,16 +3146,15 @@ static int proc_pid_instantiate(struct inode *dir,
 	pid_update_inode(task, inode);
 
 	d_set_d_op(dentry, &pid_dentry_operations);
-	d_add(dentry, inode);
-	return 0;
+	return d_splice_alias(inode, dentry);
 }
 
 struct dentry *proc_pid_lookup(struct inode *dir, struct dentry * dentry, unsigned int flags)
 {
-	int result = -ENOENT;
 	struct task_struct *task;
 	unsigned tgid;
 	struct pid_namespace *ns;
+	struct dentry *result = ERR_PTR(-ENOENT);
 
 	tgid = name_to_int(&dentry->d_name);
 	if (tgid == ~0U)
@@ -3175,10 +3169,10 @@ struct dentry *proc_pid_lookup(struct inode *dir, struct dentry * dentry, unsign
 	if (!task)
 		goto out;
 
-	result = proc_pid_instantiate(dir, dentry, task, NULL);
+	result = proc_pid_instantiate(dentry, task, NULL);
 	put_task_struct(task);
 out:
-	return ERR_PTR(result);
+	return result;
 }
 
 /*
@@ -3426,13 +3420,13 @@ static const struct inode_operations proc_tid_base_inode_operations = {
 	.setattr	= proc_setattr,
 };
 
-static int proc_task_instantiate(struct inode *dir,
-	struct dentry *dentry, struct task_struct *task, const void *ptr)
+static struct dentry *proc_task_instantiate(struct dentry *dentry,
+	struct task_struct *task, const void *ptr)
 {
 	struct inode *inode;
-	inode = proc_pid_make_inode(dir->i_sb, task, S_IFDIR | S_IRUGO | S_IXUGO);
+	inode = proc_pid_make_inode(dentry->d_sb, task, S_IFDIR | S_IRUGO | S_IXUGO);
 	if (!inode)
-		return -ENOENT;
+		return ERR_PTR(-ENOENT);
 
 	inode->i_op = &proc_tid_base_inode_operations;
 	inode->i_fop = &proc_tid_base_operations;
@@ -3442,17 +3436,16 @@ static int proc_task_instantiate(struct inode *dir,
 	pid_update_inode(task, inode);
 
 	d_set_d_op(dentry, &pid_dentry_operations);
-	d_add(dentry, inode);
-	return 0;
+	return d_splice_alias(inode, dentry);
 }
 
 static struct dentry *proc_task_lookup(struct inode *dir, struct dentry * dentry, unsigned int flags)
 {
-	int result = -ENOENT;
 	struct task_struct *task;
 	struct task_struct *leader = get_proc_task(dir);
 	unsigned tid;
 	struct pid_namespace *ns;
+	struct dentry *result = ERR_PTR(-ENOENT);
 
 	if (!leader)
 		goto out_no_task;
@@ -3472,13 +3465,13 @@ static struct dentry *proc_task_lookup(struct inode *dir, struct dentry * dentry
 	if (!same_thread_group(leader, task))
 		goto out_drop_task;
 
-	result = proc_task_instantiate(dir, dentry, task, NULL);
+	result = proc_task_instantiate(dentry, task, NULL);
 out_drop_task:
 	put_task_struct(task);
 out:
 	put_task_struct(leader);
 out_no_task:
-	return ERR_PTR(result);
+	return result;
 }
 
 /*
diff --git a/fs/proc/fd.c b/fs/proc/fd.c
index 694faeacf42f..e6a69fd6a2bb 100644
--- a/fs/proc/fd.c
+++ b/fs/proc/fd.c
@@ -179,17 +179,16 @@ struct fd_data {
 	unsigned fd;
 };
 
-static int
-proc_fd_instantiate(struct inode *dir, struct dentry *dentry,
-		    struct task_struct *task, const void *ptr)
+static struct dentry *proc_fd_instantiate(struct dentry *dentry,
+	struct task_struct *task, const void *ptr)
 {
 	const struct fd_data *data = ptr;
 	struct proc_inode *ei;
 	struct inode *inode;
 
-	inode = proc_pid_make_inode(dir->i_sb, task, S_IFLNK);
+	inode = proc_pid_make_inode(dentry->d_sb, task, S_IFLNK);
 	if (!inode)
-		return -ENOENT;
+		return ERR_PTR(-ENOENT);
 
 	ei = PROC_I(inode);
 	ei->fd = data->fd;
@@ -201,8 +200,7 @@ proc_fd_instantiate(struct inode *dir, struct dentry *dentry,
 	tid_fd_update_inode(task, inode, data->mode);
 
 	d_set_d_op(dentry, &tid_fd_dentry_operations);
-	d_add(dentry, inode);
-	return 0;
+	return d_splice_alias(inode, dentry);
 }
 
 static struct dentry *proc_lookupfd_common(struct inode *dir,
@@ -210,8 +208,8 @@ static struct dentry *proc_lookupfd_common(struct inode *dir,
 					   instantiate_t instantiate)
 {
 	struct task_struct *task = get_proc_task(dir);
-	int result = -ENOENT;
 	struct fd_data data = {.fd = name_to_int(&dentry->d_name)};
+	struct dentry *result = ERR_PTR(-ENOENT);
 
 	if (!task)
 		goto out_no_task;
@@ -220,11 +218,11 @@ static struct dentry *proc_lookupfd_common(struct inode *dir,
 	if (!tid_fd_mode(task, data.fd, &data.mode))
 		goto out;
 
-	result = instantiate(dir, dentry, task, &data);
+	result = instantiate(dentry, task, &data);
 out:
 	put_task_struct(task);
 out_no_task:
-	return ERR_PTR(result);
+	return result;
 }
 
 static int proc_readfd_common(struct file *file, struct dir_context *ctx,
@@ -320,17 +318,16 @@ const struct inode_operations proc_fd_inode_operations = {
 	.setattr	= proc_setattr,
 };
 
-static int
-proc_fdinfo_instantiate(struct inode *dir, struct dentry *dentry,
-			struct task_struct *task, const void *ptr)
+static struct dentry *proc_fdinfo_instantiate(struct dentry *dentry,
+	struct task_struct *task, const void *ptr)
 {
 	const struct fd_data *data = ptr;
 	struct proc_inode *ei;
 	struct inode *inode;
 
-	inode = proc_pid_make_inode(dir->i_sb, task, S_IFREG | S_IRUSR);
+	inode = proc_pid_make_inode(dentry->d_sb, task, S_IFREG | S_IRUSR);
 	if (!inode)
-		return -ENOENT;
+		return ERR_PTR(-ENOENT);
 
 	ei = PROC_I(inode);
 	ei->fd = data->fd;
@@ -339,8 +336,7 @@ proc_fdinfo_instantiate(struct inode *dir, struct dentry *dentry,
 	tid_fd_update_inode(task, inode, 0);
 
 	d_set_d_op(dentry, &tid_fd_dentry_operations);
-	d_add(dentry, inode);
-	return 0;
+	return d_splice_alias(inode, dentry);
 }
 
 static struct dentry *
diff --git a/fs/proc/internal.h b/fs/proc/internal.h
index 04a455b9ae69..275b062e58af 100644
--- a/fs/proc/internal.h
+++ b/fs/proc/internal.h
@@ -154,7 +154,7 @@ extern struct dentry *proc_pid_lookup(struct inode *, struct dentry *, unsigned
 extern loff_t mem_lseek(struct file *, loff_t, int);
 
 /* Lookups */
-typedef int instantiate_t(struct inode *, struct dentry *,
+typedef struct dentry *instantiate_t(struct dentry *,
 				     struct task_struct *, const void *);
 extern bool proc_fill_cache(struct file *, struct dir_context *, const char *, int,
 			   instantiate_t, struct task_struct *, const void *);
diff --git a/fs/proc/namespaces.c b/fs/proc/namespaces.c
index ad1adce6541d..dd2b35f78b09 100644
--- a/fs/proc/namespaces.c
+++ b/fs/proc/namespaces.c
@@ -87,16 +87,16 @@ static const struct inode_operations proc_ns_link_inode_operations = {
 	.setattr	= proc_setattr,
 };
 
-static int proc_ns_instantiate(struct inode *dir,
-	struct dentry *dentry, struct task_struct *task, const void *ptr)
+static struct dentry *proc_ns_instantiate(struct dentry *dentry,
+	struct task_struct *task, const void *ptr)
 {
 	const struct proc_ns_operations *ns_ops = ptr;
 	struct inode *inode;
 	struct proc_inode *ei;
 
-	inode = proc_pid_make_inode(dir->i_sb, task, S_IFLNK | S_IRWXUGO);
+	inode = proc_pid_make_inode(dentry->d_sb, task, S_IFLNK | S_IRWXUGO);
 	if (!inode)
-		return -ENOENT;
+		return ERR_PTR(-ENOENT);
 
 	ei = PROC_I(inode);
 	inode->i_op = &proc_ns_link_inode_operations;
@@ -104,8 +104,7 @@ static int proc_ns_instantiate(struct inode *dir,
 	pid_update_inode(task, inode);
 
 	d_set_d_op(dentry, &pid_dentry_operations);
-	d_add(dentry, inode);
-	return 0;
+	return d_splice_alias(inode, dentry);
 }
 
 static int proc_ns_dir_readdir(struct file *file, struct dir_context *ctx)
@@ -144,12 +143,10 @@ const struct file_operations proc_ns_dir_operations = {
 static struct dentry *proc_ns_dir_lookup(struct inode *dir,
 				struct dentry *dentry, unsigned int flags)
 {
-	int error;
 	struct task_struct *task = get_proc_task(dir);
 	const struct proc_ns_operations **entry, **last;
 	unsigned int len = dentry->d_name.len;
-
-	error = -ENOENT;
+	struct dentry *res = ERR_PTR(-ENOENT);
 
 	if (!task)
 		goto out_no_task;
@@ -164,11 +161,11 @@ static struct dentry *proc_ns_dir_lookup(struct inode *dir,
 	if (entry == last)
 		goto out;
 
-	error = proc_ns_instantiate(dir, dentry, task, *entry);
+	res = proc_ns_instantiate(dentry, task, *entry);
 out:
 	put_task_struct(task);
 out_no_task:
-	return ERR_PTR(error);
+	return res;
 }
 
 const struct inode_operations proc_ns_dir_inode_operations = {
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 46+ messages in thread

* [PATCH 5/5] switch the rest of procfs lookups to d_splice_alias()
  2018-05-26  0:04     ` [PATCH 1/5] procfs: get rid of ancient BS in pid_revalidate() uses Al Viro
                         ` (2 preceding siblings ...)
  2018-05-26  0:04       ` [PATCH 4/5] procfs: switch instantiate_t to d_splice_alias() Al Viro
@ 2018-05-26  0:04       ` Al Viro
  2018-05-31  8:28       ` [PATCH 1/5] procfs: get rid of ancient BS in pid_revalidate() uses Alexey Dobriyan
  4 siblings, 0 replies; 46+ messages in thread
From: Al Viro @ 2018-05-26  0:04 UTC (permalink / raw)
  To: linux-fsdevel; +Cc: Alexey Dobriyan

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

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/proc/generic.c     |  3 +--
 fs/proc/proc_sysctl.c | 15 ++++++++++++---
 2 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/fs/proc/generic.c b/fs/proc/generic.c
index 2078e70e1595..b77034a694ef 100644
--- a/fs/proc/generic.c
+++ b/fs/proc/generic.c
@@ -256,8 +256,7 @@ struct dentry *proc_lookup_de(struct inode *dir, struct dentry *dentry,
 		if (!inode)
 			return ERR_PTR(-ENOMEM);
 		d_set_d_op(dentry, &proc_misc_dentry_ops);
-		d_add(dentry, inode);
-		return NULL;
+		return d_splice_alias(inode, dentry);
 	}
 	read_unlock(&proc_subdir_lock);
 	return ERR_PTR(-ENOENT);
diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
index 8989936f2995..4d765e5e91ed 100644
--- a/fs/proc/proc_sysctl.c
+++ b/fs/proc/proc_sysctl.c
@@ -554,9 +554,8 @@ static struct dentry *proc_sys_lookup(struct inode *dir, struct dentry *dentry,
 	if (!inode)
 		goto out;
 
-	err = NULL;
 	d_set_d_op(dentry, &proc_sys_dentry_operations);
-	d_add(dentry, inode);
+	err = d_splice_alias(inode, dentry);
 
 out:
 	if (h)
@@ -684,6 +683,7 @@ static bool proc_sys_fill_cache(struct file *file,
 		if (IS_ERR(child))
 			return false;
 		if (d_in_lookup(child)) {
+			struct dentry *res;
 			inode = proc_sys_make_inode(dir->d_sb, head, table);
 			if (!inode) {
 				d_lookup_done(child);
@@ -691,7 +691,16 @@ static bool proc_sys_fill_cache(struct file *file,
 				return false;
 			}
 			d_set_d_op(child, &proc_sys_dentry_operations);
-			d_add(child, inode);
+			res = d_splice_alias(inode, child);
+			d_lookup_done(child);
+			if (unlikely(res)) {
+				if (IS_ERR(res)) {
+					dput(child);
+					return false;
+				}
+				dput(child);
+				child = res;
+			}
 		}
 	}
 	inode = d_inode(child);
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 46+ messages in thread

* Re: [RFC][PATCHES] reducing d_add() use, part 3 (procfs)
  2018-05-26  0:03   ` [RFC][PATCHES] reducing d_add() use, part 3 (procfs) Al Viro
  2018-05-26  0:04     ` [PATCH 1/5] procfs: get rid of ancient BS in pid_revalidate() uses Al Viro
@ 2018-05-26 13:07     ` Alexey Dobriyan
  2018-05-26 13:56       ` Alexey Dobriyan
  1 sibling, 1 reply; 46+ messages in thread
From: Alexey Dobriyan @ 2018-05-26 13:07 UTC (permalink / raw)
  To: Al Viro; +Cc: linux-fsdevel

On Sat, May 26, 2018 at 01:03:02AM +0100, Al Viro wrote:
> On Sat, May 26, 2018 at 12:53:26AM +0100, Al Viro wrote:

> ... and a lot more convoluted ones - procfs side.
> I would really like an ACK/NAK from Alexey on that part...

Is /proc part self contained? This happens with 5 patches against mainline:

Welcome to Debian GNU/Linux 8 (jessie)!

[    0.764706] systemd[1]: Failed to insert module 'autofs4'
[    0.765357] systemd[1]: Failed to insert module 'ipv6'
[    0.766047] systemd[1]: Set hostname to <debian88>.
[    0.778266] random: systemd-sysv-ge: uninitialized urandom read (16 bytes read)
[    0.785611] random: systemd: uninitialized urandom read (16 bytes read)
[    0.790655] ------------[ cut here ]------------

			[nothing here]

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [RFC][PATCHES] reducing d_add() use, part 3 (procfs)
  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
  0 siblings, 1 reply; 46+ messages in thread
From: Alexey Dobriyan @ 2018-05-26 13:56 UTC (permalink / raw)
  To: Al Viro; +Cc: linux-fsdevel

On Sat, May 26, 2018 at 04:07:26PM +0300, Alexey Dobriyan wrote:
> On Sat, May 26, 2018 at 01:03:02AM +0100, Al Viro wrote:
> > On Sat, May 26, 2018 at 12:53:26AM +0100, Al Viro wrote:
> 
> > ... and a lot more convoluted ones - procfs side.
> > I would really like an ACK/NAK from Alexey on that part...
> 
> Is /proc part self contained? This happens with 5 patches against mainline:
> 
> Welcome to Debian GNU/Linux 8 (jessie)!
> 
> [    0.764706] systemd[1]: Failed to insert module 'autofs4'
> [    0.765357] systemd[1]: Failed to insert module 'ipv6'
> [    0.766047] systemd[1]: Set hostname to <debian88>.
> [    0.778266] random: systemd-sysv-ge: uninitialized urandom read (16 bytes read)
> [    0.785611] random: systemd: uninitialized urandom read (16 bytes read)
> [    0.790655] ------------[ cut here ]------------

Regardless, the broken patch is
"[PATCH 3/5] don't bother with tid_fd_revalidate() in lookups".

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [RFC][PATCHES] reducing d_add() use, part 3 (procfs)
  2018-05-26 13:56       ` Alexey Dobriyan
@ 2018-05-26 18:20         ` Al Viro
  2018-05-26 18:38           ` Matthew Wilcox
  0 siblings, 1 reply; 46+ messages in thread
From: Al Viro @ 2018-05-26 18:20 UTC (permalink / raw)
  To: Alexey Dobriyan; +Cc: linux-fsdevel

On Sat, May 26, 2018 at 04:56:40PM +0300, Alexey Dobriyan wrote:
> On Sat, May 26, 2018 at 04:07:26PM +0300, Alexey Dobriyan wrote:
> > On Sat, May 26, 2018 at 01:03:02AM +0100, Al Viro wrote:
> > > On Sat, May 26, 2018 at 12:53:26AM +0100, Al Viro wrote:
> > 
> > > ... and a lot more convoluted ones - procfs side.
> > > I would really like an ACK/NAK from Alexey on that part...
> > 
> > Is /proc part self contained? This happens with 5 patches against mainline:
> > 
> > Welcome to Debian GNU/Linux 8 (jessie)!
> > 
> > [    0.764706] systemd[1]: Failed to insert module 'autofs4'
> > [    0.765357] systemd[1]: Failed to insert module 'ipv6'
> > [    0.766047] systemd[1]: Set hostname to <debian88>.
> > [    0.778266] random: systemd-sysv-ge: uninitialized urandom read (16 bytes read)
> > [    0.785611] random: systemd: uninitialized urandom read (16 bytes read)
> > [    0.790655] ------------[ cut here ]------------
> 
> Regardless, the broken patch is
> "[PATCH 3/5] don't bother with tid_fd_revalidate() in lookups".

D'oh...  Incremental would be

diff --git a/fs/proc/fd.c b/fs/proc/fd.c
index 694faeacf42f..f5de22a9e9e0 100644
--- a/fs/proc/fd.c
+++ b/fs/proc/fd.c
@@ -247,15 +247,15 @@ static int proc_readfd_common(struct file *file, struct dir_context *ctx,
 	for (fd = ctx->pos - 2;
 	     fd < files_fdtable(files)->max_fds;
 	     fd++, ctx->pos++) {
-		struct file *file;
+		struct file *f;
 		struct fd_data data;
 		char name[10 + 1];
 		int len;
 
-		file = fcheck_files(files, fd);
-		if (!file)
+		f = fcheck_files(files, fd);
+		if (!f)
 			continue;
-		data.mode = file->f_mode;
+		data.mode = f->f_mode;
 		rcu_read_unlock();
 		data.fd = fd;
 

^ permalink raw reply related	[flat|nested] 46+ messages in thread

* Re: [RFC][PATCHES] reducing d_add() use, part 3 (procfs)
  2018-05-26 18:20         ` Al Viro
@ 2018-05-26 18:38           ` Matthew Wilcox
  0 siblings, 0 replies; 46+ messages in thread
From: Matthew Wilcox @ 2018-05-26 18:38 UTC (permalink / raw)
  To: Al Viro; +Cc: Alexey Dobriyan, linux-fsdevel

On Sat, May 26, 2018 at 07:20:21PM +0100, Al Viro wrote:
> D'oh...  Incremental would be
> 
> diff --git a/fs/proc/fd.c b/fs/proc/fd.c
> index 694faeacf42f..f5de22a9e9e0 100644
> --- a/fs/proc/fd.c
> +++ b/fs/proc/fd.c
> @@ -247,15 +247,15 @@ static int proc_readfd_common(struct file *file, struct dir_context *ctx,
>  	for (fd = ctx->pos - 2;
>  	     fd < files_fdtable(files)->max_fds;
>  	     fd++, ctx->pos++) {
> -		struct file *file;
> +		struct file *f;

Ugh.  -Wshadow doesn't get turned on until W=2 which is really damn noisy.
It's going to take a _lot_ of work to move that over into W=1 or even
set by default.

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH 1/5] procfs: get rid of ancient BS in pid_revalidate() uses
  2018-05-26  0:04     ` [PATCH 1/5] procfs: get rid of ancient BS in pid_revalidate() uses Al Viro
                         ` (3 preceding siblings ...)
  2018-05-26  0:04       ` [PATCH 5/5] switch the rest of procfs lookups " Al Viro
@ 2018-05-31  8:28       ` Alexey Dobriyan
  4 siblings, 0 replies; 46+ messages in thread
From: Alexey Dobriyan @ 2018-05-31  8:28 UTC (permalink / raw)
  To: Al Viro; +Cc: linux-fsdevel

On Sat, May 26, 2018 at 01:04:06AM +0100, Al Viro wrote:
> +	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;
>  }

ACK this. I never understood what race is prevented if process can
exit after pid_revalidate().

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH 02/10] orangefs_lookup: simplify
  2018-05-25 23:54     ` [PATCH 02/10] orangefs_lookup: simplify Al Viro
@ 2018-05-31 18:23       ` Mike Marshall
  0 siblings, 0 replies; 46+ messages in thread
From: Mike Marshall @ 2018-05-31 18:23 UTC (permalink / raw)
  To: Al Viro; +Cc: linux-fsdevel

Thanks for the cleanup. This runs through xfstests with no regressions
on 4.17-rc7.

Please add: Tested-by: Mike Marshall <hubcap@omnibond.com>

-Mike

On Fri, May 25, 2018 at 7:54 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> From: Al Viro <viro@zeniv.linux.org.uk>
>
> d_splice_alias() can handle NULL and ERR_PTR() for inode just fine...
>
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---
>  fs/orangefs/namei.c | 64 +++++++----------------------------------------------
>  1 file changed, 8 insertions(+), 56 deletions(-)
>
> diff --git a/fs/orangefs/namei.c b/fs/orangefs/namei.c
> index 1b5707c44c3f..365cd73d9109 100644
> --- a/fs/orangefs/namei.c
> +++ b/fs/orangefs/namei.c
> @@ -110,7 +110,6 @@ static struct dentry *orangefs_lookup(struct inode *dir, struct dentry *dentry,
>         struct orangefs_inode_s *parent = ORANGEFS_I(dir);
>         struct orangefs_kernel_op_s *new_op;
>         struct inode *inode;
> -       struct dentry *res;
>         int ret = -EINVAL;
>
>         /*
> @@ -158,65 +157,18 @@ static struct dentry *orangefs_lookup(struct inode *dir, struct dentry *dentry,
>                      new_op->downcall.resp.lookup.refn.fs_id,
>                      ret);
>
> -       if (ret < 0) {
> -               if (ret == -ENOENT) {
> -                       /*
> -                        * if no inode was found, add a negative dentry to
> -                        * dcache anyway; if we don't, we don't hold expected
> -                        * lookup semantics and we most noticeably break
> -                        * during directory renames.
> -                        *
> -                        * however, if the operation failed or exited, do not
> -                        * add the dentry (e.g. in the case that a touch is
> -                        * issued on a file that already exists that was
> -                        * interrupted during this lookup -- no need to add
> -                        * another negative dentry for an existing file)
> -                        */
> -
> -                       gossip_debug(GOSSIP_NAME_DEBUG,
> -                                    "orangefs_lookup: Adding *negative* dentry "
> -                                    "%p for %pd\n",
> -                                    dentry,
> -                                    dentry);
> -
> -                       d_add(dentry, NULL);
> -                       res = NULL;
> -                       goto out;
> -               }
> -
> +       if (ret >= 0) {
> +               orangefs_set_timeout(dentry);
> +               inode = orangefs_iget(dir->i_sb, &new_op->downcall.resp.lookup.refn);
> +       } else if (ret == -ENOENT) {
> +               inode = NULL;
> +       } else {
>                 /* must be a non-recoverable error */
> -               res = ERR_PTR(ret);
> -               goto out;
> -       }
> -
> -       orangefs_set_timeout(dentry);
> -
> -       inode = orangefs_iget(dir->i_sb, &new_op->downcall.resp.lookup.refn);
> -       if (IS_ERR(inode)) {
> -               gossip_debug(GOSSIP_NAME_DEBUG,
> -                       "error %ld from iget\n", PTR_ERR(inode));
> -               res = ERR_CAST(inode);
> -               goto out;
> +               inode = ERR_PTR(ret);
>         }
>
> -       gossip_debug(GOSSIP_NAME_DEBUG,
> -                    "%s:%s:%d "
> -                    "Found good inode [%lu] with count [%d]\n",
> -                    __FILE__,
> -                    __func__,
> -                    __LINE__,
> -                    inode->i_ino,
> -                    (int)atomic_read(&inode->i_count));
> -
> -       /* update dentry/inode pair into dcache */
> -       res = d_splice_alias(inode, dentry);
> -
> -       gossip_debug(GOSSIP_NAME_DEBUG,
> -                    "Lookup success (inode ct = %d)\n",
> -                    (int)atomic_read(&inode->i_count));
> -out:
>         op_release(new_op);
> -       return res;
> +       return d_splice_alias(inode, dentry);
>  }
>
>  /* return 0 on success; non-zero otherwise */
> --
> 2.11.0
>

^ permalink raw reply	[flat|nested] 46+ messages in thread

end of thread, other threads:[~2018-05-31 18:23 UTC | newest]

Thread overview: 46+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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     ` [PATCH 1/5] procfs: get rid of ancient BS in pid_revalidate() uses Al Viro
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

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