All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] x86: Drop fpregs lock before inheriting FPU permissions
@ 2022-11-10 12:44 Mel Gorman
  2022-11-10 16:14 ` [tip: x86/urgent] x86/fpu: " tip-bot2 for Mel Gorman
  0 siblings, 1 reply; 2+ messages in thread
From: Mel Gorman @ 2022-11-10 12:44 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: Chang S. Bae, Borislav Petkov, Ingo Molnar, Dave Hansen,
	Mike Galbraith, LKML, Linux-X86, Linux-RT

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 <efault@gmx.de>
Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
---
 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.

^ permalink raw reply related	[flat|nested] 2+ messages in thread

* [tip: x86/urgent] x86/fpu: Drop fpregs lock before inheriting FPU permissions
  2022-11-10 12:44 [PATCH] x86: Drop fpregs lock before inheriting FPU permissions Mel Gorman
@ 2022-11-10 16:14 ` tip-bot2 for Mel Gorman
  0 siblings, 0 replies; 2+ messages in thread
From: tip-bot2 for Mel Gorman @ 2022-11-10 16:14 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: Mike Galbraith, Mel Gorman, Borislav Petkov, Thomas Gleixner,
	stable, x86, linux-kernel

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 <mgorman@techsingularity.net>
AuthorDate:    Thu, 10 Nov 2022 12:44:00 
Committer:     Borislav Petkov <bp@suse.de>
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 <efault@gmx.de>
Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
Signed-off-by: Borislav Petkov <bp@suse.de>
Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
Cc: <stable@vger.kernel.org>
Link: https://lore.kernel.org/r/20221110124400.zgymc2lnwqjukgfh@techsingularity.net
---
 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.

^ permalink raw reply related	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2022-11-10 16:14 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-10 12:44 [PATCH] x86: Drop fpregs lock before inheriting FPU permissions Mel Gorman
2022-11-10 16:14 ` [tip: x86/urgent] x86/fpu: " tip-bot2 for Mel Gorman

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.