* [PATCH 1/1] Null Pointer when make_indexed_dir returns -ENOSPC @ 2011-05-07 23:54 Allison Henderson 2011-05-09 0:38 ` Ted Ts'o 2011-05-09 11:03 ` Jan Kara 0 siblings, 2 replies; 17+ messages in thread From: Allison Henderson @ 2011-05-07 23:54 UTC (permalink / raw) To: Ext4 Developers List; +Cc: Jan Kara 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 1/1] Null Pointer when make_indexed_dir returns -ENOSPC 2011-05-07 23:54 [PATCH 1/1] Null Pointer when make_indexed_dir returns -ENOSPC Allison Henderson @ 2011-05-09 0:38 ` Ted Ts'o 2011-05-09 11:03 ` Jan Kara 1 sibling, 0 replies; 17+ messages in thread From: Ted Ts'o @ 2011-05-09 0:38 UTC (permalink / raw) To: Allison Henderson; +Cc: Ext4 Developers List, Jan Kara 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/1] Null Pointer when make_indexed_dir returns -ENOSPC 2011-05-07 23:54 [PATCH 1/1] Null Pointer when make_indexed_dir returns -ENOSPC Allison Henderson 2011-05-09 0:38 ` Ted Ts'o @ 2011-05-09 11:03 ` Jan Kara 2011-05-09 11:18 ` Yongqiang Yang 1 sibling, 1 reply; 17+ messages in thread From: Jan Kara @ 2011-05-09 11:03 UTC (permalink / raw) To: Allison Henderson; +Cc: Ext4 Developers List, 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/1] Null Pointer when make_indexed_dir returns -ENOSPC 2011-05-09 11:03 ` Jan Kara @ 2011-05-09 11:18 ` Yongqiang Yang 2011-05-09 11:20 ` Yongqiang Yang 2011-05-09 11:30 ` Jan Kara 0 siblings, 2 replies; 17+ messages in thread From: Yongqiang Yang @ 2011-05-09 11:18 UTC (permalink / raw) To: Jan Kara; +Cc: Allison Henderson, Ext4 Developers List 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/1] Null Pointer when make_indexed_dir returns -ENOSPC 2011-05-09 11:18 ` Yongqiang Yang @ 2011-05-09 11:20 ` Yongqiang Yang 2011-05-09 11:21 ` Yongqiang Yang 2011-05-09 11:30 ` Jan Kara 1 sibling, 1 reply; 17+ messages in thread From: Yongqiang Yang @ 2011-05-09 11:20 UTC (permalink / raw) To: Jan Kara; +Cc: Allison Henderson, Ext4 Developers List 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/1] Null Pointer when make_indexed_dir returns -ENOSPC 2011-05-09 11:20 ` Yongqiang Yang @ 2011-05-09 11:21 ` Yongqiang Yang 0 siblings, 0 replies; 17+ messages in thread From: Yongqiang Yang @ 2011-05-09 11:21 UTC (permalink / raw) To: Jan Kara; +Cc: Allison Henderson, Ext4 Developers List 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/1] Null Pointer when make_indexed_dir returns -ENOSPC 2011-05-09 11:18 ` Yongqiang Yang 2011-05-09 11:20 ` Yongqiang Yang @ 2011-05-09 11:30 ` Jan Kara 2011-05-09 11:33 ` Yongqiang Yang 2011-05-09 13:55 ` Ted Ts'o 1 sibling, 2 replies; 17+ messages in thread From: Jan Kara @ 2011-05-09 11:30 UTC (permalink / raw) To: Yongqiang Yang; +Cc: Jan Kara, Allison Henderson, Ext4 Developers List 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/1] Null Pointer when make_indexed_dir returns -ENOSPC 2011-05-09 11:30 ` Jan Kara @ 2011-05-09 11:33 ` Yongqiang Yang 2011-05-09 11:36 ` Jan Kara 2011-05-09 13:55 ` Ted Ts'o 1 sibling, 1 reply; 17+ messages in thread From: Yongqiang Yang @ 2011-05-09 11:33 UTC (permalink / raw) To: Jan Kara; +Cc: Allison Henderson, Ext4 Developers List 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/1] Null Pointer when make_indexed_dir returns -ENOSPC 2011-05-09 11:33 ` Yongqiang Yang @ 2011-05-09 11:36 ` Jan Kara 0 siblings, 0 replies; 17+ messages in thread From: Jan Kara @ 2011-05-09 11:36 UTC (permalink / raw) To: Yongqiang Yang; +Cc: Jan Kara, Allison Henderson, Ext4 Developers List 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/1] Null Pointer when make_indexed_dir returns -ENOSPC 2011-05-09 11:30 ` Jan Kara 2011-05-09 11:33 ` Yongqiang Yang @ 2011-05-09 13:55 ` Ted Ts'o 2011-05-09 14:05 ` Jan Kara 1 sibling, 1 reply; 17+ messages in thread From: Ted Ts'o @ 2011-05-09 13:55 UTC (permalink / raw) To: Jan Kara; +Cc: Yongqiang Yang, Allison Henderson, Ext4 Developers List 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/1] Null Pointer when make_indexed_dir returns -ENOSPC 2011-05-09 13:55 ` Ted Ts'o @ 2011-05-09 14:05 ` Jan Kara 2011-05-09 14:22 ` Ted Ts'o 0 siblings, 1 reply; 17+ messages in thread From: Jan Kara @ 2011-05-09 14:05 UTC (permalink / raw) To: Ted Ts'o Cc: Jan Kara, Yongqiang Yang, Allison Henderson, Ext4 Developers List 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/1] Null Pointer when make_indexed_dir returns -ENOSPC 2011-05-09 14:05 ` Jan Kara @ 2011-05-09 14:22 ` Ted Ts'o 2011-05-09 14:27 ` [PATCH -v2] ext4: don't dereference null pointer when make_indexed_dir() fails Theodore Ts'o 2011-05-09 14:42 ` [PATCH 1/1] Null Pointer when make_indexed_dir returns -ENOSPC Jan Kara 0 siblings, 2 replies; 17+ messages in thread From: Ted Ts'o @ 2011-05-09 14:22 UTC (permalink / raw) To: Jan Kara; +Cc: Yongqiang Yang, Allison Henderson, Ext4 Developers List 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH -v2] ext4: don't dereference null pointer when make_indexed_dir() fails 2011-05-09 14:22 ` Ted Ts'o @ 2011-05-09 14:27 ` Theodore Ts'o 2011-05-09 14:56 ` Eric Sandeen 2011-05-09 14:42 ` [PATCH 1/1] Null Pointer when make_indexed_dir returns -ENOSPC Jan Kara 1 sibling, 1 reply; 17+ messages in thread From: Theodore Ts'o @ 2011-05-09 14:27 UTC (permalink / raw) To: Ext4 Developers List; +Cc: Allison Henderson, 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH -v2] ext4: don't dereference null pointer when make_indexed_dir() fails 2011-05-09 14:27 ` [PATCH -v2] ext4: don't dereference null pointer when make_indexed_dir() fails Theodore Ts'o @ 2011-05-09 14:56 ` Eric Sandeen 0 siblings, 0 replies; 17+ messages in thread From: Eric Sandeen @ 2011-05-09 14:56 UTC (permalink / raw) To: Theodore Ts'o; +Cc: ext4 development, Allison Henderson 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; > } ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/1] Null Pointer when make_indexed_dir returns -ENOSPC 2011-05-09 14:22 ` Ted Ts'o 2011-05-09 14:27 ` [PATCH -v2] ext4: don't dereference null pointer when make_indexed_dir() fails Theodore Ts'o @ 2011-05-09 14:42 ` Jan Kara 2011-05-09 20:39 ` Allison Henderson 1 sibling, 1 reply; 17+ messages in thread From: Jan Kara @ 2011-05-09 14:42 UTC (permalink / raw) To: Ted Ts'o Cc: Jan Kara, Yongqiang Yang, Allison Henderson, Ext4 Developers List [-- 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 1/1] Null Pointer when make_indexed_dir returns -ENOSPC 2011-05-09 14:42 ` [PATCH 1/1] Null Pointer when make_indexed_dir returns -ENOSPC Jan Kara @ 2011-05-09 20:39 ` Allison Henderson 2011-05-10 13:34 ` Jan Kara 0 siblings, 1 reply; 17+ messages in thread From: Allison Henderson @ 2011-05-09 20:39 UTC (permalink / raw) To: Jan Kara; +Cc: Ted Ts'o, Yongqiang Yang, Ext4 Developers List 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/1] Null Pointer when make_indexed_dir returns -ENOSPC 2011-05-09 20:39 ` Allison Henderson @ 2011-05-10 13:34 ` Jan Kara 0 siblings, 0 replies; 17+ messages in thread From: Jan Kara @ 2011-05-10 13:34 UTC (permalink / raw) To: Allison Henderson Cc: Jan Kara, Ted Ts'o, Yongqiang Yang, Ext4 Developers List 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2011-05-10 13:34 UTC | newest] Thread overview: 17+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-05-07 23:54 [PATCH 1/1] Null Pointer when make_indexed_dir returns -ENOSPC Allison Henderson 2011-05-09 0:38 ` Ted Ts'o 2011-05-09 11:03 ` Jan Kara 2011-05-09 11:18 ` Yongqiang Yang 2011-05-09 11:20 ` Yongqiang Yang 2011-05-09 11:21 ` Yongqiang Yang 2011-05-09 11:30 ` Jan Kara 2011-05-09 11:33 ` Yongqiang Yang 2011-05-09 11:36 ` Jan Kara 2011-05-09 13:55 ` Ted Ts'o 2011-05-09 14:05 ` Jan Kara 2011-05-09 14:22 ` Ted Ts'o 2011-05-09 14:27 ` [PATCH -v2] ext4: don't dereference null pointer when make_indexed_dir() fails Theodore Ts'o 2011-05-09 14:56 ` Eric Sandeen 2011-05-09 14:42 ` [PATCH 1/1] Null Pointer when make_indexed_dir returns -ENOSPC Jan Kara 2011-05-09 20:39 ` Allison Henderson 2011-05-10 13:34 ` Jan Kara
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).