| Message ID | 4DC5DBB3.9030207@linux.vnet.ibm.com |
|---|---|
| State | Accepted, archived |
| Headers | show |
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
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 >
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 >
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 >
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 >
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
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 >
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
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
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
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
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; }
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(-)