[PATCH] x86/speculation: Update TIF_SPEC_IB before ibpb barrier

[PATCH] x86/speculation: Update TIF_SPEC_IB before ibpb barrier

[Zhenzhong Duan]

When a task is set for updating TIF_SPEC_IB throuth SECCOMP by others
and it's scheduled in the first time, a stale TIF_SPEC_IB value is
picked in cond_ibpb(). This is due to TIF_SPEC_IB is updated later at
__switch_to_xtra().

Add an extra call to speculation_ctrl_update_tif() to update it before
IBPB barrier.

Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com>
---
 arch/x86/include/asm/spec-ctrl.h | 1 +
 arch/x86/kernel/process.c        | 2 +-
 arch/x86/mm/tlb.c                | 4 +++-
 3 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/spec-ctrl.h b/arch/x86/include/asm/spec-ctrl.h
index 5393bab..8b2814a 100644
--- a/arch/x86/include/asm/spec-ctrl.h
+++ b/arch/x86/include/asm/spec-ctrl.h
@@ -82,6 +82,7 @@ static inline u64 ssbd_tif_to_amd_ls_cfg(u64 tifn)
 static inline void speculative_store_bypass_ht_init(void) { }
 #endif
 
+extern unsigned long speculation_ctrl_update_tif(struct task_struct *tsk);
 extern void speculation_ctrl_update(unsigned long tif);
 extern void speculation_ctrl_update_current(void);
 
diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index 90ae0ca..454e71d 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -446,7 +446,7 @@ static __always_inline void __speculation_ctrl_update(unsigned long tifp,
 		wrmsrl(MSR_IA32_SPEC_CTRL, msr);
 }
 
-static unsigned long speculation_ctrl_update_tif(struct task_struct *tsk)
+unsigned long speculation_ctrl_update_tif(struct task_struct *tsk)
 {
 	if (test_and_clear_tsk_thread_flag(tsk, TIF_SPEC_FORCE_UPDATE)) {
 		if (task_spec_ssb_disable(tsk))
diff --git a/arch/x86/mm/tlb.c b/arch/x86/mm/tlb.c
index 999d6d8..c0f3fcf 100644
--- a/arch/x86/mm/tlb.c
+++ b/arch/x86/mm/tlb.c
@@ -8,6 +8,7 @@
 #include <linux/cpu.h>
 #include <linux/debugfs.h>
 
+#include <asm/spec-ctrl.h>
 #include <asm/tlbflush.h>
 #include <asm/mmu_context.h>
 #include <asm/nospec-branch.h>
@@ -190,7 +191,8 @@ static void sync_current_stack_to_mm(struct mm_struct *mm)
 
 static inline unsigned long mm_mangle_tif_spec_ib(struct task_struct *next)
 {
-	unsigned long next_tif = task_thread_info(next)->flags;
+	/* Update the flag bits to newest value actively */
+	unsigned long next_tif = speculation_ctrl_update_tif(next);
 	unsigned long ibpb = (next_tif >> TIF_SPEC_IB) & LAST_USER_MM_IBPB;
 
 	return (unsigned long)next->mm | ibpb;
-- 
1.8.3.1

Re: [PATCH] x86/speculation: Update TIF_SPEC_IB before ibpb barrier

[Thomas Gleixner]

On Fri, 18 Jan 2019, Zhenzhong Duan wrote:

> When a task is set for updating TIF_SPEC_IB throuth SECCOMP by others
> and it's scheduled in the first time, a stale TIF_SPEC_IB value is
> picked in cond_ibpb(). This is due to TIF_SPEC_IB is updated later at
> __switch_to_xtra().
> 
> Add an extra call to speculation_ctrl_update_tif() to update it before
> IBPB barrier.

Errm. No. It adds that call to speculation_ctrl_update_tif() for every
mm switch, most of the time for nothing.

If at all, and we discussed that before and decided not to worry about it
(because it gets fixed up on the next context switch), then you want to
handle ibpb() from there:

        if (likely(!((tifp | tifn) & _TIF_SPEC_FORCE_UPDATE))) {
                __speculation_ctrl_update(tifp, tifn);
        } else {
                speculation_ctrl_update_tif(prev_p);
                tifn = speculation_ctrl_update_tif(next_p);

                /* Enforce MSR update to ensure consistent state */
                __speculation_ctrl_update(~tifn, tifn);

------> Force update IBPB

        }

Thanks,

	tglx

Re: [PATCH] x86/speculation: Update TIF_SPEC_IB before ibpb barrier

[Thomas Gleixner]

On Wed, 23 Jan 2019, Thomas Gleixner wrote:

> On Fri, 18 Jan 2019, Zhenzhong Duan wrote:
> 
> > When a task is set for updating TIF_SPEC_IB throuth SECCOMP by others
> > and it's scheduled in the first time, a stale TIF_SPEC_IB value is
> > picked in cond_ibpb(). This is due to TIF_SPEC_IB is updated later at
> > __switch_to_xtra().
> > 
> > Add an extra call to speculation_ctrl_update_tif() to update it before
> > IBPB barrier.
> 
> Errm. No. It adds that call to speculation_ctrl_update_tif() for every
> mm switch, most of the time for nothing.
> 
> If at all, and we discussed that before and decided not to worry about it
> (because it gets fixed up on the next context switch), then you want to
> handle ibpb() from there:

Actually we need to do that. It's not only the scheduled in first
problem. That whole thing might become completely stale in either
direction. Care to whip up a patch?

Thanks,

	tglx

Re: [PATCH] x86/speculation: Update TIF_SPEC_IB before ibpb barrier

[Thomas Gleixner]

On Fri, 25 Jan 2019, Thomas Gleixner wrote:
> On Wed, 23 Jan 2019, Thomas Gleixner wrote:
> 
> > On Fri, 18 Jan 2019, Zhenzhong Duan wrote:
> > 
> > > When a task is set for updating TIF_SPEC_IB throuth SECCOMP by others
> > > and it's scheduled in the first time, a stale TIF_SPEC_IB value is
> > > picked in cond_ibpb(). This is due to TIF_SPEC_IB is updated later at
> > > __switch_to_xtra().
> > > 
> > > Add an extra call to speculation_ctrl_update_tif() to update it before
> > > IBPB barrier.
> > 
> > Errm. No. It adds that call to speculation_ctrl_update_tif() for every
> > mm switch, most of the time for nothing.
> > 
> > If at all, and we discussed that before and decided not to worry about it
> > (because it gets fixed up on the next context switch), then you want to
> > handle ibpb() from there:
> 
> Actually we need to do that. It's not only the scheduled in first
> problem. That whole thing might become completely stale in either
> direction. Care to whip up a patch?

Bah, nonsense. Brain was clearly still out for lunch and I confused IBPB
and STIBP for a moment. cond_ibpb() is the thing issues in switch_mm() and
that is not leaving a stale MSR around because we only write to it when we
need the barrier. The bit is not stale because the barrier is only issued
with the write. The bit has not to be cleared.

So the only 'issue' what happens is that switch_to() either issues a
barrier too much or misses one. That's really not a problem.

Thanks,

	tglx

Re: [PATCH] x86/speculation: Update TIF_SPEC_IB before ibpb barrier

[Zhenzhong Duan]

On 2019/1/26 2:03, Thomas Gleixner wrote:
> On Fri, 25 Jan 2019, Thomas Gleixner wrote:
>> On Wed, 23 Jan 2019, Thomas Gleixner wrote:
>>
>>> On Fri, 18 Jan 2019, Zhenzhong Duan wrote:
>>>
>>>> When a task is set for updating TIF_SPEC_IB throuth SECCOMP by others
>>>> and it's scheduled in the first time, a stale TIF_SPEC_IB value is
>>>> picked in cond_ibpb(). This is due to TIF_SPEC_IB is updated later at
>>>> __switch_to_xtra().
>>>>
>>>> Add an extra call to speculation_ctrl_update_tif() to update it before
>>>> IBPB barrier.
>>>
>>> Errm. No. It adds that call to speculation_ctrl_update_tif() for every
>>> mm switch, most of the time for nothing.
>>>
>>> If at all, and we discussed that before and decided not to worry about it
>>> (because it gets fixed up on the next context switch), then you want to
>>> handle ibpb() from there:
>>
>> Actually we need to do that. It's not only the scheduled in first
>> problem. That whole thing might become completely stale in either
>> direction. Care to whip up a patch?
> 
> Bah, nonsense. Brain was clearly still out for lunch and I confused IBPB
> and STIBP for a moment. cond_ibpb() is the thing issues in switch_mm() and
> that is not leaving a stale MSR around because we only write to it when we
> need the barrier. The bit is not stale because the barrier is only issued
> with the write. The bit has not to be cleared.
> 
> So the only 'issue' what happens is that switch_to() either issues a
> barrier too much or misses one. That's really not a problem.

Ok, yes, the purpose of this patch is to avoid the one missed barrier.
Thanks for your reply.

Zhenzhong

Re: [PATCH] x86/speculation: Update TIF_SPEC_IB before ibpb barrier

[Thomas Gleixner]

On Mon, 28 Jan 2019, Zhenzhong Duan wrote:
> On 2019/1/26 2:03, Thomas Gleixner wrote:
> > Bah, nonsense. Brain was clearly still out for lunch and I confused IBPB
> > and STIBP for a moment. cond_ibpb() is the thing issues in switch_mm() and
> > that is not leaving a stale MSR around because we only write to it when we
> > need the barrier. The bit is not stale because the barrier is only issued
> > with the write. The bit has not to be cleared.
> > 
> > So the only 'issue' what happens is that switch_to() either issues a
> > barrier too much or misses one. That's really not a problem.
> 
> Ok, yes, the purpose of this patch is to avoid the one missed barrier.

And that missed barrier is not worth it to do extra work in switch_to/mm
simply because it's a one off event and there is no way to exploit that
reliably.

Thanks,

	tglx

Re: [PATCH] x86/speculation: Update TIF_SPEC_IB before ibpb barrier

[Zhenzhong Duan]

On 2019/1/28 16:36, Thomas Gleixner wrote:
> On Mon, 28 Jan 2019, Zhenzhong Duan wrote:
>> On 2019/1/26 2:03, Thomas Gleixner wrote:
>>> Bah, nonsense. Brain was clearly still out for lunch and I confused IBPB
>>> and STIBP for a moment. cond_ibpb() is the thing issues in switch_mm() and
>>> that is not leaving a stale MSR around because we only write to it when we
>>> need the barrier. The bit is not stale because the barrier is only issued
>>> with the write. The bit has not to be cleared.
>>>
>>> So the only 'issue' what happens is that switch_to() either issues a
>>> barrier too much or misses one. That's really not a problem.
>>
>> Ok, yes, the purpose of this patch is to avoid the one missed barrier.
> 
> And that missed barrier is not worth it to do extra work in switch_to/mm
> simply because it's a one off event and there is no way to exploit that
> reliably.

Got it.

Thanks
Zhenzhong