Skip to contentSkip to conversation

proc: avoid extra pde_put() in proc_fill_super()

PATCH In review Maxim Patlasov 2 participants 3 messages Jan 22, 2013 – Jan 24, 2013 Proc filesystem

lore
Share & tools
lore

Maxim Patlasov

author
#0 lore raw

PATCH 1/1 proc: avoid extra pde_put() in proc_fill_super()

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 <[email protected]>
---
 fs/proc/inode.c |   21 +++++++++++++++------
 1 files changed, 15 insertions(+), 6 deletions(-)
fs/proc/inode.c +15 -6
index 439ae688..c7f8b24 100644
@@ -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;
 }
#1 lore raw
Quoted 3 lines · 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().

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 <[email protected]>

---

fs/proc/generic.c +9 -16
index 76ddae8..8b487e5 100644
@@ -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,
fs/proc/inode.c +1 -4
index 439ae688..e2bbe20 100644
@@ -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;
 }
author
#2 lore raw

Hi Al,

Quoted 9 lines · Al Viro
Quoted 3 lines · 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().

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 <[email protected]>
Signed-off-by Maxim Patlasov <[email protected]>

---

fs/proc/generic.c +9 -16
index 76ddae8..8b487e5 100644
@@ -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,
fs/proc/inode.c +1 -4
index c7f8b24..99a9eb7 100644
@@ -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;
 	}