* [PATCH] arm64: fix oops in concurrently setting insn_emulation sysctls
@ 2022-07-01 2:27 haibinzhang(张海斌)
2022-07-01 11:17 ` Will Deacon
0 siblings, 1 reply; 2+ messages in thread
From: haibinzhang(张海斌) @ 2022-07-01 2:27 UTC (permalink / raw)
To: Catalin Marinas, Will Deacon, Ard Biesheuvel, Mark Rutland,
linux-arm-kernel, linux-kernel
How to reproduce:
launch two shell executions:
#!/bin/bash
while [ 1 ];
do
echo 1 > /proc/sys/abi/swp
done
Oops info:
Unable to handle kernel NULL pointer dereference at virtual address 0000000000000010
Internal error: Oops: 96000006 [#1] SMP
Call trace:
update_insn_emulation_mode+0xc0/0x148
emulation_proc_handler+0x64/0xb8
proc_sys_call_handler+0x9c/0xf8
proc_sys_write+0x18/0x20
__vfs_write+0x20/0x48
vfs_write+0xe4/0x1d0
ksys_write+0x70/0xf8
__arm64_sys_write+0x20/0x28
el0_svc_common.constprop.0+0x7c/0x1c0
el0_svc_handler+0x2c/0xa0
el0_svc+0x8/0x200
emulation_proc_handler changes table->data for proc_dointvec_minmax
and so it isn't allowed to reenter before restoring table->data,
which isn't right now.
To fix this issue, Add mutal exclusion covering related code section.
Signed-off-by: Haibin Zhang <haibinzhang@tencent.com>
---
arch/arm64/kernel/armv8_deprecated.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/arch/arm64/kernel/armv8_deprecated.c b/arch/arm64/kernel/armv8_deprecated.c
index 6875a16..c519792 100644
--- a/arch/arm64/kernel/armv8_deprecated.c
+++ b/arch/arm64/kernel/armv8_deprecated.c
@@ -207,8 +207,12 @@ static int emulation_proc_handler(struct ctl_table *table, int write,
loff_t *ppos)
{
int ret = 0;
- struct insn_emulation *insn = (struct insn_emulation *) table->data;
- enum insn_emulation_mode prev_mode = insn->current_mode;
+ struct insn_emulation *insn;
+ enum insn_emulation_mode prev_mode;
+
+ raw_spin_lock(&insn_emulation_lock);
+ insn = (struct insn_emulation *) table->data;
+ prev_mode = insn->current_mode;
table->data = &insn->current_mode;
ret = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
@@ -224,6 +228,7 @@ static int emulation_proc_handler(struct ctl_table *table, int write,
}
ret:
table->data = insn;
+ raw_spin_unlock(&insn_emulation_lock);
return ret;
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH] arm64: fix oops in concurrently setting insn_emulation sysctls
2022-07-01 2:27 [PATCH] arm64: fix oops in concurrently setting insn_emulation sysctls haibinzhang(张海斌)
@ 2022-07-01 11:17 ` Will Deacon
0 siblings, 0 replies; 2+ messages in thread
From: Will Deacon @ 2022-07-01 11:17 UTC (permalink / raw)
To: haibinzhang(张海斌)
Cc: Catalin Marinas, Ard Biesheuvel, Mark Rutland, linux-arm-kernel,
linux-kernel
On Fri, Jul 01, 2022 at 02:27:17AM +0000, haibinzhang(张海斌) wrote:
> How to reproduce:
> launch two shell executions:
> #!/bin/bash
> while [ 1 ];
> do
> echo 1 > /proc/sys/abi/swp
> done
>
> Oops info:
> Unable to handle kernel NULL pointer dereference at virtual address 0000000000000010
> Internal error: Oops: 96000006 [#1] SMP
> Call trace:
> update_insn_emulation_mode+0xc0/0x148
> emulation_proc_handler+0x64/0xb8
> proc_sys_call_handler+0x9c/0xf8
> proc_sys_write+0x18/0x20
> __vfs_write+0x20/0x48
> vfs_write+0xe4/0x1d0
> ksys_write+0x70/0xf8
> __arm64_sys_write+0x20/0x28
> el0_svc_common.constprop.0+0x7c/0x1c0
> el0_svc_handler+0x2c/0xa0
> el0_svc+0x8/0x200
>
> emulation_proc_handler changes table->data for proc_dointvec_minmax
> and so it isn't allowed to reenter before restoring table->data,
> which isn't right now.
> To fix this issue, Add mutal exclusion covering related code section.
typo: mutual
> Signed-off-by: Haibin Zhang <haibinzhang@tencent.com>
> ---
> arch/arm64/kernel/armv8_deprecated.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/kernel/armv8_deprecated.c b/arch/arm64/kernel/armv8_deprecated.c
> index 6875a16..c519792 100644
> --- a/arch/arm64/kernel/armv8_deprecated.c
> +++ b/arch/arm64/kernel/armv8_deprecated.c
> @@ -207,8 +207,12 @@ static int emulation_proc_handler(struct ctl_table *table, int write,
> loff_t *ppos)
> {
> int ret = 0;
> - struct insn_emulation *insn = (struct insn_emulation *) table->data;
> - enum insn_emulation_mode prev_mode = insn->current_mode;
> + struct insn_emulation *insn;
> + enum insn_emulation_mode prev_mode;
> +
> + raw_spin_lock(&insn_emulation_lock);
> + insn = (struct insn_emulation *) table->data;
> + prev_mode = insn->current_mode;
>
> table->data = &insn->current_mode;
> ret = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
> @@ -224,6 +228,7 @@ static int emulation_proc_handler(struct ctl_table *table, int write,
> }
> ret:
> table->data = insn;
> + raw_spin_unlock(&insn_emulation_lock);
> return ret;
This looks very similar to the patch previously posted here:
https://lore.kernel.org/r/20220128090324.2727688-1-hewenliang4@huawei.com
but Catalin's suggestion was ignored:
https://lore.kernel.org/all/Yf0dxon1d07rzxZH@arm.com/
Please can have you send a v2 along the line that he suggested?
I also think a mutex is probably better than a spinlock given that we
can end up cross-calling in the proc handler.
Thanks,
Will
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2022-07-01 11:17 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-01 2:27 [PATCH] arm64: fix oops in concurrently setting insn_emulation sysctls haibinzhang(张海斌)
2022-07-01 11:17 ` Will Deacon
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).