| Message ID | 20230131065211.2826133-1-ruanjinjie@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v5.10] arm64: fix a concurrency issue in emulation_proc_handler() | expand |
On Tue, Jan 31, 2023 at 02:52:11PM +0800, ruanjinjie wrote: > This patch is addressing an issue in stable linux-5.10 only. > > In linux-6.1, the related code is refactored in 124c49b1b("arm64: > armv8_deprecated: rework deprected instruction handling") and > this issue was incidentally fixed. However, the patch changes a lot and > is not specific to this issue. Then what about 5.15.y? You can not upgrade to that kernel and have a regression, right? And nit, you need a ' ' before the '(' character. But why can we just not take the original commit that fixed this issue? That way almost always is the best (prevents regressions, makes backports easier, is actually tested, etc.) ? thanks, greg k-h
On 2023/1/31 15:27, Greg KH wrote: > On Tue, Jan 31, 2023 at 02:52:11PM +0800, ruanjinjie wrote: >> This patch is addressing an issue in stable linux-5.10 only. >> >> In linux-6.1, the related code is refactored in 124c49b1b("arm64: >> armv8_deprecated: rework deprected instruction handling") and >> this issue was incidentally fixed. However, the patch changes a lot and >> is not specific to this issue. > > Then what about 5.15.y? You can not upgrade to that kernel and have a > regression, right? This patch has a pre-dependency af483947d ("arm64: fix oops in concurrently setting insn_emulation sysctls"), which has not merged into branches except 5.10.y, so the other branches don't apply. > > And nit, you need a ' ' before the '(' character. > > But why can we just not take the original commit that fixed this issue? > That way almost always is the best (prevents regressions, makes > backports easier, is actually tested, etc.) ? Thank you! It is ok to take the original commit to fix this issue. > > thanks, > > greg k-h
On Tue, Jan 31, 2023 at 04:23:19PM +0800, Ruan Jinjie wrote: > > > On 2023/1/31 15:27, Greg KH wrote: > > On Tue, Jan 31, 2023 at 02:52:11PM +0800, ruanjinjie wrote: > >> This patch is addressing an issue in stable linux-5.10 only. > >> > >> In linux-6.1, the related code is refactored in 124c49b1b("arm64: > >> armv8_deprecated: rework deprected instruction handling") and > >> this issue was incidentally fixed. However, the patch changes a lot and > >> is not specific to this issue. > > > > Then what about 5.15.y? You can not upgrade to that kernel and have a > > regression, right? > > This patch has a pre-dependency af483947d ("arm64: fix oops in > concurrently setting insn_emulation sysctls"), which has not merged into > branches except 5.10.y, so the other branches don't apply. So there is no bug in 5.15.y? That's confusing. > > And nit, you need a ' ' before the '(' character. > > > > But why can we just not take the original commit that fixed this issue? > > That way almost always is the best (prevents regressions, makes > > backports easier, is actually tested, etc.) ? > > Thank you! It is ok to take the original commit to fix this issue. Please submit it after testing. thanks, greg k-h
diff --git a/arch/arm64/kernel/armv8_deprecated.c b/arch/arm64/kernel/armv8_deprecated.c index 91b8a8378ba3..5f8da0d7b832 100644 --- a/arch/arm64/kernel/armv8_deprecated.c +++ b/arch/arm64/kernel/armv8_deprecated.c @@ -208,10 +208,12 @@ static int emulation_proc_handler(struct ctl_table *table, int write, loff_t *ppos) { int ret = 0; - struct insn_emulation *insn = container_of(table->data, struct insn_emulation, current_mode); - enum insn_emulation_mode prev_mode = insn->current_mode; + struct insn_emulation *insn; + enum insn_emulation_mode prev_mode; mutex_lock(&insn_emulation_mutex); + insn = container_of(table->data, struct insn_emulation, current_mode); + prev_mode = insn->current_mode; ret = proc_dointvec_minmax(table, write, buffer, lenp, ppos); if (ret || !write || prev_mode == insn->current_mode)
This patch is addressing an issue in stable linux-5.10 only. In linux-6.1, the related code is refactored in 124c49b1b("arm64: armv8_deprecated: rework deprected instruction handling") and this issue was incidentally fixed. However, the patch changes a lot and is not specific to this issue. This patch is designed to solve the problem of repeated addition of linked lists described below with few changes. 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. 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 Fixes: af483947d472 ("arm64: fix oops in concurrently setting insn_emulation sysctls") Cc: stable@vger.kernel.org#5.10.x Cc: gregkh@linuxfoundation.org Signed-off-by: ruanjinjie <ruanjinjie@huawei.com> Acked-by: Mark Rutland <mark.rutland@arm.com> --- arch/arm64/kernel/armv8_deprecated.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)