2023-05-22 18:29:48

by Daniel Díaz

[permalink] [raw]
Subject: Stable backport request: skbuff: Proactively round up to kmalloc bucket size

Hello!

Would the stable maintainers please consider backporting the following
commit to the 6.1? We are trying to build gki_defconfig (plus a few
extras) on Arm64 and test it under Qemu-arm64, but it fails to boot.
Bisection has pointed here.

We have verified that cherry-picking this patch on top of v6.1.29
applies cleanly and allows the kernel to boot.

commit 12d6c1d3a2ad0c199ec57c201cdc71e8e157a232
Author: Kees Cook <[email protected]>
Date: Tue Oct 25 15:39:35 2022 -0700

skbuff: Proactively round up to kmalloc bucket size

Instead of discovering the kmalloc bucket size _after_ allocation, round
up proactively so the allocation is explicitly made for the full size,
allowing the compiler to correctly reason about the resulting size of
the buffer through the existing __alloc_size() hint.

This will allow for kernels built with CONFIG_UBSAN_BOUNDS or the
coming dynamic bounds checking under CONFIG_FORTIFY_SOURCE to gain
back the __alloc_size() hints that were temporarily reverted in commit
93dd04ab0b2b ("slab: remove __alloc_size attribute from
__kmalloc_track_caller")

Cc: "David S. Miller" <[email protected]>
Cc: Eric Dumazet <[email protected]>
Cc: Jakub Kicinski <[email protected]>
Cc: Paolo Abeni <[email protected]>
Cc: [email protected]
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Nick Desaulniers <[email protected]>
Cc: David Rientjes <[email protected]>
Acked-by: Vlastimil Babka <[email protected]>
Link: https://patchwork.kernel.org/project/netdevbpf/patch/[email protected]/
Signed-off-by: Kees Cook <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Paolo Abeni <[email protected]>


Thanks and greetings!

Daniel Díaz
[email protected]


2023-05-22 18:37:55

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: Stable backport request: skbuff: Proactively round up to kmalloc bucket size

On Mon, May 22, 2023 at 12:23:50PM -0600, Daniel D?az wrote:
> Hello!
>
> Would the stable maintainers please consider backporting the following
> commit to the 6.1? We are trying to build gki_defconfig (plus a few
> extras) on Arm64 and test it under Qemu-arm64, but it fails to boot.
> Bisection has pointed here.

I do not see a "gki_defconfig" in the kernel tree, is this just
out-of-tree stuff?

If so, why not just add this to your out-of-tree stuff?

> We have verified that cherry-picking this patch on top of v6.1.29
> applies cleanly and allows the kernel to boot.

So what is breaking that requires this to fix the problem? What is the
problem?

>
> commit 12d6c1d3a2ad0c199ec57c201cdc71e8e157a232
> Author: Kees Cook <[email protected]>
> Date: Tue Oct 25 15:39:35 2022 -0700
>
> skbuff: Proactively round up to kmalloc bucket size
>
> Instead of discovering the kmalloc bucket size _after_ allocation, round
> up proactively so the allocation is explicitly made for the full size,
> allowing the compiler to correctly reason about the resulting size of
> the buffer through the existing __alloc_size() hint.
>
> This will allow for kernels built with CONFIG_UBSAN_BOUNDS or the
> coming dynamic bounds checking under CONFIG_FORTIFY_SOURCE to gain
> back the __alloc_size() hints that were temporarily reverted in commit
> 93dd04ab0b2b ("slab: remove __alloc_size attribute from
> __kmalloc_track_caller")
>
> Cc: "David S. Miller" <[email protected]>
> Cc: Eric Dumazet <[email protected]>
> Cc: Jakub Kicinski <[email protected]>
> Cc: Paolo Abeni <[email protected]>
> Cc: [email protected]
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: Nick Desaulniers <[email protected]>
> Cc: David Rientjes <[email protected]>
> Acked-by: Vlastimil Babka <[email protected]>
> Link: https://patchwork.kernel.org/project/netdevbpf/patch/[email protected]/
> Signed-off-by: Kees Cook <[email protected]>
> Link: https://lore.kernel.org/r/[email protected]
> Signed-off-by: Paolo Abeni <[email protected]>

This feels like a new feature, why would a 6.1.y system need it? What
commit id does it fix?

thanks,

greg k-h

2023-05-22 19:06:19

by Nick Desaulniers

[permalink] [raw]
Subject: Re: Stable backport request: skbuff: Proactively round up to kmalloc bucket size

On Mon, May 22, 2023 at 11:24 AM Daniel Díaz <[email protected]> wrote:
>
> Hello!
>
> Would the stable maintainers please consider backporting the following
> commit to the 6.1? We are trying to build gki_defconfig (plus a few

Does android's gki_defconfig fail to boot on the `android14-6.1`
branch of https://android.googlesource.com/kernel/common?

(i.e. downstream branch from linux stable's linux-6.1.y)?

We just ran CI successfully on that branch 10 hours ago.
https://github.com/ClangBuiltLinux/continuous-integration2/actions/runs/5042504560/jobs/9045030265

Do you have more information on the observed boot failure? (panic splat?)

> extras) on Arm64 and test it under Qemu-arm64, but it fails to boot.
> Bisection has pointed here.
>
> We have verified that cherry-picking this patch on top of v6.1.29
> applies cleanly and allows the kernel to boot.
>
> commit 12d6c1d3a2ad0c199ec57c201cdc71e8e157a232
> Author: Kees Cook <[email protected]>
> Date: Tue Oct 25 15:39:35 2022 -0700
>
> skbuff: Proactively round up to kmalloc bucket size
>
> Instead of discovering the kmalloc bucket size _after_ allocation, round
> up proactively so the allocation is explicitly made for the full size,
> allowing the compiler to correctly reason about the resulting size of
> the buffer through the existing __alloc_size() hint.
>
> This will allow for kernels built with CONFIG_UBSAN_BOUNDS or the
> coming dynamic bounds checking under CONFIG_FORTIFY_SOURCE to gain
> back the __alloc_size() hints that were temporarily reverted in commit
> 93dd04ab0b2b ("slab: remove __alloc_size attribute from
> __kmalloc_track_caller")
>
> Cc: "David S. Miller" <[email protected]>
> Cc: Eric Dumazet <[email protected]>
> Cc: Jakub Kicinski <[email protected]>
> Cc: Paolo Abeni <[email protected]>
> Cc: [email protected]
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: Nick Desaulniers <[email protected]>
> Cc: David Rientjes <[email protected]>
> Acked-by: Vlastimil Babka <[email protected]>
> Link: https://patchwork.kernel.org/project/netdevbpf/patch/[email protected]/
> Signed-off-by: Kees Cook <[email protected]>
> Link: https://lore.kernel.org/r/[email protected]
> Signed-off-by: Paolo Abeni <[email protected]>
>
>
> Thanks and greetings!
>
> Daniel Díaz
> [email protected]



--
Thanks,
~Nick Desaulniers

2023-05-23 06:45:36

by Vlastimil Babka

[permalink] [raw]
Subject: Re: Stable backport request: skbuff: Proactively round up to kmalloc bucket size

On 5/22/23 20:23, Daniel Díaz wrote:
> Hello!
>
> Would the stable maintainers please consider backporting the following
> commit to the 6.1? We are trying to build gki_defconfig (plus a few
> extras) on Arm64 and test it under Qemu-arm64, but it fails to boot.
> Bisection has pointed here.

You mean the bisection was done to find the first "good" commit between 6.1
and e.g. 6.3?

As others said, this commit wasn't expected to be a fix to a known bug.
Maybe you found one that we didn't know of, or it might be accidentaly
masking some other bug.

> We have verified that cherry-picking this patch on top of v6.1.29
> applies cleanly and allows the kernel to boot.
>
> commit 12d6c1d3a2ad0c199ec57c201cdc71e8e157a232
> Author: Kees Cook <[email protected]>
> Date: Tue Oct 25 15:39:35 2022 -0700
>
> skbuff: Proactively round up to kmalloc bucket size
>
> Instead of discovering the kmalloc bucket size _after_ allocation, round
> up proactively so the allocation is explicitly made for the full size,
> allowing the compiler to correctly reason about the resulting size of
> the buffer through the existing __alloc_size() hint.
>
> This will allow for kernels built with CONFIG_UBSAN_BOUNDS or the
> coming dynamic bounds checking under CONFIG_FORTIFY_SOURCE to gain
> back the __alloc_size() hints that were temporarily reverted in commit
> 93dd04ab0b2b ("slab: remove __alloc_size attribute from
> __kmalloc_track_caller")
>
> Cc: "David S. Miller" <[email protected]>
> Cc: Eric Dumazet <[email protected]>
> Cc: Jakub Kicinski <[email protected]>
> Cc: Paolo Abeni <[email protected]>
> Cc: [email protected]
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: Nick Desaulniers <[email protected]>
> Cc: David Rientjes <[email protected]>
> Acked-by: Vlastimil Babka <[email protected]>
> Link: https://patchwork.kernel.org/project/netdevbpf/patch/[email protected]/
> Signed-off-by: Kees Cook <[email protected]>
> Link: https://lore.kernel.org/r/[email protected]
> Signed-off-by: Paolo Abeni <[email protected]>
>
>
> Thanks and greetings!
>
> Daniel Díaz
> [email protected]


2023-05-24 03:58:33

by Daniel Díaz

[permalink] [raw]
Subject: Re: Stable backport request: skbuff: Proactively round up to kmalloc bucket size

Hello!

On Mon, 22 May 2023 at 12:37, Nick Desaulniers <[email protected]> wrote:
>
> On Mon, May 22, 2023 at 11:24 AM Daniel Díaz <[email protected]> wrote:
> >
> > Hello!
> >
> > Would the stable maintainers please consider backporting the following
> > commit to the 6.1? We are trying to build gki_defconfig (plus a few
>
> Does android's gki_defconfig fail to boot on the `android14-6.1`
> branch of https://android.googlesource.com/kernel/common?
>
> (i.e. downstream branch from linux stable's linux-6.1.y)?
>
> We just ran CI successfully on that branch 10 hours ago.
> https://github.com/ClangBuiltLinux/continuous-integration2/actions/runs/5042504560/jobs/9045030265
>
> Do you have more information on the observed boot failure? (panic splat?)

Apologies if it sounded like we were trying to boot an Android kernel.
Let me clarify: We're booting v6.1.29 from linux-stable/linux-6.1.y.

This is what we get under Qemu-arm64 for v6.1.29 with Clang 16:
-----8<-----
Unexpected kernel BRK exception at EL1
Internal error: BRK handler: 00000000f2000001 [#1] PREEMPT SMP
Modules linked in:
CPU: 0 PID: 1 Comm: swapper/0 Not tainted 6.1.29 #1
Hardware name: linux,dummy-virt (DT)
pstate: 80000005 (Nzcv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
pc : pskb_expand_head+0x448/0x480
lr : pskb_expand_head+0x13c/0x480
sp : ffff80000802b850
x29: ffff80000802b860 x28: 00000000000002c0 x27: 0000000000000ec0
x26: ffff0000c02c8ec0 x25: ffff0000c02c8000 x24: 00000000000128c0
x23: ffff0000c030e800 x22: ffff0000c030e800 x21: 0000000000000240
x20: 0000000000000000 x19: ffff0000c085e900 x18: ffff800008021068
x17: 00000000ad6b63b6 x16: 00000000ad6b63b6 x15: 0001001c00070038
x14: 0000000c00020008 x13: 00882cc00000ffff x12: 0000000000000000
x11: 0000000000000000 x10: 0000000000000000 x9 : 0000000000000001
x8 : ffff0000c030eac0 x7 : 0000000000000000 x6 : 0000000000000000
x5 : ffff0000c030eaf0 x4 : ffff0000ff7abd10 x3 : 0000000000001740
x2 : ffff0000c02c8000 x1 : 0000000000000000 x0 : 0000000000000000
Call trace:
pskb_expand_head+0x448/0x480
netlink_trim+0xa0/0xc8
netlink_broadcast+0x54/0x764
genl_ctrl_event+0x21c/0x37c
genl_register_family+0x628/0x708
thermal_netlink_init+0x28/0x3c
thermal_init+0x28/0xec
do_one_initcall+0xfc/0x358
do_initcall_level+0xd8/0x1b4
do_initcalls+0x64/0xa8
do_basic_setup+0x2c/0x3c
kernel_init_freeable+0x118/0x198
kernel_init+0x30/0x1c0
ret_from_fork+0x10/0x20
Code: f9406679 38776b28 3707eba8 17ffff67 (d4200020)
---[ end trace 0000000000000000 ]---
Kernel panic - not syncing: BRK handler: Fatal exception
SMP: stopping secondary CPUs
----->8-----

Here's a link to that test, with all artifacts:
https://tuxapi.tuxsuite.com/v1/groups/linaro/projects/daniel/tests/2QA2CVTTvG6KZETMUyZCNgS8koR

This can be reproduced locally via Tuxrun:
-----8<-----
#pip3 install -U tuxrun
tuxrun --runtime podman \
--device qemu-arm64 \
--image docker.io/lavasoftware/lava-dispatcher:2023.01.0020.gc1598238f \
--boot-args rw \
--kernel https://storage.tuxsuite.com/public/linaro/daniel/builds/2QA2CHQUpqKe27FyMZrBNILVwXi/Image.gz
\
--modules https://storage.tuxsuite.com/public/linaro/daniel/builds/2QA2CHQUpqKe27FyMZrBNILVwXi/modules.tar.xz
\
--rootfs https://storage.tuxboot.com/debian/bookworm/arm64/rootfs.ext4.xz
----->8-----

This is vanilla v6.1.29 with no extra patches, just this kernel configuration:
https://storage.tuxsuite.com/public/linaro/daniel/builds/2QA2CHQUpqKe27FyMZrBNILVwXi/config

Greetings!

Daniel Díaz
[email protected]



> > extras) on Arm64 and test it under Qemu-arm64, but it fails to boot.
> > Bisection has pointed here.
> >
> > We have verified that cherry-picking this patch on top of v6.1.29
> > applies cleanly and allows the kernel to boot.
> >
> > commit 12d6c1d3a2ad0c199ec57c201cdc71e8e157a232
> > Author: Kees Cook <[email protected]>
> > Date: Tue Oct 25 15:39:35 2022 -0700
> >
> > skbuff: Proactively round up to kmalloc bucket size
> >
> > Instead of discovering the kmalloc bucket size _after_ allocation, round
> > up proactively so the allocation is explicitly made for the full size,
> > allowing the compiler to correctly reason about the resulting size of
> > the buffer through the existing __alloc_size() hint.
> >
> > This will allow for kernels built with CONFIG_UBSAN_BOUNDS or the
> > coming dynamic bounds checking under CONFIG_FORTIFY_SOURCE to gain
> > back the __alloc_size() hints that were temporarily reverted in commit
> > 93dd04ab0b2b ("slab: remove __alloc_size attribute from
> > __kmalloc_track_caller")
> >
> > Cc: "David S. Miller" <[email protected]>
> > Cc: Eric Dumazet <[email protected]>
> > Cc: Jakub Kicinski <[email protected]>
> > Cc: Paolo Abeni <[email protected]>
> > Cc: [email protected]
> > Cc: Greg Kroah-Hartman <[email protected]>
> > Cc: Nick Desaulniers <[email protected]>
> > Cc: David Rientjes <[email protected]>
> > Acked-by: Vlastimil Babka <[email protected]>
> > Link: https://patchwork.kernel.org/project/netdevbpf/patch/[email protected]/
> > Signed-off-by: Kees Cook <[email protected]>
> > Link: https://lore.kernel.org/r/[email protected]
> > Signed-off-by: Paolo Abeni <[email protected]>
> >
> >
> > Thanks and greetings!
> >
> > Daniel Díaz
> > [email protected]
>
>
>
> --
> Thanks,
> ~Nick Desaulniers

2023-05-24 04:02:03

by Daniel Díaz

[permalink] [raw]
Subject: Re: Stable backport request: skbuff: Proactively round up to kmalloc bucket size

Hello!

On Tue, 23 May 2023 at 00:28, Vlastimil Babka <[email protected]> wrote:
> On 5/22/23 20:23, Daniel Díaz wrote:
> > Hello!
> >
> > Would the stable maintainers please consider backporting the following
> > commit to the 6.1? We are trying to build gki_defconfig (plus a few
> > extras) on Arm64 and test it under Qemu-arm64, but it fails to boot.
> > Bisection has pointed here.
>
> You mean the bisection was done to find the first "good" commit between 6.1
> and e.g. 6.3?
>
> As others said, this commit wasn't expected to be a fix to a known bug.
> Maybe you found one that we didn't know of, or it might be accidentaly
> masking some other bug.

How interesting! Yes, we happened to run a bisection between v6.1 and
v6.3 and we found where it started working with the following
configuration:
https://storage.tuxsuite.com/public/linaro/daniel/builds/2QA2CHQUpqKe27FyMZrBNILVwXi/config

With that patch on top of v6.1.29 it boots fine under Qemu-arm64; as
v6.1.y stands, it panics with this:
-----8<-----
Unexpected kernel BRK exception at EL1
Internal error: BRK handler: 00000000f2000001 [#1] PREEMPT SMP
Modules linked in:
CPU: 0 PID: 1 Comm: swapper/0 Not tainted 6.1.29 #1
Hardware name: linux,dummy-virt (DT)
pstate: 80000005 (Nzcv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
pc : pskb_expand_head+0x448/0x480
lr : pskb_expand_head+0x13c/0x480
sp : ffff80000802b850
x29: ffff80000802b860 x28: 00000000000002c0 x27: 0000000000000ec0
x26: ffff0000c02c8ec0 x25: ffff0000c02c8000 x24: 00000000000128c0
x23: ffff0000c030e800 x22: ffff0000c030e800 x21: 0000000000000240
x20: 0000000000000000 x19: ffff0000c085e900 x18: ffff800008021068
x17: 00000000ad6b63b6 x16: 00000000ad6b63b6 x15: 0001001c00070038
x14: 0000000c00020008 x13: 00882cc00000ffff x12: 0000000000000000
x11: 0000000000000000 x10: 0000000000000000 x9 : 0000000000000001
x8 : ffff0000c030eac0 x7 : 0000000000000000 x6 : 0000000000000000
x5 : ffff0000c030eaf0 x4 : ffff0000ff7abd10 x3 : 0000000000001740
x2 : ffff0000c02c8000 x1 : 0000000000000000 x0 : 0000000000000000
Call trace:
pskb_expand_head+0x448/0x480
netlink_trim+0xa0/0xc8
netlink_broadcast+0x54/0x764
genl_ctrl_event+0x21c/0x37c
genl_register_family+0x628/0x708
thermal_netlink_init+0x28/0x3c
thermal_init+0x28/0xec
do_one_initcall+0xfc/0x358
do_initcall_level+0xd8/0x1b4
do_initcalls+0x64/0xa8
do_basic_setup+0x2c/0x3c
kernel_init_freeable+0x118/0x198
kernel_init+0x30/0x1c0
ret_from_fork+0x10/0x20
Code: f9406679 38776b28 3707eba8 17ffff67 (d4200020)
---[ end trace 0000000000000000 ]---
Kernel panic - not syncing: BRK handler: Fatal exception
SMP: stopping secondary CPUs
----->8-----

Greetings!

Daniel Díaz
[email protected]

2023-05-24 14:13:22

by Kees Cook

[permalink] [raw]
Subject: Re: Stable backport request: skbuff: Proactively round up to kmalloc bucket size

On May 23, 2023 8:52:53 PM PDT, "Daniel Díaz" <[email protected]> wrote:
>Hello!
>
>On Tue, 23 May 2023 at 00:28, Vlastimil Babka <[email protected]> wrote:
>> On 5/22/23 20:23, Daniel Díaz wrote:
>> > Hello!
>> >
>> > Would the stable maintainers please consider backporting the following
>> > commit to the 6.1? We are trying to build gki_defconfig (plus a few
>> > extras) on Arm64 and test it under Qemu-arm64, but it fails to boot.
>> > Bisection has pointed here.
>>
>> You mean the bisection was done to find the first "good" commit between 6.1
>> and e.g. 6.3?
>>
>> As others said, this commit wasn't expected to be a fix to a known bug.
>> Maybe you found one that we didn't know of, or it might be accidentaly
>> masking some other bug.
>
>How interesting! Yes, we happened to run a bisection between v6.1 and
>v6.3 and we found where it started working with the following
>configuration:
> https://storage.tuxsuite.com/public/linaro/daniel/builds/2QA2CHQUpqKe27FyMZrBNILVwXi/config

Ah yes, from CONFIG_UBSAN_BOUNDS=y and CONFIG_UBSAN_TRAP=y

This was a known issue in upstream, oddly only exposed on arm64. Something re-broke with __alloc_size after commit 93dd04ab0b2b had tried to work around it. I didn't think any kernel released with this broken, though, so perhaps what broke it got added to -stable?

>With that patch on top of v6.1.29 it boots fine under Qemu-arm64; as
>v6.1.y stands, it panics with this:

It should be fine to backport the patch, IMO.


--
Kees Cook