Fix for a null pointer bug found while running punch hole tests
Signed-off-by: Allison Henderson <[email protected]>
---
: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
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 <[email protected]>
Thanks, I've added this to the ext4 tree.
- Ted
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 <[email protected]>
> ---
> :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 <[email protected]>
SUSE Labs, CR
On Mon, May 9, 2011 at 7:03 PM, Jan Kara <[email protected]> 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 <[email protected]>
>> ---
>> :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 <[email protected]>
> SUSE Labs, CR
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to [email protected]
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
>
--
Best Wishes
Yongqiang Yang
On Mon, May 9, 2011 at 7:18 PM, Yongqiang Yang <[email protected]> wrote:
> On Mon, May 9, 2011 at 7:03 PM, Jan Kara <[email protected]> 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 <[email protected]>
>>> ---
>>> :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 <[email protected]>
>> SUSE Labs, CR
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
>> the body of a message to [email protected]
>> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
>>
>
>
>
> --
> Best Wishes
> Yongqiang Yang
>
--
Best Wishes
Yongqiang Yang
On Mon, May 9, 2011 at 7:20 PM, Yongqiang Yang <[email protected]> wrote:
> On Mon, May 9, 2011 at 7:18 PM, Yongqiang Yang <[email protected]> wrote:
>> On Mon, May 9, 2011 at 7:03 PM, Jan Kara <[email protected]> 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 <[email protected]>
>>>> ---
>>>> :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 <[email protected]>
>>> SUSE Labs, CR
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
>>> the body of a message to [email protected]
>>> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
>>>
>>
>>
>>
>> --
>> Best Wishes
>> Yongqiang Yang
>>
>
>
>
> --
> Best Wishes
> Yongqiang Yang
>
--
Best Wishes
Yongqiang Yang
On Mon 09-05-11 19:18:37, Yongqiang Yang wrote:
> On Mon, May 9, 2011 at 7:03 PM, Jan Kara <[email protected]> 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 <[email protected]>
> >> ---
> >> :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 <[email protected]>
SUSE Labs, CR
On Mon, May 9, 2011 at 7:30 PM, Jan Kara <[email protected]> wrote:
> On Mon 09-05-11 19:18:37, Yongqiang Yang wrote:
>> On Mon, May 9, 2011 at 7:03 PM, Jan Kara <[email protected]> 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 <[email protected]>
>> >> ---
>> >> :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 <[email protected]>
> SUSE Labs, CR
>
--
Best Wishes
Yongqiang Yang
On Mon 09-05-11 19:33:19, Yongqiang Yang wrote:
> On Mon, May 9, 2011 at 7:30 PM, Jan Kara <[email protected]> wrote:
> > On Mon 09-05-11 19:18:37, Yongqiang Yang wrote:
> >> On Mon, May 9, 2011 at 7:03 PM, Jan Kara <[email protected]> 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 <[email protected]>
> >> >> ---
> >> >> :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 <[email protected]>
SUSE Labs, CR
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
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 <[email protected]>
SUSE Labs, CR
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
From: Allison Henderson <[email protected]>
Fix for a null pointer bug found while running punch hole tests
Signed-off-by: Allison Henderson <[email protected]>
Signed-off-by: "Theodore Ts'o" <[email protected]>
---
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
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 <[email protected]>
SUSE Labs, CR
On 5/9/11 9:27 AM, Theodore Ts'o wrote:
> From: Allison Henderson <[email protected]>
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 <[email protected]>
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 <[email protected]>
> Signed-off-by: "Theodore Ts'o" <[email protected]>
> ---
> 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;
> }
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
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 <[email protected]>
SUSE Labs, CR