Messages in this thread | | | Date | Wed, 11 Oct 2023 15:51:41 +0800 | Subject | Re: [PATCH v5.10 00/15] arm64: fix a concurrency issue in emulation_proc_handler() | From | Jinjie Ruan <> |
| |
On 2023/10/11 15:34, Greg KH wrote: > 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 :(
Thank you! I'll add the mainline commit id to every patch.
> > 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.
OK, I'll adapt them to 5.15 too and test it.
> > thanks, > > greg k-h
| |