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

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

[Allison Henderson]

Fix for a null pointer bug found while running punch hole tests

Signed-off-by: Allison Henderson <achender@us.ibm.com>
---
:100644 100644 3c7a06e... 3302a6c... M	fs/ext4/namei.c
 fs/ext4/namei.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index 3c7a06e..3302a6c 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -1422,7 +1422,8 @@ static int make_indexed_dir(handle_t *handle, struct dentry *dentry,
 		 */
 		ext4_mark_inode_dirty(handle, dir);
 		ext4_handle_dirty_metadata(handle, dir, frame->bh);
-		ext4_handle_dirty_metadata(handle, dir, bh);
+		if (bh)
+			ext4_handle_dirty_metadata(handle, dir, bh);
 		dx_release(frames);
 		return retval;
 	}
-- 
1.7.1

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

[Ted Ts'o]

On Sat, May 07, 2011 at 04:54:27PM -0700, Allison Henderson wrote:
> Fix for a null pointer bug found while running punch hole tests
> 
> Signed-off-by: Allison Henderson <achender@us.ibm.com>

Thanks, I've added this to the ext4 tree.

	     	   	       	    	  - Ted

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

[Jan Kara]

On Sat 07-05-11 16:54:27, Allison Henderson wrote:
> Fix for a null pointer bug found while running punch hole tests
> 
> Signed-off-by: Allison Henderson <achender@us.ibm.com>
> ---
> :100644 100644 3c7a06e... 3302a6c... M	fs/ext4/namei.c
>  fs/ext4/namei.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> index 3c7a06e..3302a6c 100644
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c
> @@ -1422,7 +1422,8 @@ static int make_indexed_dir(handle_t *handle, struct dentry *dentry,
>  		 */
>  		ext4_mark_inode_dirty(handle, dir);
>  		ext4_handle_dirty_metadata(handle, dir, frame->bh);
> -		ext4_handle_dirty_metadata(handle, dir, bh);
> +		if (bh)
> +			ext4_handle_dirty_metadata(handle, dir, bh);
  I'm puzzled - bh here is bh2 from the beginning of the function and we
check it for being NULL after we ext4_append() it. So how can this happen?

									Honza
>  		dx_release(frames);
>  		return retval;
>  	}
> -- 
> 1.7.1
> 
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

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

[Yongqiang Yang]

On Mon, May 9, 2011 at 7:03 PM, Jan Kara <jack@suse.cz> wrote:
> On Sat 07-05-11 16:54:27, Allison Henderson wrote:
>> Fix for a null pointer bug found while running punch hole tests
>>
>> Signed-off-by: Allison Henderson <achender@us.ibm.com>
>> ---
>> :100644 100644 3c7a06e... 3302a6c... M        fs/ext4/namei.c
>>  fs/ext4/namei.c |    3 ++-
>>  1 files changed, 2 insertions(+), 1 deletions(-)
>>
>> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
>> index 3c7a06e..3302a6c 100644
>> --- a/fs/ext4/namei.c
>> +++ b/fs/ext4/namei.c
>> @@ -1422,7 +1422,8 @@ static int make_indexed_dir(handle_t *handle, struct dentry *dentry,
>>                */
>>               ext4_mark_inode_dirty(handle, dir);
>>               ext4_handle_dirty_metadata(handle, dir, frame->bh);
>> -             ext4_handle_dirty_metadata(handle, dir, bh);
>> +             if (bh)
>> +                     ext4_handle_dirty_metadata(handle, dir, bh);
>  I'm puzzled - bh here is bh2 from the beginning of the function and we
> check it for being NULL after we ext4_append() it. So how can this happen?
do_split() encounters a journal error and set bh to NULL before returning.

>
>                                                                        Honza
>>               dx_release(frames);
>>               return retval;
>>       }
>> --
>> 1.7.1
>>
> --
> Jan Kara <jack@suse.cz>
> SUSE Labs, CR
> --
> 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
>



-- 
Best Wishes
Yongqiang Yang
--
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

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

[Yongqiang Yang]

On Mon, May 9, 2011 at 7:18 PM, Yongqiang Yang <xiaoqiangnk@gmail.com> wrote:
> On Mon, May 9, 2011 at 7:03 PM, Jan Kara <jack@suse.cz> wrote:
>> On Sat 07-05-11 16:54:27, Allison Henderson wrote:
>>> Fix for a null pointer bug found while running punch hole tests
>>>
>>> Signed-off-by: Allison Henderson <achender@us.ibm.com>
>>> ---
>>> :100644 100644 3c7a06e... 3302a6c... M        fs/ext4/namei.c
>>>  fs/ext4/namei.c |    3 ++-
>>>  1 files changed, 2 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
>>> index 3c7a06e..3302a6c 100644
>>> --- a/fs/ext4/namei.c
>>> +++ b/fs/ext4/namei.c
>>> @@ -1422,7 +1422,8 @@ static int make_indexed_dir(handle_t *handle, struct dentry *dentry,
>>>                */
>>>               ext4_mark_inode_dirty(handle, dir);
>>>               ext4_handle_dirty_metadata(handle, dir, frame->bh);
>>> -             ext4_handle_dirty_metadata(handle, dir, bh);
>>> +             if (bh)
>>> +                     ext4_handle_dirty_metadata(handle, dir, bh);
>>  I'm puzzled - bh here is bh2 from the beginning of the function and we
>> check it for being NULL after we ext4_append() it. So how can this happen?
> do_split() encounters a journal error and set bh to NULL before returning.
Sorry, it does not make sense.

>
>>
>>                                                                        Honza
>>>               dx_release(frames);
>>>               return retval;
>>>       }
>>> --
>>> 1.7.1
>>>
>> --
>> Jan Kara <jack@suse.cz>
>> SUSE Labs, CR
>> --
>> 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
>>
>
>
>
> --
> Best Wishes
> Yongqiang Yang
>



-- 
Best Wishes
Yongqiang Yang
--
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

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

[Yongqiang Yang]

On Mon, May 9, 2011 at 7:20 PM, Yongqiang Yang <xiaoqiangnk@gmail.com> wrote:
> On Mon, May 9, 2011 at 7:18 PM, Yongqiang Yang <xiaoqiangnk@gmail.com> wrote:
>> On Mon, May 9, 2011 at 7:03 PM, Jan Kara <jack@suse.cz> wrote:
>>> On Sat 07-05-11 16:54:27, Allison Henderson wrote:
>>>> Fix for a null pointer bug found while running punch hole tests
>>>>
>>>> Signed-off-by: Allison Henderson <achender@us.ibm.com>
>>>> ---
>>>> :100644 100644 3c7a06e... 3302a6c... M        fs/ext4/namei.c
>>>>  fs/ext4/namei.c |    3 ++-
>>>>  1 files changed, 2 insertions(+), 1 deletions(-)
>>>>
>>>> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
>>>> index 3c7a06e..3302a6c 100644
>>>> --- a/fs/ext4/namei.c
>>>> +++ b/fs/ext4/namei.c
>>>> @@ -1422,7 +1422,8 @@ static int make_indexed_dir(handle_t *handle, struct dentry *dentry,
>>>>                */
>>>>               ext4_mark_inode_dirty(handle, dir);
>>>>               ext4_handle_dirty_metadata(handle, dir, frame->bh);
>>>> -             ext4_handle_dirty_metadata(handle, dir, bh);
>>>> +             if (bh)
>>>> +                     ext4_handle_dirty_metadata(handle, dir, bh);
>>>  I'm puzzled - bh here is bh2 from the beginning of the function and we
>>> check it for being NULL after we ext4_append() it. So how can this happen?
>> do_split() encounters a journal error and set bh to NULL before returning.
> Sorry, it does not make sense.
Sorry for being dense, it makes sense.
>
>>
>>>
>>>                                                                        Honza
>>>>               dx_release(frames);
>>>>               return retval;
>>>>       }
>>>> --
>>>> 1.7.1
>>>>
>>> --
>>> Jan Kara <jack@suse.cz>
>>> SUSE Labs, CR
>>> --
>>> 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
>>>
>>
>>
>>
>> --
>> Best Wishes
>> Yongqiang Yang
>>
>
>
>
> --
> Best Wishes
> Yongqiang Yang
>



-- 
Best Wishes
Yongqiang Yang
--
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

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

[Jan Kara]

On Mon 09-05-11 19:18:37, Yongqiang Yang wrote:
> On Mon, May 9, 2011 at 7:03 PM, Jan Kara <jack@suse.cz> wrote:
> > On Sat 07-05-11 16:54:27, Allison Henderson wrote:
> >> Fix for a null pointer bug found while running punch hole tests
> >>
> >> Signed-off-by: Allison Henderson <achender@us.ibm.com>
> >> ---
> >> :100644 100644 3c7a06e... 3302a6c... M        fs/ext4/namei.c
> >>  fs/ext4/namei.c |    3 ++-
> >>  1 files changed, 2 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> >> index 3c7a06e..3302a6c 100644
> >> --- a/fs/ext4/namei.c
> >> +++ b/fs/ext4/namei.c
> >> @@ -1422,7 +1422,8 @@ static int make_indexed_dir(handle_t *handle, struct dentry *dentry,
> >>                */
> >>               ext4_mark_inode_dirty(handle, dir);
> >>               ext4_handle_dirty_metadata(handle, dir, frame->bh);
> >> -             ext4_handle_dirty_metadata(handle, dir, bh);
> >> +             if (bh)
> >> +                     ext4_handle_dirty_metadata(handle, dir, bh);
> >  I'm puzzled - bh here is bh2 from the beginning of the function and we
> > check it for being NULL after we ext4_append() it. So how can this happen?
> do_split() encounters a journal error and set bh to NULL before returning.
Ah, I see. But then you just reintroduced the bug I was trying to fix. So
either do_split() has to do the marking of buffer dirty, or we have to do
it before calllig do_split(), or do_split() has to be changed and not
release passed buffer (and the two callers have to do it - which they seem
to do anyway). I don't mind either way but your fix is wrong.

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR
--
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

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

[Yongqiang Yang]

On Mon, May 9, 2011 at 7:30 PM, Jan Kara <jack@suse.cz> wrote:
> On Mon 09-05-11 19:18:37, Yongqiang Yang wrote:
>> On Mon, May 9, 2011 at 7:03 PM, Jan Kara <jack@suse.cz> wrote:
>> > On Sat 07-05-11 16:54:27, Allison Henderson wrote:
>> >> Fix for a null pointer bug found while running punch hole tests
>> >>
>> >> Signed-off-by: Allison Henderson <achender@us.ibm.com>
>> >> ---
>> >> :100644 100644 3c7a06e... 3302a6c... M        fs/ext4/namei.c
>> >>  fs/ext4/namei.c |    3 ++-
>> >>  1 files changed, 2 insertions(+), 1 deletions(-)
>> >>
>> >> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
>> >> index 3c7a06e..3302a6c 100644
>> >> --- a/fs/ext4/namei.c
>> >> +++ b/fs/ext4/namei.c
>> >> @@ -1422,7 +1422,8 @@ static int make_indexed_dir(handle_t *handle, struct dentry *dentry,
>> >>                */
>> >>               ext4_mark_inode_dirty(handle, dir);
>> >>               ext4_handle_dirty_metadata(handle, dir, frame->bh);
>> >> -             ext4_handle_dirty_metadata(handle, dir, bh);
>> >> +             if (bh)
>> >> +                     ext4_handle_dirty_metadata(handle, dir, bh);
>> >  I'm puzzled - bh here is bh2 from the beginning of the function and we
>> > check it for being NULL after we ext4_append() it. So how can this happen?
>> do_split() encounters a journal error and set bh to NULL before returning.
> Ah, I see. But then you just reintroduced the bug I was trying to fix. So
> either do_split() has to do the marking of buffer dirty, or we have to do
> it before calllig do_split(), or do_split() has to be changed and not
> release passed buffer (and the two callers have to do it - which they seem
> to do anyway). I don't mind either way but your fix is wrong.
The fix is made by Allison not me, I think Allison will have a look at
the thread.

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



-- 
Best Wishes
Yongqiang Yang
--
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

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

[Jan Kara]

On Mon 09-05-11 19:33:19, Yongqiang Yang wrote:
> On Mon, May 9, 2011 at 7:30 PM, Jan Kara <jack@suse.cz> wrote:
> > On Mon 09-05-11 19:18:37, Yongqiang Yang wrote:
> >> On Mon, May 9, 2011 at 7:03 PM, Jan Kara <jack@suse.cz> wrote:
> >> > On Sat 07-05-11 16:54:27, Allison Henderson wrote:
> >> >> Fix for a null pointer bug found while running punch hole tests
> >> >>
> >> >> Signed-off-by: Allison Henderson <achender@us.ibm.com>
> >> >> ---
> >> >> :100644 100644 3c7a06e... 3302a6c... M        fs/ext4/namei.c
> >> >>  fs/ext4/namei.c |    3 ++-
> >> >>  1 files changed, 2 insertions(+), 1 deletions(-)
> >> >>
> >> >> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> >> >> index 3c7a06e..3302a6c 100644
> >> >> --- a/fs/ext4/namei.c
> >> >> +++ b/fs/ext4/namei.c
> >> >> @@ -1422,7 +1422,8 @@ static int make_indexed_dir(handle_t *handle, struct dentry *dentry,
> >> >>                */
> >> >>               ext4_mark_inode_dirty(handle, dir);
> >> >>               ext4_handle_dirty_metadata(handle, dir, frame->bh);
> >> >> -             ext4_handle_dirty_metadata(handle, dir, bh);
> >> >> +             if (bh)
> >> >> +                     ext4_handle_dirty_metadata(handle, dir, bh);
> >> >  I'm puzzled - bh here is bh2 from the beginning of the function and we
> >> > check it for being NULL after we ext4_append() it. So how can this happen?
> >> do_split() encounters a journal error and set bh to NULL before returning.
> > Ah, I see. But then you just reintroduced the bug I was trying to fix. So
> > either do_split() has to do the marking of buffer dirty, or we have to do
> > it before calllig do_split(), or do_split() has to be changed and not
> > release passed buffer (and the two callers have to do it - which they seem
> > to do anyway). I don't mind either way but your fix is wrong.
> The fix is made by Allison not me, I think Allison will have a look at
> the thread.
  Right, sorry. I was addressing Allison, not you of course ;).

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR
--
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

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

[Ted Ts'o]

On Mon, May 09, 2011 at 01:30:52PM +0200, Jan Kara wrote:
> Ah, I see. But then you just reintroduced the bug I was trying to fix. So
> either do_split() has to do the marking of buffer dirty, or we have to do
> it before calllig do_split(), or do_split() has to be changed and not
> release passed buffer (and the two callers have to do it - which they seem
> to do anyway). I don't mind either way but your fix is wrong.

I think it's OK.  We do call ext4_handle_dirty_metadata on frame->bh,
which deals with the original version of bh.  And the cases where
do_split() sets bh to NULL is either (a) a journal error, in which
case we will have already aborted the journal, or an I/O error while
reading in the block, so bh won't have gotten modified yet.

Is there a case that you're worried about that I'm missing?

   	   	     	    	    	       - Ted

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

[Jan Kara]

On Mon 09-05-11 09:55:16, Ted Tso wrote:
> On Mon, May 09, 2011 at 01:30:52PM +0200, Jan Kara wrote:
> > Ah, I see. But then you just reintroduced the bug I was trying to fix. So
> > either do_split() has to do the marking of buffer dirty, or we have to do
> > it before calllig do_split(), or do_split() has to be changed and not
> > release passed buffer (and the two callers have to do it - which they seem
> > to do anyway). I don't mind either way but your fix is wrong.
> 
> I think it's OK.  We do call ext4_handle_dirty_metadata on frame->bh,
> which deals with the original version of bh.  And the cases where
> do_split() sets bh to NULL is either (a) a journal error, in which
> case we will have already aborted the journal, or an I/O error while
> reading in the block, so bh won't have gotten modified yet.
> 
> Is there a case that you're worried about that I'm missing?
  Yes. ext4_append() can return ENOSPC and passed bh will get set to NULL
without being marked dirty.  Note that we need to call
ext4_handle_dirty_metadata() on the passed bh as well specifically in the
make_indexed_dir() case because there we move contents of the first block
(in frame->bh) to the second block (passed bh) and create indexed tree root
in the first block. Then we call do_split() to further split the second
block...

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

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

[Ted Ts'o]

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?

						- Ted

[PATCH -v2] ext4: don't dereference null pointer when make_indexed_dir() fails

[Theodore Ts'o]

From: Allison Henderson <achender@linux.vnet.ibm.com>

Fix for a null pointer bug found while running punch hole tests

Signed-off-by: Allison Henderson <achender@us.ibm.com>
Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
---
 fs/ext4/namei.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index 3c7a06e..cc97feb 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -1422,7 +1422,9 @@ static int make_indexed_dir(handle_t *handle, struct dentry *dentry,
 		 */
 		ext4_mark_inode_dirty(handle, dir);
 		ext4_handle_dirty_metadata(handle, dir, frame->bh);
-		ext4_handle_dirty_metadata(handle, dir, bh);
+		ext4_handle_dirty_metadata(handle, dir, bh2);
+		if (bh)
+			ext4_handle_dirty_metadata(handle, dir, bh);
 		dx_release(frames);
 		return retval;
 	}
-- 
1.7.3.1

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

[Jan Kara]

[-- 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

Re: [PATCH -v2] ext4: don't dereference null pointer when make_indexed_dir() fails

[Eric Sandeen]

On 5/9/11 9:27 AM, Theodore Ts'o wrote:
> From: Allison Henderson <achender@linux.vnet.ibm.com>

Ted, can we be a little careful about attribution in cases like this?

If I have it straight, this isn't the patch Allison sent, it's based on it, but it is in fact:

Modified-by: Theodore Ts'o <tytso@mit.edu>

right?

I think it's important to keep track of who is making various changes to submitted patches, especially when they are functional (i.e. not whitespace or spelling, etc) changes.

Ideally, IMHO, the original submitter should be submitting V2 based on feedback, unless they are AWOL; this way patches attributed to a submitter really are their patches, and it is another layer of review if the original submitter can evaluate the proposed changes to their original patch...

Doing it this way should lighten your load too, I'd think.

Thanks,
-Eric



> Fix for a null pointer bug found while running punch hole tests
> 
> Signed-off-by: Allison Henderson <achender@us.ibm.com>
> Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
> ---
>  fs/ext4/namei.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
> 
> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> index 3c7a06e..cc97feb 100644
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c
> @@ -1422,7 +1422,9 @@ static int make_indexed_dir(handle_t *handle, struct dentry *dentry,
>  		 */
>  		ext4_mark_inode_dirty(handle, dir);
>  		ext4_handle_dirty_metadata(handle, dir, frame->bh);
> -		ext4_handle_dirty_metadata(handle, dir, bh);
> +		ext4_handle_dirty_metadata(handle, dir, bh2);
> +		if (bh)
> +			ext4_handle_dirty_metadata(handle, dir, bh);
>  		dx_release(frames);
>  		return retval;
>  	}

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

[Allison Henderson]

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

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

[Jan Kara]

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
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR