| Message ID | 20250326133521.13637-1-angelos@igalia.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | arm64: Don't call NULL in do_compat_alignment_fixup | expand |
On Wed, Mar 26, 2025 at 02:35:21PM +0100, Angelos Oikonomopoulos wrote: > do_alignment_t32_to_handler only fixes up alignment faults for specific > instructions; it returns NULL otherwise. When that's the case, signal to > the caller that it needs to proceed with the regular alignment fault > handling (i.e. SIGBUS). Did you hit this in practice? Which instruction triggered the alignment fault that was not handled by do_alignment_t32_to_handler()? Standard LDR/STR should not trigger unaligned accesses unless you have some device memory mapped in user space. > Signed-off-by: Angelos Oikonomopoulos <angelos@igalia.com> > --- > arch/arm64/kernel/compat_alignment.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/kernel/compat_alignment.c b/arch/arm64/kernel/compat_alignment.c > index deff21bfa680..76cf1c1b5bc6 100644 > --- a/arch/arm64/kernel/compat_alignment.c > +++ b/arch/arm64/kernel/compat_alignment.c > @@ -34,7 +34,7 @@ > > #define REGMASK_BITS(i) (i & 0xffff) > > -#define BAD_INSTR 0xdeadc0de > +#define BAD_INSTR 0xdeadc0de Unrelated change (white space I guess), please drop it, not worth fixing.
On Fri Mar 28, 2025 at 8:06 PM CET, Catalin Marinas wrote: > On Wed, Mar 26, 2025 at 02:35:21PM +0100, Angelos Oikonomopoulos wrote: >> do_alignment_t32_to_handler only fixes up alignment faults for specific >> instructions; it returns NULL otherwise. When that's the case, signal to >> the caller that it needs to proceed with the regular alignment fault >> handling (i.e. SIGBUS). > > Did you hit this in practice? Which instruction triggered the alignment > fault that was not handled by do_alignment_t32_to_handler()? Standard > LDR/STR should not trigger unaligned accesses unless you have some > device memory mapped in user space. Yah, I've hit this in practice. The offending instruction was an ldrex to an unaligned address, while running 32-bit code on an "Ampere(R) Altra(R) Processor Q80-30 CPU @ 3.0GHz". Fixing the unaligned access in the program is one thing, but this resulted in multiple oopses in CI. >> #define REGMASK_BITS(i) (i & 0xffff) >> >> -#define BAD_INSTR 0xdeadc0de >> +#define BAD_INSTR 0xdeadc0de > > Unrelated change (white space I guess), please drop it, not worth > fixing. That snuck past me in an amend, will send a v2. Thanks, Angelos
diff --git a/arch/arm64/kernel/compat_alignment.c b/arch/arm64/kernel/compat_alignment.c index deff21bfa680..76cf1c1b5bc6 100644 --- a/arch/arm64/kernel/compat_alignment.c +++ b/arch/arm64/kernel/compat_alignment.c @@ -34,7 +34,7 @@ #define REGMASK_BITS(i) (i & 0xffff) -#define BAD_INSTR 0xdeadc0de +#define BAD_INSTR 0xdeadc0de /* Thumb-2 32 bit format per ARMv7 DDI0406A A6.3, either f800h,e800h,f800h */ #define IS_T32(hi16) \ @@ -368,6 +368,8 @@ int do_compat_alignment_fixup(unsigned long addr, struct pt_regs *regs) return 1; } + if (!handler) + return 1; type = handler(addr, instr, regs); if (type == TYPE_ERROR || type == TYPE_FAULT)
do_alignment_t32_to_handler only fixes up alignment faults for specific instructions; it returns NULL otherwise. When that's the case, signal to the caller that it needs to proceed with the regular alignment fault handling (i.e. SIGBUS). Signed-off-by: Angelos Oikonomopoulos <angelos@igalia.com> --- arch/arm64/kernel/compat_alignment.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)