On Sat, Aug 25, 2012 at 08:12:18PM +0100, Al Viro wrote:

On Sat, Aug 25, 2012 at 10:58:29PM +0400, Cyrill Gorcunov wrote:

quoted

On Sat, Aug 25, 2012 at 06:55:04PM +0100, Al Viro wrote:

quoted

quoted

Well, this could be simplified indeed, if I understand you correctly
you propose just save f_mode in flexible array and use it instead
of struct file, right? (which will require to rewrite code a bit)

Yes.  FWIW, proc_fill_cache() is really atrocious ;-/  Not to mention

OK, thanks. I'm putting this cleanup task in my big todo list. Hope I'll
manage on the next week with it.

quoted

anything else, if we ever get a negative dentry there, we have a dentry
leak.  I don't think it's possible in practice, but...  Furthermore,

could you please elaborate, you mean this string?

I mean that if we get to that if (... || !child->d_inode) and end up
evaluating the last part at all, we have acquired a reference to that
struct dentry.  And if that last part ends up being true (i.e. if it's
a negative dentry), we'll return from function without having dropped
the reference we'd acquired.

Would the patch below improve the code? Look, I've not dropped
find_inode_number call since it's a bit unclear for me what
would happen if !child case hit

	child = d_lookup(dir, &qname);
	if (!child) {
		struct dentry *new = d_alloc(dir, &qname);
		if (new) {
			child = instantiate(dir->d_inode, new, task, ptr);
			if (child)
				dput(new);
			else
				child = new;
		}
	}

can we be sure that i_ino won't be zero here?
---
 fs/proc/base.c |   27 +++++++++++++--------------
 1 file changed, 13 insertions(+), 14 deletions(-)

Index: linux-2.6.git/fs/proc/base.c
===================================================================

--- linux-2.6.git.orig/fs/proc/base.c
+++ linux-2.6.git/fs/proc/base.c

@@ -1650,7 +1650,6 @@ int proc_fill_cache(struct file *filp, v
 	instantiate_t instantiate, struct task_struct *task, const void *ptr)
 {
 	struct dentry *child, *dir = filp->f_path.dentry;
-	struct inode *inode;
 	struct qstr qname;
 	ino_t ino = 0;
 	unsigned type = DT_UNKNOWN;

@@ -1661,8 +1660,7 @@ int proc_fill_cache(struct file *filp, v
 
 	child = d_lookup(dir, &qname);
 	if (!child) {
-		struct dentry *new;
-		new = d_alloc(dir, &qname);
+		struct dentry *new = d_alloc(dir, &qname);
 		if (new) {
 			child = instantiate(dir->d_inode, new, task, ptr);
 			if (child)

@@ -1671,19 +1669,20 @@ int proc_fill_cache(struct file *filp, v
 				child = new;
 		}
 	}
-	if (!child || IS_ERR(child) || !child->d_inode)
-		goto end_instantiate;
-	inode = child->d_inode;
-	if (inode) {
-		ino = inode->i_ino;
-		type = inode->i_mode >> 12;
-	}
+	if (IS_ERR_OR_NULL(child))
+		goto err;
+	if (!child->d_inode)
+		goto err_put;
+	ino = child->d_inode->i_ino;
+	type = child->d_inode->i_mode >> 12;
+err_put:
 	dput(child);
-end_instantiate:
-	if (!ino)
+err:
+	if (!ino) {
 		ino = find_inode_number(dir, &qname);
-	if (!ino)
-		ino = 1;
+		if (!ino)
+			ino = 1;
+	}
 	return filldir(dirent, name, len, filp->f_pos, ino, type);
 }