Re: [PATCH 1/1] Null Pointer when make_indexed_dir returns -ENOSPC

[Jan Kara <jack@suse.cz>]

[-- Attachment #1: Type: text/plain, Size: 1018 bytes --]

On Mon 09-05-11 10:22:37, Ted Tso wrote:
> On Mon, May 09, 2011 at 04:05:37PM +0200, Jan Kara wrote:
> >   Yes. ext4_append() can return ENOSPC and passed bh will get set to NULL
> > without being marked dirty.
> 
> Ah, so the right fix then is to add to make the cleanup code like this:
> 
> 		ext4_mark_inode_dirty(handle, dir);
> 		ext4_handle_dirty_metadata(handle, dir, frame->bh);
> +	        ext4_handle_dirty_metadata(handle, dir, bh2);
> +		if (bh)
> +			ext4_handle_dirty_metadata(handle, dir, bh);
> 		dx_release(frames);
> 		return retval;
> 
> Agreed?
  Not quite. make_indexed_dir() does frame->bh = bh and bh = bh2 before
calling do_split(). So bh2 is not really carrying a valid buffer reference
at this point - even more so because do_split() does brelse() on the passed
bh so it need not be around when are at this point. The code is a real
mess. But for example attached patch will work because both callers of
do_split() do brelse() anyway.

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

[-- Attachment #2: 0001-ext4-Stop-releasing-passed-bh-in-do_split.patch --]
[-- Type: text/x-patch, Size: 1170 bytes --]

>From 59729e0ab18a763ba36616a1025ce606a8721f1c Mon Sep 17 00:00:00 2001
From: Jan Kara <jack@suse.cz>
Date: Mon, 9 May 2011 16:39:34 +0200
Subject: [PATCH] ext4: Stop releasing passed bh in do_split()

make_indexed_dir() needs to do error recovery on the passed bh when do_split()
fails. So do not release it early in do_split().

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext4/namei.c |    7 +------
 1 files changed, 1 insertions(+), 6 deletions(-)

diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index 3c7a06e..1cddab9 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -1165,11 +1165,8 @@ static struct ext4_dir_entry_2 *do_split(handle_t *handle, struct inode *dir,
 	int	err = 0, i;
 
 	bh2 = ext4_append (handle, dir, &newblock, &err);
-	if (!(bh2)) {
-		brelse(*bh);
-		*bh = NULL;
+	if (!bh2)
 		goto errout;
-	}
 
 	BUFFER_TRACE(*bh, "get_write_access");
 	err = ext4_journal_get_write_access(handle, *bh);
@@ -1235,9 +1232,7 @@ static struct ext4_dir_entry_2 *do_split(handle_t *handle, struct inode *dir,
 	return de;
 
 journal_error:
-	brelse(*bh);
 	brelse(bh2);
-	*bh = NULL;
 	ext4_std_error(dir->i_sb, err);
 errout:
 	*error = err;
-- 
1.7.1