From patchwork Tue Dec 3 00:25:03 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Theodore Ts'o X-Patchwork-Id: 296042 Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id BEA812C009F for ; Tue, 3 Dec 2013 11:25:14 +1100 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751729Ab3LCAZN (ORCPT ); Mon, 2 Dec 2013 19:25:13 -0500 Received: from imap.thunk.org ([74.207.234.97]:35776 "EHLO imap.thunk.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751728Ab3LCAZM (ORCPT ); Mon, 2 Dec 2013 19:25:12 -0500 Received: from root (helo=closure.thunk.org) by imap.thunk.org with local-esmtp (Exim 4.80) (envelope-from ) id 1VndnV-0004sf-9o; Tue, 03 Dec 2013 00:25:05 +0000 Received: by closure.thunk.org (Postfix, from userid 15806) id 329AF580949; Mon, 2 Dec 2013 19:25:03 -0500 (EST) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=thunk.org; s=mail; t=1386030303; bh=QtmsSwkGydfJf+WzQ4YlTj4RK0Iczc1NRB/cUUREVYw=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=BcTjxLZpTue2WJFiu1m6I2i9B9vSDgHTL+eVniPPCDDdagaxuB1XswBEx0587vFMj gTJXwZAQ3Fv2OU4gJiq8MRwnyJVF8bLygugtwlbdc6GITraM9PY2tM6e46IKlVnfpx e8Yw0ZPsyjFb6rN7vePjApKYWJK6MakXfjGmF38A= Date: Mon, 2 Dec 2013 19:25:03 -0500 From: Theodore Ts'o To: Kit Westneat Cc: "linux-ext4@vger.kernel.org" , "Dilger, Andreas" Subject: Re: [PATCH] e2fsck: blk64_t to blk_t truncation Message-ID: <20131203002503.GA18601@thunk.org> References: <529CF0D2.2010707@ddn.com> <529CF1E8.3010003@ddn.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <529CF1E8.3010003@ddn.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-SA-Exim-Connect-IP: X-SA-Exim-Mail-From: tytso@thunk.org X-SA-Exim-Scanned: No (on imap.thunk.org); SAEximRunCond expanded to false Sender: linux-ext4-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-ext4@vger.kernel.org On Mon, Dec 02, 2013 at 03:47:36PM -0500, Kit Westneat wrote: > > We recently ran into an issue where e2fsck was claiming to clear > deleted/unused inodes, but was not actually doing it. We tracked it > down to a bad blk64_t to blk_t conversion in pass2 where > ext2fs_write_dir_block is being called instead of > ext2fs_write_dir_block3. There is another similar call in > allocate_dir_block. These appear to be fixed in master as part of > the move to adding checksums to the end of directory leaf nodes, but > it's still an issue on maint. > > I ran gcc with -Wconversion and found a number of other places that > have blk64_t to blk_t truncations, but I'm not sure how many of them > are actually problems. For example in block_iterate_ind, block_nr is > blk_t but it seems to be a specific choice, since there is also the > blk64 variable. Another instance is with the EA refcounting, which > is all 32-bit. In expand_dir_proc in pass3.c, it uses the 32-bit > ext2fs_write_dir_block. Against 1.42.7, gcc reports 103 blk64_t to > blk_t conversions, so this is just a small sample. We can only reference 32-bit block numbers using the indirect block scheme, so that's probably OK. The EA refcounting is definitely a problem. As is the usages of expand_dir_proc in e2fsck/pass3.c. Thanks for pointing that out. We'll need to do an audit with -Wconversion. Unfortunately it is quite noisy, as you've noted. I took a quick check about whether we could add -Wconversion to the list of things which "make gcc-wall" would use, but the problem is that it's quite noisy about things things things such as the bit swapping and bit operations in bitops.h. i've expanded your patch to fix up all of the useages of ext2fs_write_dir_block() in e2fsck and applied it to my maint branch. Thanks for pointing this out! - Ted commit 7bef6d52125ef3f1ef07d9da71a13546f6843c56 Author: Kit Westneat Date: Mon Dec 2 19:11:52 2013 -0500 e2fsck: use ext2fs_write_dir_block3() instead of ext2fs_write_dir_block() The use of ext2fs_write_dir_block() meant that attempts to fix deleted/unused inodes in a directory would not be fixed for file systems with 64-bit block numbers. (And some random block with the high 32-bits cleared would get corrupted.) Fix a similar problem when expanding directories and when creating the lost+found dirctory. Signed-off-by: Kit Westneat Signed-off-by: "Theodore Ts'o" Reviewed-by: Darrick J. Wong --- 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/e2fsck/pass2.c b/e2fsck/pass2.c index bceadfe..f2ac2dd 100644 --- a/e2fsck/pass2.c +++ b/e2fsck/pass2.c @@ -1132,7 +1132,7 @@ out_htree: } } if (dir_modified) { - cd->pctx.errcode = ext2fs_write_dir_block(fs, block_nr, buf); + cd->pctx.errcode = ext2fs_write_dir_block3(fs, block_nr, buf, 0); if (cd->pctx.errcode) { if (!fix_problem(ctx, PR_2_WRITE_DIRBLOCK, &cd->pctx)) @@ -1455,7 +1455,7 @@ static int allocate_dir_block(e2fsck_t ctx, return 1; } - pctx->errcode = ext2fs_write_dir_block(fs, blk, block); + pctx->errcode = ext2fs_write_dir_block3(fs, blk, block, 0); ext2fs_free_mem(&block); if (pctx->errcode) { pctx->str = "ext2fs_write_dir_block"; diff --git a/e2fsck/pass3.c b/e2fsck/pass3.c index e358bb2..926f462 100644 --- a/e2fsck/pass3.c +++ b/e2fsck/pass3.c @@ -198,9 +198,9 @@ static void check_root(e2fsck_t ctx) return; } - pctx.errcode = ext2fs_write_dir_block(fs, blk, block); + pctx.errcode = ext2fs_write_dir_block3(fs, blk, block, 0); if (pctx.errcode) { - pctx.str = "ext2fs_write_dir_block"; + pctx.str = "ext2fs_write_dir_block3"; fix_problem(ctx, PR_3_CREATE_ROOT_ERROR, &pctx); ctx->flags |= E2F_FLAG_ABORT; return; @@ -444,7 +444,7 @@ ext2_ino_t e2fsck_get_lost_and_found(e2fsck_t ctx, int fix) return 0; } - retval = ext2fs_write_dir_block(fs, blk, block); + retval = ext2fs_write_dir_block3(fs, blk, block, 0); ext2fs_free_mem(&block); if (retval) { pctx.errcode = retval; @@ -725,7 +725,7 @@ static int expand_dir_proc(ext2_filsys fs, return BLOCK_ABORT; } es->num--; - retval = ext2fs_write_dir_block(fs, new_blk, block); + retval = ext2fs_write_dir_block3(fs, new_blk, block, 0); } else { retval = ext2fs_get_mem(fs->blocksize, &block); if (retval) {