lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAEivzxfjnKq2fgCfYwhZukAO-ZfoUiC5n0Y5yaUpuz-y7kDf+g@mail.gmail.com>
Date: Wed, 25 Sep 2024 18:17:39 +0200
From: Aleksandr Mikhalitsyn <aleksandr.mikhalitsyn@...onical.com>
To: Jan Kara <jack@...e.cz>
Cc: tytso@....edu, stable@...r.kernel.org, 
	Andreas Dilger <adilger.kernel@...ger.ca>, Baokun Li <libaokun1@...wei.com>, 
	Stéphane Graber <stgraber@...raber.org>, 
	Christian Brauner <brauner@...nel.org>, linux-kernel@...r.kernel.org, 
	linux-fsdevel@...r.kernel.org, linux-ext4@...r.kernel.org, 
	Wesley Hershberger <wesley.hershberger@...onical.com>
Subject: Re: [PATCH 1/1] ext4: fix crash on BUG_ON in ext4_alloc_group_tables

On Wed, Sep 25, 2024 at 5:57 PM Jan Kara <jack@...e.cz> wrote:
>
> On Wed 25-09-24 16:33:24, Alexander Mikhalitsyn wrote:
> > [   33.882936] EXT4-fs (dm-5): mounted filesystem 8aaf41b2-6ac0-4fa8-b92b-77d10e1d16ca r/w with ordered data mode. Quota mode: none.
> > [   33.888365] EXT4-fs (dm-5): resizing filesystem from 7168 to 786432 blocks
> > [   33.888740] ------------[ cut here ]------------
> > [   33.888742] kernel BUG at fs/ext4/resize.c:324!
>
> Ah, I was staring at this for a while before I understood what's going on
> (it would be great to explain this in the changelog BTW).  As far as I
> understand commit 665d3e0af4d3 ("ext4: reduce unnecessary memory allocation
> in alloc_flex_gd()") can actually make flex_gd->resize_bg larger than
> flexbg_size (for example when ogroup = flexbg_size, ngroup = 2*flexbg_size
> - 1) which then confuses things. I think that was not really intended and

Hi Jan,

First of all, thanks for your reaction/review on this one ;-)

You are absolutely right, have just checked with our reproducer and
this modification:

diff --git a/fs/ext4/resize.c b/fs/ext4/resize.c
index e04eb08b9060..530a918f0cab 100644
--- a/fs/ext4/resize.c
+++ b/fs/ext4/resize.c
@@ -258,6 +258,8 @@ static struct ext4_new_flex_group_data
*alloc_flex_gd(unsigned int flexbg_size,
                flex_gd->resize_bg = 1 << max(fls(last_group - o_group + 1),
                                              fls(n_group - last_group));

+       BUG_ON(flex_gd->resize_bg > flexbg_size);
+
        flex_gd->groups = kmalloc_array(flex_gd->resize_bg,
                                        sizeof(struct ext4_new_group_data),
                                        GFP_NOFS);

and yes, it crashes on this BUG_ON. So it looks like instead of making
flex_gd->resize_bg to be smaller
than flexbg_size in most cases we can actually have an opposite effect
here. I guess we really need to fix alloc_flex_gd() too.

> instead of fixing up ext4_alloc_group_tables() we should really change
> the logic in alloc_flex_gd() to make sure flex_gd->resize_bg never exceeds
> flexbg size. Baokun?

At the same time, if I understand the code right, as we can have
flex_gd->resize_bg != flexbg_size after
5d1935ac02ca5a ("ext4: avoid online resizing failures due to oversized
flex bg") and
665d3e0af4d3 ("ext4: reduce unnecessary memory allocation in alloc_flex_gd()")
we should always refer to flex_gd->resize_bg value which means that
ext4_alloc_group_tables() fix is needed too.
Am I correct in my understanding?

>
>                                                                 Honza

Kind regards,
Alex

>
>
> > [   33.889075] Oops: invalid opcode: 0000 [#1] PREEMPT SMP NOPTI
> > [   33.889503] CPU: 9 UID: 0 PID: 3576 Comm: resize2fs Not tainted 6.11.0+ #27
> > [   33.890039] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.15.0-1 04/01/2014
> > [   33.890705] RIP: 0010:ext4_resize_fs+0x1212/0x12d0
> > [   33.891063] Code: b8 45 31 c0 4c 89 ff 45 31 c9 31 c9 ba 0e 08 00 00 48 c7 c6 68 75 65 b8 e8 2b 79 01 00 41 b8 ea ff ff ff 41 5f e9 8d f1 ff ff <0f> 0b 48 83 bd 70 ff ff ff 00 75 32 45 31 c0 e9 53 f1 ff ff 41 b8
> > [   33.892701] RSP: 0018:ffffa97f413f3cc8 EFLAGS: 00010202
> > [   33.893081] RAX: 0000000000000018 RBX: 0000000000000001 RCX: 00000000fffffff0
> > [   33.893639] RDX: 0000000000000017 RSI: 0000000000000016 RDI: 00000000e8c2c810
> > [   33.894197] RBP: ffffa97f413f3d90 R08: 0000000000000000 R09: 0000000000008000
> > [   33.894755] R10: ffffa97f413f3cc8 R11: ffffa2c1845bfc80 R12: 0000000000000000
> > [   33.895317] R13: ffffa2c1843d6000 R14: 0000000000008000 R15: ffffa2c199963000
> > [   33.895877] FS:  00007f46efd17000(0000) GS:ffffa2c89fc40000(0000) knlGS:0000000000000000
> > [   33.896524] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > [   33.896954] CR2: 00005630a4a1cc88 CR3: 000000010532c000 CR4: 0000000000350eb0
> > [   33.897516] Call Trace:
> > [   33.897638]  <TASK>
> > [   33.897728]  ? show_regs+0x6d/0x80
> > [   33.897942]  ? die+0x3c/0xa0
> > [   33.898106]  ? do_trap+0xe5/0x110
> > [   33.898311]  ? do_error_trap+0x6e/0x90
> > [   33.898555]  ? ext4_resize_fs+0x1212/0x12d0
> > [   33.898844]  ? exc_invalid_op+0x57/0x80
> > [   33.899101]  ? ext4_resize_fs+0x1212/0x12d0
> > [   33.899387]  ? asm_exc_invalid_op+0x1f/0x30
> > [   33.899675]  ? ext4_resize_fs+0x1212/0x12d0
> > [   33.899961]  ? ext4_resize_fs+0x745/0x12d0
> > [   33.900239]  __ext4_ioctl+0x4e0/0x1800
> > [   33.900489]  ? srso_alias_return_thunk+0x5/0xfbef5
> > [   33.900832]  ? putname+0x5b/0x70
> > [   33.901028]  ? srso_alias_return_thunk+0x5/0xfbef5
> > [   33.901374]  ? do_sys_openat2+0x87/0xd0
> > [   33.901632]  ? srso_alias_return_thunk+0x5/0xfbef5
> > [   33.901981]  ? srso_alias_return_thunk+0x5/0xfbef5
> > [   33.902324]  ? __x64_sys_openat+0x59/0xa0
> > [   33.902595]  ext4_ioctl+0x12/0x20
> > [   33.902802]  ? ext4_ioctl+0x12/0x20
> > [   33.903031]  __x64_sys_ioctl+0x99/0xd0
> > [   33.903277]  x64_sys_call+0x1206/0x20d0
> > [   33.903534]  do_syscall_64+0x72/0x110
> > [   33.903771]  ? srso_alias_return_thunk+0x5/0xfbef5
> > [   33.904115]  ? irqentry_exit+0x3f/0x50
> > [   33.904362]  ? srso_alias_return_thunk+0x5/0xfbef5
> > [   33.904707]  ? exc_page_fault+0x1aa/0x7b0
> > [   33.904979]  entry_SYSCALL_64_after_hwframe+0x76/0x7e
> > [   33.905349] RIP: 0033:0x7f46efe3294f
> > [   33.905579] Code: 00 48 89 44 24 18 31 c0 48 8d 44 24 60 c7 04 24 10 00 00 00 48 89 44 24 08 48 8d 44 24 20 48 89 44 24 10 b8 10 00 00 00 0f 05 <41> 89 c0 3d 00 f0 ff ff 77 1f 48 8b 44 24 18 64 48 2b 04 25 28 00
> > [   33.907321] RSP: 002b:00007ffe9b8833a0 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
> > [   33.907926] RAX: ffffffffffffffda RBX: 0000000000000001 RCX: 00007f46efe3294f
> > [   33.908487] RDX: 00007ffe9b8834a0 RSI: 0000000040086610 RDI: 0000000000000004
> > [   33.909046] RBP: 00005630a4a0b0e0 R08: 0000000000000000 R09: 00007ffe9b8832d7
> > [   33.909605] R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000004
> > [   33.910165] R13: 00005630a4a0c580 R14: 00005630a4a10400 R15: 0000000000000000
> > [   33.910740]  </TASK>
> > [   33.910837] Modules linked in:
> > [   33.911049] ---[ end trace 0000000000000000 ]---
> > [   33.911428] RIP: 0010:ext4_resize_fs+0x1212/0x12d0
> > [   33.911810] Code: b8 45 31 c0 4c 89 ff 45 31 c9 31 c9 ba 0e 08 00 00 48 c7 c6 68 75 65 b8 e8 2b 79 01 00 41 b8 ea ff ff ff 41 5f e9 8d f1 ff ff <0f> 0b 48 83 bd 70 ff ff ff 00 75 32 45 31 c0 e9 53 f1 ff ff 41 b8
> > [   33.913928] RSP: 0018:ffffa97f413f3cc8 EFLAGS: 00010202
> > [   33.914313] RAX: 0000000000000018 RBX: 0000000000000001 RCX: 00000000fffffff0
> > [   33.914909] RDX: 0000000000000017 RSI: 0000000000000016 RDI: 00000000e8c2c810
> > [   33.915482] RBP: ffffa97f413f3d90 R08: 0000000000000000 R09: 0000000000008000
> > [   33.916258] R10: ffffa97f413f3cc8 R11: ffffa2c1845bfc80 R12: 0000000000000000
> > [   33.917027] R13: ffffa2c1843d6000 R14: 0000000000008000 R15: ffffa2c199963000
> > [   33.917884] FS:  00007f46efd17000(0000) GS:ffffa2c89fc40000(0000) knlGS:0000000000000000
> > [   33.918818] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > [   33.919322] CR2: 00005630a4a1cc88 CR3: 000000010532c000 CR4: 0000000000350eb0
> > [   44.072293] ------------[ cut here ]------------
> >
> > Cc: stable@...r.kernel.org # v6.8+
> > Fixes: 665d3e0af4d3 ("ext4: reduce unnecessary memory allocation in alloc_flex_gd()")
> > Cc: "Theodore Ts'o" <tytso@....edu>
> > Cc: Andreas Dilger <adilger.kernel@...ger.ca>
> > Cc: Jan Kara <jack@...e.cz>
> > Cc: Baokun Li <libaokun1@...wei.com>
> > Cc: Stéphane Graber <stgraber@...raber.org>
> > Cc: Christian Brauner <brauner@...nel.org>
> > Cc: <linux-kernel@...r.kernel.org>
> > Cc: <linux-fsdevel@...r.kernel.org>
> > Cc: <linux-ext4@...r.kernel.org>
> > Reported-by: Wesley Hershberger <wesley.hershberger@...onical.com>
> > Closes: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/2081231
> > Reported-by: Stéphane Graber <stgraber@...raber.org>
> > Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@...onical.com>
> > ---
> >  fs/ext4/resize.c | 13 ++++++-------
> >  1 file changed, 6 insertions(+), 7 deletions(-)
> >
> > diff --git a/fs/ext4/resize.c b/fs/ext4/resize.c
> > index e04eb08b9060..c057a7867363 100644
> > --- a/fs/ext4/resize.c
> > +++ b/fs/ext4/resize.c
> > @@ -300,8 +300,7 @@ static void free_flex_gd(struct ext4_new_flex_group_data *flex_gd)
> >   * block group.
> >   */
> >  static int ext4_alloc_group_tables(struct super_block *sb,
> > -                             struct ext4_new_flex_group_data *flex_gd,
> > -                             unsigned int flexbg_size)
> > +                             struct ext4_new_flex_group_data *flex_gd)
> >  {
> >       struct ext4_new_group_data *group_data = flex_gd->groups;
> >       ext4_fsblk_t start_blk;
> > @@ -313,7 +312,7 @@ static int ext4_alloc_group_tables(struct super_block *sb,
> >       ext4_group_t group;
> >       ext4_group_t last_group;
> >       unsigned overhead;
> > -     __u16 uninit_mask = (flexbg_size > 1) ? ~EXT4_BG_BLOCK_UNINIT : ~0;
> > +     __u16 uninit_mask = (flex_gd->resize_bg > 1) ? ~EXT4_BG_BLOCK_UNINIT : ~0;
> >       int i;
> >
> >       BUG_ON(flex_gd->count == 0 || group_data == NULL);
> > @@ -321,8 +320,8 @@ static int ext4_alloc_group_tables(struct super_block *sb,
> >       src_group = group_data[0].group;
> >       last_group  = src_group + flex_gd->count - 1;
> >
> > -     BUG_ON((flexbg_size > 1) && ((src_group & ~(flexbg_size - 1)) !=
> > -            (last_group & ~(flexbg_size - 1))));
> > +     BUG_ON((flex_gd->resize_bg > 1) && ((src_group & ~(flex_gd->resize_bg - 1)) !=
> > +            (last_group & ~(flex_gd->resize_bg - 1))));
> >  next_group:
> >       group = group_data[0].group;
> >       if (src_group >= group_data[0].group + flex_gd->count)
> > @@ -403,7 +402,7 @@ static int ext4_alloc_group_tables(struct super_block *sb,
> >
> >               printk(KERN_DEBUG "EXT4-fs: adding a flex group with "
> >                      "%u groups, flexbg size is %u:\n", flex_gd->count,
> > -                    flexbg_size);
> > +                    flex_gd->resize_bg);
> >
> >               for (i = 0; i < flex_gd->count; i++) {
> >                       ext4_debug(
> > @@ -2158,7 +2157,7 @@ int ext4_resize_fs(struct super_block *sb, ext4_fsblk_t n_blocks_count)
> >                                        ext4_blocks_count(es));
> >                       last_update_time = jiffies;
> >               }
> > -             if (ext4_alloc_group_tables(sb, flex_gd, flexbg_size) != 0)
> > +             if (ext4_alloc_group_tables(sb, flex_gd) != 0)
> >                       break;
> >               err = ext4_flex_group_add(sb, resize_inode, flex_gd);
> >               if (unlikely(err))
> > --
> > 2.34.1
> >
> --
> Jan Kara <jack@...e.com>
> SUSE Labs, CR

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ