| Message ID | 1715955208-17109-1-git-send-email-xiaojiangfeng@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | arm64: asm-bug: Add .align 2 to the end of __BUG_ENTRY | expand |
On Fri, May 17, 2024 at 10:13:28PM +0800, Jiangfeng Xiao wrote: > I'm using the latest linux kernel mainline code, > with the default arm64 configuration: > make ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- defconfig, > and set CONFIG_EXPERT=y, CONFIG_DEBUG_BUGVERBOSE=n, > CONFIG_PANIC_ON_OOPS=y. > > Loading the following kernel module will cause kernel panic. > > The call stack is as follows: > > root@(none):/# /root/insmod hello.ko > [ 6.035003] hello: loading out-of-tree module taints kernel. > [ 6.039129] ------------[ cut here ]------------ > [ 6.039287] hello > [ 6.039704] Unexpected kernel BRK exception at EL1 > [ 6.040059] Internal error: BRK handler: 00000000f2000800 [#1] PREEMPT SMP > [ 6.040457] Modules linked in: hello(O+) > [ 6.041311] CPU: 0 PID: 50 Comm: insmod Tainted: G O 6.9.1 #8 > [ 6.041755] Hardware name: linux,dummy-virt (DT) > [ 6.042238] pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--) > [ 6.042594] pc : buginit+0x18/0x1000 [hello] > [ 6.043601] lr : buginit+0x18/0x1000 [hello] > [ 6.043852] sp : ffff800080533ae0 > [ 6.044121] x29: ffff800080533ae0 x28: 0000000000000000 x27: 0000000000000000 > [ 6.044523] x26: ffffaba8c4e70510 x25: ffff800080533c30 x24: ffffaba8c4a28a58 > [ 6.044961] x23: 0000000000000000 x22: 0000000000000000 x21: ffff3947c0eab3c0 > [ 6.045503] x20: ffffaba8c4e3f000 x19: ffffaba846464000 x18: 0000000000000006 > [ 6.046124] x17: 0000000000000000 x16: ffffaba8c2492834 x15: 0720072007200720 > [ 6.046387] x14: 0720072007200720 x13: ffffaba8c49b27c8 x12: 0000000000000312 > [ 6.046829] x11: 0000000000000106 x10: ffffaba8c4a0a7c8 x9 : ffffaba8c49b27c8 > [ 6.047293] x8 : 00000000ffffefff x7 : ffffaba8c4a0a7c8 x6 : 80000000fffff000 > [ 6.047739] x5 : 0000000000000107 x4 : 0000000000000000 x3 : 0000000000000000 > [ 6.047955] x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff3947c0eab3c0 > [ 6.048366] Call trace: > [ 6.048653] buginit+0x18/0x1000 [hello] > [ 6.048922] do_one_initcall+0x80/0x1c8 > [ 6.049333] do_init_module+0x60/0x218 > [ 6.049475] load_module+0x1ba4/0x1d70 > [ 6.049755] __do_sys_init_module+0x198/0x1d0 > [ 6.049959] __arm64_sys_init_module+0x1c/0x28 > [ 6.050160] invoke_syscall+0x48/0x114 > [ 6.050334] el0_svc_common.constprop.0+0x40/0xe0 > [ 6.050468] do_el0_svc+0x1c/0x28 > [ 6.050635] el0_svc+0x34/0xd8 > [ 6.050852] el0t_64_sync_handler+0x120/0x12c > [ 6.051088] el0t_64_sync+0x190/0x194 > [ 6.051433] Code: d0ffffe0 910003fd 91000000 9400000b (d4210000) > [ 6.052212] ---[ end trace 0000000000000000 ]--- > [ 6.052473] Kernel panic - not syncing: BRK handler: Fatal exception > > The kernel module source code is as follows: > ``` > > static int __init buginit(void) > { > WARN(1, "hello\n"); > return 0; > } > > static void __exit bugexit(void) > { > } > > module_init(buginit); > module_exit(bugexit); > MODULE_LICENSE("GPL"); > ``` > > When CONFIG_DEBUG_BUGVERBOSE macro is disabled, > the size of "__bug_table" section in hello.ko > is only 6 bytes instead of the expected 8 bytes. > As a result, > mod->num_bugs = sechdrs[i].sh_size / sizeof(struct bug_entry) = 6 / 8 = 0 > calculated in module_bug_finalize when the kernel loads ko is incorrect. > > When running `WARN()`, the following backtrace is triggered: > > module_find_bug() at lib/bug.c > find_bug() at lib/bug.c > __report_bug() at lib/bug.c > report_bug() at lib/bug.c > call_break_hook() at arch/arm64/kernel/debug-monitors.c > brk_handler() at arch/arm64/kernel/debug-monitors.c > > It will return -EFAULT because hello.ko's mod->num_bugs is 0. > Finally, the kernel OOPS is triggered. > > Add .align 2 to the end of __BUG_ENTRY > to make the object layout generated by the assembly code > consistent with that of the C struct bug_entry. > > Fixes: 9fb7410f955f ("arm64/BUG: Use BRK instruction for generic BUG traps") > > Signed-off-by: Yuanbin Xie <xieyuanbin1@huawei.com> > Signed-off-by: Jiangfeng Xiao <xiaojiangfeng@huawei.com> > --- > arch/arm64/include/asm/asm-bug.h | 1 + > 1 file changed, 1 insertion(+) Ouch; sorry about this. I reckon we should spell out the reason for this a bit more; would you be happy with the commit message below? | When CONFIG_DEBUG_BUGVERBOSE=n, we fail to add necessary padding bytes | to bug_table entries, and as a result the last entry in a bug table will | be ignored, potentially leading to an unexpected panic(). All prior | entries in the table will be handled correctly. | | The arm64 ABI requires that struct fields of up to 8 bytes are | naturally-aligned, with padding added within a struct such that struct | are suitably aligned within arrays. | | When CONFIG_DEBUG_BUGVERPOSE=y, the layout of a bug_entry is: | | struct bug_entry { | signed int bug_addr_disp; // 4 bytes | signed int file_disp; // 4 bytes | unsigned short line; // 2 bytes | unsigned short flags; // 2 bytes | } | | ... with 12 bytes total, requiring 4-byte alignment. | | When CONFIG_DEBUG_BUGVERBOSE=n, the layout of a bug_entry is: | | struct bug_entry { | signed int bug_addr_disp; // 4 bytes | unsigned short flags; // 2 bytes | < implicit padding > // 2 bytes | } | | ... with 8 bytes total, with 6 bytes of data and 2 bytes of trailing | padding, requiring 4-byte alginment. | | When we create a bug_entry in assembly, we align the start of the entry | to 4 bytes, which implicitly handles padding for any prior entries. | However, we do not align the end of the entry, and so when | CONFIG_DEBUG_BUGVERBOSE=n, the final entry lacks the trailing padding | bytes. | | For the main kernel image this is not a problem as find_bug() doesn't | depend on the trailing padding bytes when searching for entries: | | for (bug = __start___bug_table; bug < __stop___bug_table; ++bug) | if (bugaddr == bug_addr(bug)) | return bug; | | However for modules, module_bug_finalize() depends on the trailing | bytes when calculating the number of entries: | | mod->num_bugs = sechdrs[i].sh_size / sizeof(struct bug_entry); | | ... and as the last bug_entry lacks the necessary padding bytes, this entry | will not be counted, e.g. in the case of a single entry: | | sechdrs[i].sh_size == 6 | sizeof(struct bug_entry) == 8; | | sechdrs[i].sh_size / sizeof(struct bug_entry) == 0; | | Consequently module_find_bug() will miss the last bug_entry when it does: | | for (i = 0; i < mod->num_bugs; ++i, ++bug) | if (bugaddr == bug_addr(bug)) | goto out; | | ... which can lead to a kenrel panic due to an unhandled bug. | | This can be demonstrated with the following module: | | static int __init buginit(void) | { | WARN(1, "hello\n"); | return 0; | } | | static void __exit bugexit(void) | { | } | | module_init(buginit); | module_exit(bugexit); | MODULE_LICENSE("GPL"); | | ... which will trigger a kernel panic when loaded: | | ------------[ cut here ]------------ | hello | Unexpected kernel BRK exception at EL1 | Internal error: BRK handler: 00000000f2000800 [#1] PREEMPT SMP | Modules linked in: hello(O+) | CPU: 0 PID: 50 Comm: insmod Tainted: G O 6.9.1 #8 | Hardware name: linux,dummy-virt (DT) | pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--) | pc : buginit+0x18/0x1000 [hello] | lr : buginit+0x18/0x1000 [hello] | sp : ffff800080533ae0 | x29: ffff800080533ae0 x28: 0000000000000000 x27: 0000000000000000 | x26: ffffaba8c4e70510 x25: ffff800080533c30 x24: ffffaba8c4a28a58 | x23: 0000000000000000 x22: 0000000000000000 x21: ffff3947c0eab3c0 | x20: ffffaba8c4e3f000 x19: ffffaba846464000 x18: 0000000000000006 | x17: 0000000000000000 x16: ffffaba8c2492834 x15: 0720072007200720 | x14: 0720072007200720 x13: ffffaba8c49b27c8 x12: 0000000000000312 | x11: 0000000000000106 x10: ffffaba8c4a0a7c8 x9 : ffffaba8c49b27c8 | x8 : 00000000ffffefff x7 : ffffaba8c4a0a7c8 x6 : 80000000fffff000 | x5 : 0000000000000107 x4 : 0000000000000000 x3 : 0000000000000000 | x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff3947c0eab3c0 | Call trace: | buginit+0x18/0x1000 [hello] | do_one_initcall+0x80/0x1c8 | do_init_module+0x60/0x218 | load_module+0x1ba4/0x1d70 | __do_sys_init_module+0x198/0x1d0 | __arm64_sys_init_module+0x1c/0x28 | invoke_syscall+0x48/0x114 | el0_svc_common.constprop.0+0x40/0xe0 | do_el0_svc+0x1c/0x28 | el0_svc+0x34/0xd8 | el0t_64_sync_handler+0x120/0x12c | el0t_64_sync+0x190/0x194 | Code: d0ffffe0 910003fd 91000000 9400000b (d4210000) | ---[ end trace 0000000000000000 ]--- | Kernel panic - not syncing: BRK handler: Fatal exception | | Fix this by always aligning the end of a bug_entry to 4 bytes, which is | correct regardless of CONFIG_DEBUG_BUGVERBOSE. | | Fixes: 9fb7410f955f ("arm64/BUG: Use BRK instruction for generic BUG traps") | Signed-off-by: Yuanbin Xie <xieyuanbin1@huawei.com> | Signed-off-by: Jiangfeng Xiao <xiaojiangfeng@huawei.com> With that: Reviewed-by: Mark Rutland <mark.rutland@arm.com> Mark. > > > > diff --git a/arch/arm64/include/asm/asm-bug.h b/arch/arm64/include/asm/asm-bug.h > index c762038..6e73809 100644 > --- a/arch/arm64/include/asm/asm-bug.h > +++ b/arch/arm64/include/asm/asm-bug.h > @@ -28,6 +28,7 @@ > 14470: .long 14471f - .; \ > _BUGVERBOSE_LOCATION(__FILE__, __LINE__) \ > .short flags; \ > + .align 2; \ > .popsection; \ > 14471: > #else > -- > 1.8.5.6 > >
Hi, On 2024/5/20 18:33, Mark Rutland wrote: > On Fri, May 17, 2024 at 10:13:28PM +0800, Jiangfeng Xiao wrote: >> I'm using the latest linux kernel mainline code, >> with the default arm64 configuration: >> make ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- defconfig, >> and set CONFIG_EXPERT=y, CONFIG_DEBUG_BUGVERBOSE=n, >> CONFIG_PANIC_ON_OOPS=y. >> >> Loading the following kernel module will cause kernel panic. >> >> The call stack is as follows: >> >> root@(none):/# /root/insmod hello.ko >> [ 6.035003] hello: loading out-of-tree module taints kernel. >> [ 6.039129] ------------[ cut here ]------------ >> [ 6.039287] hello >> [ 6.039704] Unexpected kernel BRK exception at EL1 >> [ 6.040059] Internal error: BRK handler: 00000000f2000800 [#1] PREEMPT SMP >> [ 6.040457] Modules linked in: hello(O+) >> [ 6.041311] CPU: 0 PID: 50 Comm: insmod Tainted: G O 6.9.1 #8 >> [ 6.041755] Hardware name: linux,dummy-virt (DT) >> [ 6.042238] pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--) >> [ 6.042594] pc : buginit+0x18/0x1000 [hello] >> [ 6.043601] lr : buginit+0x18/0x1000 [hello] >> [ 6.043852] sp : ffff800080533ae0 >> [ 6.044121] x29: ffff800080533ae0 x28: 0000000000000000 x27: 0000000000000000 >> [ 6.044523] x26: ffffaba8c4e70510 x25: ffff800080533c30 x24: ffffaba8c4a28a58 >> [ 6.044961] x23: 0000000000000000 x22: 0000000000000000 x21: ffff3947c0eab3c0 >> [ 6.045503] x20: ffffaba8c4e3f000 x19: ffffaba846464000 x18: 0000000000000006 >> [ 6.046124] x17: 0000000000000000 x16: ffffaba8c2492834 x15: 0720072007200720 >> [ 6.046387] x14: 0720072007200720 x13: ffffaba8c49b27c8 x12: 0000000000000312 >> [ 6.046829] x11: 0000000000000106 x10: ffffaba8c4a0a7c8 x9 : ffffaba8c49b27c8 >> [ 6.047293] x8 : 00000000ffffefff x7 : ffffaba8c4a0a7c8 x6 : 80000000fffff000 >> [ 6.047739] x5 : 0000000000000107 x4 : 0000000000000000 x3 : 0000000000000000 >> [ 6.047955] x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff3947c0eab3c0 >> [ 6.048366] Call trace: >> [ 6.048653] buginit+0x18/0x1000 [hello] >> [ 6.048922] do_one_initcall+0x80/0x1c8 >> [ 6.049333] do_init_module+0x60/0x218 >> [ 6.049475] load_module+0x1ba4/0x1d70 >> [ 6.049755] __do_sys_init_module+0x198/0x1d0 >> [ 6.049959] __arm64_sys_init_module+0x1c/0x28 >> [ 6.050160] invoke_syscall+0x48/0x114 >> [ 6.050334] el0_svc_common.constprop.0+0x40/0xe0 >> [ 6.050468] do_el0_svc+0x1c/0x28 >> [ 6.050635] el0_svc+0x34/0xd8 >> [ 6.050852] el0t_64_sync_handler+0x120/0x12c >> [ 6.051088] el0t_64_sync+0x190/0x194 >> [ 6.051433] Code: d0ffffe0 910003fd 91000000 9400000b (d4210000) >> [ 6.052212] ---[ end trace 0000000000000000 ]--- >> [ 6.052473] Kernel panic - not syncing: BRK handler: Fatal exception >> >> The kernel module source code is as follows: >> ``` >> >> static int __init buginit(void) >> { >> WARN(1, "hello\n"); >> return 0; >> } >> >> static void __exit bugexit(void) >> { >> } >> >> module_init(buginit); >> module_exit(bugexit); >> MODULE_LICENSE("GPL"); >> ``` >> >> When CONFIG_DEBUG_BUGVERBOSE macro is disabled, >> the size of "__bug_table" section in hello.ko >> is only 6 bytes instead of the expected 8 bytes. >> As a result, >> mod->num_bugs = sechdrs[i].sh_size / sizeof(struct bug_entry) = 6 / 8 = 0 >> calculated in module_bug_finalize when the kernel loads ko is incorrect. >> >> When running `WARN()`, the following backtrace is triggered: >> >> module_find_bug() at lib/bug.c >> find_bug() at lib/bug.c >> __report_bug() at lib/bug.c >> report_bug() at lib/bug.c >> call_break_hook() at arch/arm64/kernel/debug-monitors.c >> brk_handler() at arch/arm64/kernel/debug-monitors.c >> >> It will return -EFAULT because hello.ko's mod->num_bugs is 0. >> Finally, the kernel OOPS is triggered. >> >> Add .align 2 to the end of __BUG_ENTRY >> to make the object layout generated by the assembly code >> consistent with that of the C struct bug_entry. >> >> Fixes: 9fb7410f955f ("arm64/BUG: Use BRK instruction for generic BUG traps") >> >> Signed-off-by: Yuanbin Xie <xieyuanbin1@huawei.com> >> Signed-off-by: Jiangfeng Xiao <xiaojiangfeng@huawei.com> >> --- >> arch/arm64/include/asm/asm-bug.h | 1 + >> 1 file changed, 1 insertion(+) > > Ouch; sorry about this. > > I reckon we should spell out the reason for this a bit more; would you > be happy with the commit message below? > > | When CONFIG_DEBUG_BUGVERBOSE=n, we fail to add necessary padding bytes > | to bug_table entries, and as a result the last entry in a bug table will > | be ignored, potentially leading to an unexpected panic(). All prior > | entries in the table will be handled correctly. > | > | The arm64 ABI requires that struct fields of up to 8 bytes are > | naturally-aligned, with padding added within a struct such that struct > | are suitably aligned within arrays. > | > | When CONFIG_DEBUG_BUGVERPOSE=y, the layout of a bug_entry is: > | > | struct bug_entry { > | signed int bug_addr_disp; // 4 bytes > | signed int file_disp; // 4 bytes > | unsigned short line; // 2 bytes > | unsigned short flags; // 2 bytes > | } > | > | ... with 12 bytes total, requiring 4-byte alignment. > | > | When CONFIG_DEBUG_BUGVERBOSE=n, the layout of a bug_entry is: > | > | struct bug_entry { > | signed int bug_addr_disp; // 4 bytes > | unsigned short flags; // 2 bytes > | < implicit padding > // 2 bytes > | } > | > | ... with 8 bytes total, with 6 bytes of data and 2 bytes of trailing > | padding, requiring 4-byte alginment. > | > | When we create a bug_entry in assembly, we align the start of the entry > | to 4 bytes, which implicitly handles padding for any prior entries. > | However, we do not align the end of the entry, and so when > | CONFIG_DEBUG_BUGVERBOSE=n, the final entry lacks the trailing padding > | bytes. > | > | For the main kernel image this is not a problem as find_bug() doesn't > | depend on the trailing padding bytes when searching for entries: > | > | for (bug = __start___bug_table; bug < __stop___bug_table; ++bug) > | if (bugaddr == bug_addr(bug)) > | return bug; > | > | However for modules, module_bug_finalize() depends on the trailing > | bytes when calculating the number of entries: > | > | mod->num_bugs = sechdrs[i].sh_size / sizeof(struct bug_entry); > | > | ... and as the last bug_entry lacks the necessary padding bytes, this entry > | will not be counted, e.g. in the case of a single entry: > | > | sechdrs[i].sh_size == 6 > | sizeof(struct bug_entry) == 8; > | > | sechdrs[i].sh_size / sizeof(struct bug_entry) == 0; > | > | Consequently module_find_bug() will miss the last bug_entry when it does: > | > | for (i = 0; i < mod->num_bugs; ++i, ++bug) > | if (bugaddr == bug_addr(bug)) > | goto out; > | > | ... which can lead to a kenrel panic due to an unhandled bug. > | > | This can be demonstrated with the following module: > | > | static int __init buginit(void) > | { > | WARN(1, "hello\n"); > | return 0; > | } > | > | static void __exit bugexit(void) > | { > | } > | > | module_init(buginit); > | module_exit(bugexit); > | MODULE_LICENSE("GPL"); > | > | ... which will trigger a kernel panic when loaded: > | > | ------------[ cut here ]------------ > | hello > | Unexpected kernel BRK exception at EL1 > | Internal error: BRK handler: 00000000f2000800 [#1] PREEMPT SMP > | Modules linked in: hello(O+) > | CPU: 0 PID: 50 Comm: insmod Tainted: G O 6.9.1 #8 > | Hardware name: linux,dummy-virt (DT) > | pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--) > | pc : buginit+0x18/0x1000 [hello] > | lr : buginit+0x18/0x1000 [hello] > | sp : ffff800080533ae0 > | x29: ffff800080533ae0 x28: 0000000000000000 x27: 0000000000000000 > | x26: ffffaba8c4e70510 x25: ffff800080533c30 x24: ffffaba8c4a28a58 > | x23: 0000000000000000 x22: 0000000000000000 x21: ffff3947c0eab3c0 > | x20: ffffaba8c4e3f000 x19: ffffaba846464000 x18: 0000000000000006 > | x17: 0000000000000000 x16: ffffaba8c2492834 x15: 0720072007200720 > | x14: 0720072007200720 x13: ffffaba8c49b27c8 x12: 0000000000000312 > | x11: 0000000000000106 x10: ffffaba8c4a0a7c8 x9 : ffffaba8c49b27c8 > | x8 : 00000000ffffefff x7 : ffffaba8c4a0a7c8 x6 : 80000000fffff000 > | x5 : 0000000000000107 x4 : 0000000000000000 x3 : 0000000000000000 > | x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff3947c0eab3c0 > | Call trace: > | buginit+0x18/0x1000 [hello] > | do_one_initcall+0x80/0x1c8 > | do_init_module+0x60/0x218 > | load_module+0x1ba4/0x1d70 > | __do_sys_init_module+0x198/0x1d0 > | __arm64_sys_init_module+0x1c/0x28 > | invoke_syscall+0x48/0x114 > | el0_svc_common.constprop.0+0x40/0xe0 > | do_el0_svc+0x1c/0x28 > | el0_svc+0x34/0xd8 > | el0t_64_sync_handler+0x120/0x12c > | el0t_64_sync+0x190/0x194 > | Code: d0ffffe0 910003fd 91000000 9400000b (d4210000) > | ---[ end trace 0000000000000000 ]--- > | Kernel panic - not syncing: BRK handler: Fatal exception > | > | Fix this by always aligning the end of a bug_entry to 4 bytes, which is > | correct regardless of CONFIG_DEBUG_BUGVERBOSE. > | > | Fixes: 9fb7410f955f ("arm64/BUG: Use BRK instruction for generic BUG traps") > | Signed-off-by: Yuanbin Xie <xieyuanbin1@huawei.com> > | Signed-off-by: Jiangfeng Xiao <xiaojiangfeng@huawei.com> > > With that: > > Reviewed-by: Mark Rutland <mark.rutland@arm.com> > > Mark. > >> >> >> >> diff --git a/arch/arm64/include/asm/asm-bug.h b/arch/arm64/include/asm/asm-bug.h >> index c762038..6e73809 100644 >> --- a/arch/arm64/include/asm/asm-bug.h >> +++ b/arch/arm64/include/asm/asm-bug.h >> @@ -28,6 +28,7 @@ >> 14470: .long 14471f - .; \ >> _BUGVERBOSE_LOCATION(__FILE__, __LINE__) \ >> .short flags; \ >> + .align 2; \ The use of .align 2 here is based on the assumption that struct bug_entry is 4-byte aligned. Currently, there is no problem with this assumption, but for compatibility reasons, refer to the riscv architecture and refactor the implementation of __BUG_FLAGS: #define __BUG_FLAGS(flags) \ do { \ __asm__ __volatile__ ( \ "1:\n\t" \ "ebreak\n" \ ".pushsection __bug_table,\"aw\"\n\t" \ "2:\n\t" \ __BUG_ENTRY "\n\t" \ ".org 2b + %3\n\t" \ ".popsection" \ : \ : "i" (__FILE__), "i" (__LINE__), \ "i" (flags), \ "i" (sizeof(struct bug_entry))); \ } while (0) Align the real size of struct bug_entry through .org. What do you think? >> .popsection; \ >> 14471: >> #else >> -- >> 1.8.5.6 >> >> >
On Mon, May 20, 2024 at 08:05:25PM +0800, Li Zetao wrote: > On 2024/5/20 18:33, Mark Rutland wrote: > > On Fri, May 17, 2024 at 10:13:28PM +0800, Jiangfeng Xiao wrote: > > > diff --git a/arch/arm64/include/asm/asm-bug.h b/arch/arm64/include/asm/asm-bug.h > > > index c762038..6e73809 100644 > > > --- a/arch/arm64/include/asm/asm-bug.h > > > +++ b/arch/arm64/include/asm/asm-bug.h > > > @@ -28,6 +28,7 @@ > > > 14470: .long 14471f - .; \ > > > _BUGVERBOSE_LOCATION(__FILE__, __LINE__) \ > > > .short flags; \ > > > + .align 2; \ > The use of .align 2 here is based on the assumption that struct bug_entry is > 4-byte aligned. Currently, there is no problem with this assumption, but for > compatibility reasons, refer to the riscv architecture and refactor the > implementation of __BUG_FLAGS: > > #define __BUG_FLAGS(flags) \ > do { \ > __asm__ __volatile__ ( \ > "1:\n\t" \ > "ebreak\n" \ > ".pushsection __bug_table,\"aw\"\n\t" \ > "2:\n\t" \ > __BUG_ENTRY "\n\t" \ > ".org 2b + %3\n\t" \ > ".popsection" \ > : \ > : "i" (__FILE__), "i" (__LINE__), \ > "i" (flags), \ > "i" (sizeof(struct bug_entry))); \ > } while (0) > > Align the real size of struct bug_entry through .org. What do you think? I can see why that appears nice, but we can't do that in asm-bug.h without something in asm-offets.h, and I suspect that's going to lead to pain with circular header dependencies. Regardless, if things change we'll need to make other changes here, so I don't think we gain much. I think it's preferable to have the explicit .align for now. Mark.
On 2024/5/20 20:05, Li Zetao wrote: >>> diff --git a/arch/arm64/include/asm/asm-bug.h b/arch/arm64/include/asm/asm-bug.h >>> index c762038..6e73809 100644 >>> --- a/arch/arm64/include/asm/asm-bug.h >>> +++ b/arch/arm64/include/asm/asm-bug.h >>> @@ -28,6 +28,7 @@ >>> 14470: .long 14471f - .; \ >>> _BUGVERBOSE_LOCATION(__FILE__, __LINE__) \ >>> .short flags; \ >>> + .align 2; \ > The use of .align 2 here is based on the assumption that struct bug_entry is 4-byte aligned. Currently, there is no problem with this assumption, but for compatibility reasons, refer to the riscv architecture and refactor the implementation of __BUG_FLAGS: > > #define __BUG_FLAGS(flags) \ > do { \ > __asm__ __volatile__ ( \ > "1:\n\t" \ > "ebreak\n" \ > ".pushsection __bug_table,\"aw\"\n\t" \ > "2:\n\t" \ > __BUG_ENTRY "\n\t" \ > ".org 2b + %3\n\t" \ > ".popsection" \ > : \ > : "i" (__FILE__), "i" (__LINE__), \ > "i" (flags), \ > "i" (sizeof(struct bug_entry))); \ > } while (0) > > Align the real size of struct bug_entry through .org. What do you think? The implementation of risc-v BUG_ENTRY does handle the `implicit padding` at the end of the struct correctly, however, it does not handle the `implicit padding` in the middle of the struct correctly, for example, assume that the struct bug_entry changes as follows in the future: struct bug_entry { signed int bug_addr_disp; // 4 bytes unsigned short flags; // 2 bytes < implicit padding > // 6 bytes unsigned long long flags2; // 8 bytes } Even the implementation of risc-v BUG_ENTRY can't handle this situation. Referencing risc-v solution complicates things, but doesn't completely solve the problem. In the current scenario, we know the contents of struct bug_entry and generate variables using assembly language. I don't think it's necessary to complicate things.
On 2024/5/20 18:33, Mark Rutland wrote: > I reckon we should spell out the reason for this a bit more; would you > be happy with the commit message below? > > | When CONFIG_DEBUG_BUGVERBOSE=n, we fail to add necessary padding bytes > | to bug_table entries, and as a result the last entry in a bug table will > | be ignored, potentially leading to an unexpected panic(). All prior > | entries in the table will be handled correctly. > | > | The arm64 ABI requires that struct fields of up to 8 bytes are > | naturally-aligned, with padding added within a struct such that struct > | are suitably aligned within arrays. > | > | When CONFIG_DEBUG_BUGVERPOSE=y, the layout of a bug_entry is: > | > | struct bug_entry { > | signed int bug_addr_disp; // 4 bytes > | signed int file_disp; // 4 bytes > | unsigned short line; // 2 bytes > | unsigned short flags; // 2 bytes > | } > | > | ... with 12 bytes total, requiring 4-byte alignment. > | > | When CONFIG_DEBUG_BUGVERBOSE=n, the layout of a bug_entry is: > | > | struct bug_entry { > | signed int bug_addr_disp; // 4 bytes > | unsigned short flags; // 2 bytes > | < implicit padding > // 2 bytes > | } > | > | ... with 8 bytes total, with 6 bytes of data and 2 bytes of trailing > | padding, requiring 4-byte alginment. > | > | When we create a bug_entry in assembly, we align the start of the entry > | to 4 bytes, which implicitly handles padding for any prior entries. > | However, we do not align the end of the entry, and so when > | CONFIG_DEBUG_BUGVERBOSE=n, the final entry lacks the trailing padding > | bytes. > | > | For the main kernel image this is not a problem as find_bug() doesn't > | depend on the trailing padding bytes when searching for entries: > | > | for (bug = __start___bug_table; bug < __stop___bug_table; ++bug) > | if (bugaddr == bug_addr(bug)) > | return bug; > | > | However for modules, module_bug_finalize() depends on the trailing > | bytes when calculating the number of entries: > | > | mod->num_bugs = sechdrs[i].sh_size / sizeof(struct bug_entry); > | > | ... and as the last bug_entry lacks the necessary padding bytes, this entry > | will not be counted, e.g. in the case of a single entry: > | > | sechdrs[i].sh_size == 6 > | sizeof(struct bug_entry) == 8; > | > | sechdrs[i].sh_size / sizeof(struct bug_entry) == 0; > | > | Consequently module_find_bug() will miss the last bug_entry when it does: > | > | for (i = 0; i < mod->num_bugs; ++i, ++bug) > | if (bugaddr == bug_addr(bug)) > | goto out; > | > | ... which can lead to a kenrel panic due to an unhandled bug. > | > | This can be demonstrated with the following module: > | > | static int __init buginit(void) > | { > | WARN(1, "hello\n"); > | return 0; > | } > | > | static void __exit bugexit(void) > | { > | } > | > | module_init(buginit); > | module_exit(bugexit); > | MODULE_LICENSE("GPL"); > | > | ... which will trigger a kernel panic when loaded: > | > | ------------[ cut here ]------------ > | hello > | Unexpected kernel BRK exception at EL1 > | Internal error: BRK handler: 00000000f2000800 [#1] PREEMPT SMP > | Modules linked in: hello(O+) > | CPU: 0 PID: 50 Comm: insmod Tainted: G O 6.9.1 #8 > | Hardware name: linux,dummy-virt (DT) > | pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--) > | pc : buginit+0x18/0x1000 [hello] > | lr : buginit+0x18/0x1000 [hello] > | sp : ffff800080533ae0 > | x29: ffff800080533ae0 x28: 0000000000000000 x27: 0000000000000000 > | x26: ffffaba8c4e70510 x25: ffff800080533c30 x24: ffffaba8c4a28a58 > | x23: 0000000000000000 x22: 0000000000000000 x21: ffff3947c0eab3c0 > | x20: ffffaba8c4e3f000 x19: ffffaba846464000 x18: 0000000000000006 > | x17: 0000000000000000 x16: ffffaba8c2492834 x15: 0720072007200720 > | x14: 0720072007200720 x13: ffffaba8c49b27c8 x12: 0000000000000312 > | x11: 0000000000000106 x10: ffffaba8c4a0a7c8 x9 : ffffaba8c49b27c8 > | x8 : 00000000ffffefff x7 : ffffaba8c4a0a7c8 x6 : 80000000fffff000 > | x5 : 0000000000000107 x4 : 0000000000000000 x3 : 0000000000000000 > | x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff3947c0eab3c0 > | Call trace: > | buginit+0x18/0x1000 [hello] > | do_one_initcall+0x80/0x1c8 > | do_init_module+0x60/0x218 > | load_module+0x1ba4/0x1d70 > | __do_sys_init_module+0x198/0x1d0 > | __arm64_sys_init_module+0x1c/0x28 > | invoke_syscall+0x48/0x114 > | el0_svc_common.constprop.0+0x40/0xe0 > | do_el0_svc+0x1c/0x28 > | el0_svc+0x34/0xd8 > | el0t_64_sync_handler+0x120/0x12c > | el0t_64_sync+0x190/0x194 > | Code: d0ffffe0 910003fd 91000000 9400000b (d4210000) > | ---[ end trace 0000000000000000 ]--- > | Kernel panic - not syncing: BRK handler: Fatal exception > | > | Fix this by always aligning the end of a bug_entry to 4 bytes, which is > | correct regardless of CONFIG_DEBUG_BUGVERBOSE. > | > | Fixes: 9fb7410f955f ("arm64/BUG: Use BRK instruction for generic BUG traps") > | Signed-off-by: Yuanbin Xie <xieyuanbin1@huawei.com> > | Signed-off-by: Jiangfeng Xiao <xiaojiangfeng@huawei.com> > > With that: > > Reviewed-by: Mark Rutland <mark.rutland@arm.com> > > Mark. > Thank you very much for your patience and professional review. Indeed, we have not clearly expressed the specific layout of `struct bug_entry` under the arm64 ABI and how it causes the problem. We have only briefly described that the assembly code does not consistent with the binary layout of the C struct. Your commit messge is much more accurate in describing the problem, and we couldn't agree more. I will follow your advice to submit our V2 patch.
diff --git a/arch/arm64/include/asm/asm-bug.h b/arch/arm64/include/asm/asm-bug.h index c762038..6e73809 100644 --- a/arch/arm64/include/asm/asm-bug.h +++ b/arch/arm64/include/asm/asm-bug.h @@ -28,6 +28,7 @@ 14470: .long 14471f - .; \ _BUGVERBOSE_LOCATION(__FILE__, __LINE__) \ .short flags; \ + .align 2; \ .popsection; \ 14471: #else
I'm using the latest linux kernel mainline code, with the default arm64 configuration: make ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- defconfig, and set CONFIG_EXPERT=y, CONFIG_DEBUG_BUGVERBOSE=n, CONFIG_PANIC_ON_OOPS=y. Loading the following kernel module will cause kernel panic. The call stack is as follows: root@(none):/# /root/insmod hello.ko [ 6.035003] hello: loading out-of-tree module taints kernel. [ 6.039129] ------------[ cut here ]------------ [ 6.039287] hello [ 6.039704] Unexpected kernel BRK exception at EL1 [ 6.040059] Internal error: BRK handler: 00000000f2000800 [#1] PREEMPT SMP [ 6.040457] Modules linked in: hello(O+) [ 6.041311] CPU: 0 PID: 50 Comm: insmod Tainted: G O 6.9.1 #8 [ 6.041755] Hardware name: linux,dummy-virt (DT) [ 6.042238] pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--) [ 6.042594] pc : buginit+0x18/0x1000 [hello] [ 6.043601] lr : buginit+0x18/0x1000 [hello] [ 6.043852] sp : ffff800080533ae0 [ 6.044121] x29: ffff800080533ae0 x28: 0000000000000000 x27: 0000000000000000 [ 6.044523] x26: ffffaba8c4e70510 x25: ffff800080533c30 x24: ffffaba8c4a28a58 [ 6.044961] x23: 0000000000000000 x22: 0000000000000000 x21: ffff3947c0eab3c0 [ 6.045503] x20: ffffaba8c4e3f000 x19: ffffaba846464000 x18: 0000000000000006 [ 6.046124] x17: 0000000000000000 x16: ffffaba8c2492834 x15: 0720072007200720 [ 6.046387] x14: 0720072007200720 x13: ffffaba8c49b27c8 x12: 0000000000000312 [ 6.046829] x11: 0000000000000106 x10: ffffaba8c4a0a7c8 x9 : ffffaba8c49b27c8 [ 6.047293] x8 : 00000000ffffefff x7 : ffffaba8c4a0a7c8 x6 : 80000000fffff000 [ 6.047739] x5 : 0000000000000107 x4 : 0000000000000000 x3 : 0000000000000000 [ 6.047955] x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff3947c0eab3c0 [ 6.048366] Call trace: [ 6.048653] buginit+0x18/0x1000 [hello] [ 6.048922] do_one_initcall+0x80/0x1c8 [ 6.049333] do_init_module+0x60/0x218 [ 6.049475] load_module+0x1ba4/0x1d70 [ 6.049755] __do_sys_init_module+0x198/0x1d0 [ 6.049959] __arm64_sys_init_module+0x1c/0x28 [ 6.050160] invoke_syscall+0x48/0x114 [ 6.050334] el0_svc_common.constprop.0+0x40/0xe0 [ 6.050468] do_el0_svc+0x1c/0x28 [ 6.050635] el0_svc+0x34/0xd8 [ 6.050852] el0t_64_sync_handler+0x120/0x12c [ 6.051088] el0t_64_sync+0x190/0x194 [ 6.051433] Code: d0ffffe0 910003fd 91000000 9400000b (d4210000) [ 6.052212] ---[ end trace 0000000000000000 ]--- [ 6.052473] Kernel panic - not syncing: BRK handler: Fatal exception The kernel module source code is as follows: ``` static int __init buginit(void) { WARN(1, "hello\n"); return 0; } static void __exit bugexit(void) { } module_init(buginit); module_exit(bugexit); MODULE_LICENSE("GPL"); ``` When CONFIG_DEBUG_BUGVERBOSE macro is disabled, the size of "__bug_table" section in hello.ko is only 6 bytes instead of the expected 8 bytes. As a result, mod->num_bugs = sechdrs[i].sh_size / sizeof(struct bug_entry) = 6 / 8 = 0 calculated in module_bug_finalize when the kernel loads ko is incorrect. When running `WARN()`, the following backtrace is triggered: module_find_bug() at lib/bug.c find_bug() at lib/bug.c __report_bug() at lib/bug.c report_bug() at lib/bug.c call_break_hook() at arch/arm64/kernel/debug-monitors.c brk_handler() at arch/arm64/kernel/debug-monitors.c It will return -EFAULT because hello.ko's mod->num_bugs is 0. Finally, the kernel OOPS is triggered. Add .align 2 to the end of __BUG_ENTRY to make the object layout generated by the assembly code consistent with that of the C struct bug_entry. Fixes: 9fb7410f955f ("arm64/BUG: Use BRK instruction for generic BUG traps") Signed-off-by: Yuanbin Xie <xieyuanbin1@huawei.com> Signed-off-by: Jiangfeng Xiao <xiaojiangfeng@huawei.com> --- arch/arm64/include/asm/asm-bug.h | 1 + 1 file changed, 1 insertion(+)