Patchwork [1/1] Null Pointer when make_indexed_dir returns -ENOSPC

login
register
mail settings
Submitter Jan Kara
Date May 9, 2011, 2:42 p.m.
Message ID <20110509144201.GP4122@quack.suse.cz>
Download mbox | patch
Permalink /patch/94772/
State Superseded
Headers show

Comments

Jan Kara - May 9, 2011, 2:42 p.m.
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
Allison Henderson - May 9, 2011, 8:39 p.m.
On 5/9/2011 7:42 AM, Jan Kara wrote:
> 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

Hi all,

Oh, I understand the problem now.  Sorry for the late response, I had to 
stop and dig around with this one for a bit.  Would people prefer to add 
the new code before the do_split like this:

+	ext4_handle_dirty_metadata(handle, dir, frame->bh);
+	ext4_handle_dirty_metadata(handle, dir, bh);
+
  	de = do_split(handle,dir, &bh, frame, &hinfo, &retval);
  	if (!de) {
  		/*
@@ -1421,8 +1425,6 @@ static int make_indexed_dir(handle_t *handle, 
struct dentry *dentry,
  		 * with corrupted filesystem.
  		 */
  		ext4_mark_inode_dirty(handle, dir);
-		ext4_handle_dirty_metadata(handle, dir, frame->bh);
-		ext4_handle_dirty_metadata(handle, dir, bh);
  		dx_release(frames);
  		return retval;
  	}

I've tested both patches and they both seem to resolve the null pointer. 
  The only other solution that comes to mind would be to add flags to 
the do_split to skip the brelse or to do the mark dirty before the 
brelse as you suggest.

Allison Henderson
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jan Kara - May 10, 2011, 1:34 p.m.
On Mon 09-05-11 13:39:07, Allison Henderson wrote:
> On 5/9/2011 7:42 AM, Jan Kara wrote:
> >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
> 
> Hi all,
> 
> Oh, I understand the problem now.  Sorry for the late response, I
> had to stop and dig around with this one for a bit.  Would people
> prefer to add the new code before the do_split like this:
> 
> +	ext4_handle_dirty_metadata(handle, dir, frame->bh);
> +	ext4_handle_dirty_metadata(handle, dir, bh);
> +
>  	de = do_split(handle,dir, &bh, frame, &hinfo, &retval);
>  	if (!de) {
>  		/*
> @@ -1421,8 +1425,6 @@ static int make_indexed_dir(handle_t *handle,
> struct dentry *dentry,
>  		 * with corrupted filesystem.
>  		 */
>  		ext4_mark_inode_dirty(handle, dir);
> -		ext4_handle_dirty_metadata(handle, dir, frame->bh);
> -		ext4_handle_dirty_metadata(handle, dir, bh);
>  		dx_release(frames);
>  		return retval;
>  	}
  This would be fine with me as well. It has a slight overhead of marking
buffer dirty twice but I don't think it really matters.

> I've tested both patches and they both seem to resolve the null
> pointer.  The only other solution that comes to mind would be to add
> flags to the do_split to skip the brelse or to do the mark dirty
> before the brelse as you suggest.
  Yes, I don't mind which of them Ted chooses...

								Honza

Patch

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