| Message ID | 20200213063821.30455-3-yi.zhang@huawei.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | jbd2: fix an oops problem | expand |
On Thu 13-02-20 14:38:21, zhangyi (F) wrote: > Commit 904cdbd41d74 ("jbd2: clear dirty flag when revoking a buffer from > an older transaction") set the BH_Freed flag when forgetting a metadata > buffer which belongs to the committing transaction, it indicate the > committing process clear dirty bits when it is done with the buffer. But > it also clear the BH_Mapped flag at the same time, which may trigger > below NULL pointer oops when block_size < PAGE_SIZE. > > rmdir 1 kjournald2 mkdir 2 > jbd2_journal_commit_transaction > commit transaction N > jbd2_journal_forget > set_buffer_freed(bh1) > jbd2_journal_commit_transaction > commit transaction N+1 > ... > clear_buffer_mapped(bh1) > ext4_getblk(bh2 ummapped) > ... > grow_dev_page > init_page_buffers > bh1->b_private=NULL > bh2->b_private=NULL > jbd2_journal_put_journal_head(jh1) > __journal_remove_journal_head(hb1) > jh1 is NULL and trigger oops > > *) Dir entry block bh1 and bh2 belongs to one page, and the bh2 has > already been unmapped. > > For the metadata buffer we forgetting, we should always keep the mapped > flag and clear the dirty flags is enough, so this patch pick out the > these buffers and keep their BH_Mapped flag. > > Fixes: 904cdbd41d74 ("jbd2: clear dirty flag when revoking a buffer from an older transaction") > Signed-off-by: zhangyi (F) <yi.zhang@huawei.com> > --- > fs/jbd2/commit.c | 25 +++++++++++++++++++++---- > 1 file changed, 21 insertions(+), 4 deletions(-) Thanks! The patch looks good. I have just some comment reformulation suggestion below, otherwise feel free to add: Reviewed-by: Jan Kara <jack@suse.cz> > diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c > index 6396fe70085b..27373f5792a4 100644 > --- a/fs/jbd2/commit.c > +++ b/fs/jbd2/commit.c > @@ -985,12 +985,29 @@ void jbd2_journal_commit_transaction(journal_t *journal) > * pagesize and it is attached to the last partial page. > */ > if (buffer_freed(bh) && !jh->b_next_transaction) { > + struct address_space *mapping; > + > clear_buffer_freed(bh); > clear_buffer_jbddirty(bh); > - clear_buffer_mapped(bh); > - clear_buffer_new(bh); > - clear_buffer_req(bh); > - bh->b_bdev = NULL; > + > + /* > + * Block device buffers need to stay mapped all the > + * time, so it is enough to clear buffer_jbddirty and > + * buffer_freed bits. For the file mapping buffers (i.e. > + * journalled data) we need to unmap buffer and clear > + * more bits. We also need to be careful about the check > + * because the data page mapping can get cleared under > + * out hands, which alse need not to clear more bits ^^^ our ^^^^ Maybe I'd rephrase this like: ... under our hands. Note that if mapping == NULL, we don't need to make buffer unmapped because the page is already detached from the mapping and buffers cannot get reused. > + * because the page and buffers will be freed and can > + * never be reused once we are done with them. > + */ > + mapping = READ_ONCE(bh->b_page->mapping); > + if (mapping && !sb_is_blkdev_sb(mapping->host->i_sb)) { > + clear_buffer_mapped(bh); > + clear_buffer_new(bh); > + clear_buffer_req(bh); > + bh->b_bdev = NULL; > + } > } Honza
Hi, On 2020/2/17 17:36, Jan Kara wrote: > On Thu 13-02-20 14:38:21, zhangyi (F) wrote: >> Commit 904cdbd41d74 ("jbd2: clear dirty flag when revoking a buffer from >> an older transaction") set the BH_Freed flag when forgetting a metadata >> buffer which belongs to the committing transaction, it indicate the >> committing process clear dirty bits when it is done with the buffer. But >> it also clear the BH_Mapped flag at the same time, which may trigger >> below NULL pointer oops when block_size < PAGE_SIZE. >> >> rmdir 1 kjournald2 mkdir 2 >> jbd2_journal_commit_transaction >> commit transaction N >> jbd2_journal_forget >> set_buffer_freed(bh1) >> jbd2_journal_commit_transaction >> commit transaction N+1 >> ... >> clear_buffer_mapped(bh1) >> ext4_getblk(bh2 ummapped) >> ... >> grow_dev_page >> init_page_buffers >> bh1->b_private=NULL >> bh2->b_private=NULL >> jbd2_journal_put_journal_head(jh1) >> __journal_remove_journal_head(hb1) >> jh1 is NULL and trigger oops >> >> *) Dir entry block bh1 and bh2 belongs to one page, and the bh2 has >> already been unmapped. >> >> For the metadata buffer we forgetting, we should always keep the mapped >> flag and clear the dirty flags is enough, so this patch pick out the >> these buffers and keep their BH_Mapped flag. >> >> Fixes: 904cdbd41d74 ("jbd2: clear dirty flag when revoking a buffer from an older transaction") >> Signed-off-by: zhangyi (F) <yi.zhang@huawei.com> >> --- >> fs/jbd2/commit.c | 25 +++++++++++++++++++++---- >> 1 file changed, 21 insertions(+), 4 deletions(-) > > Thanks! The patch looks good. I have just some comment reformulation > suggestion below, otherwise feel free to add: > > Reviewed-by: Jan Kara <jack@suse.cz> > >> diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c >> index 6396fe70085b..27373f5792a4 100644 >> --- a/fs/jbd2/commit.c >> +++ b/fs/jbd2/commit.c >> @@ -985,12 +985,29 @@ void jbd2_journal_commit_transaction(journal_t *journal) >> * pagesize and it is attached to the last partial page. >> */ >> if (buffer_freed(bh) && !jh->b_next_transaction) { >> + struct address_space *mapping; >> + >> clear_buffer_freed(bh); >> clear_buffer_jbddirty(bh); >> - clear_buffer_mapped(bh); >> - clear_buffer_new(bh); >> - clear_buffer_req(bh); >> - bh->b_bdev = NULL; >> + >> + /* >> + * Block device buffers need to stay mapped all the >> + * time, so it is enough to clear buffer_jbddirty and >> + * buffer_freed bits. For the file mapping buffers (i.e. >> + * journalled data) we need to unmap buffer and clear >> + * more bits. We also need to be careful about the check >> + * because the data page mapping can get cleared under >> + * out hands, which alse need not to clear more bits > ^^^ our ^^^^ Maybe I'd rephrase this like: > > ... under our hands. Note that if mapping == NULL, we don't need to make > buffer unmapped because the page is already detached from the mapping and > buffers cannot get reused. > Thanks for your suggestion, Ted has already pushed this patch to upstream, I could write an appending patch to fix this. Thanks, Yi.
On 2/13/20 12:08 PM, zhangyi (F) wrote: > Commit 904cdbd41d74 ("jbd2: clear dirty flag when revoking a buffer from > an older transaction") set the BH_Freed flag when forgetting a metadata > buffer which belongs to the committing transaction, it indicate the > committing process clear dirty bits when it is done with the buffer. But > it also clear the BH_Mapped flag at the same time, which may trigger > below NULL pointer oops when block_size < PAGE_SIZE. > > rmdir 1 kjournald2 mkdir 2 > jbd2_journal_commit_transaction > commit transaction N > jbd2_journal_forget > set_buffer_freed(bh1) > jbd2_journal_commit_transaction > commit transaction N+1 > ... > clear_buffer_mapped(bh1) > ext4_getblk(bh2 ummapped) > ... > grow_dev_page > init_page_buffers > bh1->b_private=NULL > bh2->b_private=NULL > jbd2_journal_put_journal_head(jh1) > __journal_remove_journal_head(hb1) > jh1 is NULL and trigger oops > > *) Dir entry block bh1 and bh2 belongs to one page, and the bh2 has > already been unmapped. > > For the metadata buffer we forgetting, we should always keep the mapped > flag and clear the dirty flags is enough, so this patch pick out the > these buffers and keep their BH_Mapped flag. > > Fixes: 904cdbd41d74 ("jbd2: clear dirty flag when revoking a buffer from an older transaction") > Signed-off-by: zhangyi (F) <yi.zhang@huawei.com> This should be a stable candidate I guess. -ritesh > --- > fs/jbd2/commit.c | 25 +++++++++++++++++++++---- > 1 file changed, 21 insertions(+), 4 deletions(-) > > diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c > index 6396fe70085b..27373f5792a4 100644 > --- a/fs/jbd2/commit.c > +++ b/fs/jbd2/commit.c > @@ -985,12 +985,29 @@ void jbd2_journal_commit_transaction(journal_t *journal) > * pagesize and it is attached to the last partial page. > */ > if (buffer_freed(bh) && !jh->b_next_transaction) { > + struct address_space *mapping; > + > clear_buffer_freed(bh); > clear_buffer_jbddirty(bh); > - clear_buffer_mapped(bh); > - clear_buffer_new(bh); > - clear_buffer_req(bh); > - bh->b_bdev = NULL; > + > + /* > + * Block device buffers need to stay mapped all the > + * time, so it is enough to clear buffer_jbddirty and > + * buffer_freed bits. For the file mapping buffers (i.e. > + * journalled data) we need to unmap buffer and clear > + * more bits. We also need to be careful about the check > + * because the data page mapping can get cleared under > + * out hands, which alse need not to clear more bits > + * because the page and buffers will be freed and can > + * never be reused once we are done with them. > + */ > + mapping = READ_ONCE(bh->b_page->mapping); > + if (mapping && !sb_is_blkdev_sb(mapping->host->i_sb)) { > + clear_buffer_mapped(bh); > + clear_buffer_new(bh); > + clear_buffer_req(bh); > + bh->b_bdev = NULL; > + } > } > > if (buffer_jbddirty(bh)) { >
On Tue, Feb 18, 2020 at 10:48:13AM +0530, Ritesh Harjani wrote: > > Fixes: 904cdbd41d74 ("jbd2: clear dirty flag when revoking a buffer from an older transaction") > > Signed-off-by: zhangyi (F) <yi.zhang@huawei.com> > > This should be a stable candidate I guess. I took care of that before sending a pull request to Linus. :-) - Ted
On Mon, Feb 17, 2020 at 06:38:23PM +0800, zhangyi (F) wrote: > >> + /* > >> + * Block device buffers need to stay mapped all the > >> + * time, so it is enough to clear buffer_jbddirty and > >> + * buffer_freed bits. For the file mapping buffers (i.e. > >> + * journalled data) we need to unmap buffer and clear > >> + * more bits. We also need to be careful about the check > >> + * because the data page mapping can get cleared under > >> + * out hands, which alse need not to clear more bits > > ^^^ our ^^^^ Maybe I'd rephrase this like: > > > > ... under our hands. Note that if mapping == NULL, we don't need to make > > buffer unmapped because the page is already detached from the mapping and > > buffers cannot get reused. > > > Thanks for your suggestion, Ted has already pushed this patch to upstream, > I could write an appending patch to fix this. Feel free to send a patch to fix up the comment if you like. Thanks, - Ted
diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c index 6396fe70085b..27373f5792a4 100644 --- a/fs/jbd2/commit.c +++ b/fs/jbd2/commit.c @@ -985,12 +985,29 @@ void jbd2_journal_commit_transaction(journal_t *journal) * pagesize and it is attached to the last partial page. */ if (buffer_freed(bh) && !jh->b_next_transaction) { + struct address_space *mapping; + clear_buffer_freed(bh); clear_buffer_jbddirty(bh); - clear_buffer_mapped(bh); - clear_buffer_new(bh); - clear_buffer_req(bh); - bh->b_bdev = NULL; + + /* + * Block device buffers need to stay mapped all the + * time, so it is enough to clear buffer_jbddirty and + * buffer_freed bits. For the file mapping buffers (i.e. + * journalled data) we need to unmap buffer and clear + * more bits. We also need to be careful about the check + * because the data page mapping can get cleared under + * out hands, which alse need not to clear more bits + * because the page and buffers will be freed and can + * never be reused once we are done with them. + */ + mapping = READ_ONCE(bh->b_page->mapping); + if (mapping && !sb_is_blkdev_sb(mapping->host->i_sb)) { + clear_buffer_mapped(bh); + clear_buffer_new(bh); + clear_buffer_req(bh); + bh->b_bdev = NULL; + } } if (buffer_jbddirty(bh)) {
Commit 904cdbd41d74 ("jbd2: clear dirty flag when revoking a buffer from an older transaction") set the BH_Freed flag when forgetting a metadata buffer which belongs to the committing transaction, it indicate the committing process clear dirty bits when it is done with the buffer. But it also clear the BH_Mapped flag at the same time, which may trigger below NULL pointer oops when block_size < PAGE_SIZE. rmdir 1 kjournald2 mkdir 2 jbd2_journal_commit_transaction commit transaction N jbd2_journal_forget set_buffer_freed(bh1) jbd2_journal_commit_transaction commit transaction N+1 ... clear_buffer_mapped(bh1) ext4_getblk(bh2 ummapped) ... grow_dev_page init_page_buffers bh1->b_private=NULL bh2->b_private=NULL jbd2_journal_put_journal_head(jh1) __journal_remove_journal_head(hb1) jh1 is NULL and trigger oops *) Dir entry block bh1 and bh2 belongs to one page, and the bh2 has already been unmapped. For the metadata buffer we forgetting, we should always keep the mapped flag and clear the dirty flags is enough, so this patch pick out the these buffers and keep their BH_Mapped flag. Fixes: 904cdbd41d74 ("jbd2: clear dirty flag when revoking a buffer from an older transaction") Signed-off-by: zhangyi (F) <yi.zhang@huawei.com> --- fs/jbd2/commit.c | 25 +++++++++++++++++++++---- 1 file changed, 21 insertions(+), 4 deletions(-)