Re: [PATCH v5.15 00/15] arm64: Fix a concurrency issue in emulation_proc_handler()

[Greg KH]

On Wed, Oct 11, 2023 at 10:06:40AM +0000, Jinjie Ruan wrote:
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
> 
> In linux-6.1, the related code is refactored in commit 124c49b1b5d9
> ("arm64: armv8_deprecated: rework deprected instruction handling") and this
> issue was incidentally fixed. This patch set try to adapt the refactoring
> patches to stable 5.15 to solve the problem of repeated addition of linked
> lists described below.
> 
> How to reproduce:
> CONFIG_ARMV8_DEPRECATED=y, CONFIG_SWP_EMULATION=y, and CONFIG_DEBUG_LIST=y,
> then launch two shell executions:
>        #!/bin/bash
>        while [ 1 ];
>        do
>            echo 1 > /proc/sys/abi/swp
>        done
> 
> or "echo 1 > /proc/sys/abi/swp" and then launch two shell executions:
>        #!/bin/bash
>        while [ 1 ];
>        do
>            echo 0 > /proc/sys/abi/swp
>        done
> 
> In emulation_proc_handler(), read and write operations are performed on
> insn->current_mode. In the concurrency scenario, mutex only protects
> writing insn->current_mode, and not protects the read. Suppose there are
> two concurrent tasks, task1 updates insn->current_mode to INSN_EMULATE
> in the critical section, the prev_mode of task2 is still the old data
> INSN_UNDEF of insn->current_mode. As a result, two tasks call
> update_insn_emulation_mode twice with prev_mode = INSN_UNDEF and
> current_mode = INSN_EMULATE, then call register_emulation_hooks twice,
> resulting in a list_add double problem.
> 
> commit 124c49b1b5d9 ("arm64: armv8_deprecated: rework deprected instruction
> handling") remove the dynamic registration and unregistration so remove the
> register_undef_hook() function, so the below problem was incidentally
> fixed.
> 
> Call trace:
>  __list_add_valid+0xd8/0xe4
>  register_undef_hook+0x94/0x13c
>  update_insn_emulation_mode+0xd0/0x12c
>  emulation_proc_handler+0xd8/0xf4
>  proc_sys_call_handler+0x140/0x250
>  proc_sys_write+0x1c/0x2c
>  new_sync_write+0xec/0x18c
>  vfs_write+0x214/0x2ac
>  ksys_write+0x70/0xfc
>  __arm64_sys_write+0x24/0x30
>  el0_svc_common.constprop.0+0x7c/0x1bc
>  do_el0_svc+0x2c/0x94
>  el0_svc+0x20/0x30
>  el0_sync_handler+0xb0/0xb4
>  el0_sync+0x160/0x180
> 
> Call trace:
>  __list_del_entry_valid+0xac/0x110
>  unregister_undef_hook+0x34/0x80
>  update_insn_emulation_mode+0xf0/0x180
>  emulation_proc_handler+0x8c/0xd8
>  proc_sys_call_handler+0x1d8/0x208
>  proc_sys_write+0x14/0x20
>  new_sync_write+0xf0/0x190
>  vfs_write+0x304/0x388
>  ksys_write+0x6c/0x100
>  __arm64_sys_write+0x1c/0x28
>  el0_svc_common.constprop.4+0x68/0x188
>  do_el0_svc+0x24/0xa0
>  el0_svc+0x14/0x20
>  el0_sync_handler+0x90/0xb8
>  el0_sync+0x160/0x180
> 
> The first 5 patches is a patch set which provides context for subsequent
> refactoring 9 patches, especially commit 0f2cb928a154 ("arm64:
> consistently pass ESR_ELx to die()") which modify do_undefinstr() to add a
> ESR_ELx value arg, and then commit 61d64a376ea8 ("arm64: split EL0/EL1
> UNDEF handlers") splits do_undefinstr() handler into separate
> do_el0_undef() and do_el1_undef() handlers.
> 
> The 9 patches after that is another refactoring patch set, which is in
> preparation for the main rework commit 124c49b1b5d9 ("arm64:
> armv8_deprecated: rework deprected instruction handling"). To remove struct
> undef_hook, commit bff8f413c71f ("arm64: factor out EL1 SSBS emulation
> hook") factor out EL1 SSBS emulation hook, which also avoid call
> call_undef_hook() in do_el1_undef(), commit f5962add74b6 ("arm64: rework
> EL0 MRS emulation") factor out EL0 MRS emulation hook, which also prepare
> for replacing call_undef_hook() in do_el0_undef(). To replace
> call_undef_hook() function, commit 61d64a376ea8 ("arm64: split EL0/EL1
> UNDEF handlers") split the do_undefinstr() into do_el0_undef() and
> do_el1_undef() functions, and commit dbfbd87efa79 ("arm64: factor insn
> read out of call_undef_hook()") factor user_insn_read() from
> call_undef_hook() so the main rework patch can replace the
> call_undef_hook() in do_el0_undef().
> 
> The last patch is a bugfix for the main rework patch.
> 
> I've tested this with userspace programs which use each of the
> deprecated instructions on Raspberry Pi 4B KVM/Qemu, and I've concurrently
> modified the support level for each of the features back-and-forth between
> HW and emulated to check that there are no oops or above repeated addition
> or deletion call trace.
> 
> Fixes: af483947d472 ("arm64: fix oops in concurrently setting insn_emulation sysctls")
> Cc: stable@xxxxxxxxxxxxxxx#5.15.x
> Cc: gregkh@xxxxxxxxxxxxxxxxxxx
> Signed-off-by: Jinjie Ruan <ruanjinjie@xxxxxxxxxx>

All now queued up, thanks.

greg k-h