Messages in this thread | | | Date | Thu, 9 Mar 2023 15:54:51 -0800 | From | Jaegeuk Kim <> | Subject | Re: [PATCH] f2fs: fix unaligned field offset in 32-bits platform |
| |
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)
| |