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

[Greg KH]

On Wed, Oct 11, 2023 at 07:26:46AM +0000, Jinjie Ruan wrote:
> 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.10 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 aunch 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 910fc428b80c ("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.

You don't actually list what commit is what in the commits that you sent
out, so we have no way of matching them up properly :(

Please fix up the series and resend it.

> And for LTS 5.4 and 5.15, re-adaptation is required.

And we can't take this unless you provide a 5.15 series either as you
can't update to a newer kernel and have a regression, right?

So please resend this when you also have a series for the newer trees as
well.

thanks,

greg k-h