From patchwork Tue Jul 31 19:50:28 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Theodore Ts'o X-Patchwork-Id: 174325 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 C8F432C01E5 for ; Wed, 1 Aug 2012 05:50:38 +1000 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754012Ab2GaTug (ORCPT ); Tue, 31 Jul 2012 15:50:36 -0400 Received: from li9-11.members.linode.com ([67.18.176.11]:42346 "EHLO imap.thunk.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751735Ab2GaTug (ORCPT ); Tue, 31 Jul 2012 15:50:36 -0400 Received: from root (helo=closure.thunk.org) by imap.thunk.org with local-esmtp (Exim 4.72) (envelope-from ) id 1SwISa-0005D8-6g; Tue, 31 Jul 2012 19:50:28 +0000 Received: by closure.thunk.org (Postfix, from userid 15806) id B474E241F09; Tue, 31 Jul 2012 15:50:28 -0400 (EDT) Date: Tue, 31 Jul 2012 15:50:28 -0400 From: Theodore Ts'o To: Zheng Liu Cc: linux-ext4@vger.kernel.org, "Darrick J. Wong" , Zheng Liu Subject: Re: [PATCH] e2fsck: delay metadata checksum in pass1 Message-ID: <20120731195028.GC32228@thunk.org> References: <1340937356-12493-1-git-send-email-wenqing.lz@taobao.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <1340937356-12493-1-git-send-email-wenqing.lz@taobao.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 Fri, Jun 29, 2012 at 10:35:56AM +0800, Zheng Liu wrote: > From: Zheng Liu > > in __ext4_get_inode_loc, when all other inodes are free, we will skip I/O. > Thus, all of inodes in this block are set to 0. Then when we scan these inodes > in pass1, we will get a metadata checksum error. However, we don't need to scan > these inodes because they have been freed. > > This bug can be reproduced by xfstests #013. > > Reported-by: Tao Ma > Cc: Darrick J. Wong > Cc: "Theodore Ts'o" > Signed-off-by: Zheng Liu The problem with this patch is that it means that we're not checking the checksums of some of the reserved inodes (since they happen before the point where the check has been moved, and some of them --- including the journal inode --- have their own special case code and they then continue out). It also doesn't solve the problem with other potential users of ext2fs_get_next_inode(). So I chose to fix the problem in ext2fs_inode_csum_verify() instead. Regards, - Ted commit 4f0ba51ece06286deed18b86ef9b154d602dd9c6 Author: Theodore Ts'o Date: Tue Jul 31 15:27:29 2012 -0400 libext2fs: when checking the inode's checksum, allow an all-zero inode When the kernel writes an inode where all of the other inodes in in the inode table (itable) block are unused, it skips reading the itable block from disk, and instead uses an all zeros block. This can cause e2fsck to complain when it iterates over the inodes using ext2fs_get_next_inode() since the inode apparently has an invalid checksum. Normally the inode won't be returned at all if it is at the end of the block group's part of the inode table, thanks to the bg_itable_unused field. But it's possible for this situation to happen earlier in the inode table block. Fix this by changing ext2fs_inode_csum_verify() to allow the inode to be all zero's; if the checksum fails, and the inode is all zero's, treat it as a valid checksum. Reported-by: Zheng Liu Signed-off-by: "Theodore Ts'o" --- 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/lib/ext2fs/csum.c b/lib/ext2fs/csum.c index 86f5fd2..9196903 100644 --- a/lib/ext2fs/csum.c +++ b/lib/ext2fs/csum.c @@ -667,7 +667,8 @@ int ext2fs_inode_csum_verify(ext2_filsys fs, ext2_ino_t inum, { errcode_t retval; __u32 provided, calculated; - int has_hi; + int i, has_hi; + char *cp; if (fs->super->s_creator_os != EXT2_OS_LINUX || !EXT2_HAS_RO_COMPAT_FEATURE(fs->super, @@ -687,7 +688,23 @@ int ext2fs_inode_csum_verify(ext2_filsys fs, ext2_ino_t inum, } else calculated &= 0xFFFF; - return provided == calculated; + if (provided == calculated) + return 1; + + /* + * If the checksum didn't match, it's possible it was due to + * the inode being all zero's. It's unlikely this is the + * case, but it can happen. So check for it here. (We only + * check the base inode since that's good enough, and it's not + * worth the bother to figure out how much of the extended + * inode, if any, is present.) + */ + for (cp = (char *) inode, i = 0; + i < sizeof(struct ext2_inode); + cp++, i++) + if (*cp) + return 0; + return 1; /* Inode must have been all zero's */ } errcode_t ext2fs_inode_csum_set(ext2_filsys fs, ext2_ino_t inum,