Re: [tip:x86/pti] x86/speculation: Use IBRS if available before calling into firmware

[Peter Zijlstra]

On Mon, Feb 19, 2018 at 09:29:12AM +, David Woodhouse wrote:
> On Mon, 2018-02-19 at 10:20 +0100, Peter Zijlstra wrote:
> > 
> > I did not update or otherwise change packages while I was bisecting; the
> > machine is:
> > 
> > vendor_id   : GenuineIntel
> > cpu family  : 6
> > model   : 62
> > model name  : Intel(R) Xeon(R) CPU E5-2680 v2 @ 2.80GHz
> > stepping    : 4
> > microcode   : 0x428
> 
> That's IVX with a microcode that doesn't *have* IBRS/IBPB. I don't
> think there's a publicly available microcode that does; I assume you
> didn't have one and build it into your kernel for early loading, and
> thus you really weren't even using IBRS here? The code never even gets
> patched in?

I wasn't using IRBS afaik. I was bisceting tip/master before the
IBRS/firmware patches (not sure they're in now or not).

No fancy ucode setup, I think I have ucode image in the initrd, but that
would be same for all kernels.

Re: [tip:x86/pti] x86/speculation: Use IBRS if available before calling into firmware

[Peter Zijlstra]

On Mon, Feb 19, 2018 at 09:29:12AM +, David Woodhouse wrote:
> On Mon, 2018-02-19 at 10:20 +0100, Peter Zijlstra wrote:
> > 
> > I did not update or otherwise change packages while I was bisecting; the
> > machine is:
> > 
> > vendor_id   : GenuineIntel
> > cpu family  : 6
> > model   : 62
> > model name  : Intel(R) Xeon(R) CPU E5-2680 v2 @ 2.80GHz
> > stepping    : 4
> > microcode   : 0x428
> 
> That's IVX with a microcode that doesn't *have* IBRS/IBPB. I don't
> think there's a publicly available microcode that does; I assume you
> didn't have one and build it into your kernel for early loading, and
> thus you really weren't even using IBRS here? The code never even gets
> patched in?

I wasn't using IRBS afaik. I was bisceting tip/master before the
IBRS/firmware patches (not sure they're in now or not).

No fancy ucode setup, I think I have ucode image in the initrd, but that
would be same for all kernels.

Re: [tip:x86/pti] x86/speculation: Use IBRS if available before calling into firmware

[David Woodhouse]

On Mon, 2018-02-19 at 10:39 +0100, Ingo Molnar wrote:
> * David Woodhouse  wrote:
> 
> > 
> > On Mon, 2018-02-19 at 10:20 +0100, Peter Zijlstra wrote:
> > > 
> > > 
> > > I did not update or otherwise change packages while I was bisecting; the
> > > machine is:
> > > 
> > > vendor_id   : GenuineIntel
> > > cpu family  : 6
> > > model   : 62
> > > model name  : Intel(R) Xeon(R) CPU E5-2680 v2 @ 2.80GHz
> > > stepping    : 4
> > > microcode   : 0x428
> > That's IVX with a microcode that doesn't *have* IBRS/IBPB. I don't
> > think there's a publicly available microcode that does; I assume you
> > didn't have one and build it into your kernel for early loading, and
> > thus you really weren't even using IBRS here? The code never even gets
> > patched in?
> Note that PeterZ's boot troubles only match the *symptoms* of the spurious 
> failures reported by Tim Chen. Your commit wasn't bisected to.
> 
> I linked these two reports on the (remote) possibility that they might be 
> related 
> via some alignment dependent bug somewhere else in the x86 kernel - possibly 
> completely unrelated to any IBRS/IBPB details.

Understood. I was merely *accentuating* the "completely unrelated to
any IBRS/IBPB" bit, in a "la la la I'm ignoring this because I'm
working on other things" kind of a way... :)


smime.p7s
Description: S/MIME cryptographic signature

Re: [tip:x86/pti] x86/speculation: Use IBRS if available before calling into firmware

[David Woodhouse]

On Mon, 2018-02-19 at 10:39 +0100, Ingo Molnar wrote:
> * David Woodhouse  wrote:
> 
> > 
> > On Mon, 2018-02-19 at 10:20 +0100, Peter Zijlstra wrote:
> > > 
> > > 
> > > I did not update or otherwise change packages while I was bisecting; the
> > > machine is:
> > > 
> > > vendor_id   : GenuineIntel
> > > cpu family  : 6
> > > model   : 62
> > > model name  : Intel(R) Xeon(R) CPU E5-2680 v2 @ 2.80GHz
> > > stepping    : 4
> > > microcode   : 0x428
> > That's IVX with a microcode that doesn't *have* IBRS/IBPB. I don't
> > think there's a publicly available microcode that does; I assume you
> > didn't have one and build it into your kernel for early loading, and
> > thus you really weren't even using IBRS here? The code never even gets
> > patched in?
> Note that PeterZ's boot troubles only match the *symptoms* of the spurious 
> failures reported by Tim Chen. Your commit wasn't bisected to.
> 
> I linked these two reports on the (remote) possibility that they might be 
> related 
> via some alignment dependent bug somewhere else in the x86 kernel - possibly 
> completely unrelated to any IBRS/IBPB details.

Understood. I was merely *accentuating* the "completely unrelated to
any IBRS/IBPB" bit, in a "la la la I'm ignoring this because I'm
working on other things" kind of a way... :)


smime.p7s
Description: S/MIME cryptographic signature

Re: [tip:x86/pti] x86/speculation: Use IBRS if available before calling into firmware

[Ingo Molnar]

* David Woodhouse  wrote:

> On Mon, 2018-02-19 at 10:20 +0100, Peter Zijlstra wrote:
> > 
> > I did not update or otherwise change packages while I was bisecting; the
> > machine is:
> > 
> > vendor_id   : GenuineIntel
> > cpu family  : 6
> > model   : 62
> > model name  : Intel(R) Xeon(R) CPU E5-2680 v2 @ 2.80GHz
> > stepping    : 4
> > microcode   : 0x428
> 
> That's IVX with a microcode that doesn't *have* IBRS/IBPB. I don't
> think there's a publicly available microcode that does; I assume you
> didn't have one and build it into your kernel for early loading, and
> thus you really weren't even using IBRS here? The code never even gets
> patched in?

Note that PeterZ's boot troubles only match the *symptoms* of the spurious 
failures reported by Tim Chen. Your commit wasn't bisected to.

I linked these two reports on the (remote) possibility that they might be 
related 
via some alignment dependent bug somewhere else in the x86 kernel - possibly 
completely unrelated to any IBRS/IBPB details.

Thanks,

Ingo

Re: [tip:x86/pti] x86/speculation: Use IBRS if available before calling into firmware

[Ingo Molnar]

* David Woodhouse  wrote:

> On Mon, 2018-02-19 at 10:20 +0100, Peter Zijlstra wrote:
> > 
> > I did not update or otherwise change packages while I was bisecting; the
> > machine is:
> > 
> > vendor_id   : GenuineIntel
> > cpu family  : 6
> > model   : 62
> > model name  : Intel(R) Xeon(R) CPU E5-2680 v2 @ 2.80GHz
> > stepping    : 4
> > microcode   : 0x428
> 
> That's IVX with a microcode that doesn't *have* IBRS/IBPB. I don't
> think there's a publicly available microcode that does; I assume you
> didn't have one and build it into your kernel for early loading, and
> thus you really weren't even using IBRS here? The code never even gets
> patched in?

Note that PeterZ's boot troubles only match the *symptoms* of the spurious 
failures reported by Tim Chen. Your commit wasn't bisected to.

I linked these two reports on the (remote) possibility that they might be 
related 
via some alignment dependent bug somewhere else in the x86 kernel - possibly 
completely unrelated to any IBRS/IBPB details.

Thanks,

Ingo

Re: [tip:x86/pti] x86/speculation: Use IBRS if available before calling into firmware

[Ingo Molnar]

* Peter Zijlstra  wrote:

> On Sat, Feb 17, 2018 at 11:26:16AM +0100, Ingo Molnar wrote:
> > Note that PeterZ was struggling with intermittent boot hangs yesterday as 
> > well, 
> > which hangs came and went during severeal (fruitless) bisection attempts. 
> > Then at 
> > a certain point the hangs went away altogether.
> > 
> > The symptoms for both his and your hangs are consistent with an alignment 
> > dependent bug.
> 
> Mine would consistently hang right after
> 
>   "Freeing SMP alternatives memory: 44K"
> 
> At one point I bisected it to commit:
> 
>   a06cc94f3f8d ("x86/build: Drop superfluous ALIGN from the linker script")

So, just to make sure this commit had no effect: I cannot really see anything 
wrong with that commit, it does a single substantial change, which is to remove 
this explicit alignment:

. = ALIGN(8);
TEXT_TEXT

which seems fine to me, since it expanded to:

. = ALIGN(8);
ALIGN_FUNCTION();   \
...

which expanded to:

. = ALIGN(8);
. = ALIGN(8);
...

... which duplication the commit removed.

... where all the relevant defitions of TEXT_TEXT and ALIGN_FUNCTION are 
unconditional and not overriden anywhere for arch/x86 builds.

I.e. the commit is a NOP AFAICS.

Thanks,

Ingo

Re: [tip:x86/pti] x86/speculation: Use IBRS if available before calling into firmware

[Ingo Molnar]

* Peter Zijlstra  wrote:

> On Sat, Feb 17, 2018 at 11:26:16AM +0100, Ingo Molnar wrote:
> > Note that PeterZ was struggling with intermittent boot hangs yesterday as 
> > well, 
> > which hangs came and went during severeal (fruitless) bisection attempts. 
> > Then at 
> > a certain point the hangs went away altogether.
> > 
> > The symptoms for both his and your hangs are consistent with an alignment 
> > dependent bug.
> 
> Mine would consistently hang right after
> 
>   "Freeing SMP alternatives memory: 44K"
> 
> At one point I bisected it to commit:
> 
>   a06cc94f3f8d ("x86/build: Drop superfluous ALIGN from the linker script")

So, just to make sure this commit had no effect: I cannot really see anything 
wrong with that commit, it does a single substantial change, which is to remove 
this explicit alignment:

. = ALIGN(8);
TEXT_TEXT

which seems fine to me, since it expanded to:

. = ALIGN(8);
ALIGN_FUNCTION();   \
...

which expanded to:

. = ALIGN(8);
. = ALIGN(8);
...

... which duplication the commit removed.

... where all the relevant defitions of TEXT_TEXT and ALIGN_FUNCTION are 
unconditional and not overriden anywhere for arch/x86 builds.

I.e. the commit is a NOP AFAICS.

Thanks,

Ingo

Re: [tip:x86/pti] x86/speculation: Use IBRS if available before calling into firmware

[David Woodhouse]

On Mon, 2018-02-19 at 10:20 +0100, Peter Zijlstra wrote:
> 
> I did not update or otherwise change packages while I was bisecting; the
> machine is:
> 
> vendor_id   : GenuineIntel
> cpu family  : 6
> model   : 62
> model name  : Intel(R) Xeon(R) CPU E5-2680 v2 @ 2.80GHz
> stepping    : 4
> microcode   : 0x428

That's IVX with a microcode that doesn't *have* IBRS/IBPB. I don't
think there's a publicly available microcode that does; I assume you
didn't have one and build it into your kernel for early loading, and
thus you really weren't even using IBRS here? The code never even gets
patched in?

smime.p7s
Description: S/MIME cryptographic signature

Re: [tip:x86/pti] x86/speculation: Use IBRS if available before calling into firmware

[David Woodhouse]

On Mon, 2018-02-19 at 10:20 +0100, Peter Zijlstra wrote:
> 
> I did not update or otherwise change packages while I was bisecting; the
> machine is:
> 
> vendor_id   : GenuineIntel
> cpu family  : 6
> model   : 62
> model name  : Intel(R) Xeon(R) CPU E5-2680 v2 @ 2.80GHz
> stepping    : 4
> microcode   : 0x428

That's IVX with a microcode that doesn't *have* IBRS/IBPB. I don't
think there's a publicly available microcode that does; I assume you
didn't have one and build it into your kernel for early loading, and
thus you really weren't even using IBRS here? The code never even gets
patched in?

smime.p7s
Description: S/MIME cryptographic signature

Re: [tip:x86/pti] x86/speculation: Use IBRS if available before calling into firmware

[Peter Zijlstra]

On Sat, Feb 17, 2018 at 11:26:16AM +0100, Ingo Molnar wrote:
> Note that PeterZ was struggling with intermittent boot hangs yesterday as 
> well, 
> which hangs came and went during severeal (fruitless) bisection attempts. 
> Then at 
> a certain point the hangs went away altogether.
> 
> The symptoms for both his and your hangs are consistent with an alignment 
> dependent bug.

Mine would consistently hang right after

  "Freeing SMP alternatives memory: 44K"

At one point I bisected it to commit:

  a06cc94f3f8d ("x86/build: Drop superfluous ALIGN from the linker script")

But shortly thereafter I started having trouble reproducing, and now I
can run kernels that before would consistently fail to boot :/

> My other guess is that it's perhaps somehow microcode related?

I did not update or otherwise change packages while I was bisecting; the
machine is:

vendor_id   : GenuineIntel
cpu family  : 6
model   : 62
model name  : Intel(R) Xeon(R) CPU E5-2680 v2 @ 2.80GHz
stepping: 4
microcode   : 0x428

Like I wrote on IRC; what _seems_ to have 'cured' things is clearing out
my /boot. The amount of kernels generated by the bisect was immense and
running 'update-grub' was taking _much_ longer than actually building a
new kernel.

What I have not tried is again generating and installing that many
kernels to see if that will make it go 'funny' again.

Re: [tip:x86/pti] x86/speculation: Use IBRS if available before calling into firmware

[Peter Zijlstra]

On Sat, Feb 17, 2018 at 11:26:16AM +0100, Ingo Molnar wrote:
> Note that PeterZ was struggling with intermittent boot hangs yesterday as 
> well, 
> which hangs came and went during severeal (fruitless) bisection attempts. 
> Then at 
> a certain point the hangs went away altogether.
> 
> The symptoms for both his and your hangs are consistent with an alignment 
> dependent bug.

Mine would consistently hang right after

  "Freeing SMP alternatives memory: 44K"

At one point I bisected it to commit:

  a06cc94f3f8d ("x86/build: Drop superfluous ALIGN from the linker script")

But shortly thereafter I started having trouble reproducing, and now I
can run kernels that before would consistently fail to boot :/

> My other guess is that it's perhaps somehow microcode related?

I did not update or otherwise change packages while I was bisecting; the
machine is:

vendor_id   : GenuineIntel
cpu family  : 6
model   : 62
model name  : Intel(R) Xeon(R) CPU E5-2680 v2 @ 2.80GHz
stepping: 4
microcode   : 0x428

Like I wrote on IRC; what _seems_ to have 'cured' things is clearing out
my /boot. The amount of kernels generated by the bisect was immense and
running 'update-grub' was taking _much_ longer than actually building a
new kernel.

What I have not tried is again generating and installing that many
kernels to see if that will make it go 'funny' again.

Re: [tip:x86/pti] x86/speculation: Use IBRS if available before calling into firmware

[Ingo Molnar]

* Tim Chen  wrote:

> On 02/16/2018 11:16 AM, David Woodhouse wrote:
> > On Fri, 2018-02-16 at 10:44 -0800, Tim Chen wrote:
> >>
> >> I encountered hang on a machine but not others when using the above
> >> macro.  It is probably an alignment thing with ALTERNATIVE as the
> >> problem went
> >> away after I made the change below:
> >>
> >> Tim
> >>
> >> diff --git a/arch/x86/include/asm/nospec-branch.h
> >> b/arch/x86/include/asm/nospec-branch.h
> >> index 8f2ff74..0f65bd2 100644
> >> --- a/arch/x86/include/asm/nospec-branch.h
> >> +++ b/arch/x86/include/asm/nospec-branch.h
> >> @@ -148,6 +148,7 @@ extern char __indirect_thunk_end[];
> >>  
> >>  #define alternative_msr_write(_msr, _val, _feature)\
> >> asm volatile(ALTERNATIVE("",\
> >> +   ".align 16\n\t"\
> >> "movl %[msr], %%ecx\n\t"   \
> >> "movl %[val], %%eax\n\t"   \
> >> "movl $0, %%edx\n\t"   \
> > 
> > That's weird. Note that .align in an altinstr section isn't actually
> > going to do what you'd expect; the oldinstr and altinstr sections
> > aren't necessarily aligned the same, so however many NOPs it inserts
> > into the alternative, might be deliberately *misaligning* it in the
> > code that actually gets executed.
> > 
> > Are you sure you're not running a kernel where the alternatives code
> > would turn that alternative which *starts* with a NOP, into *all* NOPs?
> > 
> 
> I rebuild the kernel again without the align. I'm no longer
> seeing the issue again on that machine that had an issue earlier.  
> So let's ignore this for now as I can't reproduce the problem.
> 
> It should be other issues causing the hang I saw earlier.

Note that PeterZ was struggling with intermittent boot hangs yesterday as well, 
which hangs came and went during severeal (fruitless) bisection attempts. Then 
at 
a certain point the hangs went away altogether.

The symptoms for both his and your hangs are consistent with an alignment 
dependent bug.

My other guess is that it's perhaps somehow microcode related?

I'm not seeing any hangs myself, on various test systems.

Thanks,

Ingo

Re: [tip:x86/pti] x86/speculation: Use IBRS if available before calling into firmware

[Ingo Molnar]

* Tim Chen  wrote:

> On 02/16/2018 11:16 AM, David Woodhouse wrote:
> > On Fri, 2018-02-16 at 10:44 -0800, Tim Chen wrote:
> >>
> >> I encountered hang on a machine but not others when using the above
> >> macro.  It is probably an alignment thing with ALTERNATIVE as the
> >> problem went
> >> away after I made the change below:
> >>
> >> Tim
> >>
> >> diff --git a/arch/x86/include/asm/nospec-branch.h
> >> b/arch/x86/include/asm/nospec-branch.h
> >> index 8f2ff74..0f65bd2 100644
> >> --- a/arch/x86/include/asm/nospec-branch.h
> >> +++ b/arch/x86/include/asm/nospec-branch.h
> >> @@ -148,6 +148,7 @@ extern char __indirect_thunk_end[];
> >>  
> >>  #define alternative_msr_write(_msr, _val, _feature)\
> >> asm volatile(ALTERNATIVE("",\
> >> +   ".align 16\n\t"\
> >> "movl %[msr], %%ecx\n\t"   \
> >> "movl %[val], %%eax\n\t"   \
> >> "movl $0, %%edx\n\t"   \
> > 
> > That's weird. Note that .align in an altinstr section isn't actually
> > going to do what you'd expect; the oldinstr and altinstr sections
> > aren't necessarily aligned the same, so however many NOPs it inserts
> > into the alternative, might be deliberately *misaligning* it in the
> > code that actually gets executed.
> > 
> > Are you sure you're not running a kernel where the alternatives code
> > would turn that alternative which *starts* with a NOP, into *all* NOPs?
> > 
> 
> I rebuild the kernel again without the align. I'm no longer
> seeing the issue again on that machine that had an issue earlier.  
> So let's ignore this for now as I can't reproduce the problem.
> 
> It should be other issues causing the hang I saw earlier.

Note that PeterZ was struggling with intermittent boot hangs yesterday as well, 
which hangs came and went during severeal (fruitless) bisection attempts. Then 
at 
a certain point the hangs went away altogether.

The symptoms for both his and your hangs are consistent with an alignment 
dependent bug.

My other guess is that it's perhaps somehow microcode related?

I'm not seeing any hangs myself, on various test systems.

Thanks,

Ingo

Re: [tip:x86/pti] x86/speculation: Use IBRS if available before calling into firmware

[Tim Chen]

On 02/16/2018 11:16 AM, David Woodhouse wrote:
> On Fri, 2018-02-16 at 10:44 -0800, Tim Chen wrote:
>>
>> I encountered hang on a machine but not others when using the above
>> macro.  It is probably an alignment thing with ALTERNATIVE as the
>> problem went
>> away after I made the change below:
>>
>> Tim
>>
>> diff --git a/arch/x86/include/asm/nospec-branch.h
>> b/arch/x86/include/asm/nospec-branch.h
>> index 8f2ff74..0f65bd2 100644
>> --- a/arch/x86/include/asm/nospec-branch.h
>> +++ b/arch/x86/include/asm/nospec-branch.h
>> @@ -148,6 +148,7 @@ extern char __indirect_thunk_end[];
>>  
>>  #define alternative_msr_write(_msr, _val, _feature)\
>> asm volatile(ALTERNATIVE("",\
>> +   ".align 16\n\t"\
>> "movl %[msr], %%ecx\n\t"   \
>> "movl %[val], %%eax\n\t"   \
>> "movl $0, %%edx\n\t"   \
> 
> That's weird. Note that .align in an altinstr section isn't actually
> going to do what you'd expect; the oldinstr and altinstr sections
> aren't necessarily aligned the same, so however many NOPs it inserts
> into the alternative, might be deliberately *misaligning* it in the
> code that actually gets executed.
> 
> Are you sure you're not running a kernel where the alternatives code
> would turn that alternative which *starts* with a NOP, into *all* NOPs?
> 

I rebuild the kernel again without the align. I'm no longer
seeing the issue again on that machine that had an issue earlier.  
So let's ignore this for now as I can't reproduce the problem.

It should be other issues causing the hang I saw earlier.

Thanks.

Tim

Re: [tip:x86/pti] x86/speculation: Use IBRS if available before calling into firmware

[Tim Chen]

On 02/16/2018 11:16 AM, David Woodhouse wrote:
> On Fri, 2018-02-16 at 10:44 -0800, Tim Chen wrote:
>>
>> I encountered hang on a machine but not others when using the above
>> macro.  It is probably an alignment thing with ALTERNATIVE as the
>> problem went
>> away after I made the change below:
>>
>> Tim
>>
>> diff --git a/arch/x86/include/asm/nospec-branch.h
>> b/arch/x86/include/asm/nospec-branch.h
>> index 8f2ff74..0f65bd2 100644
>> --- a/arch/x86/include/asm/nospec-branch.h
>> +++ b/arch/x86/include/asm/nospec-branch.h
>> @@ -148,6 +148,7 @@ extern char __indirect_thunk_end[];
>>  
>>  #define alternative_msr_write(_msr, _val, _feature)\
>> asm volatile(ALTERNATIVE("",\
>> +   ".align 16\n\t"\
>> "movl %[msr], %%ecx\n\t"   \
>> "movl %[val], %%eax\n\t"   \
>> "movl $0, %%edx\n\t"   \
> 
> That's weird. Note that .align in an altinstr section isn't actually
> going to do what you'd expect; the oldinstr and altinstr sections
> aren't necessarily aligned the same, so however many NOPs it inserts
> into the alternative, might be deliberately *misaligning* it in the
> code that actually gets executed.
> 
> Are you sure you're not running a kernel where the alternatives code
> would turn that alternative which *starts* with a NOP, into *all* NOPs?
> 

I rebuild the kernel again without the align. I'm no longer
seeing the issue again on that machine that had an issue earlier.  
So let's ignore this for now as I can't reproduce the problem.

It should be other issues causing the hang I saw earlier.

Thanks.

Tim

Re: [tip:x86/pti] x86/speculation: Use IBRS if available before calling into firmware

[David Woodhouse]

On Fri, 2018-02-16 at 10:44 -0800, Tim Chen wrote:
> 
> I encountered hang on a machine but not others when using the above
> macro.  It is probably an alignment thing with ALTERNATIVE as the
> problem went
> away after I made the change below:
> 
> Tim
> 
> diff --git a/arch/x86/include/asm/nospec-branch.h
> b/arch/x86/include/asm/nospec-branch.h
> index 8f2ff74..0f65bd2 100644
> --- a/arch/x86/include/asm/nospec-branch.h
> +++ b/arch/x86/include/asm/nospec-branch.h
> @@ -148,6 +148,7 @@ extern char __indirect_thunk_end[];
>  
>  #define alternative_msr_write(_msr, _val, _feature)    \
>     asm volatile(ALTERNATIVE("",    \
> +   ".align 16\n\t"    \
>     "movl %[msr], %%ecx\n\t"   \
>     "movl %[val], %%eax\n\t"   \
>     "movl $0, %%edx\n\t"   \

That's weird. Note that .align in an altinstr section isn't actually
going to do what you'd expect; the oldinstr and altinstr sections
aren't necessarily aligned the same, so however many NOPs it inserts
into the alternative, might be deliberately *misaligning* it in the
code that actually gets executed.

Are you sure you're not running a kernel where the alternatives code
would turn that alternative which *starts* with a NOP, into *all* NOPs?

smime.p7s
Description: S/MIME cryptographic signature

Re: [tip:x86/pti] x86/speculation: Use IBRS if available before calling into firmware

[David Woodhouse]

On Fri, 2018-02-16 at 10:44 -0800, Tim Chen wrote:
> 
> I encountered hang on a machine but not others when using the above
> macro.  It is probably an alignment thing with ALTERNATIVE as the
> problem went
> away after I made the change below:
> 
> Tim
> 
> diff --git a/arch/x86/include/asm/nospec-branch.h
> b/arch/x86/include/asm/nospec-branch.h
> index 8f2ff74..0f65bd2 100644
> --- a/arch/x86/include/asm/nospec-branch.h
> +++ b/arch/x86/include/asm/nospec-branch.h
> @@ -148,6 +148,7 @@ extern char __indirect_thunk_end[];
>  
>  #define alternative_msr_write(_msr, _val, _feature)    \
>     asm volatile(ALTERNATIVE("",    \
> +   ".align 16\n\t"    \
>     "movl %[msr], %%ecx\n\t"   \
>     "movl %[val], %%eax\n\t"   \
>     "movl $0, %%edx\n\t"   \

That's weird. Note that .align in an altinstr section isn't actually
going to do what you'd expect; the oldinstr and altinstr sections
aren't necessarily aligned the same, so however many NOPs it inserts
into the alternative, might be deliberately *misaligning* it in the
code that actually gets executed.

Are you sure you're not running a kernel where the alternatives code
would turn that alternative which *starts* with a NOP, into *all* NOPs?

smime.p7s
Description: S/MIME cryptographic signature

Re: [tip:x86/pti] x86/speculation: Use IBRS if available before calling into firmware

[Tim Chen]

On 02/11/2018 11:19 AM, tip-bot for David Woodhouse wrote:
\
>  })
> diff --git a/arch/x86/include/asm/nospec-branch.h 
> b/arch/x86/include/asm/nospec-branch.h
> index 300cc15..788c4da 100644
> --- a/arch/x86/include/asm/nospec-branch.h
> +++ b/arch/x86/include/asm/nospec-branch.h
> @@ -162,17 +162,36 @@ static inline void vmexit_fill_RSB(void)
>  #endif
>  }
>  
> +#define alternative_msr_write(_msr, _val, _feature)  \
> + asm volatile(ALTERNATIVE("",\
> +  "movl %[msr], %%ecx\n\t"   \
> +  "movl %[val], %%eax\n\t"   \
> +  "movl $0, %%edx\n\t"   \
> +  "wrmsr",   \
> +  _feature)  \
> +  : : [msr] "i" (_msr), [val] "i" (_val) \
> +  : "eax", "ecx", "edx", "memory")
> +

I encountered hang on a machine but not others when using the above
macro.  It is probably an alignment thing with ALTERNATIVE as the problem went
away after I made the change below:

Tim

diff --git a/arch/x86/include/asm/nospec-branch.h 
b/arch/x86/include/asm/nospec-branch.h
index 8f2ff74..0f65bd2 100644
--- a/arch/x86/include/asm/nospec-branch.h
+++ b/arch/x86/include/asm/nospec-branch.h
@@ -148,6 +148,7 @@ extern char __indirect_thunk_end[];
 
 #define alternative_msr_write(_msr, _val, _feature)\
asm volatile(ALTERNATIVE("",\
+   ".align 16\n\t"\
"movl %[msr], %%ecx\n\t"   \
"movl %[val], %%eax\n\t"   \
"movl $0, %%edx\n\t"   \

Re: [tip:x86/pti] x86/speculation: Use IBRS if available before calling into firmware

[Tim Chen]

On 02/11/2018 11:19 AM, tip-bot for David Woodhouse wrote:
\
>  })
> diff --git a/arch/x86/include/asm/nospec-branch.h 
> b/arch/x86/include/asm/nospec-branch.h
> index 300cc15..788c4da 100644
> --- a/arch/x86/include/asm/nospec-branch.h
> +++ b/arch/x86/include/asm/nospec-branch.h
> @@ -162,17 +162,36 @@ static inline void vmexit_fill_RSB(void)
>  #endif
>  }
>  
> +#define alternative_msr_write(_msr, _val, _feature)  \
> + asm volatile(ALTERNATIVE("",\
> +  "movl %[msr], %%ecx\n\t"   \
> +  "movl %[val], %%eax\n\t"   \
> +  "movl $0, %%edx\n\t"   \
> +  "wrmsr",   \
> +  _feature)  \
> +  : : [msr] "i" (_msr), [val] "i" (_val) \
> +  : "eax", "ecx", "edx", "memory")
> +

I encountered hang on a machine but not others when using the above
macro.  It is probably an alignment thing with ALTERNATIVE as the problem went
away after I made the change below:

Tim

diff --git a/arch/x86/include/asm/nospec-branch.h 
b/arch/x86/include/asm/nospec-branch.h
index 8f2ff74..0f65bd2 100644
--- a/arch/x86/include/asm/nospec-branch.h
+++ b/arch/x86/include/asm/nospec-branch.h
@@ -148,6 +148,7 @@ extern char __indirect_thunk_end[];
 
 #define alternative_msr_write(_msr, _val, _feature)\
asm volatile(ALTERNATIVE("",\
+   ".align 16\n\t"\
"movl %[msr], %%ecx\n\t"   \
"movl %[val], %%eax\n\t"   \
"movl $0, %%edx\n\t"   \

Re: [tip:x86/pti] x86/speculation: Use IBRS if available before calling into firmware

[Tim Chen]

On 02/14/2018 03:19 PM, Ingo Molnar wrote:
> 
> * Tim Chen  wrote:
> 
>> On 02/14/2018 12:56 AM, Peter Zijlstra wrote:
>>
>>>
>>> At the very least this must disable and re-enable preemption, such that
>>> we guarantee we inc/dec the same counter. ISTR some firmware calls (EFI)
>>> actually are preemptible so that wouldn't work.
>>>
>>> Further, consider:
>>>
>>> this_cpu_inc_return()   // 0->1
>>> 
>>> this_cpu_inc_return()   // 1->2
>>> call_broken_arse_firmware()
>>> this_cpu_dec_return()   // 2->1
>>> 
>>> wrmsr(SPEC_CTRL, IBRS);
>>>
>>> /* from dodgy firmware crap */
>>>
>>> this_cpu_dec_return()   // 1->0
>>> wrmsr(SPEC_CTRL, 0);
>>>
>>
>> How about the following patch.
> 
> These fragile complications of the interface should now be unnecessary, as 
> the 
> only driver that called firmware from NMI callbacks (hpwdt.c) is going to 
> remove 
> those firmware callbacks in the near future - solving the problem at the 
> source.
> 
> Thanks,
> 
>   Ingo
> 

Sounds good. I sent this out before seeing the other mails on removing NMI 
callbacks
from hpwdt.c

Tim

Re: [tip:x86/pti] x86/speculation: Use IBRS if available before calling into firmware

[Tim Chen]

On 02/14/2018 03:19 PM, Ingo Molnar wrote:
> 
> * Tim Chen  wrote:
> 
>> On 02/14/2018 12:56 AM, Peter Zijlstra wrote:
>>
>>>
>>> At the very least this must disable and re-enable preemption, such that
>>> we guarantee we inc/dec the same counter. ISTR some firmware calls (EFI)
>>> actually are preemptible so that wouldn't work.
>>>
>>> Further, consider:
>>>
>>> this_cpu_inc_return()   // 0->1
>>> 
>>> this_cpu_inc_return()   // 1->2
>>> call_broken_arse_firmware()
>>> this_cpu_dec_return()   // 2->1
>>> 
>>> wrmsr(SPEC_CTRL, IBRS);
>>>
>>> /* from dodgy firmware crap */
>>>
>>> this_cpu_dec_return()   // 1->0
>>> wrmsr(SPEC_CTRL, 0);
>>>
>>
>> How about the following patch.
> 
> These fragile complications of the interface should now be unnecessary, as 
> the 
> only driver that called firmware from NMI callbacks (hpwdt.c) is going to 
> remove 
> those firmware callbacks in the near future - solving the problem at the 
> source.
> 
> Thanks,
> 
>   Ingo
> 

Sounds good. I sent this out before seeing the other mails on removing NMI 
callbacks
from hpwdt.c

Tim

Re: [tip:x86/pti] x86/speculation: Use IBRS if available before calling into firmware

[Ingo Molnar]

* Tim Chen  wrote:

> On 02/14/2018 12:56 AM, Peter Zijlstra wrote:
> 
> > 
> > At the very least this must disable and re-enable preemption, such that
> > we guarantee we inc/dec the same counter. ISTR some firmware calls (EFI)
> > actually are preemptible so that wouldn't work.
> > 
> > Further, consider:
> > 
> > this_cpu_inc_return()   // 0->1
> > 
> > this_cpu_inc_return()   // 1->2
> > call_broken_arse_firmware()
> > this_cpu_dec_return()   // 2->1
> > 
> > wrmsr(SPEC_CTRL, IBRS);
> > 
> > /* from dodgy firmware crap */
> > 
> > this_cpu_dec_return()   // 1->0
> > wrmsr(SPEC_CTRL, 0);
> > 
> 
> How about the following patch.

These fragile complications of the interface should now be unnecessary, as the 
only driver that called firmware from NMI callbacks (hpwdt.c) is going to 
remove 
those firmware callbacks in the near future - solving the problem at the source.

Thanks,

Ingo

Re: [tip:x86/pti] x86/speculation: Use IBRS if available before calling into firmware

[Ingo Molnar]

* Tim Chen  wrote:

> On 02/14/2018 12:56 AM, Peter Zijlstra wrote:
> 
> > 
> > At the very least this must disable and re-enable preemption, such that
> > we guarantee we inc/dec the same counter. ISTR some firmware calls (EFI)
> > actually are preemptible so that wouldn't work.
> > 
> > Further, consider:
> > 
> > this_cpu_inc_return()   // 0->1
> > 
> > this_cpu_inc_return()   // 1->2
> > call_broken_arse_firmware()
> > this_cpu_dec_return()   // 2->1
> > 
> > wrmsr(SPEC_CTRL, IBRS);
> > 
> > /* from dodgy firmware crap */
> > 
> > this_cpu_dec_return()   // 1->0
> > wrmsr(SPEC_CTRL, 0);
> > 
> 
> How about the following patch.

These fragile complications of the interface should now be unnecessary, as the 
only driver that called firmware from NMI callbacks (hpwdt.c) is going to 
remove 
those firmware callbacks in the near future - solving the problem at the source.

Thanks,

Ingo

Re: [tip:x86/pti] x86/speculation: Use IBRS if available before calling into firmware

[Tim Chen]

On 02/14/2018 12:56 AM, Peter Zijlstra wrote:

> 
> At the very least this must disable and re-enable preemption, such that
> we guarantee we inc/dec the same counter. ISTR some firmware calls (EFI)
> actually are preemptible so that wouldn't work.
> 
> Further, consider:
> 
>   this_cpu_inc_return()   // 0->1
>   
>   this_cpu_inc_return()   // 1->2
>   call_broken_arse_firmware()
>   this_cpu_dec_return()   // 2->1
>   
>   wrmsr(SPEC_CTRL, IBRS);
> 
>   /* from dodgy firmware crap */
> 
>   this_cpu_dec_return()   // 1->0
>   wrmsr(SPEC_CTRL, 0);
> 

How about the following patch.

Thanks.

Tim

---
>From a37d28622781acf2789dd63f2fdb57be733f15a4 Mon Sep 17 00:00:00 2001
From: Tim Chen 
Date: Tue, 13 Feb 2018 04:10:41 -0800
Subject: [PATCH] x86/firmware: Prevent IBRS from being turned off prematurely.

Dave Woodhoue proposed using IBRS to protect the firmware
call path against Spectre exploit.  However, firmware path
can go through NMI and we can get nested calls, causing
unsafe firmware calls with missing IBRS as illustrated below:

firmware_restrict_branch_speculation_start (set IBRS=1)
NMI
firmware_restrict_branch_speculation_start (set IBRS=1)
firmware call
firmware_restrict_branch_speculation_end (set IBRS=0)
NMI return
firmware call (with IBRS=0)  < unsafe call, premature IBRS disabling
firmware_restrict_branch_speculation_end (set IBRS=0)

This patch proposes using a per cpu counter to track the IBRS
firmware call nesting depth, to ensure that we don't turn off
IBRS prematurely before calling firmware.

Signed-off-by: Tim Chen 
---
 arch/x86/include/asm/nospec-branch.h | 10 --
 arch/x86/kernel/cpu/bugs.c   |  2 ++
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/nospec-branch.h 
b/arch/x86/include/asm/nospec-branch.h
index 297d457..a8dd9ea 100644
--- a/arch/x86/include/asm/nospec-branch.h
+++ b/arch/x86/include/asm/nospec-branch.h
@@ -146,6 +146,8 @@ enum spectre_v2_mitigation {
 extern char __indirect_thunk_start[];
 extern char __indirect_thunk_end[];
 
+DECLARE_PER_CPU(int, spec_ctrl_ibrs_fw_depth);
+
 /*
  * On VMEXIT we must ensure that no RSB predictions learned in the guest
  * can be followed in the host, by overwriting the RSB completely. Both
@@ -186,14 +188,18 @@ static inline void 
indirect_branch_prediction_barrier(void)
  */
 static inline void firmware_restrict_branch_speculation_start(void)
 {
+   preempt_disable();
+   this_cpu_inc(spec_ctrl_ibrs_fw_depth);
alternative_msr_write(MSR_IA32_SPEC_CTRL, SPEC_CTRL_IBRS,
  X86_FEATURE_USE_IBRS_FW);
 }
 
 static inline void firmware_restrict_branch_speculation_end(void)
 {
-   alternative_msr_write(MSR_IA32_SPEC_CTRL, 0,
- X86_FEATURE_USE_IBRS_FW);
+   if (this_cpu_dec_return(spec_ctrl_ibrs_fw_depth) == 0)
+   alternative_msr_write(MSR_IA32_SPEC_CTRL, 0,
+ X86_FEATURE_USE_IBRS_FW);
+   preempt_enable();
 }
 #endif /* __ASSEMBLY__ */
 #endif /* _ASM_X86_NOSPEC_BRANCH_H_ */
diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index c994dab..4ab13f0 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -27,6 +27,8 @@
 #include 
 
 static void __init spectre_v2_select_mitigation(void);
+DEFINE_PER_CPU(int, spec_ctrl_ibrs_fw_depth);
+EXPORT_PER_CPU_SYMBOL(spec_ctrl_ibrs_fw_depth);
 
 void __init check_bugs(void)
 {
-- 
2.7.4

Re: [tip:x86/pti] x86/speculation: Use IBRS if available before calling into firmware

[Tim Chen]

On 02/14/2018 12:56 AM, Peter Zijlstra wrote:

> 
> At the very least this must disable and re-enable preemption, such that
> we guarantee we inc/dec the same counter. ISTR some firmware calls (EFI)
> actually are preemptible so that wouldn't work.
> 
> Further, consider:
> 
>   this_cpu_inc_return()   // 0->1
>   
>   this_cpu_inc_return()   // 1->2
>   call_broken_arse_firmware()
>   this_cpu_dec_return()   // 2->1
>   
>   wrmsr(SPEC_CTRL, IBRS);
> 
>   /* from dodgy firmware crap */
> 
>   this_cpu_dec_return()   // 1->0
>   wrmsr(SPEC_CTRL, 0);
> 

How about the following patch.

Thanks.

Tim

---
>From a37d28622781acf2789dd63f2fdb57be733f15a4 Mon Sep 17 00:00:00 2001
From: Tim Chen 
Date: Tue, 13 Feb 2018 04:10:41 -0800
Subject: [PATCH] x86/firmware: Prevent IBRS from being turned off prematurely.

Dave Woodhoue proposed using IBRS to protect the firmware
call path against Spectre exploit.  However, firmware path
can go through NMI and we can get nested calls, causing
unsafe firmware calls with missing IBRS as illustrated below:

firmware_restrict_branch_speculation_start (set IBRS=1)
NMI
firmware_restrict_branch_speculation_start (set IBRS=1)
firmware call
firmware_restrict_branch_speculation_end (set IBRS=0)
NMI return
firmware call (with IBRS=0)  < unsafe call, premature IBRS disabling
firmware_restrict_branch_speculation_end (set IBRS=0)

This patch proposes using a per cpu counter to track the IBRS
firmware call nesting depth, to ensure that we don't turn off
IBRS prematurely before calling firmware.

Signed-off-by: Tim Chen 
---
 arch/x86/include/asm/nospec-branch.h | 10 --
 arch/x86/kernel/cpu/bugs.c   |  2 ++
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/nospec-branch.h 
b/arch/x86/include/asm/nospec-branch.h
index 297d457..a8dd9ea 100644
--- a/arch/x86/include/asm/nospec-branch.h
+++ b/arch/x86/include/asm/nospec-branch.h
@@ -146,6 +146,8 @@ enum spectre_v2_mitigation {
 extern char __indirect_thunk_start[];
 extern char __indirect_thunk_end[];
 
+DECLARE_PER_CPU(int, spec_ctrl_ibrs_fw_depth);
+
 /*
  * On VMEXIT we must ensure that no RSB predictions learned in the guest
  * can be followed in the host, by overwriting the RSB completely. Both
@@ -186,14 +188,18 @@ static inline void 
indirect_branch_prediction_barrier(void)
  */
 static inline void firmware_restrict_branch_speculation_start(void)
 {
+   preempt_disable();
+   this_cpu_inc(spec_ctrl_ibrs_fw_depth);
alternative_msr_write(MSR_IA32_SPEC_CTRL, SPEC_CTRL_IBRS,
  X86_FEATURE_USE_IBRS_FW);
 }
 
 static inline void firmware_restrict_branch_speculation_end(void)
 {
-   alternative_msr_write(MSR_IA32_SPEC_CTRL, 0,
- X86_FEATURE_USE_IBRS_FW);
+   if (this_cpu_dec_return(spec_ctrl_ibrs_fw_depth) == 0)
+   alternative_msr_write(MSR_IA32_SPEC_CTRL, 0,
+ X86_FEATURE_USE_IBRS_FW);
+   preempt_enable();
 }
 #endif /* __ASSEMBLY__ */
 #endif /* _ASM_X86_NOSPEC_BRANCH_H_ */
diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index c994dab..4ab13f0 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -27,6 +27,8 @@
 #include 
 
 static void __init spectre_v2_select_mitigation(void);
+DEFINE_PER_CPU(int, spec_ctrl_ibrs_fw_depth);
+EXPORT_PER_CPU_SYMBOL(spec_ctrl_ibrs_fw_depth);
 
 void __init check_bugs(void)
 {
-- 
2.7.4

Re: [tip:x86/pti] x86/speculation: Use IBRS if available before calling into firmware

[Peter Zijlstra]

On Wed, Feb 14, 2018 at 09:56:14AM +0100, Peter Zijlstra wrote:
> On Tue, Feb 13, 2018 at 05:49:47PM -0800, Tim Chen wrote:
> 
> >  static inline void firmware_restrict_branch_speculation_start(void)
> >  {
> > +   if (this_cpu_inc_return(spec_ctrl_ibrs_fw_depth) == 1)
> > +   alternative_msr_write(MSR_IA32_SPEC_CTRL, SPEC_CTRL_IBRS,
> >   X86_FEATURE_USE_IBRS_FW);
> >  }
> >  
> >  static inline void firmware_restrict_branch_speculation_end(void)
> >  {
> > +   if (this_cpu_dec_return(spec_ctrl_ibrs_fw_depth) == 0)
> > +   alternative_msr_write(MSR_IA32_SPEC_CTRL, 0,
> > + X86_FEATURE_USE_IBRS_FW);
> >  }
> 
> 
> At the very least this must disable and re-enable preemption, such that
> we guarantee we inc/dec the same counter. ISTR some firmware calls (EFI)
> actually are preemptible so that wouldn't work.
> 
> Further, consider:
> 
>   this_cpu_inc_return()   // 0->1
>   
>   this_cpu_inc_return()   // 1->2
>   call_broken_arse_firmware()
>   this_cpu_dec_return()   // 2->1
>   
>   wrmsr(SPEC_CTRL, IBRS);
> 
>   /* from dodgy firmware crap */

s/from/more/

typing hard.

>   this_cpu_dec_return()   // 1->0
>   wrmsr(SPEC_CTRL, 0);

Re: [tip:x86/pti] x86/speculation: Use IBRS if available before calling into firmware

[Peter Zijlstra]

On Wed, Feb 14, 2018 at 09:56:14AM +0100, Peter Zijlstra wrote:
> On Tue, Feb 13, 2018 at 05:49:47PM -0800, Tim Chen wrote:
> 
> >  static inline void firmware_restrict_branch_speculation_start(void)
> >  {
> > +   if (this_cpu_inc_return(spec_ctrl_ibrs_fw_depth) == 1)
> > +   alternative_msr_write(MSR_IA32_SPEC_CTRL, SPEC_CTRL_IBRS,
> >   X86_FEATURE_USE_IBRS_FW);
> >  }
> >  
> >  static inline void firmware_restrict_branch_speculation_end(void)
> >  {
> > +   if (this_cpu_dec_return(spec_ctrl_ibrs_fw_depth) == 0)
> > +   alternative_msr_write(MSR_IA32_SPEC_CTRL, 0,
> > + X86_FEATURE_USE_IBRS_FW);
> >  }
> 
> 
> At the very least this must disable and re-enable preemption, such that
> we guarantee we inc/dec the same counter. ISTR some firmware calls (EFI)
> actually are preemptible so that wouldn't work.
> 
> Further, consider:
> 
>   this_cpu_inc_return()   // 0->1
>   
>   this_cpu_inc_return()   // 1->2
>   call_broken_arse_firmware()
>   this_cpu_dec_return()   // 2->1
>   
>   wrmsr(SPEC_CTRL, IBRS);
> 
>   /* from dodgy firmware crap */

s/from/more/

typing hard.

>   this_cpu_dec_return()   // 1->0
>   wrmsr(SPEC_CTRL, 0);

Re: [tip:x86/pti] x86/speculation: Use IBRS if available before calling into firmware

[Peter Zijlstra]

On Tue, Feb 13, 2018 at 05:49:47PM -0800, Tim Chen wrote:

>  static inline void firmware_restrict_branch_speculation_start(void)
>  {
> + if (this_cpu_inc_return(spec_ctrl_ibrs_fw_depth) == 1)
> + alternative_msr_write(MSR_IA32_SPEC_CTRL, SPEC_CTRL_IBRS,
> X86_FEATURE_USE_IBRS_FW);
>  }
>  
>  static inline void firmware_restrict_branch_speculation_end(void)
>  {
> + if (this_cpu_dec_return(spec_ctrl_ibrs_fw_depth) == 0)
> + alternative_msr_write(MSR_IA32_SPEC_CTRL, 0,
> +   X86_FEATURE_USE_IBRS_FW);
>  }


At the very least this must disable and re-enable preemption, such that
we guarantee we inc/dec the same counter. ISTR some firmware calls (EFI)
actually are preemptible so that wouldn't work.

Further, consider:

this_cpu_inc_return()   // 0->1

this_cpu_inc_return()   // 1->2
call_broken_arse_firmware()
this_cpu_dec_return()   // 2->1

wrmsr(SPEC_CTRL, IBRS);

/* from dodgy firmware crap */

this_cpu_dec_return()   // 1->0
wrmsr(SPEC_CTRL, 0);

Re: [tip:x86/pti] x86/speculation: Use IBRS if available before calling into firmware

[Peter Zijlstra]

On Tue, Feb 13, 2018 at 05:49:47PM -0800, Tim Chen wrote:

>  static inline void firmware_restrict_branch_speculation_start(void)
>  {
> + if (this_cpu_inc_return(spec_ctrl_ibrs_fw_depth) == 1)
> + alternative_msr_write(MSR_IA32_SPEC_CTRL, SPEC_CTRL_IBRS,
> X86_FEATURE_USE_IBRS_FW);
>  }
>  
>  static inline void firmware_restrict_branch_speculation_end(void)
>  {
> + if (this_cpu_dec_return(spec_ctrl_ibrs_fw_depth) == 0)
> + alternative_msr_write(MSR_IA32_SPEC_CTRL, 0,
> +   X86_FEATURE_USE_IBRS_FW);
>  }


At the very least this must disable and re-enable preemption, such that
we guarantee we inc/dec the same counter. ISTR some firmware calls (EFI)
actually are preemptible so that wouldn't work.

Further, consider:

this_cpu_inc_return()   // 0->1

this_cpu_inc_return()   // 1->2
call_broken_arse_firmware()
this_cpu_dec_return()   // 2->1

wrmsr(SPEC_CTRL, IBRS);

/* from dodgy firmware crap */

this_cpu_dec_return()   // 1->0
wrmsr(SPEC_CTRL, 0);

Re: [tip:x86/pti] x86/speculation: Use IBRS if available before calling into firmware

[Tim Chen]

On 02/12/2018 11:55 PM, Ingo Molnar wrote:
> 
> * Peter Zijlstra  wrote:
> 
>> On Mon, Feb 12, 2018 at 08:13:31AM -0800, Dave Hansen wrote:
>>> On 02/12/2018 02:22 AM, Ingo Molnar wrote:
> +static inline void firmware_restrict_branch_speculation_end(void)
> +{
> + alternative_msr_write(MSR_IA32_SPEC_CTRL, 0,
> +   X86_FEATURE_USE_IBRS_FW);
 BTW., there's a detail that only occurred to me today, this 
 enabling/disabling 
 sequence is not NMI safe, and it might be called from NMI context:
>>>
>>> FWIW, Tim Chen and I talked about this a bunch.  We ended up just
>>> saving/restoring the MSR verbatim in the NMI handler the same way we do
>>> CR3, stashing it in a high general-purpose-register (r%12?).  That costs
>>> a RDMSR (at least) and an WRMSR (which you can optimize out).  We have a
>>> patch for that somewhere if anybody wants it.
>>
>> I would really rather not do that on the NMI path.. And if we _have_ to,
>> please keep a software shadow of that MSR state, such that we can avoid
>> touching that MSR 99% of the time.
> 
> Yeah, I'd rather avoid doing firmware calls from NMI context altogether.
> 
> Thanks,
> 
>   Ingo
> 

Dave Hansen and I had some discussions on how to handle the nested NMI
and firmware calls.  We thought of using 
a per cpu counter to record the nesting call depth
and toggle IBRS appropriately for the depth 0->1 and 1->0
transition.  Will this change be sufficient?

Thanks.

Tim

commit 55546c27a006198630c57b900abcbd3baaabf63a
Author: Tim Chen 
Date:   Tue Feb 13 04:10:41 2018 -0800

x86/firmware: Prevent IBRS from being turned off prematurely.

Dave Woodhoue proposed to use IBRS to protect the firmware
call path against Spectre exploit.  However, firmware path
can go through NMI and we can get nested calls, causing
unsafe firmware calls with missing IBRS as illustrated below:

firmware_restrict_branch_speculation_start (set IBRS=1)
NMI
firmware_restrict_branch_speculation_start (set IBRS=1)
firmware call
firmware_restrict_branch_speculation_end (set IBRS=0)
NMI return
firmware call (with IBRS=0)  < unsafe call, premature IBRS disabling
firmware_restrict_branch_speculation_end (set IBRS=0)

This patch proposes using a per cpu counter to track the IBRS
firmware call nesting depth, to ensure that we don't turn off
IBRS prematurely before calling firmware.

Signed-off-by: Tim Chen 
---
diff --git a/arch/x86/include/asm/nospec-branch.h 
b/arch/x86/include/asm/nospec-branch.h
index 297d457..1e9c828 100644
--- a/arch/x86/include/asm/nospec-branch.h
+++ b/arch/x86/include/asm/nospec-branch.h
@@ -146,6 +146,8 @@ enum spectre_v2_mitigation {
 extern char __indirect_thunk_start[];
 extern char __indirect_thunk_end[];
 
+DECLARE_PER_CPU(int, spec_ctrl_ibrs_fw_depth);
+
 /*
  * On VMEXIT we must ensure that no RSB predictions learned in the guest
  * can be followed in the host, by overwriting the RSB completely. Both
@@ -186,14 +188,16 @@ static inline void 
indirect_branch_prediction_barrier(void)
  */
 static inline void firmware_restrict_branch_speculation_start(void)
 {
-   alternative_msr_write(MSR_IA32_SPEC_CTRL, SPEC_CTRL_IBRS,
+   if (this_cpu_inc_return(spec_ctrl_ibrs_fw_depth) == 1)
+   alternative_msr_write(MSR_IA32_SPEC_CTRL, SPEC_CTRL_IBRS,
  X86_FEATURE_USE_IBRS_FW);
 }
 
 static inline void firmware_restrict_branch_speculation_end(void)
 {
-   alternative_msr_write(MSR_IA32_SPEC_CTRL, 0,
- X86_FEATURE_USE_IBRS_FW);
+   if (this_cpu_dec_return(spec_ctrl_ibrs_fw_depth) == 0)
+   alternative_msr_write(MSR_IA32_SPEC_CTRL, 0,
+ X86_FEATURE_USE_IBRS_FW);
 }
 #endif /* __ASSEMBLY__ */
 #endif /* _ASM_X86_NOSPEC_BRANCH_H_ */
diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index c994dab..4ab13f0 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -27,6 +27,8 @@
 #include 
 
 static void __init spectre_v2_select_mitigation(void);
+DEFINE_PER_CPU(int, spec_ctrl_ibrs_fw_depth);
+EXPORT_PER_CPU_SYMBOL(spec_ctrl_ibrs_fw_depth);
 
 void __init check_bugs(void)
 {

Re: [tip:x86/pti] x86/speculation: Use IBRS if available before calling into firmware

[Tim Chen]

On 02/12/2018 11:55 PM, Ingo Molnar wrote:
> 
> * Peter Zijlstra  wrote:
> 
>> On Mon, Feb 12, 2018 at 08:13:31AM -0800, Dave Hansen wrote:
>>> On 02/12/2018 02:22 AM, Ingo Molnar wrote:
> +static inline void firmware_restrict_branch_speculation_end(void)
> +{
> + alternative_msr_write(MSR_IA32_SPEC_CTRL, 0,
> +   X86_FEATURE_USE_IBRS_FW);
 BTW., there's a detail that only occurred to me today, this 
 enabling/disabling 
 sequence is not NMI safe, and it might be called from NMI context:
>>>
>>> FWIW, Tim Chen and I talked about this a bunch.  We ended up just
>>> saving/restoring the MSR verbatim in the NMI handler the same way we do
>>> CR3, stashing it in a high general-purpose-register (r%12?).  That costs
>>> a RDMSR (at least) and an WRMSR (which you can optimize out).  We have a
>>> patch for that somewhere if anybody wants it.
>>
>> I would really rather not do that on the NMI path.. And if we _have_ to,
>> please keep a software shadow of that MSR state, such that we can avoid
>> touching that MSR 99% of the time.
> 
> Yeah, I'd rather avoid doing firmware calls from NMI context altogether.
> 
> Thanks,
> 
>   Ingo
> 

Dave Hansen and I had some discussions on how to handle the nested NMI
and firmware calls.  We thought of using 
a per cpu counter to record the nesting call depth
and toggle IBRS appropriately for the depth 0->1 and 1->0
transition.  Will this change be sufficient?

Thanks.

Tim

commit 55546c27a006198630c57b900abcbd3baaabf63a
Author: Tim Chen 
Date:   Tue Feb 13 04:10:41 2018 -0800

x86/firmware: Prevent IBRS from being turned off prematurely.

Dave Woodhoue proposed to use IBRS to protect the firmware
call path against Spectre exploit.  However, firmware path
can go through NMI and we can get nested calls, causing
unsafe firmware calls with missing IBRS as illustrated below:

firmware_restrict_branch_speculation_start (set IBRS=1)
NMI
firmware_restrict_branch_speculation_start (set IBRS=1)
firmware call
firmware_restrict_branch_speculation_end (set IBRS=0)
NMI return
firmware call (with IBRS=0)  < unsafe call, premature IBRS disabling
firmware_restrict_branch_speculation_end (set IBRS=0)

This patch proposes using a per cpu counter to track the IBRS
firmware call nesting depth, to ensure that we don't turn off
IBRS prematurely before calling firmware.

Signed-off-by: Tim Chen 
---
diff --git a/arch/x86/include/asm/nospec-branch.h 
b/arch/x86/include/asm/nospec-branch.h
index 297d457..1e9c828 100644
--- a/arch/x86/include/asm/nospec-branch.h
+++ b/arch/x86/include/asm/nospec-branch.h
@@ -146,6 +146,8 @@ enum spectre_v2_mitigation {
 extern char __indirect_thunk_start[];
 extern char __indirect_thunk_end[];
 
+DECLARE_PER_CPU(int, spec_ctrl_ibrs_fw_depth);
+
 /*
  * On VMEXIT we must ensure that no RSB predictions learned in the guest
  * can be followed in the host, by overwriting the RSB completely. Both
@@ -186,14 +188,16 @@ static inline void 
indirect_branch_prediction_barrier(void)
  */
 static inline void firmware_restrict_branch_speculation_start(void)
 {
-   alternative_msr_write(MSR_IA32_SPEC_CTRL, SPEC_CTRL_IBRS,
+   if (this_cpu_inc_return(spec_ctrl_ibrs_fw_depth) == 1)
+   alternative_msr_write(MSR_IA32_SPEC_CTRL, SPEC_CTRL_IBRS,
  X86_FEATURE_USE_IBRS_FW);
 }
 
 static inline void firmware_restrict_branch_speculation_end(void)
 {
-   alternative_msr_write(MSR_IA32_SPEC_CTRL, 0,
- X86_FEATURE_USE_IBRS_FW);
+   if (this_cpu_dec_return(spec_ctrl_ibrs_fw_depth) == 0)
+   alternative_msr_write(MSR_IA32_SPEC_CTRL, 0,
+ X86_FEATURE_USE_IBRS_FW);
 }
 #endif /* __ASSEMBLY__ */
 #endif /* _ASM_X86_NOSPEC_BRANCH_H_ */
diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index c994dab..4ab13f0 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -27,6 +27,8 @@
 #include 
 
 static void __init spectre_v2_select_mitigation(void);
+DEFINE_PER_CPU(int, spec_ctrl_ibrs_fw_depth);
+EXPORT_PER_CPU_SYMBOL(spec_ctrl_ibrs_fw_depth);
 
 void __init check_bugs(void)
 {

Re: [tip:x86/pti] x86/speculation: Use IBRS if available before calling into firmware

[Ingo Molnar]

* David Woodhouse  wrote:

> On Mon, 2018-02-12 at 12:50 +0100, Peter Zijlstra wrote:
> > On Mon, Feb 12, 2018 at 11:22:11AM +0100, Ingo Molnar wrote:
> > > > +static inline void firmware_restrict_branch_speculation_start(void)
> > > > +{
> > > > +   alternative_msr_write(MSR_IA32_SPEC_CTRL, SPEC_CTRL_IBRS,
> > > > + X86_FEATURE_USE_IBRS_FW);
> > > > +}
> > > > +
> > > > +static inline void firmware_restrict_branch_speculation_end(void)
> > > > +{
> > > > +   alternative_msr_write(MSR_IA32_SPEC_CTRL, 0,
> > > > + X86_FEATURE_USE_IBRS_FW);
> > > 
> > > BTW., there's a detail that only occurred to me today, this 
> > > enabling/disabling 
> > > sequence is not NMI safe, and it might be called from NMI context:
> > 
> > Wait, we're doing firmware from NMI? That sounds like a _REALLY_ bad
> > idea.
> 
> And spin_lock_irqsave() too. Which is probably why I missed the fact
> that this was being called in NMI context.
> 
> Yay for HP and their persistent attempts to "value subtract" in their
> firmware offerings.
> 
> I'm tempted to drop that part of the patch and declare that if you're
> using this driver, the potential for stray branch prediction when you
> call into the firmware from the NMI handler is the *least* of your
> problems.
> 
> I *will* go back over the other parts of the patch and audit them for
> preempt safety though; there could potentially be a similar issue
> there. I think I put them close enough to the actual firmware calls
> that if we aren't already preempt-safe then we were screwed anyway, but
> *maybe* there's merit in making the macros explicitly bump the preempt
> count anyway.

Ok, meanwhile I'm removing this patch from the x86/pti branch, and since the 
branch has to be rebased anyway, I'll merge these into a single patch:

85d8426e0720: x86/speculation: Correct Speculation Control microcode blacklist 
again
1751342095f0: x86/speculation: Update Speculation Control microcode blacklist

Thanks,

Ingo

Re: [tip:x86/pti] x86/speculation: Use IBRS if available before calling into firmware

[Ingo Molnar]

* David Woodhouse  wrote:

> On Mon, 2018-02-12 at 12:50 +0100, Peter Zijlstra wrote:
> > On Mon, Feb 12, 2018 at 11:22:11AM +0100, Ingo Molnar wrote:
> > > > +static inline void firmware_restrict_branch_speculation_start(void)
> > > > +{
> > > > +   alternative_msr_write(MSR_IA32_SPEC_CTRL, SPEC_CTRL_IBRS,
> > > > + X86_FEATURE_USE_IBRS_FW);
> > > > +}
> > > > +
> > > > +static inline void firmware_restrict_branch_speculation_end(void)
> > > > +{
> > > > +   alternative_msr_write(MSR_IA32_SPEC_CTRL, 0,
> > > > + X86_FEATURE_USE_IBRS_FW);
> > > 
> > > BTW., there's a detail that only occurred to me today, this 
> > > enabling/disabling 
> > > sequence is not NMI safe, and it might be called from NMI context:
> > 
> > Wait, we're doing firmware from NMI? That sounds like a _REALLY_ bad
> > idea.
> 
> And spin_lock_irqsave() too. Which is probably why I missed the fact
> that this was being called in NMI context.
> 
> Yay for HP and their persistent attempts to "value subtract" in their
> firmware offerings.
> 
> I'm tempted to drop that part of the patch and declare that if you're
> using this driver, the potential for stray branch prediction when you
> call into the firmware from the NMI handler is the *least* of your
> problems.
> 
> I *will* go back over the other parts of the patch and audit them for
> preempt safety though; there could potentially be a similar issue
> there. I think I put them close enough to the actual firmware calls
> that if we aren't already preempt-safe then we were screwed anyway, but
> *maybe* there's merit in making the macros explicitly bump the preempt
> count anyway.

Ok, meanwhile I'm removing this patch from the x86/pti branch, and since the 
branch has to be rebased anyway, I'll merge these into a single patch:

85d8426e0720: x86/speculation: Correct Speculation Control microcode blacklist 
again
1751342095f0: x86/speculation: Update Speculation Control microcode blacklist

Thanks,

Ingo

Re: [tip:x86/pti] x86/speculation: Use IBRS if available before calling into firmware

[Ingo Molnar]

* Peter Zijlstra  wrote:

> On Mon, Feb 12, 2018 at 08:13:31AM -0800, Dave Hansen wrote:
> > On 02/12/2018 02:22 AM, Ingo Molnar wrote:
> > >> +static inline void firmware_restrict_branch_speculation_end(void)
> > >> +{
> > >> +alternative_msr_write(MSR_IA32_SPEC_CTRL, 0,
> > >> +  X86_FEATURE_USE_IBRS_FW);
> > > BTW., there's a detail that only occurred to me today, this 
> > > enabling/disabling 
> > > sequence is not NMI safe, and it might be called from NMI context:
> > 
> > FWIW, Tim Chen and I talked about this a bunch.  We ended up just
> > saving/restoring the MSR verbatim in the NMI handler the same way we do
> > CR3, stashing it in a high general-purpose-register (r%12?).  That costs
> > a RDMSR (at least) and an WRMSR (which you can optimize out).  We have a
> > patch for that somewhere if anybody wants it.
> 
> I would really rather not do that on the NMI path.. And if we _have_ to,
> please keep a software shadow of that MSR state, such that we can avoid
> touching that MSR 99% of the time.

Yeah, I'd rather avoid doing firmware calls from NMI context altogether.

Thanks,

Ingo

Re: [tip:x86/pti] x86/speculation: Use IBRS if available before calling into firmware

[Ingo Molnar]

* Peter Zijlstra  wrote:

> On Mon, Feb 12, 2018 at 08:13:31AM -0800, Dave Hansen wrote:
> > On 02/12/2018 02:22 AM, Ingo Molnar wrote:
> > >> +static inline void firmware_restrict_branch_speculation_end(void)
> > >> +{
> > >> +alternative_msr_write(MSR_IA32_SPEC_CTRL, 0,
> > >> +  X86_FEATURE_USE_IBRS_FW);
> > > BTW., there's a detail that only occurred to me today, this 
> > > enabling/disabling 
> > > sequence is not NMI safe, and it might be called from NMI context:
> > 
> > FWIW, Tim Chen and I talked about this a bunch.  We ended up just
> > saving/restoring the MSR verbatim in the NMI handler the same way we do
> > CR3, stashing it in a high general-purpose-register (r%12?).  That costs
> > a RDMSR (at least) and an WRMSR (which you can optimize out).  We have a
> > patch for that somewhere if anybody wants it.
> 
> I would really rather not do that on the NMI path.. And if we _have_ to,
> please keep a software shadow of that MSR state, such that we can avoid
> touching that MSR 99% of the time.

Yeah, I'd rather avoid doing firmware calls from NMI context altogether.

Thanks,

Ingo

Re: [tip:x86/pti] x86/speculation: Use IBRS if available before calling into firmware

[Peter Zijlstra]

On Mon, Feb 12, 2018 at 08:13:31AM -0800, Dave Hansen wrote:
> On 02/12/2018 02:22 AM, Ingo Molnar wrote:
> >> +static inline void firmware_restrict_branch_speculation_end(void)
> >> +{
> >> +  alternative_msr_write(MSR_IA32_SPEC_CTRL, 0,
> >> +X86_FEATURE_USE_IBRS_FW);
> > BTW., there's a detail that only occurred to me today, this 
> > enabling/disabling 
> > sequence is not NMI safe, and it might be called from NMI context:
> 
> FWIW, Tim Chen and I talked about this a bunch.  We ended up just
> saving/restoring the MSR verbatim in the NMI handler the same way we do
> CR3, stashing it in a high general-purpose-register (r%12?).  That costs
> a RDMSR (at least) and an WRMSR (which you can optimize out).  We have a
> patch for that somewhere if anybody wants it.

I would really rather not do that on the NMI path.. And if we _have_ to,
please keep a software shadow of that MSR state, such that we can avoid
touching that MSR 99% of the time.

Re: [tip:x86/pti] x86/speculation: Use IBRS if available before calling into firmware

[Peter Zijlstra]

On Mon, Feb 12, 2018 at 08:13:31AM -0800, Dave Hansen wrote:
> On 02/12/2018 02:22 AM, Ingo Molnar wrote:
> >> +static inline void firmware_restrict_branch_speculation_end(void)
> >> +{
> >> +  alternative_msr_write(MSR_IA32_SPEC_CTRL, 0,
> >> +X86_FEATURE_USE_IBRS_FW);
> > BTW., there's a detail that only occurred to me today, this 
> > enabling/disabling 
> > sequence is not NMI safe, and it might be called from NMI context:
> 
> FWIW, Tim Chen and I talked about this a bunch.  We ended up just
> saving/restoring the MSR verbatim in the NMI handler the same way we do
> CR3, stashing it in a high general-purpose-register (r%12?).  That costs
> a RDMSR (at least) and an WRMSR (which you can optimize out).  We have a
> patch for that somewhere if anybody wants it.

I would really rather not do that on the NMI path.. And if we _have_ to,
please keep a software shadow of that MSR state, such that we can avoid
touching that MSR 99% of the time.

Re: [tip:x86/pti] x86/speculation: Use IBRS if available before calling into firmware

[David Woodhouse]

On Mon, 2018-02-12 at 11:29 +0530, afzal mohammed wrote:
> Hi,
> 
> On Sun, Feb 11, 2018 at 11:19:10AM -0800, tip-bot for David Woodhouse wrote:
> 
> > 
> > x86/speculation: Use IBRS if available before calling into firmware
> > 
> > Retpoline means the kernel is safe because it has no indirect branches.
> > But firmware isn't, so use IBRS for firmware calls if it's available.
>
> afaui, so only retpoline means still mitigation not enough.
> 
> Also David W has mentioned [1] that even with retpoline, IBPB is also
> required (except Sky Lake).

Retpoline is sufficient to protect the *kernel*, which is the biggest
target. (Except on Skylake, where IBRS is the only full mitigation and
people are still working trying to come up with a "good enough"
mitigation that isn't IBRS.)

On all CPUs, you need IBPB to protect userspace processes from each
other, although since it's slow we don't actually *do* that for every
context switch; only when switching to non-dumpable processes.

That IBPB requirement for protecting userspace is true even on the next
generation of CPUs with the "Enhanced IBRS" (IBRS_ALL) feature. It only
goes away in CPUs which are even *further* in the future, when Intel
manage to fix it completely in hardware. They haven't even documented
the feature bit they're going to advertise to indicate that fix yet!


> If IBPB & IBRS is not supported by ucode, shouldn't the below indicate
> some thing on the lines of Mitigation not enough ?
> 
> > 
> > -   return sprintf(buf, "%s%s%s\n", spectre_v2_strings[spectre_v2_enabled],
> > +   return sprintf(buf, "%s%s%s%s\n", 
> > spectre_v2_strings[spectre_v2_enabled],
> >        boot_cpu_has(X86_FEATURE_USE_IBPB) ? ", IBPB" : "",
> > +      boot_cpu_has(X86_FEATURE_USE_IBRS_FW) ? ", IBRS_FW" : "",
> >        spectre_v2_module_string());
> On 4.16-rc1, w/ GCC 7.3.0,
> 
> /sys/devices/system/cpu/vulnerabilities/meltdown:Mitigation: PTI
> /sys/devices/system/cpu/vulnerabilities/spectre_v1:Mitigation: __user pointer 
> sanitization
> /sys/devices/system/cpu/vulnerabilities/spectre_v2:Mitigation: Full generic 
> retpoline
> 
> Here for the user (at least for me), it is not clear whether the
> mitigation is enough. In the present system (Ivy Bridge), as ucode
> update is not available, IBPB is not printed along with
> "spectre_v2:Mitigation", so unless i am missing something, till then
> this system should be considered vulnerable, but for a user not
> familiar with details of the issue, it cannot be deduced.
> 
> Perhaps an additional status field [OKAY,PARTIAL] to Mitigation in
> sysfs might be helpful. All these changes are in the air for me, this
> is from a user perspective, sorry if my feedback seems idiotic.

Given that we only do it for non-dumpable processes, it's *always*
going to be only partial. (Although I think Thomas was looking at a
command line option to  make that happen on every context switch?)

And on Skylake the current plan is that strictly speaking it would also
be partial.

I understand the concern, but I'm not sure that there's much we can do
to improve it. If it says "Mitigation:" that's generally OK, and if it
says anything else, it's not.


smime.p7s
Description: S/MIME cryptographic signature

Re: [tip:x86/pti] x86/speculation: Use IBRS if available before calling into firmware

[David Woodhouse]

On Mon, 2018-02-12 at 11:29 +0530, afzal mohammed wrote:
> Hi,
> 
> On Sun, Feb 11, 2018 at 11:19:10AM -0800, tip-bot for David Woodhouse wrote:
> 
> > 
> > x86/speculation: Use IBRS if available before calling into firmware
> > 
> > Retpoline means the kernel is safe because it has no indirect branches.
> > But firmware isn't, so use IBRS for firmware calls if it's available.
>
> afaui, so only retpoline means still mitigation not enough.
> 
> Also David W has mentioned [1] that even with retpoline, IBPB is also
> required (except Sky Lake).

Retpoline is sufficient to protect the *kernel*, which is the biggest
target. (Except on Skylake, where IBRS is the only full mitigation and
people are still working trying to come up with a "good enough"
mitigation that isn't IBRS.)

On all CPUs, you need IBPB to protect userspace processes from each
other, although since it's slow we don't actually *do* that for every
context switch; only when switching to non-dumpable processes.

That IBPB requirement for protecting userspace is true even on the next
generation of CPUs with the "Enhanced IBRS" (IBRS_ALL) feature. It only
goes away in CPUs which are even *further* in the future, when Intel
manage to fix it completely in hardware. They haven't even documented
the feature bit they're going to advertise to indicate that fix yet!


> If IBPB & IBRS is not supported by ucode, shouldn't the below indicate
> some thing on the lines of Mitigation not enough ?
> 
> > 
> > -   return sprintf(buf, "%s%s%s\n", spectre_v2_strings[spectre_v2_enabled],
> > +   return sprintf(buf, "%s%s%s%s\n", 
> > spectre_v2_strings[spectre_v2_enabled],
> >        boot_cpu_has(X86_FEATURE_USE_IBPB) ? ", IBPB" : "",
> > +      boot_cpu_has(X86_FEATURE_USE_IBRS_FW) ? ", IBRS_FW" : "",
> >        spectre_v2_module_string());
> On 4.16-rc1, w/ GCC 7.3.0,
> 
> /sys/devices/system/cpu/vulnerabilities/meltdown:Mitigation: PTI
> /sys/devices/system/cpu/vulnerabilities/spectre_v1:Mitigation: __user pointer 
> sanitization
> /sys/devices/system/cpu/vulnerabilities/spectre_v2:Mitigation: Full generic 
> retpoline
> 
> Here for the user (at least for me), it is not clear whether the
> mitigation is enough. In the present system (Ivy Bridge), as ucode
> update is not available, IBPB is not printed along with
> "spectre_v2:Mitigation", so unless i am missing something, till then
> this system should be considered vulnerable, but for a user not
> familiar with details of the issue, it cannot be deduced.
> 
> Perhaps an additional status field [OKAY,PARTIAL] to Mitigation in
> sysfs might be helpful. All these changes are in the air for me, this
> is from a user perspective, sorry if my feedback seems idiotic.

Given that we only do it for non-dumpable processes, it's *always*
going to be only partial. (Although I think Thomas was looking at a
command line option to  make that happen on every context switch?)

And on Skylake the current plan is that strictly speaking it would also
be partial.

I understand the concern, but I'm not sure that there's much we can do
to improve it. If it says "Mitigation:" that's generally OK, and if it
says anything else, it's not.


smime.p7s
Description: S/MIME cryptographic signature

Re: [tip:x86/pti] x86/speculation: Use IBRS if available before calling into firmware

[Dave Hansen]

On 02/12/2018 02:22 AM, Ingo Molnar wrote:
>> +static inline void firmware_restrict_branch_speculation_end(void)
>> +{
>> +alternative_msr_write(MSR_IA32_SPEC_CTRL, 0,
>> +  X86_FEATURE_USE_IBRS_FW);
> BTW., there's a detail that only occurred to me today, this 
> enabling/disabling 
> sequence is not NMI safe, and it might be called from NMI context:

FWIW, Tim Chen and I talked about this a bunch.  We ended up just
saving/restoring the MSR verbatim in the NMI handler the same way we do
CR3, stashing it in a high general-purpose-register (r%12?).  That costs
a RDMSR (at least) and an WRMSR (which you can optimize out).  We have a
patch for that somewhere if anybody wants it.

We also came to the same conclusion that it's a rather challenging thing
to exploit these cases, especially when you consider that we can easily
do RSB stuffing on NMI exit just before going back to the firmware.

Re: [tip:x86/pti] x86/speculation: Use IBRS if available before calling into firmware

[Dave Hansen]

On 02/12/2018 02:22 AM, Ingo Molnar wrote:
>> +static inline void firmware_restrict_branch_speculation_end(void)
>> +{
>> +alternative_msr_write(MSR_IA32_SPEC_CTRL, 0,
>> +  X86_FEATURE_USE_IBRS_FW);
> BTW., there's a detail that only occurred to me today, this 
> enabling/disabling 
> sequence is not NMI safe, and it might be called from NMI context:

FWIW, Tim Chen and I talked about this a bunch.  We ended up just
saving/restoring the MSR verbatim in the NMI handler the same way we do
CR3, stashing it in a high general-purpose-register (r%12?).  That costs
a RDMSR (at least) and an WRMSR (which you can optimize out).  We have a
patch for that somewhere if anybody wants it.

We also came to the same conclusion that it's a rather challenging thing
to exploit these cases, especially when you consider that we can easily
do RSB stuffing on NMI exit just before going back to the firmware.

Re: [tip:x86/pti] x86/speculation: Use IBRS if available before calling into firmware

[Peter Zijlstra]

On Mon, Feb 12, 2018 at 12:27:19PM +, David Woodhouse wrote:
> On Mon, 2018-02-12 at 12:50 +0100, Peter Zijlstra wrote:

> > Wait, we're doing firmware from NMI? That sounds like a _REALLY_ bad
> > idea.
> 
> And spin_lock_irqsave() too. Which is probably why I missed the fact
> that this was being called in NMI context.
> 
> Yay for HP and their persistent attempts to "value subtract" in their
> firmware offerings.
> 
> I'm tempted to drop that part of the patch and declare that if you're
> using this driver, the potential for stray branch prediction when you
> call into the firmware from the NMI handler is the *least* of your
> problems.

We should probably mark it BROKEN or something, or move it into staging.

Re: [tip:x86/pti] x86/speculation: Use IBRS if available before calling into firmware

[Peter Zijlstra]

On Mon, Feb 12, 2018 at 12:27:19PM +, David Woodhouse wrote:
> On Mon, 2018-02-12 at 12:50 +0100, Peter Zijlstra wrote:

> > Wait, we're doing firmware from NMI? That sounds like a _REALLY_ bad
> > idea.
> 
> And spin_lock_irqsave() too. Which is probably why I missed the fact
> that this was being called in NMI context.
> 
> Yay for HP and their persistent attempts to "value subtract" in their
> firmware offerings.
> 
> I'm tempted to drop that part of the patch and declare that if you're
> using this driver, the potential for stray branch prediction when you
> call into the firmware from the NMI handler is the *least* of your
> problems.

We should probably mark it BROKEN or something, or move it into staging.

Re: [tip:x86/pti] x86/speculation: Use IBRS if available before calling into firmware

[Peter Zijlstra]

On Mon, Feb 12, 2018 at 12:50:02PM +0100, Peter Zijlstra wrote:
> On Mon, Feb 12, 2018 at 11:22:11AM +0100, Ingo Molnar wrote:
> > > +static inline void firmware_restrict_branch_speculation_start(void)
> > > +{
> > > + alternative_msr_write(MSR_IA32_SPEC_CTRL, SPEC_CTRL_IBRS,
> > > +   X86_FEATURE_USE_IBRS_FW);
> > > +}
> > > +
> > > +static inline void firmware_restrict_branch_speculation_end(void)
> > > +{
> > > + alternative_msr_write(MSR_IA32_SPEC_CTRL, 0,
> > > +   X86_FEATURE_USE_IBRS_FW);
> > 
> > BTW., there's a detail that only occurred to me today, this 
> > enabling/disabling 
> > sequence is not NMI safe, and it might be called from NMI context:
> 
> Wait, we're doing firmware from NMI? That sounds like a _REALLY_ bad
> idea.

Argh, its that stupid watchdog driver again.. Not only does it call
firmware, it also uses (!raw) spinlock.

Re: [tip:x86/pti] x86/speculation: Use IBRS if available before calling into firmware

[Peter Zijlstra]

On Mon, Feb 12, 2018 at 12:50:02PM +0100, Peter Zijlstra wrote:
> On Mon, Feb 12, 2018 at 11:22:11AM +0100, Ingo Molnar wrote:
> > > +static inline void firmware_restrict_branch_speculation_start(void)
> > > +{
> > > + alternative_msr_write(MSR_IA32_SPEC_CTRL, SPEC_CTRL_IBRS,
> > > +   X86_FEATURE_USE_IBRS_FW);
> > > +}
> > > +
> > > +static inline void firmware_restrict_branch_speculation_end(void)
> > > +{
> > > + alternative_msr_write(MSR_IA32_SPEC_CTRL, 0,
> > > +   X86_FEATURE_USE_IBRS_FW);
> > 
> > BTW., there's a detail that only occurred to me today, this 
> > enabling/disabling 
> > sequence is not NMI safe, and it might be called from NMI context:
> 
> Wait, we're doing firmware from NMI? That sounds like a _REALLY_ bad
> idea.

Argh, its that stupid watchdog driver again.. Not only does it call
firmware, it also uses (!raw) spinlock.

Re: [tip:x86/pti] x86/speculation: Use IBRS if available before calling into firmware

[David Woodhouse]

On Mon, 2018-02-12 at 12:50 +0100, Peter Zijlstra wrote:
> On Mon, Feb 12, 2018 at 11:22:11AM +0100, Ingo Molnar wrote:
> > > +static inline void firmware_restrict_branch_speculation_start(void)
> > > +{
> > > +   alternative_msr_write(MSR_IA32_SPEC_CTRL, SPEC_CTRL_IBRS,
> > > + X86_FEATURE_USE_IBRS_FW);
> > > +}
> > > +
> > > +static inline void firmware_restrict_branch_speculation_end(void)
> > > +{
> > > +   alternative_msr_write(MSR_IA32_SPEC_CTRL, 0,
> > > + X86_FEATURE_USE_IBRS_FW);
> > 
> > BTW., there's a detail that only occurred to me today, this 
> > enabling/disabling 
> > sequence is not NMI safe, and it might be called from NMI context:
> 
> Wait, we're doing firmware from NMI? That sounds like a _REALLY_ bad
> idea.

And spin_lock_irqsave() too. Which is probably why I missed the fact
that this was being called in NMI context.

Yay for HP and their persistent attempts to "value subtract" in their
firmware offerings.

I'm tempted to drop that part of the patch and declare that if you're
using this driver, the potential for stray branch prediction when you
call into the firmware from the NMI handler is the *least* of your
problems.

I *will* go back over the other parts of the patch and audit them for
preempt safety though; there could potentially be a similar issue
there. I think I put them close enough to the actual firmware calls
that if we aren't already preempt-safe then we were screwed anyway, but
*maybe* there's merit in making the macros explicitly bump the preempt
count anyway.

smime.p7s
Description: S/MIME cryptographic signature

Re: [tip:x86/pti] x86/speculation: Use IBRS if available before calling into firmware

[David Woodhouse]

On Mon, 2018-02-12 at 12:50 +0100, Peter Zijlstra wrote:
> On Mon, Feb 12, 2018 at 11:22:11AM +0100, Ingo Molnar wrote:
> > > +static inline void firmware_restrict_branch_speculation_start(void)
> > > +{
> > > +   alternative_msr_write(MSR_IA32_SPEC_CTRL, SPEC_CTRL_IBRS,
> > > + X86_FEATURE_USE_IBRS_FW);
> > > +}
> > > +
> > > +static inline void firmware_restrict_branch_speculation_end(void)
> > > +{
> > > +   alternative_msr_write(MSR_IA32_SPEC_CTRL, 0,
> > > + X86_FEATURE_USE_IBRS_FW);
> > 
> > BTW., there's a detail that only occurred to me today, this 
> > enabling/disabling 
> > sequence is not NMI safe, and it might be called from NMI context:
> 
> Wait, we're doing firmware from NMI? That sounds like a _REALLY_ bad
> idea.

And spin_lock_irqsave() too. Which is probably why I missed the fact
that this was being called in NMI context.

Yay for HP and their persistent attempts to "value subtract" in their
firmware offerings.

I'm tempted to drop that part of the patch and declare that if you're
using this driver, the potential for stray branch prediction when you
call into the firmware from the NMI handler is the *least* of your
problems.

I *will* go back over the other parts of the patch and audit them for
preempt safety though; there could potentially be a similar issue
there. I think I put them close enough to the actual firmware calls
that if we aren't already preempt-safe then we were screwed anyway, but
*maybe* there's merit in making the macros explicitly bump the preempt
count anyway.

smime.p7s
Description: S/MIME cryptographic signature

Re: [tip:x86/pti] x86/speculation: Use IBRS if available before calling into firmware

[Peter Zijlstra]

On Mon, Feb 12, 2018 at 11:22:11AM +0100, Ingo Molnar wrote:
> > +static inline void firmware_restrict_branch_speculation_start(void)
> > +{
> > +   alternative_msr_write(MSR_IA32_SPEC_CTRL, SPEC_CTRL_IBRS,
> > + X86_FEATURE_USE_IBRS_FW);
> > +}
> > +
> > +static inline void firmware_restrict_branch_speculation_end(void)
> > +{
> > +   alternative_msr_write(MSR_IA32_SPEC_CTRL, 0,
> > + X86_FEATURE_USE_IBRS_FW);
> 
> BTW., there's a detail that only occurred to me today, this 
> enabling/disabling 
> sequence is not NMI safe, and it might be called from NMI context:

Wait, we're doing firmware from NMI? That sounds like a _REALLY_ bad
idea.

Re: [tip:x86/pti] x86/speculation: Use IBRS if available before calling into firmware

[Peter Zijlstra]

On Mon, Feb 12, 2018 at 11:22:11AM +0100, Ingo Molnar wrote:
> > +static inline void firmware_restrict_branch_speculation_start(void)
> > +{
> > +   alternative_msr_write(MSR_IA32_SPEC_CTRL, SPEC_CTRL_IBRS,
> > + X86_FEATURE_USE_IBRS_FW);
> > +}
> > +
> > +static inline void firmware_restrict_branch_speculation_end(void)
> > +{
> > +   alternative_msr_write(MSR_IA32_SPEC_CTRL, 0,
> > + X86_FEATURE_USE_IBRS_FW);
> 
> BTW., there's a detail that only occurred to me today, this 
> enabling/disabling 
> sequence is not NMI safe, and it might be called from NMI context:

Wait, we're doing firmware from NMI? That sounds like a _REALLY_ bad
idea.

Re: [tip:x86/pti] x86/speculation: Use IBRS if available before calling into firmware

[Ingo Molnar]

* tip-bot for David Woodhouse  wrote:

> Commit-ID:  670c3e8da87fa4046a55077b1409cf250865a203
> Gitweb: 
> https://git.kernel.org/tip/670c3e8da87fa4046a55077b1409cf250865a203
> Author: David Woodhouse 
> AuthorDate: Sun, 11 Feb 2018 15:19:19 +
> Committer:  Ingo Molnar 
> CommitDate: Sun, 11 Feb 2018 19:44:46 +0100
> 
> x86/speculation: Use IBRS if available before calling into firmware
> 
> Retpoline means the kernel is safe because it has no indirect branches.
> But firmware isn't, so use IBRS for firmware calls if it's available.
> 
> Signed-off-by: David Woodhouse 
> Cc: Linus Torvalds 
> Cc: Peter Zijlstra 
> Cc: Thomas Gleixner 
> Link: 
> http://lkml.kernel.org/r/1518362359-1005-1-git-send-email-d...@amazon.co.uk
> Signed-off-by: Ingo Molnar 
> ---
>  arch/x86/include/asm/apm.h   |  6 ++
>  arch/x86/include/asm/cpufeatures.h   |  1 +
>  arch/x86/include/asm/efi.h   | 17 +++--
>  arch/x86/include/asm/nospec-branch.h | 37 
> +++-
>  arch/x86/kernel/cpu/bugs.c   | 12 +++-
>  drivers/watchdog/hpwdt.c |  3 +++
>  6 files changed, 64 insertions(+), 12 deletions(-)

> --- a/arch/x86/include/asm/nospec-branch.h
> +++ b/arch/x86/include/asm/nospec-branch.h

> +/*
> + * With retpoline, we must use IBRS to restrict branch prediction
> + * before calling into firmware.
> + */
> +static inline void firmware_restrict_branch_speculation_start(void)
> +{
> + alternative_msr_write(MSR_IA32_SPEC_CTRL, SPEC_CTRL_IBRS,
> +   X86_FEATURE_USE_IBRS_FW);
> +}
> +
> +static inline void firmware_restrict_branch_speculation_end(void)
> +{
> + alternative_msr_write(MSR_IA32_SPEC_CTRL, 0,
> +   X86_FEATURE_USE_IBRS_FW);

BTW., there's a detail that only occurred to me today, this enabling/disabling 
sequence is not NMI safe, and it might be called from NMI context:

> --- a/drivers/watchdog/hpwdt.c
> +++ b/drivers/watchdog/hpwdt.c
> @@ -38,6 +38,7 @@
>  #endif /* CONFIG_HPWDT_NMI_DECODING */
>  #include 
>  #include 
> +#include 
>  
>  #define HPWDT_VERSION"1.4.0"
>  #define SECS_TO_TICKS(secs)  ((secs) * 1000 / 128)
> @@ -486,11 +487,13 @@ static int hpwdt_pretimeout(unsigned int ulReason, 
> struct pt_regs *regs)
>   if (!hpwdt_nmi_decoding)
>   return NMI_DONE;
>  
> + firmware_restrict_branch_speculation_start();
>   spin_lock_irqsave(_lock, rom_pl);
>   if (!die_nmi_called && !is_icru && !is_uefi)
>   asminline_call(_regs, cru_rom_addr);
>   die_nmi_called = 1;
>   spin_unlock_irqrestore(_lock, rom_pl);
> + firmware_restrict_branch_speculation_end();
>  
>   if (allow_kdump)
>   hpwdt_stop();

But ... this is a (comparatively rare) hardware-watchdog tick function, and the 
chance of racing with say an EFI call seems minuscule. The race will result in 
an 
EFI call being performed with speculation enabled, sporadically.

Is this something we should worry about?

Thanks,

Ingo

Re: [tip:x86/pti] x86/speculation: Use IBRS if available before calling into firmware

[Ingo Molnar]

* tip-bot for David Woodhouse  wrote:

> Commit-ID:  670c3e8da87fa4046a55077b1409cf250865a203
> Gitweb: 
> https://git.kernel.org/tip/670c3e8da87fa4046a55077b1409cf250865a203
> Author: David Woodhouse 
> AuthorDate: Sun, 11 Feb 2018 15:19:19 +
> Committer:  Ingo Molnar 
> CommitDate: Sun, 11 Feb 2018 19:44:46 +0100
> 
> x86/speculation: Use IBRS if available before calling into firmware
> 
> Retpoline means the kernel is safe because it has no indirect branches.
> But firmware isn't, so use IBRS for firmware calls if it's available.
> 
> Signed-off-by: David Woodhouse 
> Cc: Linus Torvalds 
> Cc: Peter Zijlstra 
> Cc: Thomas Gleixner 
> Link: 
> http://lkml.kernel.org/r/1518362359-1005-1-git-send-email-d...@amazon.co.uk
> Signed-off-by: Ingo Molnar 
> ---
>  arch/x86/include/asm/apm.h   |  6 ++
>  arch/x86/include/asm/cpufeatures.h   |  1 +
>  arch/x86/include/asm/efi.h   | 17 +++--
>  arch/x86/include/asm/nospec-branch.h | 37 
> +++-
>  arch/x86/kernel/cpu/bugs.c   | 12 +++-
>  drivers/watchdog/hpwdt.c |  3 +++
>  6 files changed, 64 insertions(+), 12 deletions(-)

> --- a/arch/x86/include/asm/nospec-branch.h
> +++ b/arch/x86/include/asm/nospec-branch.h

> +/*
> + * With retpoline, we must use IBRS to restrict branch prediction
> + * before calling into firmware.
> + */
> +static inline void firmware_restrict_branch_speculation_start(void)
> +{
> + alternative_msr_write(MSR_IA32_SPEC_CTRL, SPEC_CTRL_IBRS,
> +   X86_FEATURE_USE_IBRS_FW);
> +}
> +
> +static inline void firmware_restrict_branch_speculation_end(void)
> +{
> + alternative_msr_write(MSR_IA32_SPEC_CTRL, 0,
> +   X86_FEATURE_USE_IBRS_FW);

BTW., there's a detail that only occurred to me today, this enabling/disabling 
sequence is not NMI safe, and it might be called from NMI context:

> --- a/drivers/watchdog/hpwdt.c
> +++ b/drivers/watchdog/hpwdt.c
> @@ -38,6 +38,7 @@
>  #endif /* CONFIG_HPWDT_NMI_DECODING */
>  #include 
>  #include 
> +#include 
>  
>  #define HPWDT_VERSION"1.4.0"
>  #define SECS_TO_TICKS(secs)  ((secs) * 1000 / 128)
> @@ -486,11 +487,13 @@ static int hpwdt_pretimeout(unsigned int ulReason, 
> struct pt_regs *regs)
>   if (!hpwdt_nmi_decoding)
>   return NMI_DONE;
>  
> + firmware_restrict_branch_speculation_start();
>   spin_lock_irqsave(_lock, rom_pl);
>   if (!die_nmi_called && !is_icru && !is_uefi)
>   asminline_call(_regs, cru_rom_addr);
>   die_nmi_called = 1;
>   spin_unlock_irqrestore(_lock, rom_pl);
> + firmware_restrict_branch_speculation_end();
>  
>   if (allow_kdump)
>   hpwdt_stop();

But ... this is a (comparatively rare) hardware-watchdog tick function, and the 
chance of racing with say an EFI call seems minuscule. The race will result in 
an 
EFI call being performed with speculation enabled, sporadically.

Is this something we should worry about?

Thanks,

Ingo

Re: [tip:x86/pti] x86/speculation: Use IBRS if available before calling into firmware

[afzal mohammed]

Hi,

On Sun, Feb 11, 2018 at 11:19:10AM -0800, tip-bot for David Woodhouse wrote:

> x86/speculation: Use IBRS if available before calling into firmware
> 
> Retpoline means the kernel is safe because it has no indirect branches.
> But firmware isn't, so use IBRS for firmware calls if it's available.

afaui, so only retpoline means still mitigation not enough.

Also David W has mentioned [1] that even with retpoline, IBPB is also
required (except Sky Lake).

If IBPB & IBRS is not supported by ucode, shouldn't the below indicate
some thing on the lines of Mitigation not enough ?

> - return sprintf(buf, "%s%s%s\n", spectre_v2_strings[spectre_v2_enabled],
> + return sprintf(buf, "%s%s%s%s\n", 
> spectre_v2_strings[spectre_v2_enabled],
>  boot_cpu_has(X86_FEATURE_USE_IBPB) ? ", IBPB" : "",
> +boot_cpu_has(X86_FEATURE_USE_IBRS_FW) ? ", IBRS_FW" : "",
>  spectre_v2_module_string());

On 4.16-rc1, w/ GCC 7.3.0,

/sys/devices/system/cpu/vulnerabilities/meltdown:Mitigation: PTI
/sys/devices/system/cpu/vulnerabilities/spectre_v1:Mitigation: __user pointer 
sanitization
/sys/devices/system/cpu/vulnerabilities/spectre_v2:Mitigation: Full generic 
retpoline

Here for the user (at least for me), it is not clear whether the
mitigation is enough. In the present system (Ivy Bridge), as ucode
update is not available, IBPB is not printed along with
"spectre_v2:Mitigation", so unless i am missing something, till then
this system should be considered vulnerable, but for a user not
familiar with details of the issue, it cannot be deduced.

Perhaps an additional status field [OKAY,PARTIAL] to Mitigation in
sysfs might be helpful. All these changes are in the air for me, this
is from a user perspective, sorry if my feedback seems idiotic.

afzal


[1] lkml.kernel.org/r/1516638426.9521.20.ca...@infradead.org

Re: [tip:x86/pti] x86/speculation: Use IBRS if available before calling into firmware

[afzal mohammed]

Hi,

On Sun, Feb 11, 2018 at 11:19:10AM -0800, tip-bot for David Woodhouse wrote:

> x86/speculation: Use IBRS if available before calling into firmware
> 
> Retpoline means the kernel is safe because it has no indirect branches.
> But firmware isn't, so use IBRS for firmware calls if it's available.

afaui, so only retpoline means still mitigation not enough.

Also David W has mentioned [1] that even with retpoline, IBPB is also
required (except Sky Lake).

If IBPB & IBRS is not supported by ucode, shouldn't the below indicate
some thing on the lines of Mitigation not enough ?

> - return sprintf(buf, "%s%s%s\n", spectre_v2_strings[spectre_v2_enabled],
> + return sprintf(buf, "%s%s%s%s\n", 
> spectre_v2_strings[spectre_v2_enabled],
>  boot_cpu_has(X86_FEATURE_USE_IBPB) ? ", IBPB" : "",
> +boot_cpu_has(X86_FEATURE_USE_IBRS_FW) ? ", IBRS_FW" : "",
>  spectre_v2_module_string());

On 4.16-rc1, w/ GCC 7.3.0,

/sys/devices/system/cpu/vulnerabilities/meltdown:Mitigation: PTI
/sys/devices/system/cpu/vulnerabilities/spectre_v1:Mitigation: __user pointer 
sanitization
/sys/devices/system/cpu/vulnerabilities/spectre_v2:Mitigation: Full generic 
retpoline

Here for the user (at least for me), it is not clear whether the
mitigation is enough. In the present system (Ivy Bridge), as ucode
update is not available, IBPB is not printed along with
"spectre_v2:Mitigation", so unless i am missing something, till then
this system should be considered vulnerable, but for a user not
familiar with details of the issue, it cannot be deduced.

Perhaps an additional status field [OKAY,PARTIAL] to Mitigation in
sysfs might be helpful. All these changes are in the air for me, this
is from a user perspective, sorry if my feedback seems idiotic.

afzal


[1] lkml.kernel.org/r/1516638426.9521.20.ca...@infradead.org