Message ID | 20230307151408.58490-1-chao@kernel.org |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:5915:0:0:0:0:0 with SMTP id v21csp2498668wrd; Tue, 7 Mar 2023 07:36:55 -0800 (PST) X-Google-Smtp-Source: AK7set+T+JJySmqsBWRsLWCWKNVY1qrEqOqal27T3WFAjTc9C//aWjRNZqZBckldHF3TNDaWp7O4 X-Received: by 2002:a05:6a21:7885:b0:cb:ac6c:b02 with SMTP id bf5-20020a056a21788500b000cbac6c0b02mr14858482pzc.2.1678203415552; Tue, 07 Mar 2023 07:36:55 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1678203415; cv=none; d=google.com; s=arc-20160816; b=bcPyOaQt0XWotduCgriOSwz4+C7xVXdFN+S+9azQc042p18fnGiPQHSqDceJQmDLVn cZRHe7acs+uwBdFXf3DmgQAhFQfp9a+H/MoxzBz7FNaYnJP7L1SE7oqinFW1vwNBL8xQ Xa+Vcmq2sdGstIGhSHY+64z0CRnLMxBxX5FNTjwgB6xJmdbxsNprVsDa4pWPeljDgsCy YJlEcqWeI17CU2XGhJ624Tp8AgcTPznzjttzjbPTyZWYlPb6+5wWVO3pkHW0s1M8r1SH ib1xpzpddSECaDRtCgg4qXpjS6emZXxM2umlosMnLXrA3NKueKpy7+czU+7Yqx2DEH4e f8TA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from:dkim-signature; bh=G2vcEVoAREZUUkBDOXjxp8pa/RAGf3dIVj8UnS+wFwM=; b=sySr/V+E8ItWp1mc1f5lQy3FEAAmiOckoGplFQzx2/otdw7pVT5siIaszFsp4SjnmR AxRYwfjVX8ej7YrswWGGxsKJbqvs8NTxmqH5VondTh27Dzv+UPYvKklV55z58ccEuFGY 6gnIB3BcSnWy6UXT+MFL6qPsSqJFvk/NRA0TE0ril0CDITJQQ425r87fwi2BKLCn9hSy Z8RMqYhVeXUMhubkGM6mSjDMZK2S/8D5+JSvL6i7L9X9zxLZ4gS7AibCDC4cxn86J25t u0KBfybGqbSEO2UlMOnl025YfLITOnLz7z/KKJc6LbD3Q/cske9a3SvGLXnqey9UaqY7 vLNg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=AFUVp94n; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id h185-20020a6383c2000000b004fb647a766dsi12817338pge.7.2023.03.07.07.36.42; Tue, 07 Mar 2023 07:36:55 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=AFUVp94n; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229634AbjCGPRp (ORCPT <rfc822;toshivichauhan@gmail.com> + 99 others); Tue, 7 Mar 2023 10:17:45 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:33888 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230455AbjCGPRZ (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Tue, 7 Mar 2023 10:17:25 -0500 Received: from dfw.source.kernel.org (dfw.source.kernel.org [IPv6:2604:1380:4641:c500::1]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 5EF503D0A1 for <linux-kernel@vger.kernel.org>; Tue, 7 Mar 2023 07:14:21 -0800 (PST) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id F218D60FC1 for <linux-kernel@vger.kernel.org>; Tue, 7 Mar 2023 15:14:20 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 2EB31C4339B; Tue, 7 Mar 2023 15:14:18 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1678202060; bh=bxU+hOiIjssyJGKPM24O0mCCSmIiA+TBoXRf2lB8e1M=; h=From:To:Cc:Subject:Date:From; b=AFUVp94n/6H6wCmMRVWpMVY28wdcHDusmYUVqrnS/PddbglmSoXaqQ5Woas/0IZHy sRXaOxFOTh1tnW70t6pCEne154DfMP7GXihFrPQen44zAU+UXaRxxog6VOUKclv83/ LR8p56SJOhuTgplmWh0g9NV33t2QkhM6uXV0bcOM2BaTwNR5cg9RgVZQiOeVFUkQfc TTODcBOnbPl+fpIeyMTTust1tFxmGpolFApiTJ7OkgfIW05mB/QNMLCUe+Mg/dUOCz qT9eaMqrm8mfh1U3dJwOVooNkILRmoVcsJoJOIZTjCqXkYXUph8wwtWMJho8QfTAxI mKtEyr0hi5tVw== From: Chao Yu <chao@kernel.org> To: jaegeuk@kernel.org Cc: linux-f2fs-devel@lists.sourceforge.net, linux-kernel@vger.kernel.org, Chao Yu <chao@kernel.org>, Zhiguo Niu <zhiguo.niu@unisoc.com> Subject: [PATCH] f2fs: fix unaligned field offset in 32-bits platform Date: Tue, 7 Mar 2023 23:14:08 +0800 Message-Id: <20230307151408.58490-1-chao@kernel.org> X-Mailer: git-send-email 2.36.1 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED, SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1759723824534515692?= X-GMAIL-MSGID: =?utf-8?q?1759723824534515692?= |
Series |
f2fs: fix unaligned field offset in 32-bits platform
|
|
Commit Message
Chao Yu
March 7, 2023, 3:14 p.m. UTC
F2FS-fs (dm-x): inconsistent rbtree, cur(3470333575168) next(3320009719808) ------------[ cut here ]------------ kernel BUG at fs/f2fs/gc.c:602! Internal error: Oops - BUG: 0 [#1] PREEMPT SMP ARM PC is at get_victim_by_default+0x13c0/0x1498 LR is at f2fs_check_rb_tree_consistence+0xc4/0xd4 .... [<c04d98b0>] (get_victim_by_default) from [<c04d4f44>] (f2fs_gc+0x220/0x6cc) [<c04d4f44>] (f2fs_gc) from [<c04d4780>] (gc_thread_func+0x2ac/0x708) [<c04d4780>] (gc_thread_func) from [<c015c774>] (kthread+0x1a8/0x1b4) [<c015c774>] (kthread) from [<c01010b4>] (ret_from_fork+0x14/0x20) The reason is there is __packed attribute in struct rb_entry, but there is no __packed attribute in struct victim_entry, so wrong offset of key field will be parsed in struct rb_entry in f2fs_check_rb_tree_consistence, it describes memory layouts of struct rb_entry and struct victim_entry in 32-bits platform as below: struct rb_entry { [0] struct rb_node rb_node; union { struct {...}; [12] unsigned long long key; } __packed; } size of struct rb_entry: 20 struct victim_entry { [0] struct rb_node rb_node; union { struct {...}; [16] struct victim_info vi; }; [32] struct list_head list; } size of struct victim_entry: 40 This patch tries to add __packed attribute in below structure: - discard_info, discard_cmd - extent_info, extent_node - victim_info, victim_entry in order to fix this unaligned field offset issue in 32-bits platform. Fixes: 004b68621897 ("f2fs: use rb-tree to track pending discard commands") Fixes: 13054c548a1c ("f2fs: introduce infra macro and data structure of rb-tree extent cache") Fixes: 093749e296e2 ("f2fs: support age threshold based garbage collection") Signed-off-by: Zhiguo Niu <zhiguo.niu@unisoc.com> Signed-off-by: Chao Yu <chao@kernel.org> --- fs/f2fs/f2fs.h | 6 +++--- fs/f2fs/gc.h | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-)
Comments
Cc'ed stable. Thanks. On 03/07, Chao Yu wrote: > F2FS-fs (dm-x): inconsistent rbtree, cur(3470333575168) next(3320009719808) > ------------[ cut here ]------------ > kernel BUG at fs/f2fs/gc.c:602! > Internal error: Oops - BUG: 0 [#1] PREEMPT SMP ARM > PC is at get_victim_by_default+0x13c0/0x1498 > LR is at f2fs_check_rb_tree_consistence+0xc4/0xd4 > .... > [<c04d98b0>] (get_victim_by_default) from [<c04d4f44>] (f2fs_gc+0x220/0x6cc) > [<c04d4f44>] (f2fs_gc) from [<c04d4780>] (gc_thread_func+0x2ac/0x708) > [<c04d4780>] (gc_thread_func) from [<c015c774>] (kthread+0x1a8/0x1b4) > [<c015c774>] (kthread) from [<c01010b4>] (ret_from_fork+0x14/0x20) > > The reason is there is __packed attribute in struct rb_entry, but there > is no __packed attribute in struct victim_entry, so wrong offset of key > field will be parsed in struct rb_entry in f2fs_check_rb_tree_consistence, > it describes memory layouts of struct rb_entry and struct victim_entry in > 32-bits platform as below: > > struct rb_entry { > [0] struct rb_node rb_node; > union { > struct {...}; > [12] unsigned long long key; > } __packed; > } > size of struct rb_entry: 20 > > struct victim_entry { > [0] struct rb_node rb_node; > union { > struct {...}; > [16] struct victim_info vi; > }; > [32] struct list_head list; > } > size of struct victim_entry: 40 > > This patch tries to add __packed attribute in below structure: > - discard_info, discard_cmd > - extent_info, extent_node > - victim_info, victim_entry > in order to fix this unaligned field offset issue in 32-bits platform. > > Fixes: 004b68621897 ("f2fs: use rb-tree to track pending discard commands") > Fixes: 13054c548a1c ("f2fs: introduce infra macro and data structure of rb-tree extent cache") > Fixes: 093749e296e2 ("f2fs: support age threshold based garbage collection") > Signed-off-by: Zhiguo Niu <zhiguo.niu@unisoc.com> > Signed-off-by: Chao Yu <chao@kernel.org> > --- > fs/f2fs/f2fs.h | 6 +++--- > fs/f2fs/gc.h | 4 ++-- > 2 files changed, 5 insertions(+), 5 deletions(-) > > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h > index b0ab2062038a..17fa7572ceed 100644 > --- a/fs/f2fs/f2fs.h > +++ b/fs/f2fs/f2fs.h > @@ -349,7 +349,7 @@ struct discard_info { > block_t lstart; /* logical start address */ > block_t len; /* length */ > block_t start; /* actual start address in dev */ > -}; > +} __packed; > > struct discard_cmd { > struct rb_node rb_node; /* rb node located in rb-tree */ > @@ -361,7 +361,7 @@ struct discard_cmd { > }; > struct discard_info di; /* discard info */ > > - }; > + } __packed; > struct list_head list; /* command list */ > struct completion wait; /* compleation */ > struct block_device *bdev; /* bdev */ > @@ -660,7 +660,7 @@ struct extent_info { > unsigned long long last_blocks; > }; > }; > -}; > +} __packed; > > struct extent_node { > struct rb_node rb_node; /* rb node located in rb-tree */ > diff --git a/fs/f2fs/gc.h b/fs/f2fs/gc.h > index 15bd1d680f67..304937d9a084 100644 > --- a/fs/f2fs/gc.h > +++ b/fs/f2fs/gc.h > @@ -58,7 +58,7 @@ struct gc_inode_list { > struct victim_info { > unsigned long long mtime; /* mtime of section */ > unsigned int segno; /* section No. */ > -}; > +} __packed; > > struct victim_entry { > struct rb_node rb_node; /* rb node located in rb-tree */ > @@ -68,7 +68,7 @@ struct victim_entry { > unsigned int segno; /* segment No. */ > }; > struct victim_info vi; /* victim info */ > - }; > + } __packed; > struct list_head list; > }; > > -- > 2.36.1
Hello: This patch was applied to jaegeuk/f2fs.git (dev) by Jaegeuk Kim <jaegeuk@kernel.org>: On Tue, 7 Mar 2023 23:14:08 +0800 you wrote: > F2FS-fs (dm-x): inconsistent rbtree, cur(3470333575168) next(3320009719808) > ------------[ cut here ]------------ > kernel BUG at fs/f2fs/gc.c:602! > Internal error: Oops - BUG: 0 [#1] PREEMPT SMP ARM > PC is at get_victim_by_default+0x13c0/0x1498 > LR is at f2fs_check_rb_tree_consistence+0xc4/0xd4 > .... > [<c04d98b0>] (get_victim_by_default) from [<c04d4f44>] (f2fs_gc+0x220/0x6cc) > [<c04d4f44>] (f2fs_gc) from [<c04d4780>] (gc_thread_func+0x2ac/0x708) > [<c04d4780>] (gc_thread_func) from [<c015c774>] (kthread+0x1a8/0x1b4) > [<c015c774>] (kthread) from [<c01010b4>] (ret_from_fork+0x14/0x20) > > [...] Here is the summary with links: - [f2fs-dev] f2fs: fix unaligned field offset in 32-bits platform https://git.kernel.org/jaegeuk/f2fs/c/8cac643eaa93 You are awesome, thank you!
On 2023/3/8 1:26, Jaegeuk Kim wrote: > Cc'ed stable. Thanks. Oh, thanks for adding the missed tag! Jaegeuk. Thanks, > > On 03/07, Chao Yu wrote: >> F2FS-fs (dm-x): inconsistent rbtree, cur(3470333575168) next(3320009719808) >> ------------[ cut here ]------------ >> kernel BUG at fs/f2fs/gc.c:602! >> Internal error: Oops - BUG: 0 [#1] PREEMPT SMP ARM >> PC is at get_victim_by_default+0x13c0/0x1498 >> LR is at f2fs_check_rb_tree_consistence+0xc4/0xd4 >> .... >> [<c04d98b0>] (get_victim_by_default) from [<c04d4f44>] (f2fs_gc+0x220/0x6cc) >> [<c04d4f44>] (f2fs_gc) from [<c04d4780>] (gc_thread_func+0x2ac/0x708) >> [<c04d4780>] (gc_thread_func) from [<c015c774>] (kthread+0x1a8/0x1b4) >> [<c015c774>] (kthread) from [<c01010b4>] (ret_from_fork+0x14/0x20) >> >> The reason is there is __packed attribute in struct rb_entry, but there >> is no __packed attribute in struct victim_entry, so wrong offset of key >> field will be parsed in struct rb_entry in f2fs_check_rb_tree_consistence, >> it describes memory layouts of struct rb_entry and struct victim_entry in >> 32-bits platform as below: >> >> struct rb_entry { >> [0] struct rb_node rb_node; >> union { >> struct {...}; >> [12] unsigned long long key; >> } __packed; >> } >> size of struct rb_entry: 20 >> >> struct victim_entry { >> [0] struct rb_node rb_node; >> union { >> struct {...}; >> [16] struct victim_info vi; >> }; >> [32] struct list_head list; >> } >> size of struct victim_entry: 40 >> >> This patch tries to add __packed attribute in below structure: >> - discard_info, discard_cmd >> - extent_info, extent_node >> - victim_info, victim_entry >> in order to fix this unaligned field offset issue in 32-bits platform. >> >> Fixes: 004b68621897 ("f2fs: use rb-tree to track pending discard commands") >> Fixes: 13054c548a1c ("f2fs: introduce infra macro and data structure of rb-tree extent cache") >> Fixes: 093749e296e2 ("f2fs: support age threshold based garbage collection") >> Signed-off-by: Zhiguo Niu <zhiguo.niu@unisoc.com> >> Signed-off-by: Chao Yu <chao@kernel.org> >> --- >> fs/f2fs/f2fs.h | 6 +++--- >> fs/f2fs/gc.h | 4 ++-- >> 2 files changed, 5 insertions(+), 5 deletions(-) >> >> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h >> index b0ab2062038a..17fa7572ceed 100644 >> --- a/fs/f2fs/f2fs.h >> +++ b/fs/f2fs/f2fs.h >> @@ -349,7 +349,7 @@ struct discard_info { >> block_t lstart; /* logical start address */ >> block_t len; /* length */ >> block_t start; /* actual start address in dev */ >> -}; >> +} __packed; >> >> struct discard_cmd { >> struct rb_node rb_node; /* rb node located in rb-tree */ >> @@ -361,7 +361,7 @@ struct discard_cmd { >> }; >> struct discard_info di; /* discard info */ >> >> - }; >> + } __packed; >> struct list_head list; /* command list */ >> struct completion wait; /* compleation */ >> struct block_device *bdev; /* bdev */ >> @@ -660,7 +660,7 @@ struct extent_info { >> unsigned long long last_blocks; >> }; >> }; >> -}; >> +} __packed; >> >> struct extent_node { >> struct rb_node rb_node; /* rb node located in rb-tree */ >> diff --git a/fs/f2fs/gc.h b/fs/f2fs/gc.h >> index 15bd1d680f67..304937d9a084 100644 >> --- a/fs/f2fs/gc.h >> +++ b/fs/f2fs/gc.h >> @@ -58,7 +58,7 @@ struct gc_inode_list { >> struct victim_info { >> unsigned long long mtime; /* mtime of section */ >> unsigned int segno; /* section No. */ >> -}; >> +} __packed; >> >> struct victim_entry { >> struct rb_node rb_node; /* rb node located in rb-tree */ >> @@ -68,7 +68,7 @@ struct victim_entry { >> unsigned int segno; /* segment No. */ >> }; >> struct victim_info vi; /* victim info */ >> - }; >> + } __packed; >> struct list_head list; >> }; >> >> -- >> 2.36.1
From: Chao Yu <chao@kernel.org> > Sent: 07 March 2023 15:14 > > F2FS-fs (dm-x): inconsistent rbtree, cur(3470333575168) next(3320009719808) > ------------[ cut here ]------------ > kernel BUG at fs/f2fs/gc.c:602! > Internal error: Oops - BUG: 0 [#1] PREEMPT SMP ARM > PC is at get_victim_by_default+0x13c0/0x1498 > LR is at f2fs_check_rb_tree_consistence+0xc4/0xd4 > .... > [<c04d98b0>] (get_victim_by_default) from [<c04d4f44>] (f2fs_gc+0x220/0x6cc) > [<c04d4f44>] (f2fs_gc) from [<c04d4780>] (gc_thread_func+0x2ac/0x708) > [<c04d4780>] (gc_thread_func) from [<c015c774>] (kthread+0x1a8/0x1b4) > [<c015c774>] (kthread) from [<c01010b4>] (ret_from_fork+0x14/0x20) > > The reason is there is __packed attribute in struct rb_entry, but there > is no __packed attribute in struct victim_entry, so wrong offset of key > field will be parsed in struct rb_entry in f2fs_check_rb_tree_consistence, > it describes memory layouts of struct rb_entry and struct victim_entry in > 32-bits platform as below: > > struct rb_entry { > [0] struct rb_node rb_node; > union { > struct {...}; > [12] unsigned long long key; > } __packed; This __packed removes the 4-byte pad before the union. I bet it should be removed... > } > size of struct rb_entry: 20 > > struct victim_entry { > [0] struct rb_node rb_node; > union { > struct {...}; > [16] struct victim_info vi; > }; > [32] struct list_head list; > } > size of struct victim_entry: 40 > > This patch tries to add __packed attribute in below structure: > - discard_info, discard_cmd > - extent_info, extent_node > - victim_info, victim_entry > in order to fix this unaligned field offset issue in 32-bits platform. Have you looked at the amount of extra code that gets generated on systems that fault misaligned accesses? Plausibly adding __packed __aligned(4) will restrict the compiler to just aligning 64bit items on 32bit boundaries. But even then is you pass the address of a misaligned structure to another function it will fault later of. You haven't actually said where the misalignment comes from. If the code is doing (foo *)(ptr + 1) then that is broken when the alignments of 'ptr' and 'foo' differ. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On 03/07, Chao Yu wrote: > F2FS-fs (dm-x): inconsistent rbtree, cur(3470333575168) next(3320009719808) > ------------[ cut here ]------------ > kernel BUG at fs/f2fs/gc.c:602! > Internal error: Oops - BUG: 0 [#1] PREEMPT SMP ARM > PC is at get_victim_by_default+0x13c0/0x1498 > LR is at f2fs_check_rb_tree_consistence+0xc4/0xd4 > .... > [<c04d98b0>] (get_victim_by_default) from [<c04d4f44>] (f2fs_gc+0x220/0x6cc) > [<c04d4f44>] (f2fs_gc) from [<c04d4780>] (gc_thread_func+0x2ac/0x708) > [<c04d4780>] (gc_thread_func) from [<c015c774>] (kthread+0x1a8/0x1b4) > [<c015c774>] (kthread) from [<c01010b4>] (ret_from_fork+0x14/0x20) > > The reason is there is __packed attribute in struct rb_entry, but there > is no __packed attribute in struct victim_entry, so wrong offset of key > field will be parsed in struct rb_entry in f2fs_check_rb_tree_consistence, > it describes memory layouts of struct rb_entry and struct victim_entry in > 32-bits platform as below: > > struct rb_entry { > [0] struct rb_node rb_node; > union { > struct {...}; > [12] unsigned long long key; > } __packed; > } > size of struct rb_entry: 20 > > struct victim_entry { > [0] struct rb_node rb_node; > union { > struct {...}; > [16] struct victim_info vi; Why 16? Don't we get the key from "unsigned long long mtime" starting at [12]? struct rb_node rb_node; /* rb node located in rb-tree */ union { struct { unsigned long long mtime; /* mtime of section */ unsigned int segno; /* segment No. */ }; struct victim_info vi; /* victim info */ }; > }; > [32] struct list_head list; > } > size of struct victim_entry: 40 > > This patch tries to add __packed attribute in below structure: > - discard_info, discard_cmd > - extent_info, extent_node > - victim_info, victim_entry > in order to fix this unaligned field offset issue in 32-bits platform. > > Fixes: 004b68621897 ("f2fs: use rb-tree to track pending discard commands") > Fixes: 13054c548a1c ("f2fs: introduce infra macro and data structure of rb-tree extent cache") > Fixes: 093749e296e2 ("f2fs: support age threshold based garbage collection") > Signed-off-by: Zhiguo Niu <zhiguo.niu@unisoc.com> > Signed-off-by: Chao Yu <chao@kernel.org> > --- > fs/f2fs/f2fs.h | 6 +++--- > fs/f2fs/gc.h | 4 ++-- > 2 files changed, 5 insertions(+), 5 deletions(-) > > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h > index b0ab2062038a..17fa7572ceed 100644 > --- a/fs/f2fs/f2fs.h > +++ b/fs/f2fs/f2fs.h > @@ -349,7 +349,7 @@ struct discard_info { > block_t lstart; /* logical start address */ > block_t len; /* length */ > block_t start; /* actual start address in dev */ > -}; > +} __packed; > > struct discard_cmd { > struct rb_node rb_node; /* rb node located in rb-tree */ > @@ -361,7 +361,7 @@ struct discard_cmd { > }; > struct discard_info di; /* discard info */ > > - }; > + } __packed; > struct list_head list; /* command list */ > struct completion wait; /* compleation */ > struct block_device *bdev; /* bdev */ > @@ -660,7 +660,7 @@ struct extent_info { > unsigned long long last_blocks; > }; > }; > -}; > +} __packed; > > struct extent_node { > struct rb_node rb_node; /* rb node located in rb-tree */ > diff --git a/fs/f2fs/gc.h b/fs/f2fs/gc.h > index 15bd1d680f67..304937d9a084 100644 > --- a/fs/f2fs/gc.h > +++ b/fs/f2fs/gc.h > @@ -58,7 +58,7 @@ struct gc_inode_list { > struct victim_info { > unsigned long long mtime; /* mtime of section */ > unsigned int segno; /* section No. */ > -}; > +} __packed; > > struct victim_entry { > struct rb_node rb_node; /* rb node located in rb-tree */ > @@ -68,7 +68,7 @@ struct victim_entry { > unsigned int segno; /* segment No. */ > }; > struct victim_info vi; /* victim info */ > - }; > + } __packed; > struct list_head list; > }; > > -- > 2.36.1
On 03/08, David Laight wrote: > From: Chao Yu <chao@kernel.org> > > Sent: 07 March 2023 15:14 > > > > F2FS-fs (dm-x): inconsistent rbtree, cur(3470333575168) next(3320009719808) > > ------------[ cut here ]------------ > > kernel BUG at fs/f2fs/gc.c:602! > > Internal error: Oops - BUG: 0 [#1] PREEMPT SMP ARM > > PC is at get_victim_by_default+0x13c0/0x1498 > > LR is at f2fs_check_rb_tree_consistence+0xc4/0xd4 > > .... > > [<c04d98b0>] (get_victim_by_default) from [<c04d4f44>] (f2fs_gc+0x220/0x6cc) > > [<c04d4f44>] (f2fs_gc) from [<c04d4780>] (gc_thread_func+0x2ac/0x708) > > [<c04d4780>] (gc_thread_func) from [<c015c774>] (kthread+0x1a8/0x1b4) > > [<c015c774>] (kthread) from [<c01010b4>] (ret_from_fork+0x14/0x20) > > > > The reason is there is __packed attribute in struct rb_entry, but there > > is no __packed attribute in struct victim_entry, so wrong offset of key > > field will be parsed in struct rb_entry in f2fs_check_rb_tree_consistence, > > it describes memory layouts of struct rb_entry and struct victim_entry in > > 32-bits platform as below: > > > > struct rb_entry { > > [0] struct rb_node rb_node; > > union { > > struct {...}; > > [12] unsigned long long key; > > } __packed; > > This __packed removes the 4-byte pad before the union. > I bet it should be removed... struct rb_node { unsigned long __rb_parent_color; struct rb_node *rb_right; struct rb_node *rb_left; } __attribute__((aligned(sizeof(long)))); Hmm, isn't this aligned to 32bits originally? Why does 32bits pad 4-bytes if we remove __packed? > > > } > > size of struct rb_entry: 20 > > > > struct victim_entry { > > [0] struct rb_node rb_node; > > union { > > struct {...}; > > [16] struct victim_info vi; > > }; > > [32] struct list_head list; > > } > > size of struct victim_entry: 40 > > > > This patch tries to add __packed attribute in below structure: > > - discard_info, discard_cmd > > - extent_info, extent_node > > - victim_info, victim_entry > > in order to fix this unaligned field offset issue in 32-bits platform. > > Have you looked at the amount of extra code that gets generated > on systems that fault misaligned accesses? > > Plausibly adding __packed __aligned(4) will restrict the compiler > to just aligning 64bit items on 32bit boundaries. > But even then is you pass the address of a misaligned structure > to another function it will fault later of. > > You haven't actually said where the misalignment comes from. > If the code is doing (foo *)(ptr + 1) then that is broken > when the alignments of 'ptr' and 'foo' differ. IIUC, the problem comes since we access the same object with two structures to handle rb_tree. E.g., [struct extent_node] [struct rb_entry] struct rb_node rb_node; struct rb_node rb_node; union { struct extent_info ei; struct { unsigned int fofs; unsigned int ofs; unsigned int len; unsigned int len; }; unsigned long long key; } __packed; So, I think if we get a different offset of fofs or ofs between in extent_node and rb_entry, further work'll access a wrong memory since we simply cast the object pointer between two. > > David > > - > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK > Registration No: 1397386 (Wales)
From: Jaegeuk Kim > Sent: 09 March 2023 23:55 > > On 03/08, David Laight wrote: > > From: Chao Yu <chao@kernel.org> > > > Sent: 07 March 2023 15:14 > > > > > > F2FS-fs (dm-x): inconsistent rbtree, cur(3470333575168) next(3320009719808) > > > ------------[ cut here ]------------ > > > kernel BUG at fs/f2fs/gc.c:602! > > > Internal error: Oops - BUG: 0 [#1] PREEMPT SMP ARM > > > PC is at get_victim_by_default+0x13c0/0x1498 > > > LR is at f2fs_check_rb_tree_consistence+0xc4/0xd4 > > > .... > > > [<c04d98b0>] (get_victim_by_default) from [<c04d4f44>] (f2fs_gc+0x220/0x6cc) > > > [<c04d4f44>] (f2fs_gc) from [<c04d4780>] (gc_thread_func+0x2ac/0x708) > > > [<c04d4780>] (gc_thread_func) from [<c015c774>] (kthread+0x1a8/0x1b4) > > > [<c015c774>] (kthread) from [<c01010b4>] (ret_from_fork+0x14/0x20) > > > > > > The reason is there is __packed attribute in struct rb_entry, but there > > > is no __packed attribute in struct victim_entry, so wrong offset of key > > > field will be parsed in struct rb_entry in f2fs_check_rb_tree_consistence, > > > it describes memory layouts of struct rb_entry and struct victim_entry in > > > 32-bits platform as below: > > > > > > struct rb_entry { > > > [0] struct rb_node rb_node; > > > union { > > > struct {...}; > > > [12] unsigned long long key; > > > } __packed; > > > > This __packed removes the 4-byte pad before the union. > > I bet it should be removed... > > struct rb_node { > unsigned long __rb_parent_color; > struct rb_node *rb_right; > struct rb_node *rb_left; > } __attribute__((aligned(sizeof(long)))); > > Hmm, isn't this aligned to 32bits originally? Why does 32bits pad 4-bytes > if we remove __packed? That attribute is entirely pointless. IIRC It is a request to increase the alignment to that of long. It wouldn't have any effect even if 'packed' was also specified. (Unless you are building for 64-bit windows.) The 'issue' is that on arm32 (unlike x86) 'long long' has 64bit alignment. So without the __packed on the union there is a 4 byte hole before the union. ... > IIUC, the problem comes since we access the same object with two structures > to handle rb_tree. > > E.g., > > [struct extent_node] [struct rb_entry] > struct rb_node rb_node; struct rb_node rb_node; > union { > struct extent_info ei; struct { > unsigned int fofs; unsigned int ofs; > unsigned int len; unsigned int len; > }; > unsigned long long key; > } __packed; > > So, I think if we get a different offset of fofs or ofs between in > extent_node and rb_entry, further work'll access a wrong memory since > we simply cast the object pointer between two. That example actually happens to work. But replace 'unsigned int' with 'long long' and it all fails. That is horribly broken (by design). You can't do that and expect it to work at all. This is another case where you don't want __packed, but to request a compilation error if padding was added. The safest 'fix' (it is still broken by design) is to change the alignment of rb_node to be that of 'long long' and remove the __packed from the union. That adds a 4 byte pad to rb_node on some, but not all, 32bit architectures. Then all the code that used the above pattern is (probably) ok. You can use (if I've spelt them right) aligned(Alignof(long long)) but not aligned(__alignof(long long)) because the latter returns the preferred alignment not the actual alignment (8 not 4 on x86). I think Alignof() is ok for current kernels, but not for anything that might get backported to stable. You can use __alignof(struct {long long x;}). Actually this should also work: struct rb_node { union { long long alignment; struct { unsigned long __rb_parent_color; struct rb_node *rb_right; struct rb_node *rb_left; } } }; David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On 03/10, David Laight wrote: > From: Jaegeuk Kim > > Sent: 09 March 2023 23:55 > > > > On 03/08, David Laight wrote: > > > From: Chao Yu <chao@kernel.org> > > > > Sent: 07 March 2023 15:14 > > > > > > > > F2FS-fs (dm-x): inconsistent rbtree, cur(3470333575168) next(3320009719808) > > > > ------------[ cut here ]------------ > > > > kernel BUG at fs/f2fs/gc.c:602! > > > > Internal error: Oops - BUG: 0 [#1] PREEMPT SMP ARM > > > > PC is at get_victim_by_default+0x13c0/0x1498 > > > > LR is at f2fs_check_rb_tree_consistence+0xc4/0xd4 > > > > .... > > > > [<c04d98b0>] (get_victim_by_default) from [<c04d4f44>] (f2fs_gc+0x220/0x6cc) > > > > [<c04d4f44>] (f2fs_gc) from [<c04d4780>] (gc_thread_func+0x2ac/0x708) > > > > [<c04d4780>] (gc_thread_func) from [<c015c774>] (kthread+0x1a8/0x1b4) > > > > [<c015c774>] (kthread) from [<c01010b4>] (ret_from_fork+0x14/0x20) > > > > > > > > The reason is there is __packed attribute in struct rb_entry, but there > > > > is no __packed attribute in struct victim_entry, so wrong offset of key > > > > field will be parsed in struct rb_entry in f2fs_check_rb_tree_consistence, > > > > it describes memory layouts of struct rb_entry and struct victim_entry in > > > > 32-bits platform as below: > > > > > > > > struct rb_entry { > > > > [0] struct rb_node rb_node; > > > > union { > > > > struct {...}; > > > > [12] unsigned long long key; > > > > } __packed; > > > > > > This __packed removes the 4-byte pad before the union. > > > I bet it should be removed... > > > > struct rb_node { > > unsigned long __rb_parent_color; > > struct rb_node *rb_right; > > struct rb_node *rb_left; > > } __attribute__((aligned(sizeof(long)))); > > > > Hmm, isn't this aligned to 32bits originally? Why does 32bits pad 4-bytes > > if we remove __packed? > > That attribute is entirely pointless. > IIRC It is a request to increase the alignment to that of long. > It wouldn't have any effect even if 'packed' was also specified. > (Unless you are building for 64-bit windows.) > > The 'issue' is that on arm32 (unlike x86) 'long long' has > 64bit alignment. > So without the __packed on the union there is a 4 byte hole > before the union. > > ... > > IIUC, the problem comes since we access the same object with two structures > > to handle rb_tree. > > > > E.g., > > > > [struct extent_node] [struct rb_entry] > > struct rb_node rb_node; struct rb_node rb_node; > > union { > > struct extent_info ei; struct { > > unsigned int fofs; unsigned int ofs; > > unsigned int len; unsigned int len; > > }; > > unsigned long long key; > > } __packed; > > > > So, I think if we get a different offset of fofs or ofs between in > > extent_node and rb_entry, further work'll access a wrong memory since > > we simply cast the object pointer between two. > > That example actually happens to work. > But replace 'unsigned int' with 'long long' and it all fails. > > That is horribly broken (by design). > You can't do that and expect it to work at all. > This is another case where you don't want __packed, but to > request a compilation error if padding was added. > > The safest 'fix' (it is still broken by design) is to change the > alignment of rb_node to be that of 'long long' and remove the > __packed from the union. > That adds a 4 byte pad to rb_node on some, but not all, 32bit > architectures. > Then all the code that used the above pattern is (probably) ok. > > You can use (if I've spelt them right) aligned(Alignof(long long)) > but not aligned(__alignof(long long)) because the latter returns > the preferred alignment not the actual alignment (8 not 4 on x86). > I think Alignof() is ok for current kernels, but not for anything > that might get backported to stable. > You can use __alignof(struct {long long x;}). > > Actually this should also work: > struct rb_node { > union { > long long alignment; > struct { > unsigned long __rb_parent_color; > struct rb_node *rb_right; > struct rb_node *rb_left; > } > } > }; Thank you for the explanation. IMHO, it'd be good to keep the existing rb_node for all the other components, but a problem of wrong design in f2fs. I posted three patches to remove this buggy rb_entry sharing. https://lore.kernel.org/lkml/20230310210454.2350881-1-jaegeuk@kernel.org/T/#t > > David > > - > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK > Registration No: 1397386 (Wales)
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h index b0ab2062038a..17fa7572ceed 100644 --- a/fs/f2fs/f2fs.h +++ b/fs/f2fs/f2fs.h @@ -349,7 +349,7 @@ struct discard_info { block_t lstart; /* logical start address */ block_t len; /* length */ block_t start; /* actual start address in dev */ -}; +} __packed; struct discard_cmd { struct rb_node rb_node; /* rb node located in rb-tree */ @@ -361,7 +361,7 @@ struct discard_cmd { }; struct discard_info di; /* discard info */ - }; + } __packed; struct list_head list; /* command list */ struct completion wait; /* compleation */ struct block_device *bdev; /* bdev */ @@ -660,7 +660,7 @@ struct extent_info { unsigned long long last_blocks; }; }; -}; +} __packed; struct extent_node { struct rb_node rb_node; /* rb node located in rb-tree */ diff --git a/fs/f2fs/gc.h b/fs/f2fs/gc.h index 15bd1d680f67..304937d9a084 100644 --- a/fs/f2fs/gc.h +++ b/fs/f2fs/gc.h @@ -58,7 +58,7 @@ struct gc_inode_list { struct victim_info { unsigned long long mtime; /* mtime of section */ unsigned int segno; /* section No. */ -}; +} __packed; struct victim_entry { struct rb_node rb_node; /* rb node located in rb-tree */ @@ -68,7 +68,7 @@ struct victim_entry { unsigned int segno; /* segment No. */ }; struct victim_info vi; /* victim info */ - }; + } __packed; struct list_head list; };