Mike Galbraith reported the following against an old fork of preempt-rt
but the same issue also applies to the current preempt-rt tree.
BUG: sleeping function called from invalid context at kernel/locking/spinlock_rt.c:46
in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 1, name: systemd
preempt_count: 1, expected: 0
RCU nest depth: 0, expected: 0
Preemption disabled at:
fpu_clone+0xfa/0x480
CPU: 6 PID: 1 Comm: systemd Tainted: G E (unreleased)
Call Trace:
<TASK>
dump_stack_lvl+0x45/0x5b
? fpu_clone+0xfa/0x480
__might_resched+0x165/0x200
rt_spin_lock+0x2d/0x70
fpu_clone+0x32a/0x480
? copy_thread+0xef/0x270
? copy_process+0xd2c/0x1c00
? shmem_alloc_inode+0x16/0x30
? kmem_cache_alloc+0x120/0x2a0
? kernel_clone+0x9b/0x460
? __do_sys_clone+0x72/0xa0
? do_syscall_64+0x58/0x80
? __x64_sys_rt_sigprocmask+0x93/0xd0
? syscall_exit_to_user_mode+0x18/0x40
? do_syscall_64+0x67/0x80
? syscall_exit_to_user_mode+0x18/0x40
? do_syscall_64+0x67/0x80
? syscall_exit_to_user_mode+0x18/0x40
? do_syscall_64+0x67/0x80
? exc_page_fault+0x6a/0x190
? entry_SYSCALL_64_after_hwframe+0x61/0xcb
</TASK>
The splat comes from fpu_inherit_perms() being called under fpregs_lock(),
and us reaching the spin_lock_irq() therein due to fpu_state_size_dynamic()
returning true despite static key __fpu_state_size_dynamic having never
been enabled.
Mike's assessment looks correct. fpregs_lock on a PREEMPT_RT kernel disables
preemption so calling spin_lock_irq() in fpu_inherit_perms is unsafe. This
problem exists since commit 9e798e9aa14c ("x86/fpu: Prepare fpu_clone()
for dynamically enabled features"). Even though the original bug report
should not have enabled the paths at all, the bug still exists.
fpregs_lock is necessary when editing the FPU registers or a task's
FP state but it is not necessary for fpu_inherit_perms. The only write
of any FP state in fpu_inherit_perms is for the new child which is not
running yet and cannot context switch or be borrowed by a kernel thread
yet. Hence, fpregs_lock is not protecting anything in the new child until
clone() completes and can be dropped earlier. The siglock still needs to
be acquired by fpu_inherit_perms as the read of the parents permissions
has to be serialised.
Reported-by: Mike Galbraith <[email protected]>
Signed-off-by: Mel Gorman <[email protected]>
Reviewed-by: Thomas Gleixner <[email protected]>
---
arch/x86/kernel/fpu/core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
index 3b28c5b25e12..d00db56a8868 100644
--- a/arch/x86/kernel/fpu/core.c
+++ b/arch/x86/kernel/fpu/core.c
@@ -605,9 +605,9 @@ int fpu_clone(struct task_struct *dst, unsigned long clone_flags, bool minimal)
if (test_thread_flag(TIF_NEED_FPU_LOAD))
fpregs_restore_userregs();
save_fpregs_to_fpstate(dst_fpu);
+ fpregs_unlock();
if (!(clone_flags & CLONE_THREAD))
fpu_inherit_perms(dst_fpu);
- fpregs_unlock();
/*
* Children never inherit PASID state.
The following commit has been merged into the x86/urgent branch of tip:
Commit-ID: 36b038791e1e2baea892e9276588815fd14894b4
Gitweb: https://git.kernel.org/tip/36b038791e1e2baea892e9276588815fd14894b4
Author: Mel Gorman <[email protected]>
AuthorDate: Thu, 10 Nov 2022 12:44:00
Committer: Borislav Petkov <[email protected]>
CommitterDate: Thu, 10 Nov 2022 16:57:38 +01:00
x86/fpu: Drop fpregs lock before inheriting FPU permissions
Mike Galbraith reported the following against an old fork of preempt-rt
but the same issue also applies to the current preempt-rt tree.
BUG: sleeping function called from invalid context at kernel/locking/spinlock_rt.c:46
in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 1, name: systemd
preempt_count: 1, expected: 0
RCU nest depth: 0, expected: 0
Preemption disabled at:
fpu_clone
CPU: 6 PID: 1 Comm: systemd Tainted: G E (unreleased)
Call Trace:
<TASK>
dump_stack_lvl
? fpu_clone
__might_resched
rt_spin_lock
fpu_clone
? copy_thread
? copy_process
? shmem_alloc_inode
? kmem_cache_alloc
? kernel_clone
? __do_sys_clone
? do_syscall_64
? __x64_sys_rt_sigprocmask
? syscall_exit_to_user_mode
? do_syscall_64
? syscall_exit_to_user_mode
? do_syscall_64
? syscall_exit_to_user_mode
? do_syscall_64
? exc_page_fault
? entry_SYSCALL_64_after_hwframe
</TASK>
Mike says:
The splat comes from fpu_inherit_perms() being called under fpregs_lock(),
and us reaching the spin_lock_irq() therein due to fpu_state_size_dynamic()
returning true despite static key __fpu_state_size_dynamic having never
been enabled.
Mike's assessment looks correct. fpregs_lock on a PREEMPT_RT kernel disables
preemption so calling spin_lock_irq() in fpu_inherit_perms() is unsafe. This
problem exists since commit
9e798e9aa14c ("x86/fpu: Prepare fpu_clone() for dynamically enabled features").
Even though the original bug report should not have enabled the paths at
all, the bug still exists.
fpregs_lock is necessary when editing the FPU registers or a task's FP
state but it is not necessary for fpu_inherit_perms(). The only write
of any FP state in fpu_inherit_perms() is for the new child which is
not running yet and cannot context switch or be borrowed by a kernel
thread yet. Hence, fpregs_lock is not protecting anything in the new
child until clone() completes and can be dropped earlier. The siglock
still needs to be acquired by fpu_inherit_perms() as the read of the
parent's permissions has to be serialised.
[ bp: Cleanup splat. ]
Fixes: 9e798e9aa14c ("x86/fpu: Prepare fpu_clone() for dynamically enabled features")
Reported-by: Mike Galbraith <[email protected]>
Signed-off-by: Mel Gorman <[email protected]>
Signed-off-by: Borislav Petkov <[email protected]>
Reviewed-by: Thomas Gleixner <[email protected]>
Cc: <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
arch/x86/kernel/fpu/core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
index 3b28c5b..d00db56 100644
--- a/arch/x86/kernel/fpu/core.c
+++ b/arch/x86/kernel/fpu/core.c
@@ -605,9 +605,9 @@ int fpu_clone(struct task_struct *dst, unsigned long clone_flags, bool minimal)
if (test_thread_flag(TIF_NEED_FPU_LOAD))
fpregs_restore_userregs();
save_fpregs_to_fpstate(dst_fpu);
+ fpregs_unlock();
if (!(clone_flags & CLONE_THREAD))
fpu_inherit_perms(dst_fpu);
- fpregs_unlock();
/*
* Children never inherit PASID state.