ACK/CMNT: [SRU][Xenial][PATCH 2/3] UBUNTU: SAUCE: x86/speculation: Cleanup IBRS runtime control handling
Tyler Hicks
tyhicks at canonical.com
Wed Dec 5 15:06:54 UTC 2018
On 2018-12-05 11:41:28, Juerg Haefliger wrote:
> On Tue, 4 Dec 2018 13:48:37 -0600
> Tyler Hicks <tyhicks at canonical.com> wrote:
>
> > On 2018-12-03 14:17:49, Juerg Haefliger wrote:
> > > > > +#define ubuntu_restrict_branch_speculation_start() \
> > > > > +do { \
> > > > > + u64 val = x86_spec_ctrl_base | SPEC_CTRL_IBRS; \
> > > > > + \
> > > > > + if (ibrs_enabled) \
> > > > > + native_wrmsrl(MSR_IA32_SPEC_CTRL, val); \
> > > > > +} while (0)
> > > > > +
> > > > > +#define ubuntu_restrict_branch_speculation_end() \
> > > > > +do { \
> > > > > + u64 val = x86_spec_ctrl_base; \
> > > > > + \
> > > > > + if (ibrs_enabled) \
> > > > > + native_wrmsrl(MSR_IA32_SPEC_CTRL, val); \
> > > > > + } while (0)
> > >
> > > I just realized that the above is incorrect. If ibrs_enabled == 2 then we set
> > > the IBRS bit on every CPU and should not fiddle with them here, no?
> > >
> > > So it should be:
> > > if (ibrs_enabled == 1)
> > > native_wrmsrl(MSR_IA32_SPEC_CTRL, val);
> >
> > Yes, I think that's correct.
>
> After running some more tests I take my earlier statement back. Primarily
> because it would change the current behaviour. So I leave it as it is until
> somebody reports an issue.
>
>
> > >
> > >
> > > > > /*
> > > > > * Retpoline means the kernel is safe because it has no indirect
> > > > > * branches. Enhanced IBRS protects firmware too, so, enable restricted
> > > > > @@ -463,9 +446,23 @@ specv2_set_mode:
> > > > > * the CPU supports Enhanced IBRS, kernel might un-intentionally not
> > > > > * enable IBRS around firmware calls.
> > > > > */
> > > > > - if (boot_cpu_has(X86_FEATURE_IBRS) && mode != SPECTRE_V2_IBRS_ENHANCED) {
> > > > > - setup_force_cpu_cap(X86_FEATURE_USE_IBRS_FW);
> > > > > - pr_info("Enabling Restricted Speculation for firmware calls\n");
> > > > > + if (boot_cpu_has(X86_FEATURE_IBRS)) {
> > > > > + if (mode != SPECTRE_V2_IBRS_ENHANCED) {
> > > > > + setup_force_cpu_cap(X86_FEATURE_USE_IBRS_FW);
> > > > > + pr_info("Enabling Restricted Speculation for firmware calls\n");
> > > > > + }
> > > > > + if (noibrs ||
> > > > > + mode == SPECTRE_V2_RETPOLINE_GENERIC ||
> > > > > + mode == SPECTRE_V2_RETPOLINE_AMD ||
> > > > > + mode == SPECTRE_V2_IBRS_ENHANCED) {
> > > > > + /*
> > > > > + * IBRS disabled via commandline or the kernel is
> > > > > + * retpoline compiled or the HW supports enhanced IBRS
> > > > > + */
> > > > > + set_ibrs_enabled(0);
> > > >
> > > > It took me a while to understand that IBRS Enhanced handling here (and
> > > > then all the subtle details at runtime...). I now see that the IBRS bit
> > > > of the IA32_SPEC_CTRL MSR is set earlier in this function if IBRS
> > > > Enhanced is supported. Additionally, the SPEC_CTRL_IBRS bit is set in
> > > > the x86_spec_ctrl_base bitmask. The above call to set_ibrs_enabled(0)
> > > > ensures that ibrs_enabled is set to 0. That means that the IBRS bit of
> > > > the IA32_SPEC_CTRL MSR will not be twiddled in future calls to
> > > > ubuntu_restrict_branch_speculation_{start,end}() which is the correct
> > > > behavior for IBRS Enhanced but it is a bit confusing since ibrs_enabled
> > > > is 0.
> > >
> > > Hmm... The call set_ibrs_enabled(0) will clear the IBRS bit on all CPUs.
> > > On hindsight, that seems wrong in the presence of enhanced IBRS. Maybe we
> > > should disable the runtime control completely if mode ==
> > > SPECTRE_V2_IBRS_ENHANCED?
> >
> > The call set_ibrs_enabled(0) won't clear the IBRS bit because
> > x86_spec_ctrl_base has the SPEC_CTRL_IBRS bit set when Enhanced IBRS is
> > supported.
>
> Oh, right. Convoluted... :-(
>
>
> > However, one of the main benefits of Enhanced IBRS is that
> > you write to the MSR once and then don't have to toggle it at all so
> > set_ibrs_enabled(0) will be inefficient.
> > >
> > > > > +int set_ibrs_enabled(unsigned int val)
> > > > > +{
> > > > > + int error = 0;
> > > > > + unsigned int cpu;
> > > > > +
> > > > > + mutex_lock(&spec_ctrl_mutex);
> > > > > +
> > > > > + /* Only enable/disable IBRS if the CPU supports it */
> > > > > + if (boot_cpu_has(X86_FEATURE_IBRS)) {
> > > > > + ibrs_enabled = val;
> > > > > + pr_info("Spectre V2 : Spectre v2 mitigation: %s Indirect "
> > > >
> > > > I think the "Spectre V2 : " portion of this message is redundant and is
> > > > also not present in upstream's message which could confuse user or
> > > > scripts that are looking at log messages. Can it be removed when
> > > > applying the patch?
> > >
> > > Again, this is how upstream reported it until just recently.
> > >
> > >
> > >
> > > > > + "Branch Restricted Speculation%s\n",
> > > > > + ibrs_enabled ? "Enabling" : "Disabling",
> > > > > + ibrs_enabled == 2 ? " (user space)" : "");
> > > > > +
> > > > > + if (ibrs_enabled == 0) {
> > > > > + /* Always disable IBRS */
> > > > > + u64 val = x86_spec_ctrl_base;
> > > > > +
> > > > > + for_each_online_cpu(cpu) {
> > > > > + wrmsrl_on_cpu(cpu, MSR_IA32_SPEC_CTRL, val);
> > > > > + }
> > > > > + } else if (ibrs_enabled == 2) {
> > > > > + /* Always enable IBRS, even in user space */
> > > > > + u64 val = x86_spec_ctrl_base | SPEC_CTRL_IBRS;
> > > > > +
> > > > > + for_each_online_cpu(cpu) {
> > > > > + wrmsrl_on_cpu(cpu, MSR_IA32_SPEC_CTRL, val);
> > > > > + }
> > > > > + }
> > > > > + } else {
> > > > > + ibrs_enabled = 0;
> > > > > + if (val) {
> > > > > + /* IBRS is not supported but we try to turn it on */
> > > > > + error = -EINVAL;
> > > > > + }
> > > > > + }
> > > > > +
> > > > > + /* Update the shadow variable */
> > > > > + __ibrs_enabled = ibrs_enabled;
> > > > > +
> > > > > + mutex_unlock(&spec_ctrl_mutex);
> > > > > +
> > > > > + return error;
> > > > > +}
> > > > > +
> > > > > +static int ibrs_enabled_handler(struct ctl_table *table, int write,
> > > > > + void __user *buffer, size_t *lenp,
> > > > > + loff_t *ppos)
> > > > > +{
> > > > > + int error;
> > > > > +
> > > > > + error = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
> > > > > + if (error)
> > > > > + return error;
> > > > > +
> > > > > + return set_ibrs_enabled(__ibrs_enabled);
> > > > > +}
> > > > > #endif
> > > >
> > > > IBRS Enhanced cannot be disabled with this sysctl since ibrs_enabled is
> > > > already 0 and the corresponding bit is set in x86_spec_ctrl_base. I
> > > > personally don't feel like this is a problem but I wanted to mention it.
> > >
> > > As mentioned above, with enhanced IBRS we can still fiddle with the IBRS bit
> > > in the MSR via the runtime controls. We probably don't want that.
> >
> > Now I'm thinking that it makes sense to be able to disable and enable
> > Enhanced IBRS with the sysctl. Why wouldn't we want to support that?
>
> Because it introduces another ubuntu-only-impossible-to-test-hard-to-maintain
> feature with questionable benefit. Until there's a user request for it
> I'm leaning towards disabling the runtime controls in Enhanced IBRS mode.
I'm alright with that.
Tyler
>
> ...Juerg
>
>
>
> > Tyler
>
More information about the kernel-team
mailing list