* [PATCH v2 0/2] ext4: fix buffer references leak problem @ 2019-03-15 7:10 zhangyi (F) 2019-03-15 7:10 ` [PATCH v2 1/2] ext4: brelse all indirect buffer in ext4_ind_remove_space() zhangyi (F) 2019-03-15 7:10 ` [PATCH v2 2/2] ext4: cleanup bh release code " zhangyi (F) 0 siblings, 2 replies; 7+ messages in thread From: zhangyi (F) @ 2019-03-15 7:10 UTC (permalink / raw) To: linux-ext4; +Cc: tytso, jack, adilger.kernel, yi.zhang, miaoxie Hi, This patch set want to fix a buffer references leak problem in ext4_ind_remove_space() and do some cleanup of the releasing code. Change since v1: - Separate the cleanup code from the fix patch. zhangyi (F) (2): ext4: brelse all indirect buffer in ext4_ind_remove_space() ext4: cleanup bh release code in ext4_ind_remove_space() fs/ext4/indirect.c | 43 ++++++++++++++++++++++--------------------- 1 file changed, 22 insertions(+), 21 deletions(-) -- 2.7.4 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 1/2] ext4: brelse all indirect buffer in ext4_ind_remove_space() 2019-03-15 7:10 [PATCH v2 0/2] ext4: fix buffer references leak problem zhangyi (F) @ 2019-03-15 7:10 ` zhangyi (F) 2019-03-18 11:08 ` Jan Kara 2019-03-23 15:53 ` Theodore Ts'o 2019-03-15 7:10 ` [PATCH v2 2/2] ext4: cleanup bh release code " zhangyi (F) 1 sibling, 2 replies; 7+ messages in thread From: zhangyi (F) @ 2019-03-15 7:10 UTC (permalink / raw) To: linux-ext4; +Cc: tytso, jack, adilger.kernel, yi.zhang, miaoxie All indirect buffers get by ext4_find_shared() should be released no mater the branch should be freed or not. But now, we forget to release the lower depth indirect buffers when removing space from the same higher depth indirect block. It will lead to buffer leak and futher more, it may lead to quota information corruption when using old quota, consider the following case. - Create and mount an empty ext4 filesystem without extent and quota features, - quotacheck and enable the user & group quota, - Create some files and write some data to them, and then punch hole to some files of them, it may trigger the buffer leak problem mentioned above. - Disable quota and run quotacheck again, it will create two new aquota files and write the checked quota information to them, which probably may reuse the freed indirect block(the buffer and page cache was not freed) as data block. - Enable quota again, it will invoke vfs_load_quota_inode()->invalidate_bdev() to try to clean unused buffers and pagecache. Unfortunately, because of the buffer of quota data block is still referenced, quota code cannot read the up to date quota info from the device and lead to quota information corruption. This problem can be reproduced by xfstests generic/231 on ext3 file system or ext4 file system without extent and quota features. This patch fix this problem by brelse the missing indirect buffers, in ext4_ind_remove_space(). Reported-by: Hulk Robot <hulkci@huawei.com> Signed-off-by: zhangyi (F) <yi.zhang@huawei.com> Suggested-by: Jan Kara <jack@suse.cz> Cc: <stable@vger.kernel.org> --- fs/ext4/indirect.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/fs/ext4/indirect.c b/fs/ext4/indirect.c index bf7fa15..9e96a0b 100644 --- a/fs/ext4/indirect.c +++ b/fs/ext4/indirect.c @@ -1387,10 +1387,14 @@ int ext4_ind_remove_space(handle_t *handle, struct inode *inode, partial->p + 1, partial2->p, (chain+n-1) - partial); - BUFFER_TRACE(partial->bh, "call brelse"); - brelse(partial->bh); - BUFFER_TRACE(partial2->bh, "call brelse"); - brelse(partial2->bh); + while (partial > chain) { + BUFFER_TRACE(partial->bh, "call brelse"); + brelse(partial->bh); + } + while (partial2 > chain2) { + BUFFER_TRACE(partial2->bh, "call brelse"); + brelse(partial2->bh); + } return 0; } -- 2.7.4 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/2] ext4: brelse all indirect buffer in ext4_ind_remove_space() 2019-03-15 7:10 ` [PATCH v2 1/2] ext4: brelse all indirect buffer in ext4_ind_remove_space() zhangyi (F) @ 2019-03-18 11:08 ` Jan Kara 2019-03-23 15:53 ` Theodore Ts'o 1 sibling, 0 replies; 7+ messages in thread From: Jan Kara @ 2019-03-18 11:08 UTC (permalink / raw) To: zhangyi (F); +Cc: linux-ext4, tytso, jack, adilger.kernel, miaoxie On Fri 15-03-19 15:10:12, zhangyi (F) wrote: > All indirect buffers get by ext4_find_shared() should be released no > mater the branch should be freed or not. But now, we forget to release > the lower depth indirect buffers when removing space from the same > higher depth indirect block. It will lead to buffer leak and futher > more, it may lead to quota information corruption when using old quota, > consider the following case. > > - Create and mount an empty ext4 filesystem without extent and quota > features, > - quotacheck and enable the user & group quota, > - Create some files and write some data to them, and then punch hole > to some files of them, it may trigger the buffer leak problem > mentioned above. > - Disable quota and run quotacheck again, it will create two new > aquota files and write the checked quota information to them, which > probably may reuse the freed indirect block(the buffer and page > cache was not freed) as data block. > - Enable quota again, it will invoke > vfs_load_quota_inode()->invalidate_bdev() to try to clean unused > buffers and pagecache. Unfortunately, because of the buffer of quota > data block is still referenced, quota code cannot read the up to date > quota info from the device and lead to quota information corruption. > > This problem can be reproduced by xfstests generic/231 on ext3 file > system or ext4 file system without extent and quota features. > > This patch fix this problem by brelse the missing indirect buffers, in > ext4_ind_remove_space(). > > Reported-by: Hulk Robot <hulkci@huawei.com> > Signed-off-by: zhangyi (F) <yi.zhang@huawei.com> > Suggested-by: Jan Kara <jack@suse.cz> > Cc: <stable@vger.kernel.org> Looks good. You can add: Reviewed-by: Jan Kara <jack@suse.cz> Honza > --- > fs/ext4/indirect.c | 12 ++++++++---- > 1 file changed, 8 insertions(+), 4 deletions(-) > > diff --git a/fs/ext4/indirect.c b/fs/ext4/indirect.c > index bf7fa15..9e96a0b 100644 > --- a/fs/ext4/indirect.c > +++ b/fs/ext4/indirect.c > @@ -1387,10 +1387,14 @@ int ext4_ind_remove_space(handle_t *handle, struct inode *inode, > partial->p + 1, > partial2->p, > (chain+n-1) - partial); > - BUFFER_TRACE(partial->bh, "call brelse"); > - brelse(partial->bh); > - BUFFER_TRACE(partial2->bh, "call brelse"); > - brelse(partial2->bh); > + while (partial > chain) { > + BUFFER_TRACE(partial->bh, "call brelse"); > + brelse(partial->bh); > + } > + while (partial2 > chain2) { > + BUFFER_TRACE(partial2->bh, "call brelse"); > + brelse(partial2->bh); > + } > return 0; > } > > -- > 2.7.4 > -- Jan Kara <jack@suse.com> SUSE Labs, CR ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/2] ext4: brelse all indirect buffer in ext4_ind_remove_space() 2019-03-15 7:10 ` [PATCH v2 1/2] ext4: brelse all indirect buffer in ext4_ind_remove_space() zhangyi (F) 2019-03-18 11:08 ` Jan Kara @ 2019-03-23 15:53 ` Theodore Ts'o 1 sibling, 0 replies; 7+ messages in thread From: Theodore Ts'o @ 2019-03-23 15:53 UTC (permalink / raw) To: zhangyi (F); +Cc: linux-ext4, jack, adilger.kernel, miaoxie On Fri, Mar 15, 2019 at 03:10:12PM +0800, zhangyi (F) wrote: > All indirect buffers get by ext4_find_shared() should be released no > mater the branch should be freed or not. But now, we forget to release > the lower depth indirect buffers when removing space from the same > higher depth indirect block. It will lead to buffer leak and futher > more, it may lead to quota information corruption when using old quota, > consider the following case. > > - Create and mount an empty ext4 filesystem without extent and quota > features, > - quotacheck and enable the user & group quota, > - Create some files and write some data to them, and then punch hole > to some files of them, it may trigger the buffer leak problem > mentioned above. > - Disable quota and run quotacheck again, it will create two new > aquota files and write the checked quota information to them, which > probably may reuse the freed indirect block(the buffer and page > cache was not freed) as data block. > - Enable quota again, it will invoke > vfs_load_quota_inode()->invalidate_bdev() to try to clean unused > buffers and pagecache. Unfortunately, because of the buffer of quota > data block is still referenced, quota code cannot read the up to date > quota info from the device and lead to quota information corruption. > > This problem can be reproduced by xfstests generic/231 on ext3 file > system or ext4 file system without extent and quota features. > > This patch fix this problem by brelse the missing indirect buffers, in > ext4_ind_remove_space(). > > Reported-by: Hulk Robot <hulkci@huawei.com> > Signed-off-by: zhangyi (F) <yi.zhang@huawei.com> > Suggested-by: Jan Kara <jack@suse.cz> > Cc: <stable@vger.kernel.org> Thanks, applied. - Ted ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 2/2] ext4: cleanup bh release code in ext4_ind_remove_space() 2019-03-15 7:10 [PATCH v2 0/2] ext4: fix buffer references leak problem zhangyi (F) 2019-03-15 7:10 ` [PATCH v2 1/2] ext4: brelse all indirect buffer in ext4_ind_remove_space() zhangyi (F) @ 2019-03-15 7:10 ` zhangyi (F) 2019-03-18 11:10 ` Jan Kara 2019-03-23 15:58 ` Theodore Ts'o 1 sibling, 2 replies; 7+ messages in thread From: zhangyi (F) @ 2019-03-15 7:10 UTC (permalink / raw) To: linux-ext4; +Cc: tytso, jack, adilger.kernel, yi.zhang, miaoxie Currently, we are releasing the indirect buffer where we are done with it in ext4_ind_remove_space(), so we can see the brelse() and BUFFER_TRACE() everywhere. It seems fragile and hard to read, and we may probably forget to release the buffer some day. This patch do some cleanup stuff, put all the releasing code together to the end of this function. Signed-off-by: zhangyi (F) <yi.zhang@huawei.com> --- fs/ext4/indirect.c | 47 ++++++++++++++++++++++------------------------- 1 file changed, 22 insertions(+), 25 deletions(-) diff --git a/fs/ext4/indirect.c b/fs/ext4/indirect.c index 9e96a0b..e1801b2 100644 --- a/fs/ext4/indirect.c +++ b/fs/ext4/indirect.c @@ -1219,6 +1219,7 @@ int ext4_ind_remove_space(handle_t *handle, struct inode *inode, ext4_lblk_t offsets[4], offsets2[4]; Indirect chain[4], chain2[4]; Indirect *partial, *partial2; + Indirect *p = NULL, *p2 = NULL; ext4_lblk_t max_block; __le32 nr = 0, nr2 = 0; int n = 0, n2 = 0; @@ -1260,7 +1261,7 @@ int ext4_ind_remove_space(handle_t *handle, struct inode *inode, } - partial = ext4_find_shared(inode, n, offsets, chain, &nr); + partial = p = ext4_find_shared(inode, n, offsets, chain, &nr); if (nr) { if (partial == chain) { /* Shared branch grows from the inode */ @@ -1285,13 +1286,11 @@ int ext4_ind_remove_space(handle_t *handle, struct inode *inode, partial->p + 1, (__le32 *)partial->bh->b_data+addr_per_block, (chain+n-1) - partial); - BUFFER_TRACE(partial->bh, "call brelse"); - brelse(partial->bh); partial--; } end_range: - partial2 = ext4_find_shared(inode, n2, offsets2, chain2, &nr2); + partial2 = p2 = ext4_find_shared(inode, n2, offsets2, chain2, &nr2); if (nr2) { if (partial2 == chain2) { /* @@ -1321,16 +1320,14 @@ int ext4_ind_remove_space(handle_t *handle, struct inode *inode, (__le32 *)partial2->bh->b_data, partial2->p, (chain2+n2-1) - partial2); - BUFFER_TRACE(partial2->bh, "call brelse"); - brelse(partial2->bh); partial2--; } goto do_indirects; } /* Punch happened within the same level (n == n2) */ - partial = ext4_find_shared(inode, n, offsets, chain, &nr); - partial2 = ext4_find_shared(inode, n2, offsets2, chain2, &nr2); + partial = p = ext4_find_shared(inode, n, offsets, chain, &nr); + partial2 = p2 = ext4_find_shared(inode, n2, offsets2, chain2, &nr2); /* Free top, but only if partial2 isn't its subtree. */ if (nr) { @@ -1387,15 +1384,7 @@ int ext4_ind_remove_space(handle_t *handle, struct inode *inode, partial->p + 1, partial2->p, (chain+n-1) - partial); - while (partial > chain) { - BUFFER_TRACE(partial->bh, "call brelse"); - brelse(partial->bh); - } - while (partial2 > chain2) { - BUFFER_TRACE(partial2->bh, "call brelse"); - brelse(partial2->bh); - } - return 0; + goto cleanup; } /* @@ -1410,8 +1399,6 @@ int ext4_ind_remove_space(handle_t *handle, struct inode *inode, partial->p + 1, (__le32 *)partial->bh->b_data+addr_per_block, (chain+n-1) - partial); - BUFFER_TRACE(partial->bh, "call brelse"); - brelse(partial->bh); partial--; } if (partial2 > chain2 && depth2 <= depth) { @@ -1419,11 +1406,21 @@ int ext4_ind_remove_space(handle_t *handle, struct inode *inode, (__le32 *)partial2->bh->b_data, partial2->p, (chain2+n2-1) - partial2); - BUFFER_TRACE(partial2->bh, "call brelse"); - brelse(partial2->bh); partial2--; } } + +cleanup: + while (p && p > chain) { + BUFFER_TRACE(p->bh, "call brelse"); + brelse(p->bh); + p--; + } + while (p2 && p2 > chain2) { + BUFFER_TRACE(p2->bh, "call brelse"); + brelse(p2->bh); + p2--; + } return 0; do_indirects: @@ -1431,7 +1428,7 @@ int ext4_ind_remove_space(handle_t *handle, struct inode *inode, switch (offsets[0]) { default: if (++n >= n2) - return 0; + break; nr = i_data[EXT4_IND_BLOCK]; if (nr) { ext4_free_branches(handle, inode, NULL, &nr, &nr+1, 1); @@ -1439,7 +1436,7 @@ int ext4_ind_remove_space(handle_t *handle, struct inode *inode, } case EXT4_IND_BLOCK: if (++n >= n2) - return 0; + break; nr = i_data[EXT4_DIND_BLOCK]; if (nr) { ext4_free_branches(handle, inode, NULL, &nr, &nr+1, 2); @@ -1447,7 +1444,7 @@ int ext4_ind_remove_space(handle_t *handle, struct inode *inode, } case EXT4_DIND_BLOCK: if (++n >= n2) - return 0; + break; nr = i_data[EXT4_TIND_BLOCK]; if (nr) { ext4_free_branches(handle, inode, NULL, &nr, &nr+1, 3); @@ -1456,5 +1453,5 @@ int ext4_ind_remove_space(handle_t *handle, struct inode *inode, case EXT4_TIND_BLOCK: ; } - return 0; + goto cleanup; } -- 2.7.4 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2 2/2] ext4: cleanup bh release code in ext4_ind_remove_space() 2019-03-15 7:10 ` [PATCH v2 2/2] ext4: cleanup bh release code " zhangyi (F) @ 2019-03-18 11:10 ` Jan Kara 2019-03-23 15:58 ` Theodore Ts'o 1 sibling, 0 replies; 7+ messages in thread From: Jan Kara @ 2019-03-18 11:10 UTC (permalink / raw) To: zhangyi (F); +Cc: linux-ext4, tytso, jack, adilger.kernel, miaoxie On Fri 15-03-19 15:10:13, zhangyi (F) wrote: > Currently, we are releasing the indirect buffer where we are done with > it in ext4_ind_remove_space(), so we can see the brelse() and > BUFFER_TRACE() everywhere. It seems fragile and hard to read, and we > may probably forget to release the buffer some day. This patch do some > cleanup stuff, put all the releasing code together to the end of this > function. > > Signed-off-by: zhangyi (F) <yi.zhang@huawei.com> OK, now when the cleanup is separate, I actually like it. So feel free to add: Reviewed-by: Jan Kara <jack@suse.cz> Honza > --- > fs/ext4/indirect.c | 47 ++++++++++++++++++++++------------------------- > 1 file changed, 22 insertions(+), 25 deletions(-) > > diff --git a/fs/ext4/indirect.c b/fs/ext4/indirect.c > index 9e96a0b..e1801b2 100644 > --- a/fs/ext4/indirect.c > +++ b/fs/ext4/indirect.c > @@ -1219,6 +1219,7 @@ int ext4_ind_remove_space(handle_t *handle, struct inode *inode, > ext4_lblk_t offsets[4], offsets2[4]; > Indirect chain[4], chain2[4]; > Indirect *partial, *partial2; > + Indirect *p = NULL, *p2 = NULL; > ext4_lblk_t max_block; > __le32 nr = 0, nr2 = 0; > int n = 0, n2 = 0; > @@ -1260,7 +1261,7 @@ int ext4_ind_remove_space(handle_t *handle, struct inode *inode, > } > > > - partial = ext4_find_shared(inode, n, offsets, chain, &nr); > + partial = p = ext4_find_shared(inode, n, offsets, chain, &nr); > if (nr) { > if (partial == chain) { > /* Shared branch grows from the inode */ > @@ -1285,13 +1286,11 @@ int ext4_ind_remove_space(handle_t *handle, struct inode *inode, > partial->p + 1, > (__le32 *)partial->bh->b_data+addr_per_block, > (chain+n-1) - partial); > - BUFFER_TRACE(partial->bh, "call brelse"); > - brelse(partial->bh); > partial--; > } > > end_range: > - partial2 = ext4_find_shared(inode, n2, offsets2, chain2, &nr2); > + partial2 = p2 = ext4_find_shared(inode, n2, offsets2, chain2, &nr2); > if (nr2) { > if (partial2 == chain2) { > /* > @@ -1321,16 +1320,14 @@ int ext4_ind_remove_space(handle_t *handle, struct inode *inode, > (__le32 *)partial2->bh->b_data, > partial2->p, > (chain2+n2-1) - partial2); > - BUFFER_TRACE(partial2->bh, "call brelse"); > - brelse(partial2->bh); > partial2--; > } > goto do_indirects; > } > > /* Punch happened within the same level (n == n2) */ > - partial = ext4_find_shared(inode, n, offsets, chain, &nr); > - partial2 = ext4_find_shared(inode, n2, offsets2, chain2, &nr2); > + partial = p = ext4_find_shared(inode, n, offsets, chain, &nr); > + partial2 = p2 = ext4_find_shared(inode, n2, offsets2, chain2, &nr2); > > /* Free top, but only if partial2 isn't its subtree. */ > if (nr) { > @@ -1387,15 +1384,7 @@ int ext4_ind_remove_space(handle_t *handle, struct inode *inode, > partial->p + 1, > partial2->p, > (chain+n-1) - partial); > - while (partial > chain) { > - BUFFER_TRACE(partial->bh, "call brelse"); > - brelse(partial->bh); > - } > - while (partial2 > chain2) { > - BUFFER_TRACE(partial2->bh, "call brelse"); > - brelse(partial2->bh); > - } > - return 0; > + goto cleanup; > } > > /* > @@ -1410,8 +1399,6 @@ int ext4_ind_remove_space(handle_t *handle, struct inode *inode, > partial->p + 1, > (__le32 *)partial->bh->b_data+addr_per_block, > (chain+n-1) - partial); > - BUFFER_TRACE(partial->bh, "call brelse"); > - brelse(partial->bh); > partial--; > } > if (partial2 > chain2 && depth2 <= depth) { > @@ -1419,11 +1406,21 @@ int ext4_ind_remove_space(handle_t *handle, struct inode *inode, > (__le32 *)partial2->bh->b_data, > partial2->p, > (chain2+n2-1) - partial2); > - BUFFER_TRACE(partial2->bh, "call brelse"); > - brelse(partial2->bh); > partial2--; > } > } > + > +cleanup: > + while (p && p > chain) { > + BUFFER_TRACE(p->bh, "call brelse"); > + brelse(p->bh); > + p--; > + } > + while (p2 && p2 > chain2) { > + BUFFER_TRACE(p2->bh, "call brelse"); > + brelse(p2->bh); > + p2--; > + } > return 0; > > do_indirects: > @@ -1431,7 +1428,7 @@ int ext4_ind_remove_space(handle_t *handle, struct inode *inode, > switch (offsets[0]) { > default: > if (++n >= n2) > - return 0; > + break; > nr = i_data[EXT4_IND_BLOCK]; > if (nr) { > ext4_free_branches(handle, inode, NULL, &nr, &nr+1, 1); > @@ -1439,7 +1436,7 @@ int ext4_ind_remove_space(handle_t *handle, struct inode *inode, > } > case EXT4_IND_BLOCK: > if (++n >= n2) > - return 0; > + break; > nr = i_data[EXT4_DIND_BLOCK]; > if (nr) { > ext4_free_branches(handle, inode, NULL, &nr, &nr+1, 2); > @@ -1447,7 +1444,7 @@ int ext4_ind_remove_space(handle_t *handle, struct inode *inode, > } > case EXT4_DIND_BLOCK: > if (++n >= n2) > - return 0; > + break; > nr = i_data[EXT4_TIND_BLOCK]; > if (nr) { > ext4_free_branches(handle, inode, NULL, &nr, &nr+1, 3); > @@ -1456,5 +1453,5 @@ int ext4_ind_remove_space(handle_t *handle, struct inode *inode, > case EXT4_TIND_BLOCK: > ; > } > - return 0; > + goto cleanup; > } > -- > 2.7.4 > -- Jan Kara <jack@suse.com> SUSE Labs, CR ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 2/2] ext4: cleanup bh release code in ext4_ind_remove_space() 2019-03-15 7:10 ` [PATCH v2 2/2] ext4: cleanup bh release code " zhangyi (F) 2019-03-18 11:10 ` Jan Kara @ 2019-03-23 15:58 ` Theodore Ts'o 1 sibling, 0 replies; 7+ messages in thread From: Theodore Ts'o @ 2019-03-23 15:58 UTC (permalink / raw) To: zhangyi (F); +Cc: linux-ext4, jack, adilger.kernel, miaoxie On Fri, Mar 15, 2019 at 03:10:13PM +0800, zhangyi (F) wrote: > Currently, we are releasing the indirect buffer where we are done with > it in ext4_ind_remove_space(), so we can see the brelse() and > BUFFER_TRACE() everywhere. It seems fragile and hard to read, and we > may probably forget to release the buffer some day. This patch do some > cleanup stuff, put all the releasing code together to the end of this > function. > > Signed-off-by: zhangyi (F) <yi.zhang@huawei.com> Thanks, applied. - Ted ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-03-23 15:59 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-03-15 7:10 [PATCH v2 0/2] ext4: fix buffer references leak problem zhangyi (F) 2019-03-15 7:10 ` [PATCH v2 1/2] ext4: brelse all indirect buffer in ext4_ind_remove_space() zhangyi (F) 2019-03-18 11:08 ` Jan Kara 2019-03-23 15:53 ` Theodore Ts'o 2019-03-15 7:10 ` [PATCH v2 2/2] ext4: cleanup bh release code " zhangyi (F) 2019-03-18 11:10 ` Jan Kara 2019-03-23 15:58 ` Theodore Ts'o
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).