[PATCH] proc: avoid extra pde_put() in proc_fill_super()

[PATCH] proc: avoid extra pde_put() in proc_fill_super()

[Maxim Patlasov]

If proc_get_inode() succeeded, but d_make_root() failed, pde_put() for
proc_root will be called twice: the first time due to iput() called from
d_make_root() and the second time directly in the end of proc_fill_super().

Signed-off-by: Maxim Patlasov <mpatlasov@parallels.com>
---
 fs/proc/inode.c |   21 +++++++++++++++------
 1 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/fs/proc/inode.c b/fs/proc/inode.c
index 439ae688..c7f8b24 100644
--- a/fs/proc/inode.c
+++ b/fs/proc/inode.c
@@ -486,6 +486,8 @@ struct inode *proc_get_inode(struct super_block *sb, struct proc_dir_entry *de)
 
 int proc_fill_super(struct super_block *s)
 {
+	struct inode *root_inode;
+
 	s->s_flags |= MS_NODIRATIME | MS_NOSUID | MS_NOEXEC;
 	s->s_blocksize = 1024;
 	s->s_blocksize_bits = 10;
@@ -494,11 +496,18 @@ int proc_fill_super(struct super_block *s)
 	s->s_time_gran = 1;
 	
 	pde_get(&proc_root);
-	s->s_root = d_make_root(proc_get_inode(s, &proc_root));
-	if (s->s_root)
-		return 0;
+	root_inode = proc_get_inode(s, &proc_root);
+	if (!root_inode) {
+		printk(KERN_ERR "proc_fill_super: get root inode failed\n");
+		pde_put(&proc_root);
+		return -ENOMEM;
+	}
 
-	printk("proc_read_super: get root inode failed\n");
-	pde_put(&proc_root);
-	return -ENOMEM;
+	s->s_root = d_make_root(root_inode);
+	if (!s->s_root) {
+		printk(KERN_ERR "proc_fill_super: allocate dentry failed\n");
+		return -ENOMEM;
+	}
+
+	return 0;
 }

Re: [PATCH] proc: avoid extra pde_put() in proc_fill_super()

[Al Viro]

On Tue, Jan 22, 2013 at 09:03:26PM +0400, Maxim Patlasov wrote:
> If proc_get_inode() succeeded, but d_make_root() failed, pde_put() for
> proc_root will be called twice: the first time due to iput() called from
> d_make_root() and the second time directly in the end of proc_fill_super().

I don't like that solution, TBH.  How about making proc_get_inode() call
pde_put() on failure instead?  Note that it already does so if inode had
been found in icache, so this will just make the refcounting consistent.
Completely untested patch follows:

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
diff --git a/fs/proc/generic.c b/fs/proc/generic.c
index 76ddae8..8b487e5 100644
--- a/fs/proc/generic.c
+++ b/fs/proc/generic.c
@@ -413,31 +413,24 @@ struct dentry *proc_lookup_de(struct proc_dir_entry *de, struct inode *dir,
 		struct dentry *dentry)
 {
 	struct inode *inode = NULL;
-	int error = -ENOENT;
 
 	spin_lock(&proc_subdir_lock);
 	for (de = de->subdir; de ; de = de->next) {
 		if (de->namelen != dentry->d_name.len)
 			continue;
-		if (!memcmp(dentry->d_name.name, de->name, de->namelen)) {
-			pde_get(de);
-			spin_unlock(&proc_subdir_lock);
-			error = -ENOMEM;
-			inode = proc_get_inode(dir->i_sb, de);
-			goto out_unlock;
-		}
-	}
-	spin_unlock(&proc_subdir_lock);
-out_unlock:
-
-	if (inode) {
+		if (memcmp(dentry->d_name.name, de->name, de->namelen))
+			continue;
+		pde_get(de);
+		spin_unlock(&proc_subdir_lock);
+		inode = proc_get_inode(dir->i_sb, de);
+		if (!inode)
+			return ERR_PTR(-ENOMEM);
 		d_set_d_op(dentry, &proc_dentry_operations);
 		d_add(dentry, inode);
 		return NULL;
 	}
-	if (de)
-		pde_put(de);
-	return ERR_PTR(error);
+	spin_unlock(&proc_subdir_lock);
+	return ERR_PTR(-ENOENT);
 }
 
 struct dentry *proc_lookup(struct inode *dir, struct dentry *dentry,
diff --git a/fs/proc/inode.c b/fs/proc/inode.c
index 439ae688..e2bbe20 100644
--- a/fs/proc/inode.c
+++ b/fs/proc/inode.c
@@ -448,9 +448,7 @@ struct inode *proc_get_inode(struct super_block *sb, struct proc_dir_entry *de)
 	struct inode * inode;
 
 	inode = iget_locked(sb, de->low_ino);
-	if (!inode)
-		return NULL;
-	if (inode->i_state & I_NEW) {
+	if (inode && inode->i_state & I_NEW) {
 		inode->i_mtime = inode->i_atime = inode->i_ctime = CURRENT_TIME;
 		PROC_I(inode)->pde = de;
 
@@ -499,6 +497,5 @@ int proc_fill_super(struct super_block *s)
 		return 0;
 
 	printk("proc_read_super: get root inode failed\n");
-	pde_put(&proc_root);
 	return -ENOMEM;
 }

Re: [PATCH] proc: avoid extra pde_put() in proc_fill_super()

[Maxim Patlasov]

Hi Al,

01/23/2013 08:17 PM, Al Viro wrote:
> On Tue, Jan 22, 2013 at 09:03:26PM +0400, Maxim Patlasov wrote:
>> If proc_get_inode() succeeded, but d_make_root() failed, pde_put() for
>> proc_root will be called twice: the first time due to iput() called from
>> d_make_root() and the second time directly in the end of proc_fill_super().
>
> I don't like that solution, TBH.  How about making proc_get_inode() call
> pde_put() on failure instead?  Note that it already does so if inode had
> been found in icache, so this will just make the refcounting consistent.
> Completely untested patch follows:

The patch looks correct and its idea is very reasonable. I agree, it makes
refcounting consistent. Also, proc_lookup_de() is simplified. But your patch
misses a few (minor) bits implemented in mine:

 * add KERN_ERR to printk
 * s/proc_read_super/proc_fill_super in message string(s)
 * avoid misleading user ("get root inode failed") if getting inode succeeded,
   but allocating dentry failed.

The thing I don't like in your patch is mixing one-line fix (removal pde_put()
from proc_fill_super()) with general cleanup of proc_get_inode() and friends.
I'd suggest to apply your cleanup patch on top of my bugfix patch. Below is
your patch (essentially unmodified, excepting very last hunk) in the form
to be applied on top of my patch.

Thanks,
Maxim

Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Maxim Patlasov <mpatlasov@parallels.com>
---
diff --git a/fs/proc/generic.c b/fs/proc/generic.c
index 76ddae8..8b487e5 100644
--- a/fs/proc/generic.c
+++ b/fs/proc/generic.c
@@ -413,31 +413,24 @@ struct dentry *proc_lookup_de(struct proc_dir_entry *de, struct inode *dir,
 		struct dentry *dentry)
 {
 	struct inode *inode = NULL;
-	int error = -ENOENT;
 
 	spin_lock(&proc_subdir_lock);
 	for (de = de->subdir; de ; de = de->next) {
 		if (de->namelen != dentry->d_name.len)
 			continue;
-		if (!memcmp(dentry->d_name.name, de->name, de->namelen)) {
-			pde_get(de);
-			spin_unlock(&proc_subdir_lock);
-			error = -ENOMEM;
-			inode = proc_get_inode(dir->i_sb, de);
-			goto out_unlock;
-		}
-	}
-	spin_unlock(&proc_subdir_lock);
-out_unlock:
-
-	if (inode) {
+		if (memcmp(dentry->d_name.name, de->name, de->namelen))
+			continue;
+		pde_get(de);
+		spin_unlock(&proc_subdir_lock);
+		inode = proc_get_inode(dir->i_sb, de);
+		if (!inode)
+			return ERR_PTR(-ENOMEM);
 		d_set_d_op(dentry, &proc_dentry_operations);
 		d_add(dentry, inode);
 		return NULL;
 	}
-	if (de)
-		pde_put(de);
-	return ERR_PTR(error);
+	spin_unlock(&proc_subdir_lock);
+	return ERR_PTR(-ENOENT);
 }
 
 struct dentry *proc_lookup(struct inode *dir, struct dentry *dentry,
diff --git a/fs/proc/inode.c b/fs/proc/inode.c
index c7f8b24..99a9eb7 100644
--- a/fs/proc/inode.c
+++ b/fs/proc/inode.c
@@ -448,9 +448,7 @@ struct inode *proc_get_inode(struct super_block *sb, struct proc_dir_entry *de)
 	struct inode * inode;
 
 	inode = iget_locked(sb, de->low_ino);
-	if (!inode)
-		return NULL;
-	if (inode->i_state & I_NEW) {
+	if (inode && inode->i_state & I_NEW) {
 		inode->i_mtime = inode->i_atime = inode->i_ctime = CURRENT_TIME;
 		PROC_I(inode)->pde = de;
 
@@ -499,7 +497,6 @@ int proc_fill_super(struct super_block *s)
 	root_inode = proc_get_inode(s, &proc_root);
 	if (!root_inode) {
 		printk(KERN_ERR "proc_fill_super: get root inode failed\n");
-		pde_put(&proc_root);
 		return -ENOMEM;
 	}