[patch] vfs: fix lookup on deleted directory

[patch] vfs: fix lookup on deleted directory

[Miklos Szeredi]

From: Miklos Szeredi <mszeredi@suse.cz>

Lookup can install a child dentry for a deleted directory.  This keeps
the directory dentry alive, and the inode pinned in the cache and on
disk, even after all external references have gone away.

This isn't a big problem normally, since memory pressure or umount
will clear out the directory dentry and its children, releasing the
inode.  But for UBIFS this causes problems because its orphan area can
overflow.

Fix this by returning ENOENT for all lookups on a S_DEAD directory
before creating a child dentry.

Thanks to Zoltan Sogor for noticing this while testing UBIFS, and
Artem for the excellent analysis of the problem and testing.

Reported-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
Tested-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
---
 fs/namei.c |   19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

Index: linux-2.6/fs/namei.c
===================================================================
--- linux-2.6.orig/fs/namei.c	2008-07-02 17:25:55.000000000 +0200
+++ linux-2.6/fs/namei.c	2008-07-02 17:25:57.000000000 +0200
@@ -519,7 +519,14 @@ static struct dentry * real_lookup(struc
 	 */
 	result = d_lookup(parent, name);
 	if (!result) {
-		struct dentry * dentry = d_alloc(parent, name);
+		struct dentry *dentry;
+
+		/* Don't create child dentry for a dead directory. */
+		result = ERR_PTR(-ENOENT);
+		if (IS_DEADDIR(dir))
+			goto out_unlock;
+
+		dentry = d_alloc(parent, name);
 		result = ERR_PTR(-ENOMEM);
 		if (dentry) {
 			result = dir->i_op->lookup(dir, dentry, nd);
@@ -528,6 +535,7 @@ static struct dentry * real_lookup(struc
 			else
 				result = dentry;
 		}
+out_unlock:
 		mutex_unlock(&dir->i_mutex);
 		return result;
 	}
@@ -1317,7 +1325,14 @@ static struct dentry *__lookup_hash(stru
 
 	dentry = cached_lookup(base, name, nd);
 	if (!dentry) {
-		struct dentry *new = d_alloc(base, name);
+		struct dentry *new;
+
+		/* Don't create child dentry for a dead directory. */
+		dentry = ERR_PTR(-ENOENT);
+		if (IS_DEADDIR(inode))
+			goto out;
+
+		new = d_alloc(base, name);
 		dentry = ERR_PTR(-ENOMEM);
 		if (!new)
 			goto out;

Re: [patch] vfs: fix lookup on deleted directory

[Al Viro]

On Wed, Jul 02, 2008 at 09:30:15PM +0200, Miklos Szeredi wrote:

> Lookup can install a child dentry for a deleted directory.  This keeps
> the directory dentry alive, and the inode pinned in the cache and on
> disk, even after all external references have gone away.
> 
> This isn't a big problem normally, since memory pressure or umount
> will clear out the directory dentry and its children, releasing the
> inode.  But for UBIFS this causes problems because its orphan area can
> overflow.
> 
> Fix this by returning ENOENT for all lookups on a S_DEAD directory
> before creating a child dentry.

I wonder if it would be better to do that in a slightly different way:
if ->lookup() returns negative dentry, check if parent is dead and drop
the sucker if it is.  OTOH, "no ->lookup() will be ever called for
dead ones" seems to be a nice property and we are on sufficiently slow
path to not worry about the cost of check-and-branch-not-taken, so I'll
probably drop my variant and pick yours instead.

Re: [patch] vfs: fix lookup on deleted directory

[Miklos Szeredi]

Al,

On Wed, 2 Jul 2008, Al Viro wrote:
> I wonder if it would be better to do that in a slightly different way:
> if ->lookup() returns negative dentry, check if parent is dead and drop
> the sucker if it is.

Either is fine by me.

OT: could we have some feedback about patches I submitted during the
last couple of weeks?  They aren't terribly complex, so really I don't
think it would take more than 10 minutes of your time to review and
ack/nack them.

But even just saying that you're busy and will look at them at
whatever time would be better than the complete silence that usually
follows patch submissions.

Also there's the unprivileged mounts patchset, that has been waiting
for the last year or so.  That's a bit more complex stuff, but it's
not going to get better by sitting around in -mm.  Is there a
possibility of that getting reviewed for 2.6.27?

Thanks,
Miklos