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

login
register
mail settings
Submitter Allison Henderson
Date May 7, 2011, 11:54 p.m.
Message ID <4DC5DBB3.9030207@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/94531/
State Accepted
Headers show

Comments

Allison Henderson - May 7, 2011, 11:54 p.m.
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(-)
Theodore Ts'o - May 9, 2011, 12:38 a.m.
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
--
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 9, 2011, 11:03 a.m.
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
>
Yongqiang Yang - May 9, 2011, 11:18 a.m.
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
>
Yongqiang Yang - May 9, 2011, 11:20 a.m.
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
>
Yongqiang Yang - May 9, 2011, 11:21 a.m.
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
>
Jan Kara - May 9, 2011, 11:30 a.m.
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
Yongqiang Yang - May 9, 2011, 11:33 a.m.
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
>
Jan Kara - May 9, 2011, 11:36 a.m.
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
Theodore Ts'o - May 9, 2011, 1:55 p.m.
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
--
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 9, 2011, 2:05 p.m.
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
Theodore Ts'o - May 9, 2011, 2:22 p.m.
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
--
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

Patch

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;
 	}