From patchwork Fri Apr 6 19:14:01 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Sami Liedes X-Patchwork-Id: 151253 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 34582B7094 for ; Sat, 7 Apr 2012 05:14:19 +1000 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755153Ab2DFTOR (ORCPT ); Fri, 6 Apr 2012 15:14:17 -0400 Received: from smtp-3.hut.fi ([130.233.228.93]:34155 "EHLO smtp-3.hut.fi" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754495Ab2DFTOR (ORCPT ); Fri, 6 Apr 2012 15:14:17 -0400 Received: from localhost (katosiko.hut.fi [130.233.228.115]) by smtp-3.hut.fi (8.13.6/8.12.10) with ESMTP id q36JE8mQ002375; Fri, 6 Apr 2012 22:14:08 +0300 Received: from smtp-3.hut.fi ([130.233.228.93]) by localhost (katosiko.hut.fi [130.233.228.115]) (amavisd-new, port 10024) with LMTP id 19623-353; Fri, 6 Apr 2012 22:14:08 +0300 (EEST) Received: from kosh.localdomain (kosh.hut.fi [130.233.228.12]) by smtp-3.hut.fi (8.13.6/8.12.10) with ESMTP id q36JE2Zp002365; Fri, 6 Apr 2012 22:14:02 +0300 Received: by kosh.localdomain (Postfix, from userid 43888) id 46DDD7E7; Fri, 6 Apr 2012 22:14:02 +0300 (EEST) Date: Fri, 6 Apr 2012 22:14:01 +0300 From: Sami Liedes To: Eric Sandeen Cc: "Richard W.M. Jones" , Ext4 Developers List Subject: Re: Commit c1a1e7fc24d6 causes segfault in ext2fs_new_inode Message-ID: <20120406191401.GA8816@sli.dy.fi> References: <20120330125726.GA26221@amd.home.annexia.org> <20120330131936.GB26221@amd.home.annexia.org> <4F760BC2.4070401@redhat.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <4F760BC2.4070401@redhat.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-TKK-Virus-Scanned: by amavisd-new-2.1.2-hutcc at katosiko.hut.fi Sender: linux-ext4-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-ext4@vger.kernel.org On Fri, Mar 30, 2012 at 02:38:42PM -0500, Eric Sandeen wrote: > On 3/30/12 8:19 AM, Richard W.M. Jones wrote: > > It seems like a non-64-bit-compatible bitmap was being created, and > > that doesn't have the bitmap->bitmap_ops field initialized because > > gen_bitmap.c doesn't use this field. Somehow, though, we end up > > calling a function in gen_bitmap64.c which requires that this field be > > defined. Argh, indeed. I thought the 32-bit bitfields also have the bitmap_ops field (and in the same offset), but they don't. > Well here's what's busted: > > if (bitmap->bitmap_ops->find_first_zero) > return bitmap->bitmap_ops->find_first_zero(bitmap, start, end, out); > > if (!bitmap || !EXT2FS_IS_64_BITMAP(bitmap) || bitmap->cluster_bits) > return EINVAL; > > bitmap->bitmap_ops->find_first_zero only exists for a 64-bit bitmap, which > gets tested after we try to deref it :( Right, that's quite bogus. The !bitmap test should obviously be first, not after we first dereference it. Then we should test for 64-bitness, and only ever touch the bitmap_ops field if we have a 64-bit bitmap. > I am a little confused by the existence of two different > struct ext2fs_struct_generic_bitmap's in the code. But treating one as the > other looks doomed to failure ;) In addition to that, there are actually three different versions of many operations; they are named ext2_foo_bmap(), ext2_foo_bitmap() and ext2_foo_bitmap2(). I'm quite confused too. While I suggest passing EXT2_FLAG_64BITS to ext2fs_open() - it should never break anything and only makes things faster - the code obviously shouldn't break when that is not passed. (I wonder if it would make sense to have something like EXT2_BASE_FLAGS that could include any flags which never hurt, currently apparently only EXT2_FLAG_64BITS. From the name of the flag EXT2_FLAG_64BITS it's not obvious that you should always use it. As far as I understand correctly, it's there only for ABI compatibility with code compiled before the flag was introduced and it never makes sense to not pass it in any new code.) The patch below unbreaks the code (well, at least resize2fs modified to not pass EXT2_FLAG_64BITS) for me. Sami Liedes ------------------------------------------------------------ commit bb8fe012a3b1705809f6129cd9c78d2cd855b1f8 Author: Sami Liedes Date: Fri Apr 6 22:06:30 2012 +0300 Fix ext2fs_find_first_zero_generic_bmap() for 32-bit bitmaps. ext2fs_find_first_zero_generic_bmap() tries to handle old-style 32-bit bitmaps, but fails in several different ways. Fix the order of the (in)validity tests and the fallback code to never dereference the bitmap, as we don't know at that point if it's a 32-bit bitmap or a 64-bitmap bitmap whose backend implementation does not support find_first_zero(). Use the generic bitop functions for everything instead. Addresses-Red-Hat-Bugzilla: #808421 Signed-off-by: Sami Liedes Reported-by: Richard W.M. Jones diff --git a/lib/ext2fs/gen_bitmap64.c b/lib/ext2fs/gen_bitmap64.c index e765d2c..7e9b8a0 100644 --- a/lib/ext2fs/gen_bitmap64.c +++ b/lib/ext2fs/gen_bitmap64.c @@ -770,19 +770,25 @@ errcode_t ext2fs_find_first_zero_generic_bmap(ext2fs_generic_bitmap bitmap, { int b; - if (bitmap->bitmap_ops->find_first_zero) + if (!bitmap) + return EINVAL; + + if (EXT2FS_IS_64_BITMAP(bitmap) && bitmap->bitmap_ops->find_first_zero) return bitmap->bitmap_ops->find_first_zero(bitmap, start, end, out); - if (!bitmap || !EXT2FS_IS_64_BITMAP(bitmap) || bitmap->cluster_bits) + if (!EXT2FS_IS_32_BITMAP(bitmap) || bitmap->cluster_bits) return EINVAL; - if (start < bitmap->start || end > bitmap->end || start > end) { + // We must be careful to not use any of bitmap's fields here, + // as it may actually be the different 32-bit version of the structure + if (start < ext2fs_get_block_bitmap_start(bitmap) || + end > ext2fs_get_block_bitmap_end(bitmap) || start > end) { warn_bitmap(bitmap, EXT2FS_TEST_ERROR, start); return EINVAL; } while (start <= end) { - b = bitmap->bitmap_ops->test_bmap(bitmap, start); + b = ext2fs_test_generic_bitmap(bitmap, start); if (!b) { *out = start; return 0;