Messages in this thread | | | Subject | Re: [Patch v4 17/18] x86/speculation: Update SPEC_CTRL MSRs of remote CPUs | From | Tim Chen <> | Date | Wed, 7 Nov 2018 15:15:12 -0800 |
| |
On 11/07/2018 10:33 AM, Waiman Long wrote: > On 11/06/2018 07:18 PM, Tim Chen wrote: >> Thomas, >> >>>>> 2) Add _TIF_UPDATE_SPEC_CTRL to the SYSCALL_EXIT_WORK_FLAGS and handle it >>>>> in the slow work path. >>>> There can be tasks that don't do any syscalls, and it seems like we can >>>> have MSRs getting out of sync? >>> Setting the TIF flag directly in a remote task is wrong. It needs to be >>> handled when the _TIF_UPDATE_SPEC_CTRL is evaluated, i.e. the information >>> needs to be stored process wide e.g. in task->mm. >>> >>> But yes, if the remote task runs in user space forever, it won't >>> help. Though the point is that dumpable is usually set when the process >>> starts, so it's probably mostly a theoretical issue. >>> >> I took a crack to implement what you suggested to update >> remote task's flag and remote SPEC_CTRL MSR on the syscall exit slow path. >> >> This looks reasobale? >> >> Tim >> >> >> ------------ >> >> diff --git a/arch/x86/entry/common.c b/arch/x86/entry/common.c >> index 3b2490b..614594a 100644 >> --- a/arch/x86/entry/common.c >> +++ b/arch/x86/entry/common.c >> @@ -216,7 +216,7 @@ __visible inline void prepare_exit_to_usermode(struct pt_regs *regs) >> >> #define SYSCALL_EXIT_WORK_FLAGS \ >> (_TIF_SYSCALL_TRACE | _TIF_SYSCALL_AUDIT | \ >> - _TIF_SINGLESTEP | _TIF_SYSCALL_TRACEPOINT) >> + _TIF_SINGLESTEP | _TIF_SYSCALL_TRACEPOINT | _TIF_UPDATE_SPEC_CTRL) >> >> static void syscall_slow_exit_work(struct pt_regs *regs, u32 cached_flags) >> { >> @@ -227,6 +227,8 @@ static void syscall_slow_exit_work(struct pt_regs *regs, u32 cached_flags) >> if (cached_flags & _TIF_SYSCALL_TRACEPOINT) >> trace_sys_exit(regs, regs->ax); >> >> + if (cached_flags & _TIF_UPDATE_SPEC_CTRL) >> + spec_ctrl_do_pending_update(); >> /* >> * If TIF_SYSCALL_EMU is set, we only get here because of >> * TIF_SINGLESTEP (i.e. this is PTRACE_SYSEMU_SINGLESTEP). >> diff --git a/arch/x86/include/asm/nospec-branch.h b/arch/x86/include/asm/nospec-branch.h >> index c59a6c4..f124597 100644 >> --- a/arch/x86/include/asm/nospec-branch.h >> +++ b/arch/x86/include/asm/nospec-branch.h >> @@ -276,6 +276,8 @@ static inline void indirect_branch_prediction_barrier(void) >> alternative_msr_write(MSR_IA32_PRED_CMD, val, X86_FEATURE_USE_IBPB); >> } >> >> +void spec_ctrl_do_pending_update(void); >> + >> /* The Intel SPEC CTRL MSR base value cache */ >> extern u64 x86_spec_ctrl_base; >> >> diff --git a/arch/x86/include/asm/thread_info.h b/arch/x86/include/asm/thread_info.h >> index 4f6a7a9..b78db59 100644 >> --- a/arch/x86/include/asm/thread_info.h >> +++ b/arch/x86/include/asm/thread_info.h >> @@ -97,6 +97,7 @@ struct thread_info { >> #define TIF_USER_RETURN_NOTIFY 14 /* Notify kernel of userspace return */ >> #define TIF_PATCH_PENDING 15 /* Pending live patching update */ >> #define TIF_FSCHECK 16 /* Check FS is USER_DS on return */ >> +#define TIF_UPDATE_SPEC_CTRL 17 /* Pending update of speculation control */ >> >> /* Task status */ >> #define TIF_UPROBE 18 /* Breakpointed or singlestepping */ >> @@ -131,6 +132,7 @@ struct thread_info { >> #define _TIF_USER_RETURN_NOTIFY (1 << TIF_USER_RETURN_NOTIFY) >> #define _TIF_PATCH_PENDING (1 << TIF_PATCH_PENDING) >> #define _TIF_FSCHECK (1 << TIF_FSCHECK) >> +#define _TIF_UPDATE_SPEC_CTRL (1 << TIF_UPDATE_SPEC_CTRL) >> >> #define _TIF_UPROBE (1 << TIF_UPROBE) >> #define _TIF_MEMDIE (1 << TIF_MEMDIE) >> diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c >> index 4c15c86..d82d3f8 100644 >> --- a/arch/x86/kernel/cpu/bugs.c >> +++ b/arch/x86/kernel/cpu/bugs.c >> @@ -14,6 +14,8 @@ >> #include <linux/module.h> >> #include <linux/nospec.h> >> #include <linux/prctl.h> >> +#include <linux/sched/coredump.h> >> +#include <linux/sched/signal.h> >> >> #include <asm/spec-ctrl.h> >> #include <asm/cmdline.h> >> @@ -770,6 +772,69 @@ static int ssb_prctl_set(struct task_struct *task, unsigned long ctrl) >> return 0; >> } >> >> +static void set_task_stibp(struct task_struct *tsk, bool stibp_on) >> +{ >> + bool update = false; >> + >> + if (stibp_on) >> + update = !test_and_set_tsk_thread_flag(tsk, TIF_STIBP); >> + else >> + update = test_and_clear_tsk_thread_flag(tsk, TIF_STIBP); >> + >> + if (tsk == current && update) >> + speculation_ctrl_update_current(); >> +} >> + >> +void spec_ctrl_do_pending_update(void) >> +{ >> + if (!static_branch_unlikely(&spectre_v2_app_lite)) >> + return; >> + >> + if (!current->mm) >> + return; >> + >> + if (get_dumpable(current->mm) != SUID_DUMP_USER) >> + set_tsk_thread_flag(current, TIF_STIBP); >> + else >> + clear_tsk_thread_flag(current, TIF_STIBP); >> + >> + clear_tsk_thread_flag(current, TIF_UPDATE_SPEC_CTRL); >> + speculation_ctrl_update_current(); >> +} >> + >> +int arch_update_spec_ctrl_restriction(struct task_struct *task) >> +{ >> + unsigned long flags; >> + struct task_struct *t; >> + bool stibp_on = false; >> + >> + if (!static_branch_unlikely(&spectre_v2_app_lite)) >> + return 0; >> + >> + if (!task->mm) >> + return -EINVAL; >> + >> + if (!lock_task_sighand(task, &flags)) >> + return -ESRCH; >> + >> + if (get_dumpable(task->mm) != SUID_DUMP_USER) >> + stibp_on = true; >> + >> + for_each_thread(task, t) { >> + if (task_cpu(task) == smp_processor_id()) >> + set_task_stibp(task, stibp_on); > > I think "t" is the iterator, not "task". BTW, a thread is on the same > CPU doesn't mean it is running. Should you just check "(t == current)" here?
Ah yes, should be t. t==current is checked in set_task_stibp.
Tim
> >> + else if (test_tsk_thread_flag(task, TIF_STIBP) != stibp_on) >> + set_tsk_thread_flag(task, TIF_UPDATE_SPEC_CTRL); >> + } >> + >> + unlock_task_sighand(task, &flags); >> + return 0; >> +} >> + >> int arch_prctl_spec_ctrl_set(struct task_struct *task, unsigned long which, >> unsigned long ctrl) >> { > > Cheers, > Longman >
| |