* [PATCH] arm64/traps: Avoid unnecessary kernel/user pointer conversion
@ 2021-09-14 15:27 ` Amit Daniel Kachhap
0 siblings, 0 replies; 6+ messages in thread
From: Amit Daniel Kachhap @ 2021-09-14 15:27 UTC (permalink / raw)
To: linux-arm-kernel
Cc: linux-kernel, Vincenzo Frascino, Amit Daniel Kachhap,
Catalin Marinas, Will Deacon
Annotating a pointer from kernel to __user and then back again might
confuse sparse. In call_undef_hook() it can be avoided by not using the
intermediate user pointer variable.
Note: This patch adds no functional changes to code.
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Signed-off-by: Amit Daniel Kachhap <amit.kachhap@arm.com>
---
arch/arm64/kernel/traps.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
index b03e383d944a..357d10a8bbf5 100644
--- a/arch/arm64/kernel/traps.c
+++ b/arch/arm64/kernel/traps.c
@@ -404,7 +404,8 @@ static int call_undef_hook(struct pt_regs *regs)
if (!user_mode(regs)) {
__le32 instr_le;
- if (get_kernel_nofault(instr_le, (__force __le32 *)pc))
+ if (get_kernel_nofault(instr_le,
+ (__le32 *)instruction_pointer(regs)))
goto exit;
instr = le32_to_cpu(instr_le);
} else if (compat_thumb_mode(regs)) {
--
2.17.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH] arm64/traps: Avoid unnecessary kernel/user pointer conversion
@ 2021-09-14 15:27 ` Amit Daniel Kachhap
0 siblings, 0 replies; 6+ messages in thread
From: Amit Daniel Kachhap @ 2021-09-14 15:27 UTC (permalink / raw)
To: linux-arm-kernel
Cc: linux-kernel, Vincenzo Frascino, Amit Daniel Kachhap,
Catalin Marinas, Will Deacon
Annotating a pointer from kernel to __user and then back again might
confuse sparse. In call_undef_hook() it can be avoided by not using the
intermediate user pointer variable.
Note: This patch adds no functional changes to code.
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Signed-off-by: Amit Daniel Kachhap <amit.kachhap@arm.com>
---
arch/arm64/kernel/traps.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
index b03e383d944a..357d10a8bbf5 100644
--- a/arch/arm64/kernel/traps.c
+++ b/arch/arm64/kernel/traps.c
@@ -404,7 +404,8 @@ static int call_undef_hook(struct pt_regs *regs)
if (!user_mode(regs)) {
__le32 instr_le;
- if (get_kernel_nofault(instr_le, (__force __le32 *)pc))
+ if (get_kernel_nofault(instr_le,
+ (__le32 *)instruction_pointer(regs)))
goto exit;
instr = le32_to_cpu(instr_le);
} else if (compat_thumb_mode(regs)) {
--
2.17.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] arm64/traps: Avoid unnecessary kernel/user pointer conversion
2021-09-14 15:27 ` Amit Daniel Kachhap
@ 2021-09-14 16:00 ` Mark Rutland
-1 siblings, 0 replies; 6+ messages in thread
From: Mark Rutland @ 2021-09-14 16:00 UTC (permalink / raw)
To: Amit Daniel Kachhap
Cc: linux-arm-kernel, linux-kernel, Vincenzo Frascino,
Catalin Marinas, Will Deacon
On Tue, Sep 14, 2021 at 08:57:42PM +0530, Amit Daniel Kachhap wrote:
> Annotating a pointer from kernel to __user and then back again might
> confuse sparse. In call_undef_hook() it can be avoided by not using the
> intermediate user pointer variable.
When you say "might confuse sparse", does it complain today? If so, can
you include an example of what goes wrong?
> Note: This patch adds no functional changes to code.
>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Signed-off-by: Amit Daniel Kachhap <amit.kachhap@arm.com>
> ---
> arch/arm64/kernel/traps.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
> index b03e383d944a..357d10a8bbf5 100644
> --- a/arch/arm64/kernel/traps.c
> +++ b/arch/arm64/kernel/traps.c
> @@ -404,7 +404,8 @@ static int call_undef_hook(struct pt_regs *regs)
>
> if (!user_mode(regs)) {
> __le32 instr_le;
> - if (get_kernel_nofault(instr_le, (__force __le32 *)pc))
> + if (get_kernel_nofault(instr_le,
> + (__le32 *)instruction_pointer(regs)))
Can we make `pc` an unsigned long, instead?
It'd be nice to handle all three cases consistently, even if that means
adding __force to the two user cases.
Thanks,
Mark.
> goto exit;
> instr = le32_to_cpu(instr_le);
> } else if (compat_thumb_mode(regs)) {
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] arm64/traps: Avoid unnecessary kernel/user pointer conversion
@ 2021-09-14 16:00 ` Mark Rutland
0 siblings, 0 replies; 6+ messages in thread
From: Mark Rutland @ 2021-09-14 16:00 UTC (permalink / raw)
To: Amit Daniel Kachhap
Cc: linux-arm-kernel, linux-kernel, Vincenzo Frascino,
Catalin Marinas, Will Deacon
On Tue, Sep 14, 2021 at 08:57:42PM +0530, Amit Daniel Kachhap wrote:
> Annotating a pointer from kernel to __user and then back again might
> confuse sparse. In call_undef_hook() it can be avoided by not using the
> intermediate user pointer variable.
When you say "might confuse sparse", does it complain today? If so, can
you include an example of what goes wrong?
> Note: This patch adds no functional changes to code.
>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Signed-off-by: Amit Daniel Kachhap <amit.kachhap@arm.com>
> ---
> arch/arm64/kernel/traps.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
> index b03e383d944a..357d10a8bbf5 100644
> --- a/arch/arm64/kernel/traps.c
> +++ b/arch/arm64/kernel/traps.c
> @@ -404,7 +404,8 @@ static int call_undef_hook(struct pt_regs *regs)
>
> if (!user_mode(regs)) {
> __le32 instr_le;
> - if (get_kernel_nofault(instr_le, (__force __le32 *)pc))
> + if (get_kernel_nofault(instr_le,
> + (__le32 *)instruction_pointer(regs)))
Can we make `pc` an unsigned long, instead?
It'd be nice to handle all three cases consistently, even if that means
adding __force to the two user cases.
Thanks,
Mark.
> goto exit;
> instr = le32_to_cpu(instr_le);
> } else if (compat_thumb_mode(regs)) {
> --
> 2.17.1
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] arm64/traps: Avoid unnecessary kernel/user pointer conversion
2021-09-14 16:00 ` Mark Rutland
@ 2021-09-15 13:56 ` Amit Kachhap
-1 siblings, 0 replies; 6+ messages in thread
From: Amit Kachhap @ 2021-09-15 13:56 UTC (permalink / raw)
To: Mark Rutland
Cc: linux-arm-kernel, linux-kernel, Vincenzo Frascino,
Catalin Marinas, Will Deacon
Hi,
On 9/14/21 9:30 PM, Mark Rutland wrote:
> On Tue, Sep 14, 2021 at 08:57:42PM +0530, Amit Daniel Kachhap wrote:
>> Annotating a pointer from kernel to __user and then back again might
>> confuse sparse. In call_undef_hook() it can be avoided by not using the
>> intermediate user pointer variable.
>
> When you say "might confuse sparse", does it complain today? If so, can
> you include an example of what goes wrong?
No it does not give warning. The __force option silences the warning. My
idea is to remove the unwanted __force annotations and not mix user and
kernel pointers.
>
>> Note: This patch adds no functional changes to code.
>>
>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>> Cc: Will Deacon <will@kernel.org>
>> Signed-off-by: Amit Daniel Kachhap <amit.kachhap@arm.com>
>> ---
>> arch/arm64/kernel/traps.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
>> index b03e383d944a..357d10a8bbf5 100644
>> --- a/arch/arm64/kernel/traps.c
>> +++ b/arch/arm64/kernel/traps.c
>> @@ -404,7 +404,8 @@ static int call_undef_hook(struct pt_regs *regs)
>>
>> if (!user_mode(regs)) {
>> __le32 instr_le;
>> - if (get_kernel_nofault(instr_le, (__force __le32 *)pc))
>> + if (get_kernel_nofault(instr_le,
>> + (__le32 *)instruction_pointer(regs)))
>
> Can we make `pc` an unsigned long, instead?
I think it can be done.
>
> It'd be nice to handle all three cases consistently, even if that means
> adding __force to the two user cases.
Agree with your suggestion. Even in the 2 user cases, __force may not be
needed as the typecast will be from from unsigned long to user pointer.
BR,
Amit
>
> Thanks,
> Mark.
>
>> goto exit;
>> instr = le32_to_cpu(instr_le);
>> } else if (compat_thumb_mode(regs)) {
>> --
>> 2.17.1
>>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] arm64/traps: Avoid unnecessary kernel/user pointer conversion
@ 2021-09-15 13:56 ` Amit Kachhap
0 siblings, 0 replies; 6+ messages in thread
From: Amit Kachhap @ 2021-09-15 13:56 UTC (permalink / raw)
To: Mark Rutland
Cc: linux-arm-kernel, linux-kernel, Vincenzo Frascino,
Catalin Marinas, Will Deacon
Hi,
On 9/14/21 9:30 PM, Mark Rutland wrote:
> On Tue, Sep 14, 2021 at 08:57:42PM +0530, Amit Daniel Kachhap wrote:
>> Annotating a pointer from kernel to __user and then back again might
>> confuse sparse. In call_undef_hook() it can be avoided by not using the
>> intermediate user pointer variable.
>
> When you say "might confuse sparse", does it complain today? If so, can
> you include an example of what goes wrong?
No it does not give warning. The __force option silences the warning. My
idea is to remove the unwanted __force annotations and not mix user and
kernel pointers.
>
>> Note: This patch adds no functional changes to code.
>>
>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>> Cc: Will Deacon <will@kernel.org>
>> Signed-off-by: Amit Daniel Kachhap <amit.kachhap@arm.com>
>> ---
>> arch/arm64/kernel/traps.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
>> index b03e383d944a..357d10a8bbf5 100644
>> --- a/arch/arm64/kernel/traps.c
>> +++ b/arch/arm64/kernel/traps.c
>> @@ -404,7 +404,8 @@ static int call_undef_hook(struct pt_regs *regs)
>>
>> if (!user_mode(regs)) {
>> __le32 instr_le;
>> - if (get_kernel_nofault(instr_le, (__force __le32 *)pc))
>> + if (get_kernel_nofault(instr_le,
>> + (__le32 *)instruction_pointer(regs)))
>
> Can we make `pc` an unsigned long, instead?
I think it can be done.
>
> It'd be nice to handle all three cases consistently, even if that means
> adding __force to the two user cases.
Agree with your suggestion. Even in the 2 user cases, __force may not be
needed as the typecast will be from from unsigned long to user pointer.
BR,
Amit
>
> Thanks,
> Mark.
>
>> goto exit;
>> instr = le32_to_cpu(instr_le);
>> } else if (compat_thumb_mode(regs)) {
>> --
>> 2.17.1
>>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-09-15 13:59 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-14 15:27 [PATCH] arm64/traps: Avoid unnecessary kernel/user pointer conversion Amit Daniel Kachhap
2021-09-14 15:27 ` Amit Daniel Kachhap
2021-09-14 16:00 ` Mark Rutland
2021-09-14 16:00 ` Mark Rutland
2021-09-15 13:56 ` Amit Kachhap
2021-09-15 13:56 ` Amit Kachhap
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.