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

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

[ruanjinjie]

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(-)

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)
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Greg KH]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Ruan Jinjie]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Greg KH]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel