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: [email protected]#5.10.x
Cc: [email protected]
Signed-off-by: ruanjinjie <[email protected]>
Acked-by: Mark Rutland <[email protected]>
---
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
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