From patchwork Tue Oct 22 18:40:30 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Eryu Guan X-Patchwork-Id: 285482 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 B5F1C2C009E for ; Wed, 23 Oct 2013 05:40:36 +1100 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753785Ab3JVSkf (ORCPT ); Tue, 22 Oct 2013 14:40:35 -0400 Received: from mail-pa0-f42.google.com ([209.85.220.42]:43347 "EHLO mail-pa0-f42.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751684Ab3JVSkf (ORCPT ); Tue, 22 Oct 2013 14:40:35 -0400 Received: by mail-pa0-f42.google.com with SMTP id kx10so10276306pab.15 for ; Tue, 22 Oct 2013 11:40:34 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=date:from:to:cc:subject:message-id:references:mime-version :content-type:content-disposition:content-transfer-encoding :in-reply-to:user-agent; bh=7iyRF3NvGIYfT5Pf1TVuG0ApGhP56fTYeoxJSl2/3Ic=; b=Z0quhhFWkTHEQuxBe9PS2gfrMts0hFUJzCy0K8HPBP2lwqMBYXQgv9p2PZN2+5bH53 DuL/oFJAjXy4SsleQ/1vfDAZIf6WerT79e3pQ8fzIjq3eJIPdpmQEqfBbqUyevRhAjsg EMHggU5mTRqMeuCiumTx3mdUa3wmvF61ETQ3MhQ64mlGTUd5FkmWyIi/mIDGPAEb5bzP w7TXiZeghS/2HgYS4F+IGoz+GVwzyblm6SzqhH+FCY2R/aA54+2/5JNb2Vc1mbeMraUJ hKW0lQ5SwiArH1rMNc2qUMXLDbWWR3YST+fSYa5el7VTH49ThVzy12FcPSJXeB9hCyW/ RCmw== X-Received: by 10.68.203.34 with SMTP id kn2mr24138653pbc.82.1382467234551; Tue, 22 Oct 2013 11:40:34 -0700 (PDT) Received: from localhost ([111.199.171.190]) by mx.google.com with ESMTPSA id qn1sm29151081pbc.34.2013.10.22.11.40.32 for (version=TLSv1.2 cipher=AES128-GCM-SHA256 bits=128/128); Tue, 22 Oct 2013 11:40:33 -0700 (PDT) Date: Wed, 23 Oct 2013 02:40:30 +0800 From: Eryu Guan To: =?utf-8?B?THVrw6HFoQ==?= Czerner Cc: linux-ext4@vger.kernel.org, Theodore Ts'o Subject: Re: [PATCH v2] ext4: check for overlapping extents in ext4_valid_extent_entries() Message-ID: <20131022184030.GC2708@dhcp-13-216.nay.redhat.com> References: <1382002073-27862-1-git-send-email-guaneryu@gmail.com> <1382275647-4616-1-git-send-email-guaneryu@gmail.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-ext4-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-ext4@vger.kernel.org On Mon, Oct 21, 2013 at 06:06:23PM +0200, Lukáš Czerner wrote: > On Sun, 20 Oct 2013, Eryu Guan wrote: ... > > > > diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c > > index c9ebcb9..855b11d 100644 > > --- a/fs/ext4/extents.c > > +++ b/fs/ext4/extents.c > > @@ -387,11 +387,21 @@ static int ext4_valid_extent_entries(struct inode *inode, > > if (depth == 0) { > > /* leaf entries */ > > struct ext4_extent *ext = EXT_FIRST_EXTENT(eh); > > + ext4_lblk_t block = 0; > > + ext4_lblk_t prev = 0; > > + int len = 0; > > while (entries) { > > if (!ext4_valid_extent(inode, ext)) > > return 0; > > + > > + /* Check for overlapping extents */ > > + block = le32_to_cpu(ext->ee_block); > > + len = ext4_ext_get_actual_len(ext); > > + if ((block <= prev) && prev) > > Both ext4_valid_extent() and ext4_valid_extent_idx() are setting > s_last_error_block in the case of error. Maybe we should to the same > here ? Note that the block saved in that variable is physical, not > logical. I think that makes sense, it's better to keep the consistency. But it seems that the s_last_error_block will eventually be overwritten by ext4_error_inode() in __ext4_ext_check() ? > > Also I am curious what happens when one of the extents is corrupted > in such a way that it crosses the 16TB boundary ? In this case the > check would not recognise that since prev will underflow, but maybe > something else catches that ? Do you mean that a previous (ee_block + len - 1) could cross the 2**32 boundary? I think we can add another check in ext4_valid_extent() for this situation. I update the patch to a v3 version, could you please review again? Thanks a lot! Eryu Guan --- From 467025c05bce3ee44e607887bc7cb74ff1bfefcb Mon Sep 17 00:00:00 2001 From: Eryu Guan Date: Thu, 17 Oct 2013 23:57:22 +0800 Subject: [PATCH v3] ext4: check for overlapping extents in ext4_valid_extent_entries() A corrupted ext4 may have out of order leaf extents, i.e. extent: lblk 0--1023, len 1024, pblk 9217, flags: LEAF UNINIT extent: lblk 1000--2047, len 1024, pblk 10241, flags: LEAF UNINIT ^^^^ overlap with previous extent Reading such extent could hit BUG_ON() in ext4_es_cache_extent(). BUG_ON(end < lblk); The problem is that __read_extent_tree_block() tries to cache holes as well but assumes 'lblk' is greater than 'prev' and passes underflowed length to ext4_es_cache_extent(). Fix it by checking for overlapping extents in ext4_valid_extent_entries(). I hit this when fuzz testing ext4, and am able to reproduce it by modifying the on-disk extent by hand. Also add the check for (ee_block + len - 1) in ext4_valid_extent() to make sure the value is not overflow. Ran xfstests on patched ext4 and no regression. Cc: "Theodore Ts'o" Cc: Lukáš Czerner Signed-off-by: Eryu Guan --- v3: Address comments from Lukas - set s_last_error_block when there's overlapping extents found - check for (ee_block + len - 1) in ext4_valid_extent(), value should not overflow v2: - check for overlapping extents explicitly not hide the corruption fs/ext4/extents.c | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c index c9ebcb9..85d977f 100644 --- a/fs/ext4/extents.c +++ b/fs/ext4/extents.c @@ -360,8 +360,10 @@ static int ext4_valid_extent(struct inode *inode, struct ext4_extent *ext) { ext4_fsblk_t block = ext4_ext_pblock(ext); int len = ext4_ext_get_actual_len(ext); + ext4_lblk_t lblock = le32_to_cpu(ext->ee_block); + ext4_lblk_t last = lblock + len - 1; - if (len == 0) + if (lblock > last) return 0; return ext4_data_block_valid(EXT4_SB(inode->i_sb), block, len); } @@ -387,11 +389,26 @@ static int ext4_valid_extent_entries(struct inode *inode, if (depth == 0) { /* leaf entries */ struct ext4_extent *ext = EXT_FIRST_EXTENT(eh); + struct ext4_super_block *es = EXT4_SB(inode->i_sb)->s_es; + ext4_fsblk_t pblock = 0; + ext4_lblk_t lblock = 0; + ext4_lblk_t prev = 0; + int len = 0; while (entries) { if (!ext4_valid_extent(inode, ext)) return 0; + + /* Check for overlapping extents */ + lblock = le32_to_cpu(ext->ee_block); + len = ext4_ext_get_actual_len(ext); + if ((lblock <= prev) && prev) { + pblock = ext4_ext_pblock(ext); + es->s_last_error_block = cpu_to_le64(pblock); + return 0; + } ext++; entries--; + prev = lblock + len - 1; } } else { struct ext4_extent_idx *ext_idx = EXT_FIRST_INDEX(eh);