Re: [PATCH V2] arm64: fix oops in concurrently setting insn_emulation sysctls

[Lee Jones <lee.jones@linaro.org>]

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 <hewenliang4@huawei.com>
> Signed-off-by: Haibin Zhang <haibinzhang@tencent.com>
> ---
>  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