* [PATCH] arm64: Don't call NULL in do_compat_alignment_fixup
@ 2025-03-26 13:35 Angelos Oikonomopoulos
2025-03-28 19:06 ` Catalin Marinas
0 siblings, 1 reply; 11+ messages in thread
From: Angelos Oikonomopoulos @ 2025-03-26 13:35 UTC (permalink / raw)
To: linux-arm-kernel
Cc: catalin.marinas, will, linux-kernel, kernel-dev,
Angelos Oikonomopoulos
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(-)
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)
--
2.49.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] arm64: Don't call NULL in do_compat_alignment_fixup
2025-03-26 13:35 [PATCH] arm64: Don't call NULL in do_compat_alignment_fixup Angelos Oikonomopoulos
@ 2025-03-28 19:06 ` Catalin Marinas
2025-03-31 7:57 ` Angelos Oikonomopoulos
0 siblings, 1 reply; 11+ messages in thread
From: Catalin Marinas @ 2025-03-28 19:06 UTC (permalink / raw)
To: Angelos Oikonomopoulos; +Cc: linux-arm-kernel, will, linux-kernel, kernel-dev
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.
--
Catalin
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] arm64: Don't call NULL in do_compat_alignment_fixup
2025-03-28 19:06 ` Catalin Marinas
@ 2025-03-31 7:57 ` Angelos Oikonomopoulos
0 siblings, 0 replies; 11+ messages in thread
From: Angelos Oikonomopoulos @ 2025-03-31 7:57 UTC (permalink / raw)
To: Catalin Marinas; +Cc: linux-arm-kernel, will, linux-kernel, kernel-dev
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH] arm64: Don't call NULL in do_compat_alignment_fixup
@ 2025-03-31 8:54 Angelos Oikonomopoulos
2025-04-01 6:05 ` Anshuman Khandual
0 siblings, 1 reply; 11+ messages in thread
From: Angelos Oikonomopoulos @ 2025-03-31 8:54 UTC (permalink / raw)
To: linux-arm-kernel
Cc: catalin.marinas, will, linux-kernel, kernel-dev,
Angelos Oikonomopoulos
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 | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/arm64/kernel/compat_alignment.c b/arch/arm64/kernel/compat_alignment.c
index deff21bfa680..b68e1d328d4c 100644
--- a/arch/arm64/kernel/compat_alignment.c
+++ b/arch/arm64/kernel/compat_alignment.c
@@ -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)
--
2.49.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] arm64: Don't call NULL in do_compat_alignment_fixup
2025-03-31 8:54 Angelos Oikonomopoulos
@ 2025-04-01 6:05 ` Anshuman Khandual
2025-04-01 6:58 ` Angelos Oikonomopoulos
0 siblings, 1 reply; 11+ messages in thread
From: Anshuman Khandual @ 2025-04-01 6:05 UTC (permalink / raw)
To: Angelos Oikonomopoulos, linux-arm-kernel
Cc: catalin.marinas, will, linux-kernel, kernel-dev
On 3/31/25 14:24, 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).
>
> Signed-off-by: Angelos Oikonomopoulos <angelos@igalia.com>
> ---
> arch/arm64/kernel/compat_alignment.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/arch/arm64/kernel/compat_alignment.c b/arch/arm64/kernel/compat_alignment.c
> index deff21bfa680..b68e1d328d4c 100644
> --- a/arch/arm64/kernel/compat_alignment.c
> +++ b/arch/arm64/kernel/compat_alignment.c
> @@ -368,6 +368,8 @@ int do_compat_alignment_fixup(unsigned long addr, struct pt_regs *regs)
> return 1;
> }
>
> + if (!handler)
> + return 1;
do_alignment_t32_to_handler() could return NULL, returning 1 seems to be
the right thing to do here and consistent. Otherwise does this cause a
kernel crash during subsequent call into handler() ?
> type = handler(addr, instr, regs);
>
> if (type == TYPE_ERROR || type == TYPE_FAULT)
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] arm64: Don't call NULL in do_compat_alignment_fixup
2025-04-01 6:05 ` Anshuman Khandual
@ 2025-04-01 6:58 ` Angelos Oikonomopoulos
2025-04-01 7:47 ` Anshuman Khandual
0 siblings, 1 reply; 11+ messages in thread
From: Angelos Oikonomopoulos @ 2025-04-01 6:58 UTC (permalink / raw)
To: Anshuman Khandual, linux-arm-kernel
Cc: catalin.marinas, will, linux-kernel, kernel-dev
On Tue Apr 1, 2025 at 8:05 AM CEST, Anshuman Khandual wrote:
> On 3/31/25 14:24, 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).
>>
>> Signed-off-by: Angelos Oikonomopoulos <angelos@igalia.com>
>> ---
>> arch/arm64/kernel/compat_alignment.c | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/arch/arm64/kernel/compat_alignment.c b/arch/arm64/kernel/compat_alignment.c
>> index deff21bfa680..b68e1d328d4c 100644
>> --- a/arch/arm64/kernel/compat_alignment.c
>> +++ b/arch/arm64/kernel/compat_alignment.c
>> @@ -368,6 +368,8 @@ int do_compat_alignment_fixup(unsigned long addr, struct pt_regs *regs)
>> return 1;
>> }
>>
>> + if (!handler)
>> + return 1;
>
> do_alignment_t32_to_handler() could return NULL, returning 1 seems to be
> the right thing to do here and consistent. Otherwise does this cause a
> kernel crash during subsequent call into handler() ?
Yes. We call a NULL pointer so we Oops.
Angelos
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] arm64: Don't call NULL in do_compat_alignment_fixup
2025-04-01 6:58 ` Angelos Oikonomopoulos
@ 2025-04-01 7:47 ` Anshuman Khandual
2025-04-01 8:09 ` Angelos Oikonomopoulos
0 siblings, 1 reply; 11+ messages in thread
From: Anshuman Khandual @ 2025-04-01 7:47 UTC (permalink / raw)
To: Angelos Oikonomopoulos, linux-arm-kernel
Cc: catalin.marinas, will, linux-kernel, kernel-dev
On 4/1/25 12:28, Angelos Oikonomopoulos wrote:
> On Tue Apr 1, 2025 at 8:05 AM CEST, Anshuman Khandual wrote:
>> On 3/31/25 14:24, 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).
>>>
>>> Signed-off-by: Angelos Oikonomopoulos <angelos@igalia.com>
>>> ---
>>> arch/arm64/kernel/compat_alignment.c | 2 ++
>>> 1 file changed, 2 insertions(+)
>>>
>>> diff --git a/arch/arm64/kernel/compat_alignment.c b/arch/arm64/kernel/compat_alignment.c
>>> index deff21bfa680..b68e1d328d4c 100644
>>> --- a/arch/arm64/kernel/compat_alignment.c
>>> +++ b/arch/arm64/kernel/compat_alignment.c
>>> @@ -368,6 +368,8 @@ int do_compat_alignment_fixup(unsigned long addr, struct pt_regs *regs)
>>> return 1;
>>> }
>>>
>>> + if (!handler)
>>> + return 1;
>>
>> do_alignment_t32_to_handler() could return NULL, returning 1 seems to be
>> the right thing to do here and consistent. Otherwise does this cause a
>> kernel crash during subsequent call into handler() ?
>
> Yes. We call a NULL pointer so we Oops.
Then the commit message should have the kernel Oops splash dump and also
might need to have Fixes: and CC: stable tags etc ?
Also wondering if handler return value should be checked inside the switch
block just after do_alignment_t32_to_handler() assignment.
handler = do_alignment_t32_to_handler()
if (!handler)
return 1
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] arm64: Don't call NULL in do_compat_alignment_fixup
2025-04-01 7:47 ` Anshuman Khandual
@ 2025-04-01 8:09 ` Angelos Oikonomopoulos
2025-04-01 8:22 ` Anshuman Khandual
0 siblings, 1 reply; 11+ messages in thread
From: Angelos Oikonomopoulos @ 2025-04-01 8:09 UTC (permalink / raw)
To: Anshuman Khandual, linux-arm-kernel
Cc: catalin.marinas, will, linux-kernel, kernel-dev
On Tue Apr 1, 2025 at 9:47 AM CEST, Anshuman Khandual wrote:
>
>
> On 4/1/25 12:28, Angelos Oikonomopoulos wrote:
>> On Tue Apr 1, 2025 at 8:05 AM CEST, Anshuman Khandual wrote:
>>> On 3/31/25 14:24, Angelos Oikonomopoulos wrote:
[...]
>>>> arch/arm64/kernel/compat_alignment.c | 2 ++
>>>> 1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/arch/arm64/kernel/compat_alignment.c b/arch/arm64/kernel/compat_alignment.c
>>>> index deff21bfa680..b68e1d328d4c 100644
>>>> --- a/arch/arm64/kernel/compat_alignment.c
>>>> +++ b/arch/arm64/kernel/compat_alignment.c
>>>> @@ -368,6 +368,8 @@ int do_compat_alignment_fixup(unsigned long addr, struct pt_regs *regs)
>>>> return 1;
>>>> }
>>>>
>>>> + if (!handler)
>>>> + return 1;
>>>
>>> do_alignment_t32_to_handler() could return NULL, returning 1 seems to be
>>> the right thing to do here and consistent. Otherwise does this cause a
>>> kernel crash during subsequent call into handler() ?
>>
>> Yes. We call a NULL pointer so we Oops.
>
> Then the commit message should have the kernel Oops splash dump and also
> might need to have Fixes: and CC: stable tags etc ?
Sure, I can add those. Thanks for the suggestions!
> Also wondering if handler return value should be checked inside the switch
> block just after do_alignment_t32_to_handler() assignment.
>
> handler = do_alignment_t32_to_handler()
> if (!handler)
> return 1
I can see the appeal of that, but I think placing the check right before
the single dereference is a more future-proof fix, in that it reduce the
chances that a later patch will re-introduce a potential NULL pointer
dereference.
Angelos
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] arm64: Don't call NULL in do_compat_alignment_fixup
2025-04-01 8:09 ` Angelos Oikonomopoulos
@ 2025-04-01 8:22 ` Anshuman Khandual
2025-04-01 8:58 ` Angelos Oikonomopoulos
0 siblings, 1 reply; 11+ messages in thread
From: Anshuman Khandual @ 2025-04-01 8:22 UTC (permalink / raw)
To: Angelos Oikonomopoulos, linux-arm-kernel
Cc: catalin.marinas, will, linux-kernel, kernel-dev
On 4/1/25 13:39, Angelos Oikonomopoulos wrote:
> On Tue Apr 1, 2025 at 9:47 AM CEST, Anshuman Khandual wrote:
>>
>>
>> On 4/1/25 12:28, Angelos Oikonomopoulos wrote:
>>> On Tue Apr 1, 2025 at 8:05 AM CEST, Anshuman Khandual wrote:
>>>> On 3/31/25 14:24, Angelos Oikonomopoulos wrote:
> [...]
>>>>> arch/arm64/kernel/compat_alignment.c | 2 ++
>>>>> 1 file changed, 2 insertions(+)
>>>>>
>>>>> diff --git a/arch/arm64/kernel/compat_alignment.c b/arch/arm64/kernel/compat_alignment.c
>>>>> index deff21bfa680..b68e1d328d4c 100644
>>>>> --- a/arch/arm64/kernel/compat_alignment.c
>>>>> +++ b/arch/arm64/kernel/compat_alignment.c
>>>>> @@ -368,6 +368,8 @@ int do_compat_alignment_fixup(unsigned long addr, struct pt_regs *regs)
>>>>> return 1;
>>>>> }
>>>>>
>>>>> + if (!handler)
>>>>> + return 1;
>>>>
>>>> do_alignment_t32_to_handler() could return NULL, returning 1 seems to be
>>>> the right thing to do here and consistent. Otherwise does this cause a
>>>> kernel crash during subsequent call into handler() ?
>>>
>>> Yes. We call a NULL pointer so we Oops.
>>
>> Then the commit message should have the kernel Oops splash dump and also
>> might need to have Fixes: and CC: stable tags etc ?
>
> Sure, I can add those. Thanks for the suggestions!
>
>> Also wondering if handler return value should be checked inside the switch
>> block just after do_alignment_t32_to_handler() assignment.
>>
>> handler = do_alignment_t32_to_handler()
>> if (!handler)
>> return 1
>
> I can see the appeal of that, but I think placing the check right before
> the single dereference is a more future-proof fix, in that it reduce the
> chances that a later patch will re-introduce a potential NULL pointer
> dereference.
Makes sense. A small nit - just wondering if the following restructuring
here would make things bit more readable ? Regardless, your decision.
if (handler)
type = handler(addr, instr, regs);
else
return 1;
>
> Angelos
>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] arm64: Don't call NULL in do_compat_alignment_fixup
2025-04-01 8:22 ` Anshuman Khandual
@ 2025-04-01 8:58 ` Angelos Oikonomopoulos
2025-04-04 13:58 ` Will Deacon
0 siblings, 1 reply; 11+ messages in thread
From: Angelos Oikonomopoulos @ 2025-04-01 8:58 UTC (permalink / raw)
To: Anshuman Khandual, linux-arm-kernel
Cc: catalin.marinas, will, linux-kernel, kernel-dev
On Tue Apr 1, 2025 at 10:22 AM CEST, Anshuman Khandual wrote:
> Makes sense. A small nit - just wondering if the following restructuring
> here would make things bit more readable ? Regardless, your decision.
>
> if (handler)
> type = handler(addr, instr, regs);
> else
> return 1;
I went with the original formulation since -- to my mind at least -- an
"early exit" idiom feels more appropriate for something we found we
can't handle. Could work either way though.
Thanks,
Angelos
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] arm64: Don't call NULL in do_compat_alignment_fixup
2025-04-01 8:58 ` Angelos Oikonomopoulos
@ 2025-04-04 13:58 ` Will Deacon
0 siblings, 0 replies; 11+ messages in thread
From: Will Deacon @ 2025-04-04 13:58 UTC (permalink / raw)
To: Angelos Oikonomopoulos
Cc: Anshuman Khandual, linux-arm-kernel, catalin.marinas,
linux-kernel, kernel-dev
On Tue, Apr 01, 2025 at 10:58:48AM +0200, Angelos Oikonomopoulos wrote:
> On Tue Apr 1, 2025 at 10:22 AM CEST, Anshuman Khandual wrote:
> > Makes sense. A small nit - just wondering if the following restructuring
> > here would make things bit more readable ? Regardless, your decision.
> >
> > if (handler)
> > type = handler(addr, instr, regs);
> > else
> > return 1;
>
> I went with the original formulation since -- to my mind at least -- an
> "early exit" idiom feels more appropriate for something we found we
> can't handle. Could work either way though.
What you have is fine.
Will
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2025-04-04 13:58 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-03-26 13:35 [PATCH] arm64: Don't call NULL in do_compat_alignment_fixup Angelos Oikonomopoulos
2025-03-28 19:06 ` Catalin Marinas
2025-03-31 7:57 ` Angelos Oikonomopoulos
-- strict thread matches above, loose matches on Subject: below --
2025-03-31 8:54 Angelos Oikonomopoulos
2025-04-01 6:05 ` Anshuman Khandual
2025-04-01 6:58 ` Angelos Oikonomopoulos
2025-04-01 7:47 ` Anshuman Khandual
2025-04-01 8:09 ` Angelos Oikonomopoulos
2025-04-01 8:22 ` Anshuman Khandual
2025-04-01 8:58 ` Angelos Oikonomopoulos
2025-04-04 13:58 ` Will Deacon
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox