Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.5 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_PASS,URIBL_BLOCKED, USER_AGENT_MUTT autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 5FE93C43381 for ; Wed, 13 Mar 2019 17:34:42 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 2AF7B2075C for ; Wed, 13 Mar 2019 17:34:42 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726485AbfCMRel (ORCPT ); Wed, 13 Mar 2019 13:34:41 -0400 Received: from mx2.suse.de ([195.135.220.15]:53190 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1726033AbfCMRel (ORCPT ); Wed, 13 Mar 2019 13:34:41 -0400 X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.220.254]) by mx1.suse.de (Postfix) with ESMTP id 4BDCCAD5D; Wed, 13 Mar 2019 17:34:38 +0000 (UTC) Received: by quack2.suse.cz (Postfix, from userid 1000) id 248801E3FE8; Wed, 13 Mar 2019 18:34:37 +0100 (CET) Date: Wed, 13 Mar 2019 18:34:37 +0100 From: Jan Kara To: "zhangyi (F)" Cc: linux-ext4@vger.kernel.org, tytso@mit.edu, jack@suse.cz, adilger.kernel@dilger.ca, miaoxie@huawei.com Subject: Re: [PATCH] ext4: brelse all indirect buffers in ext4_ind_remove_space() Message-ID: <20190313173437.GL9108@quack2.suse.cz> References: <1552137547-115352-1-git-send-email-yi.zhang@huawei.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1552137547-115352-1-git-send-email-yi.zhang@huawei.com> User-Agent: Mutt/1.10.1 (2018-07-13) Sender: linux-ext4-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-ext4@vger.kernel.org On Sat 09-03-19 21:19:07, 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 filesystem without extent and quota feature. > > This patch fix this problem by brelse all indirect buffers, and also > do some cleanup of the brelse code in ext4_ind_remove_space(). > > Reported-by: Hulk Robot > Signed-off-by: zhangyi (F) Thanks for the report and the patch! I think it is correct but I'd find it simpler (not only the patch as such but also the resulting code in the function) to just fix that one place that seems to leak bh references. Something like: if (partial > chain && partial2 > chain2 && partial->bh->b_blocknr == partial2->bh->b_blocknr) { /* * We've converged on the same block. Clear the range, * then we're done. */ ext4_free_branches(handle, inode, partial->bh, 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; } Or is there some other problem I miss? Honza > --- > fs/ext4/indirect.c | 43 ++++++++++++++++++++++--------------------- > 1 file changed, 22 insertions(+), 21 deletions(-) > > diff --git a/fs/ext4/indirect.c b/fs/ext4/indirect.c > index bf7fa15..6f3f7d5 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,11 +1384,7 @@ 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); > - return 0; > + goto clean_up; > } > > /* > @@ -1406,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) { > @@ -1415,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--; > } > } > + > +clean_up: > + 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: > @@ -1427,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); > @@ -1435,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); > @@ -1443,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); > @@ -1452,5 +1453,5 @@ int ext4_ind_remove_space(handle_t *handle, struct inode *inode, > case EXT4_TIND_BLOCK: > ; > } > - return 0; > + goto clean_up; > } > -- > 2.7.4 > -- Jan Kara SUSE Labs, CR