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, keep the table->data as &insn->current_mode and
use container_of() to retrieve the insn pointer. Another mutex is
used to protect against the current_mode update but not for retrieving
insn_emulation as table->data is no longer changing.
Signed-off-by: hewenliang <[email protected]>
Signed-off-by: Haibin Zhang <[email protected]>
---
arch/arm64/kernel/armv8_deprecated.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/arch/arm64/kernel/armv8_deprecated.c b/arch/arm64/kernel/armv8_deprecated.c
index 6875a16b09d2..fb0e7c7b2e20 100644
--- a/arch/arm64/kernel/armv8_deprecated.c
+++ b/arch/arm64/kernel/armv8_deprecated.c
@@ -59,6 +59,7 @@ struct insn_emulation {
static LIST_HEAD(insn_emulation);
static int nr_insn_emulated __initdata;
static DEFINE_RAW_SPINLOCK(insn_emulation_lock);
+static DEFINE_MUTEX(insn_emulation_mutex);
static void register_emulation_hooks(struct insn_emulation_ops *ops)
{
@@ -207,10 +208,10 @@ 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;
+ struct insn_emulation *insn = container_of(table->data, struct insn_emulation, current_mode);
enum insn_emulation_mode prev_mode = insn->current_mode;
- table->data = &insn->current_mode;
+ mutex_lock(&insn_emulation_mutex);
ret = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
if (ret || !write || prev_mode == insn->current_mode)
@@ -223,7 +224,7 @@ static int emulation_proc_handler(struct ctl_table *table, int write,
update_insn_emulation_mode(insn, INSN_UNDEF);
}
ret:
- table->data = insn;
+ mutex_unlock(&insn_emulation_mutex);
return ret;
}
@@ -247,7 +248,7 @@ static void __init register_insn_emulation_sysctl(void)
sysctl->maxlen = sizeof(int);
sysctl->procname = insn->ops->name;
- sysctl->data = insn;
+ sysctl->data = &insn->current_mode;
sysctl->extra1 = &insn->min;
sysctl->extra2 = &insn->max;
sysctl->proc_handler = emulation_proc_handler;
--
2.34.1
On Sat, Jul 02, 2022 at 05:43:19AM +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, keep the table->data as &insn->current_mode and
> use container_of() to retrieve the insn pointer. Another mutex is
> used to protect against the current_mode update but not for retrieving
> insn_emulation as table->data is no longer changing.
>
> Signed-off-by: hewenliang <[email protected]>
> Signed-off-by: Haibin Zhang <[email protected]>
Reviewed-by: Catalin Marinas <[email protected]>
On Sat, 2 Jul 2022 05:43:19 +0000, haibinzhang (张海斌) wrote:
> How to reproduce:
> launch two shell executions:
> #!/bin/bash
> while [ 1 ];
> do
> echo 1 > /proc/sys/abi/swp
> done
>
> [...]
Applied to arm64 (for-next/misc), thanks!
[1/1] arm64: fix oops in concurrently setting insn_emulation sysctls
https://git.kernel.org/arm64/c/af483947d472
Cheers,
--
Will
https://fixes.arm64.dev
https://next.arm64.dev
https://will.arm64.dev
On Sat, 02 Jul 2022, 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, keep the table->data as &insn->current_mode and
> use container_of() to retrieve the insn pointer. Another mutex is
> used to protect against the current_mode update but not for retrieving
> insn_emulation as table->data is no longer changing.
Looks as though this lost its Fixes tag during the rework.
Fixes: 587064b610c7 ("arm64: Add framework for legacy instruction emulation")
Will, are you able to add this retroactively?
> Signed-off-by: hewenliang <[email protected]>
> Signed-off-by: Haibin Zhang <[email protected]>
> ---
> arch/arm64/kernel/armv8_deprecated.c | 9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm64/kernel/armv8_deprecated.c b/arch/arm64/kernel/armv8_deprecated.c
> index 6875a16b09d2..fb0e7c7b2e20 100644
> --- a/arch/arm64/kernel/armv8_deprecated.c
> +++ b/arch/arm64/kernel/armv8_deprecated.c
> @@ -59,6 +59,7 @@ struct insn_emulation {
> static LIST_HEAD(insn_emulation);
> static int nr_insn_emulated __initdata;
> static DEFINE_RAW_SPINLOCK(insn_emulation_lock);
> +static DEFINE_MUTEX(insn_emulation_mutex);
>
> static void register_emulation_hooks(struct insn_emulation_ops *ops)
> {
> @@ -207,10 +208,10 @@ 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;
> + struct insn_emulation *insn = container_of(table->data, struct insn_emulation, current_mode);
> enum insn_emulation_mode prev_mode = insn->current_mode;
>
> - table->data = &insn->current_mode;
> + mutex_lock(&insn_emulation_mutex);
> ret = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
>
> if (ret || !write || prev_mode == insn->current_mode)
> @@ -223,7 +224,7 @@ static int emulation_proc_handler(struct ctl_table *table, int write,
> update_insn_emulation_mode(insn, INSN_UNDEF);
> }
> ret:
> - table->data = insn;
> + mutex_unlock(&insn_emulation_mutex);
> return ret;
> }
>
> @@ -247,7 +248,7 @@ static void __init register_insn_emulation_sysctl(void)
> sysctl->maxlen = sizeof(int);
>
> sysctl->procname = insn->ops->name;
> - sysctl->data = insn;
> + sysctl->data = &insn->current_mode;
> sysctl->extra1 = &insn->min;
> sysctl->extra2 = &insn->max;
> sysctl->proc_handler = emulation_proc_handler;
> --
> 2.34.1
>
--
Lee Jones [李琼斯]
Principal Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
On Wed, 20 Jul 2022, Will Deacon wrote:
> On Wed, Jul 20, 2022 at 01:23:24PM +0100, Lee Jones wrote:
> > On Sat, 02 Jul 2022, 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, keep the table->data as &insn->current_mode and
> > > use container_of() to retrieve the insn pointer. Another mutex is
> > > used to protect against the current_mode update but not for retrieving
> > > insn_emulation as table->data is no longer changing.
> >
> > Looks as though this lost its Fixes tag during the rework.
> >
> > Fixes: 587064b610c7 ("arm64: Add framework for legacy instruction emulation")
> >
> > Will, are you able to add this retroactively?
>
> Sadly, this is now buried under some other patches so I'd have to rebase the
> branch if I were to add this and I don't think it's worth it just to add a
> tag.
No worries. Just thought I'd ask.
> On the plus side, the patch has a Link: tag to this thread, so the
> Fixes tag is retrievable if you're determined enough.
>
> If somebody wants this for stable, then I suppose they'll have to send
> a backport to make sure it doesn't get missed.
I'll add it to my TODO.
--
Lee Jones [李琼斯]
Principal Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
On Wed, Jul 20, 2022 at 01:23:24PM +0100, Lee Jones wrote:
> On Sat, 02 Jul 2022, 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, keep the table->data as &insn->current_mode and
> > use container_of() to retrieve the insn pointer. Another mutex is
> > used to protect against the current_mode update but not for retrieving
> > insn_emulation as table->data is no longer changing.
>
> Looks as though this lost its Fixes tag during the rework.
>
> Fixes: 587064b610c7 ("arm64: Add framework for legacy instruction emulation")
>
> Will, are you able to add this retroactively?
Sadly, this is now buried under some other patches so I'd have to rebase the
branch if I were to add this and I don't think it's worth it just to add a
tag. On the plus side, the patch has a Link: tag to this thread, so the
Fixes tag is retrievable if you're determined enough.
If somebody wants this for stable, then I suppose they'll have to send
a backport to make sure it doesn't get missed.
Will
On Wed, Jul 20, 2022 at 04:33:13PM +0100, Lee Jones wrote:
> On Wed, 20 Jul 2022, Will Deacon wrote:
> > On the plus side, the patch has a Link: tag to this thread, so the
> > Fixes tag is retrievable if you're determined enough.
> >
> > If somebody wants this for stable, then I suppose they'll have to send
> > a backport to make sure it doesn't get missed.
>
> I'll add it to my TODO.
Brill, thanks Lee.
Will