* [PATCH 0/1] x86/speculation: Allow IBPB to be conditionally enabled on CPUs with always-on STIBP
@ 2020-10-29 6:51 Anand K Mistry
2020-10-29 6:51 ` [PATCH 1/1] " Anand K Mistry
2020-10-31 14:50 ` [PATCH 0/1] " Tom Lendacky
0 siblings, 2 replies; 9+ messages in thread
From: Anand K Mistry @ 2020-10-29 6:51 UTC (permalink / raw)
To: x86, linux-kernel
Cc: Thomas.Lendacky, joelaf, asteinhauser, tglx, Anand K Mistry,
Borislav Petkov, H. Peter Anvin, Ingo Molnar, Josh Poimboeuf,
Mark Gross, Mike Rapoport, Pawan Gupta, Tony Luck,
Vineela Tummalapalli, Waiman Long, linux-kernel
When attempting to do some performance testing of IBPB on and AMD
platform, I noticed the IBPB instruction was never being issued, even
though it was conditionally on and various seccomp protected processes
were force enabling it. Turns out, on those AMD CPUs, STIBP is set to
always-on and this was causing an early-out on the prctl() which turns
off IB speculation. Here is my attempt to fix it.
I'm hoping someone that understands this better than me can explain why
I'm wrong.
Anand K Mistry (1):
x86/speculation: Allow IBPB to be conditionally enabled on CPUs with
always-on STIBP
arch/x86/kernel/cpu/bugs.c | 41 +++++++++++++++++++++-----------------
1 file changed, 23 insertions(+), 18 deletions(-)
--
2.29.1.341.ge80a0c044ae-goog
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH 1/1] x86/speculation: Allow IBPB to be conditionally enabled on CPUs with always-on STIBP 2020-10-29 6:51 [PATCH 0/1] x86/speculation: Allow IBPB to be conditionally enabled on CPUs with always-on STIBP Anand K Mistry @ 2020-10-29 6:51 ` Anand K Mistry 2020-10-31 15:05 ` Tom Lendacky 2020-10-31 14:50 ` [PATCH 0/1] " Tom Lendacky 1 sibling, 1 reply; 9+ messages in thread From: Anand K Mistry @ 2020-10-29 6:51 UTC (permalink / raw) To: x86, linux-kernel Cc: Thomas.Lendacky, joelaf, asteinhauser, tglx, Anand K Mistry, Borislav Petkov, H. Peter Anvin, Ingo Molnar, Josh Poimboeuf, Mark Gross, Mike Rapoport, Pawan Gupta, Tony Luck, Vineela Tummalapalli, Waiman Long, linux-kernel On AMD CPUs which have the feature X86_FEATURE_AMD_STIBP_ALWAYS_ON, STIBP is set to on and 'spectre_v2_user_stibp == SPECTRE_V2_USER_STRICT_PREFERRED'. At the same time, IBPB can be set to conditional. However, this leads to the case where it's impossible to turn on IBPB for a process because in the PR_SPEC_DISABLE case in ib_prctl_set, the (spectre_v2_user_stibp == SPECTRE_V2_USER_STRICT_PREFERRED) condition leads to a return before the task flag is set. Similarly, ib_prctl_get will return PR_SPEC_DISABLE even though IBPB is set to conditional. More generally, the following cases are possible: 1. STIBP = conditional && IBPB = on for spectre_v2_user=seccomp,ibpb 2. STIBP = on && IBPB = conditional for AMD CPUs with X86_FEATURE_AMD_STIBP_ALWAYS_ON The first case functions correctly today, but only because spectre_v2_user_ibpb isn't updated to reflect the IBPB mode. At a high level, this change does one thing. If either STIBP is IBPB is set to conditional, allow the prctl to change the task flag. Also, reflect that capability when querying the state. This isn't perfect since it doesn't take into account if only STIBP or IBPB is unconditionally on. But it allows the conditional feature to work as expected, without affecting the unconditional one. Signed-off-by: Anand K Mistry <amistry@google.com> --- arch/x86/kernel/cpu/bugs.c | 41 +++++++++++++++++++++----------------- 1 file changed, 23 insertions(+), 18 deletions(-) diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c index d3f0db463f96..fb64e02eed6f 100644 --- a/arch/x86/kernel/cpu/bugs.c +++ b/arch/x86/kernel/cpu/bugs.c @@ -1254,6 +1254,11 @@ static int ssb_prctl_set(struct task_struct *task, unsigned long ctrl) return 0; } +static bool is_spec_ib_user(enum spectre_v2_user_mitigation mode) +{ + return mode == SPECTRE_V2_USER_PRCTL || mode == SPECTRE_V2_USER_SECCOMP; +} + static int ib_prctl_set(struct task_struct *task, unsigned long ctrl) { switch (ctrl) { @@ -1262,13 +1267,16 @@ static int ib_prctl_set(struct task_struct *task, unsigned long ctrl) spectre_v2_user_stibp == SPECTRE_V2_USER_NONE) return 0; /* - * Indirect branch speculation is always disabled in strict - * mode. It can neither be enabled if it was force-disabled - * by a previous prctl call. + * With strict mode for both IBPB and STIBP, the instruction + * code paths avoid checking this task flag and instead, + * unconditionally run the instruction. However, STIBP and IBPB + * are independent and either can be set to conditionally + * enabled regardless of the mode of the other. If either is set + * to conditional, allow the task flag to be updated, unless it + * was force-disabled by a previous prctl call. */ - if (spectre_v2_user_ibpb == SPECTRE_V2_USER_STRICT || - spectre_v2_user_stibp == SPECTRE_V2_USER_STRICT || - spectre_v2_user_stibp == SPECTRE_V2_USER_STRICT_PREFERRED || + if ((!is_spec_ib_user(spectre_v2_user_ibpb) && + !is_spec_ib_user(spectre_v2_user_stibp)) || task_spec_ib_force_disable(task)) return -EPERM; task_clear_spec_ib_disable(task); @@ -1283,9 +1291,8 @@ static int ib_prctl_set(struct task_struct *task, unsigned long ctrl) if (spectre_v2_user_ibpb == SPECTRE_V2_USER_NONE && spectre_v2_user_stibp == SPECTRE_V2_USER_NONE) return -EPERM; - if (spectre_v2_user_ibpb == SPECTRE_V2_USER_STRICT || - spectre_v2_user_stibp == SPECTRE_V2_USER_STRICT || - spectre_v2_user_stibp == SPECTRE_V2_USER_STRICT_PREFERRED) + if (!is_spec_ib_user(spectre_v2_user_ibpb) && + !is_spec_ib_user(spectre_v2_user_stibp)) return 0; task_set_spec_ib_disable(task); if (ctrl == PR_SPEC_FORCE_DISABLE) @@ -1351,20 +1358,18 @@ static int ib_prctl_get(struct task_struct *task) if (spectre_v2_user_ibpb == SPECTRE_V2_USER_NONE && spectre_v2_user_stibp == SPECTRE_V2_USER_NONE) return PR_SPEC_ENABLE; - else if (spectre_v2_user_ibpb == SPECTRE_V2_USER_STRICT || - spectre_v2_user_stibp == SPECTRE_V2_USER_STRICT || - spectre_v2_user_stibp == SPECTRE_V2_USER_STRICT_PREFERRED) - return PR_SPEC_DISABLE; - else if (spectre_v2_user_ibpb == SPECTRE_V2_USER_PRCTL || - spectre_v2_user_ibpb == SPECTRE_V2_USER_SECCOMP || - spectre_v2_user_stibp == SPECTRE_V2_USER_PRCTL || - spectre_v2_user_stibp == SPECTRE_V2_USER_SECCOMP) { + else if (is_spec_ib_user(spectre_v2_user_ibpb) || + is_spec_ib_user(spectre_v2_user_stibp)) { if (task_spec_ib_force_disable(task)) return PR_SPEC_PRCTL | PR_SPEC_FORCE_DISABLE; if (task_spec_ib_disable(task)) return PR_SPEC_PRCTL | PR_SPEC_DISABLE; return PR_SPEC_PRCTL | PR_SPEC_ENABLE; - } else + } else if (spectre_v2_user_ibpb == SPECTRE_V2_USER_STRICT || + spectre_v2_user_stibp == SPECTRE_V2_USER_STRICT || + spectre_v2_user_stibp == SPECTRE_V2_USER_STRICT_PREFERRED) + return PR_SPEC_DISABLE; + else return PR_SPEC_NOT_AFFECTED; } -- 2.29.1.341.ge80a0c044ae-goog ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] x86/speculation: Allow IBPB to be conditionally enabled on CPUs with always-on STIBP 2020-10-29 6:51 ` [PATCH 1/1] " Anand K Mistry @ 2020-10-31 15:05 ` Tom Lendacky 2020-11-02 0:02 ` Anand K. Mistry 0 siblings, 1 reply; 9+ messages in thread From: Tom Lendacky @ 2020-10-31 15:05 UTC (permalink / raw) To: Anand K Mistry, x86, linux-kernel Cc: joelaf, asteinhauser, tglx, Borislav Petkov, H. Peter Anvin, Ingo Molnar, Josh Poimboeuf, Mark Gross, Mike Rapoport, Pawan Gupta, Tony Luck, Vineela Tummalapalli, Waiman Long, linux-kernel On 10/29/20 1:51 AM, Anand K Mistry wrote: > On AMD CPUs which have the feature X86_FEATURE_AMD_STIBP_ALWAYS_ON, > STIBP is set to on and 'spectre_v2_user_stibp == > SPECTRE_V2_USER_STRICT_PREFERRED'. At the same time, IBPB can be set to > conditional. However, this leads to the case where it's impossible to > turn on IBPB for a process because in the PR_SPEC_DISABLE case in > ib_prctl_set, the (spectre_v2_user_stibp == > SPECTRE_V2_USER_STRICT_PREFERRED) condition leads to a return before the > task flag is set. Similarly, ib_prctl_get will return PR_SPEC_DISABLE > even though IBPB is set to conditional. > > More generally, the following cases are possible: > 1. STIBP = conditional && IBPB = on for spectre_v2_user=seccomp,ibpb > 2. STIBP = on && IBPB = conditional for AMD CPUs with > X86_FEATURE_AMD_STIBP_ALWAYS_ON > > The first case functions correctly today, but only because > spectre_v2_user_ibpb isn't updated to reflect the IBPB mode. > > At a high level, this change does one thing. If either STIBP is IBPB is s/STIBP is IBPB/STIBP or IBPB/ > set to conditional, allow the prctl to change the task flag. Also, > reflect that capability when querying the state. This isn't perfect > since it doesn't take into account if only STIBP or IBPB is > unconditionally on. But it allows the conditional feature to work as > expected, without affecting the unconditional one. > > Signed-off-by: Anand K Mistry <amistry@google.com> > > --- > > arch/x86/kernel/cpu/bugs.c | 41 +++++++++++++++++++++----------------- > 1 file changed, 23 insertions(+), 18 deletions(-) > > diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c > index d3f0db463f96..fb64e02eed6f 100644 > --- a/arch/x86/kernel/cpu/bugs.c > +++ b/arch/x86/kernel/cpu/bugs.c > @@ -1254,6 +1254,11 @@ static int ssb_prctl_set(struct task_struct *task, unsigned long ctrl) > return 0; > } > > +static bool is_spec_ib_user(enum spectre_v2_user_mitigation mode) Maybe something like is_spec_ib_user_controlled() would be a better name. > +{ > + return mode == SPECTRE_V2_USER_PRCTL || mode == SPECTRE_V2_USER_SECCOMP; > +} > + I like the idea of passing in the mode you want to check, but it appears they are never used independently. The ibpb and stibp modes are always checked together in one of the if statements below, so you could make this a function that checks both modes and just have a single call. I'll leave that up to the maintainers to see what is preferred. > static int ib_prctl_set(struct task_struct *task, unsigned long ctrl) > { > switch (ctrl) { > @@ -1262,13 +1267,16 @@ static int ib_prctl_set(struct task_struct *task, unsigned long ctrl) > spectre_v2_user_stibp == SPECTRE_V2_USER_NONE) > return 0; > /* > - * Indirect branch speculation is always disabled in strict > - * mode. It can neither be enabled if it was force-disabled > - * by a previous prctl call. > + * With strict mode for both IBPB and STIBP, the instruction > + * code paths avoid checking this task flag and instead, > + * unconditionally run the instruction. However, STIBP and IBPB > + * are independent and either can be set to conditionally > + * enabled regardless of the mode of the other. If either is set > + * to conditional, allow the task flag to be updated, unless it > + * was force-disabled by a previous prctl call. You probably want to reference the STIBP always on mode that allows this situation. > */ > - if (spectre_v2_user_ibpb == SPECTRE_V2_USER_STRICT || > - spectre_v2_user_stibp == SPECTRE_V2_USER_STRICT || > - spectre_v2_user_stibp == SPECTRE_V2_USER_STRICT_PREFERRED || > + if ((!is_spec_ib_user(spectre_v2_user_ibpb) && > + !is_spec_ib_user(spectre_v2_user_stibp)) || > task_spec_ib_force_disable(task)) > return -EPERM; > task_clear_spec_ib_disable(task); > @@ -1283,9 +1291,8 @@ static int ib_prctl_set(struct task_struct *task, unsigned long ctrl) > if (spectre_v2_user_ibpb == SPECTRE_V2_USER_NONE && > spectre_v2_user_stibp == SPECTRE_V2_USER_NONE) > return -EPERM; > - if (spectre_v2_user_ibpb == SPECTRE_V2_USER_STRICT || > - spectre_v2_user_stibp == SPECTRE_V2_USER_STRICT || > - spectre_v2_user_stibp == SPECTRE_V2_USER_STRICT_PREFERRED) > + if (!is_spec_ib_user(spectre_v2_user_ibpb) && > + !is_spec_ib_user(spectre_v2_user_stibp)) The set function seems reasonable to me. > return 0; > task_set_spec_ib_disable(task); > if (ctrl == PR_SPEC_FORCE_DISABLE) > @@ -1351,20 +1358,18 @@ static int ib_prctl_get(struct task_struct *task) > if (spectre_v2_user_ibpb == SPECTRE_V2_USER_NONE && > spectre_v2_user_stibp == SPECTRE_V2_USER_NONE) > return PR_SPEC_ENABLE; > - else if (spectre_v2_user_ibpb == SPECTRE_V2_USER_STRICT || > - spectre_v2_user_stibp == SPECTRE_V2_USER_STRICT || > - spectre_v2_user_stibp == SPECTRE_V2_USER_STRICT_PREFERRED) > - return PR_SPEC_DISABLE; > - else if (spectre_v2_user_ibpb == SPECTRE_V2_USER_PRCTL || > - spectre_v2_user_ibpb == SPECTRE_V2_USER_SECCOMP || > - spectre_v2_user_stibp == SPECTRE_V2_USER_PRCTL || > - spectre_v2_user_stibp == SPECTRE_V2_USER_SECCOMP) { > + else if (is_spec_ib_user(spectre_v2_user_ibpb) || > + is_spec_ib_user(spectre_v2_user_stibp)) { > if (task_spec_ib_force_disable(task)) > return PR_SPEC_PRCTL | PR_SPEC_FORCE_DISABLE; > if (task_spec_ib_disable(task)) > return PR_SPEC_PRCTL | PR_SPEC_DISABLE; > return PR_SPEC_PRCTL | PR_SPEC_ENABLE; > - } else > + } else if (spectre_v2_user_ibpb == SPECTRE_V2_USER_STRICT || > + spectre_v2_user_stibp == SPECTRE_V2_USER_STRICT || > + spectre_v2_user_stibp == SPECTRE_V2_USER_STRICT_PREFERRED) > + return PR_SPEC_DISABLE; > + else The get function also seems reasonable. Lets hear what some of the other folks that are familiar with this area have to say. Thanks, Tom > return PR_SPEC_NOT_AFFECTED; > } > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] x86/speculation: Allow IBPB to be conditionally enabled on CPUs with always-on STIBP 2020-10-31 15:05 ` Tom Lendacky @ 2020-11-02 0:02 ` Anand K. Mistry 2020-11-03 10:57 ` Borislav Petkov 0 siblings, 1 reply; 9+ messages in thread From: Anand K. Mistry @ 2020-11-02 0:02 UTC (permalink / raw) To: Tom Lendacky Cc: x86, Joel Fernandes, Anthony Steinhauser, tglx, Borislav Petkov, H. Peter Anvin, Ingo Molnar, Josh Poimboeuf, Mark Gross, Mike Rapoport, Pawan Gupta, Tony Luck, Vineela Tummalapalli, Waiman Long, linux-kernel On Sun, 1 Nov 2020 at 02:05, Tom Lendacky <thomas.lendacky@amd.com> wrote: > > On 10/29/20 1:51 AM, Anand K Mistry wrote: > > On AMD CPUs which have the feature X86_FEATURE_AMD_STIBP_ALWAYS_ON, > > STIBP is set to on and 'spectre_v2_user_stibp == > > SPECTRE_V2_USER_STRICT_PREFERRED'. At the same time, IBPB can be set to > > conditional. However, this leads to the case where it's impossible to > > turn on IBPB for a process because in the PR_SPEC_DISABLE case in > > ib_prctl_set, the (spectre_v2_user_stibp == > > SPECTRE_V2_USER_STRICT_PREFERRED) condition leads to a return before the > > task flag is set. Similarly, ib_prctl_get will return PR_SPEC_DISABLE > > even though IBPB is set to conditional. > > > > More generally, the following cases are possible: > > 1. STIBP = conditional && IBPB = on for spectre_v2_user=seccomp,ibpb > > 2. STIBP = on && IBPB = conditional for AMD CPUs with > > X86_FEATURE_AMD_STIBP_ALWAYS_ON > > > > The first case functions correctly today, but only because > > spectre_v2_user_ibpb isn't updated to reflect the IBPB mode. > > > > At a high level, this change does one thing. If either STIBP is IBPB is > > s/STIBP is IBPB/STIBP or IBPB/ Oops. Will be fixed in v2. > > > set to conditional, allow the prctl to change the task flag. Also, > > reflect that capability when querying the state. This isn't perfect > > since it doesn't take into account if only STIBP or IBPB is > > unconditionally on. But it allows the conditional feature to work as > > expected, without affecting the unconditional one. > > > > Signed-off-by: Anand K Mistry <amistry@google.com> > > > > --- > > > > arch/x86/kernel/cpu/bugs.c | 41 +++++++++++++++++++++----------------- > > 1 file changed, 23 insertions(+), 18 deletions(-) > > > > diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c > > index d3f0db463f96..fb64e02eed6f 100644 > > --- a/arch/x86/kernel/cpu/bugs.c > > +++ b/arch/x86/kernel/cpu/bugs.c > > @@ -1254,6 +1254,11 @@ static int ssb_prctl_set(struct task_struct *task, unsigned long ctrl) > > return 0; > > } > > > > +static bool is_spec_ib_user(enum spectre_v2_user_mitigation mode) > > Maybe something like is_spec_ib_user_controlled() would be a better name. Changed in v2. > > > +{ > > + return mode == SPECTRE_V2_USER_PRCTL || mode == SPECTRE_V2_USER_SECCOMP; > > +} > > + > > I like the idea of passing in the mode you want to check, but it appears > they are never used independently. The ibpb and stibp modes are always > checked together in one of the if statements below, so you could make this > a function that checks both modes and just have a single call. I'll leave > that up to the maintainers to see what is preferred. I can see both sides to this. Personally, I think I prefer it as-is since I think it improves readability a bit by making the conditions less complicated whilst not hiding too many details. I'll wait to see what others say before changing this one. > > > static int ib_prctl_set(struct task_struct *task, unsigned long ctrl) > > { > > switch (ctrl) { > > @@ -1262,13 +1267,16 @@ static int ib_prctl_set(struct task_struct *task, unsigned long ctrl) > > spectre_v2_user_stibp == SPECTRE_V2_USER_NONE) > > return 0; > > /* > > - * Indirect branch speculation is always disabled in strict > > - * mode. It can neither be enabled if it was force-disabled > > - * by a previous prctl call. > > + * With strict mode for both IBPB and STIBP, the instruction > > + * code paths avoid checking this task flag and instead, > > + * unconditionally run the instruction. However, STIBP and IBPB > > + * are independent and either can be set to conditionally > > + * enabled regardless of the mode of the other. If either is set > > + * to conditional, allow the task flag to be updated, unless it > > + * was force-disabled by a previous prctl call. > > You probably want to reference the STIBP always on mode that allows this > situation. Updated comment in v2 to mention the X86_FEATURE_AMD_STIBP_ALWAYS_ON case. > > > */ > > - if (spectre_v2_user_ibpb == SPECTRE_V2_USER_STRICT || > > - spectre_v2_user_stibp == SPECTRE_V2_USER_STRICT || > > - spectre_v2_user_stibp == SPECTRE_V2_USER_STRICT_PREFERRED || > > + if ((!is_spec_ib_user(spectre_v2_user_ibpb) && > > + !is_spec_ib_user(spectre_v2_user_stibp)) || > > task_spec_ib_force_disable(task)) > > return -EPERM; > > task_clear_spec_ib_disable(task); > > @@ -1283,9 +1291,8 @@ static int ib_prctl_set(struct task_struct *task, unsigned long ctrl) > > if (spectre_v2_user_ibpb == SPECTRE_V2_USER_NONE && > > spectre_v2_user_stibp == SPECTRE_V2_USER_NONE) > > return -EPERM; > > - if (spectre_v2_user_ibpb == SPECTRE_V2_USER_STRICT || > > - spectre_v2_user_stibp == SPECTRE_V2_USER_STRICT || > > - spectre_v2_user_stibp == SPECTRE_V2_USER_STRICT_PREFERRED) > > + if (!is_spec_ib_user(spectre_v2_user_ibpb) && > > + !is_spec_ib_user(spectre_v2_user_stibp)) > > The set function seems reasonable to me. > > > return 0; > > task_set_spec_ib_disable(task); > > if (ctrl == PR_SPEC_FORCE_DISABLE) > > @@ -1351,20 +1358,18 @@ static int ib_prctl_get(struct task_struct *task) > > if (spectre_v2_user_ibpb == SPECTRE_V2_USER_NONE && > > spectre_v2_user_stibp == SPECTRE_V2_USER_NONE) > > return PR_SPEC_ENABLE; > > - else if (spectre_v2_user_ibpb == SPECTRE_V2_USER_STRICT || > > - spectre_v2_user_stibp == SPECTRE_V2_USER_STRICT || > > - spectre_v2_user_stibp == SPECTRE_V2_USER_STRICT_PREFERRED) > > - return PR_SPEC_DISABLE; > > - else if (spectre_v2_user_ibpb == SPECTRE_V2_USER_PRCTL || > > - spectre_v2_user_ibpb == SPECTRE_V2_USER_SECCOMP || > > - spectre_v2_user_stibp == SPECTRE_V2_USER_PRCTL || > > - spectre_v2_user_stibp == SPECTRE_V2_USER_SECCOMP) { > > + else if (is_spec_ib_user(spectre_v2_user_ibpb) || > > + is_spec_ib_user(spectre_v2_user_stibp)) { > > if (task_spec_ib_force_disable(task)) > > return PR_SPEC_PRCTL | PR_SPEC_FORCE_DISABLE; > > if (task_spec_ib_disable(task)) > > return PR_SPEC_PRCTL | PR_SPEC_DISABLE; > > return PR_SPEC_PRCTL | PR_SPEC_ENABLE; > > - } else > > + } else if (spectre_v2_user_ibpb == SPECTRE_V2_USER_STRICT || > > + spectre_v2_user_stibp == SPECTRE_V2_USER_STRICT || > > + spectre_v2_user_stibp == SPECTRE_V2_USER_STRICT_PREFERRED) > > + return PR_SPEC_DISABLE; > > + else > > The get function also seems reasonable. > > Lets hear what some of the other folks that are familiar with this area > have to say. > > Thanks, > Tom > > > return PR_SPEC_NOT_AFFECTED; > > } > > > > -- Anand K. Mistry Software Engineer Google Australia ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] x86/speculation: Allow IBPB to be conditionally enabled on CPUs with always-on STIBP 2020-11-02 0:02 ` Anand K. Mistry @ 2020-11-03 10:57 ` Borislav Petkov 2020-11-04 23:31 ` Joel Fernandes 2020-11-05 1:13 ` Anand K. Mistry 0 siblings, 2 replies; 9+ messages in thread From: Borislav Petkov @ 2020-11-03 10:57 UTC (permalink / raw) To: Anand K. Mistry Cc: Tom Lendacky, x86, Joel Fernandes, Anthony Steinhauser, tglx, H. Peter Anvin, Ingo Molnar, Josh Poimboeuf, Mark Gross, Mike Rapoport, Pawan Gupta, Tony Luck, Vineela Tummalapalli, Waiman Long, linux-kernel On Mon, Nov 02, 2020 at 11:02:10AM +1100, Anand K. Mistry wrote: > > I like the idea of passing in the mode you want to check, but it appears > > they are never used independently. The ibpb and stibp modes are always > > checked together in one of the if statements below, so you could make this > > a function that checks both modes and just have a single call. I'll leave > > that up to the maintainers to see what is preferred. > > I can see both sides to this. Personally, I think I prefer it as-is > since I think it improves readability a bit by making the conditions > less complicated whilst not hiding too many details. I'll wait to see > what others say before changing this one. Yes, but if you make it a single function with a descriptive name, you'd make the call sites even more readable: if (!is_spec_ib_conditional(..)) bla; or if (!is_spec_ib_user_controlled(..)) blu; and that function should simply check both spectre_v2_user_ibpb *and* spectre_v2_user_stibp in one go. Why should we do that? Exactly because you both got your brains twisted just from looking at this. Because this mitigation crap is such an ugly and complex maze that we would take even the smallest simplification any day of the week! Welcome to my life since meltdown. Brain twist feels good, doesn't it? :-))) Thx. -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] x86/speculation: Allow IBPB to be conditionally enabled on CPUs with always-on STIBP 2020-11-03 10:57 ` Borislav Petkov @ 2020-11-04 23:31 ` Joel Fernandes 2020-11-05 1:13 ` Anand K. Mistry 1 sibling, 0 replies; 9+ messages in thread From: Joel Fernandes @ 2020-11-04 23:31 UTC (permalink / raw) To: Borislav Petkov Cc: Anand K. Mistry, Tom Lendacky, x86, Anthony Steinhauser, tglx, H. Peter Anvin, Ingo Molnar, Josh Poimboeuf, Mark Gross, Mike Rapoport, Pawan Gupta, Tony Luck, Vineela Tummalapalli, Waiman Long, linux-kernel On Tue, Nov 03, 2020 at 11:57:57AM +0100, Borislav Petkov wrote: > On Mon, Nov 02, 2020 at 11:02:10AM +1100, Anand K. Mistry wrote: > > > I like the idea of passing in the mode you want to check, but it appears > > > they are never used independently. The ibpb and stibp modes are always > > > checked together in one of the if statements below, so you could make this > > > a function that checks both modes and just have a single call. I'll leave > > > that up to the maintainers to see what is preferred. > > > > I can see both sides to this. Personally, I think I prefer it as-is > > since I think it improves readability a bit by making the conditions > > less complicated whilst not hiding too many details. I'll wait to see > > what others say before changing this one. > > Yes, but if you make it a single function with a descriptive name, you'd > make the call sites even more readable: > > if (!is_spec_ib_conditional(..)) > bla; > > or > > if (!is_spec_ib_user_controlled(..)) > blu; > > and that function should simply check both spectre_v2_user_ibpb *and* > spectre_v2_user_stibp in one go. > > Why should we do that? > > Exactly because you both got your brains twisted just from looking at > this. Because this mitigation crap is such an ugly and complex maze that > we would take even the smallest simplification any day of the week! > > Welcome to my life since meltdown. Brain twist feels good, doesn't it? > > :-))) I hate the maze too. In theory we can get rid of STIBP if/when core-scheduling is enabled because the cross-CPU branch predictor poisioning would not be possible. Maybe that will simplify the maze a bit. thanks, - Joel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] x86/speculation: Allow IBPB to be conditionally enabled on CPUs with always-on STIBP 2020-11-03 10:57 ` Borislav Petkov 2020-11-04 23:31 ` Joel Fernandes @ 2020-11-05 1:13 ` Anand K. Mistry 1 sibling, 0 replies; 9+ messages in thread From: Anand K. Mistry @ 2020-11-05 1:13 UTC (permalink / raw) To: Borislav Petkov Cc: Tom Lendacky, x86, Joel Fernandes, Anthony Steinhauser, tglx, H. Peter Anvin, Ingo Molnar, Josh Poimboeuf, Mark Gross, Mike Rapoport, Pawan Gupta, Tony Luck, Vineela Tummalapalli, Waiman Long, linux-kernel On Tue, 3 Nov 2020 at 21:58, Borislav Petkov <bp@alien8.de> wrote: > > On Mon, Nov 02, 2020 at 11:02:10AM +1100, Anand K. Mistry wrote: > > > I like the idea of passing in the mode you want to check, but it appears > > > they are never used independently. The ibpb and stibp modes are always > > > checked together in one of the if statements below, so you could make this > > > a function that checks both modes and just have a single call. I'll leave > > > that up to the maintainers to see what is preferred. > > > > I can see both sides to this. Personally, I think I prefer it as-is > > since I think it improves readability a bit by making the conditions > > less complicated whilst not hiding too many details. I'll wait to see > > what others say before changing this one. > > Yes, but if you make it a single function with a descriptive name, you'd > make the call sites even more readable: > > if (!is_spec_ib_conditional(..)) > bla; > > or > > if (!is_spec_ib_user_controlled(..)) > blu; > > and that function should simply check both spectre_v2_user_ibpb *and* > spectre_v2_user_stibp in one go. > > Why should we do that? > > Exactly because you both got your brains twisted just from looking at > this. Because this mitigation crap is such an ugly and complex maze that > we would take even the smallest simplification any day of the week! Ok then, two votes for. I'll make the change in v2 and post that up today. > > Welcome to my life since meltdown. Brain twist feels good, doesn't it? I don't think "feels good" are the words I'd use. > > :-))) > > Thx. > > -- > Regards/Gruss, > Boris. > > https://people.kernel.org/tglx/notes-about-netiquette -- Anand K. Mistry Software Engineer Google Australia ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 0/1] x86/speculation: Allow IBPB to be conditionally enabled on CPUs with always-on STIBP 2020-10-29 6:51 [PATCH 0/1] x86/speculation: Allow IBPB to be conditionally enabled on CPUs with always-on STIBP Anand K Mistry 2020-10-29 6:51 ` [PATCH 1/1] " Anand K Mistry @ 2020-10-31 14:50 ` Tom Lendacky 2020-11-01 23:57 ` Anand K. Mistry 1 sibling, 1 reply; 9+ messages in thread From: Tom Lendacky @ 2020-10-31 14:50 UTC (permalink / raw) To: Anand K Mistry, x86, linux-kernel Cc: joelaf, asteinhauser, tglx, Borislav Petkov, H. Peter Anvin, Ingo Molnar, Josh Poimboeuf, Mark Gross, Mike Rapoport, Pawan Gupta, Tony Luck, Vineela Tummalapalli, Waiman Long, linux-kernel On 10/29/20 1:51 AM, Anand K Mistry wrote: > When attempting to do some performance testing of IBPB on and AMD > platform, I noticed the IBPB instruction was never being issued, even > though it was conditionally on and various seccomp protected processes > were force enabling it. Turns out, on those AMD CPUs, STIBP is set to > always-on and this was causing an early-out on the prctl() which turns > off IB speculation. Here is my attempt to fix it. > > I'm hoping someone that understands this better than me can explain why > I'm wrong. It all looks reasonable to me (some comments in the patch to follow). The thing that makes this tough is the command line option of being able to force IBPB using the "prctl,ibpb" or "seccomp,ibpb" while STIBP is prctl or seccomp controlled. There's an inherent quality that is assumed that if STIBP is forced then IBPB must be forced and it looks like 21998a351512 ("x86/speculation: Avoid force-disabling IBPB based on STIBP and enhanced IBRS.") used that. However, with the STIBP always on support, that doesn't hold true. Thanks, Tom > > > Anand K Mistry (1): > x86/speculation: Allow IBPB to be conditionally enabled on CPUs with > always-on STIBP > > arch/x86/kernel/cpu/bugs.c | 41 +++++++++++++++++++++----------------- > 1 file changed, 23 insertions(+), 18 deletions(-) > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 0/1] x86/speculation: Allow IBPB to be conditionally enabled on CPUs with always-on STIBP 2020-10-31 14:50 ` [PATCH 0/1] " Tom Lendacky @ 2020-11-01 23:57 ` Anand K. Mistry 0 siblings, 0 replies; 9+ messages in thread From: Anand K. Mistry @ 2020-11-01 23:57 UTC (permalink / raw) To: Tom Lendacky Cc: x86, Joel Fernandes, Anthony Steinhauser, tglx, Borislav Petkov, H. Peter Anvin, Ingo Molnar, Josh Poimboeuf, Mark Gross, Mike Rapoport, Pawan Gupta, Tony Luck, Vineela Tummalapalli, Waiman Long, linux-kernel On Sun, 1 Nov 2020 at 01:50, Tom Lendacky <thomas.lendacky@amd.com> wrote: > > On 10/29/20 1:51 AM, Anand K Mistry wrote: > > When attempting to do some performance testing of IBPB on and AMD > > platform, I noticed the IBPB instruction was never being issued, even > > though it was conditionally on and various seccomp protected processes > > were force enabling it. Turns out, on those AMD CPUs, STIBP is set to > > always-on and this was causing an early-out on the prctl() which turns > > off IB speculation. Here is my attempt to fix it. > > > > I'm hoping someone that understands this better than me can explain why > > I'm wrong. > > It all looks reasonable to me (some comments in the patch to follow). The > thing that makes this tough is the command line option of being able to > force IBPB using the "prctl,ibpb" or "seccomp,ibpb" while STIBP is prctl > or seccomp controlled. There's an inherent quality that is assumed that if > STIBP is forced then IBPB must be forced and it looks like 21998a351512 > ("x86/speculation: Avoid force-disabling IBPB based on STIBP and enhanced > IBRS.") used that. However, with the STIBP always on support, that doesn't > hold true. Yeah, and this is what I found confusing. With that commit, the number of combinations of IBPB and STIBP is 25, but only a small subset is possible. For example: - (spectre_v2_user_ibpb == SPECTRE_V2_USER_STRICT && spectre_v2_user_stibp == SPECTRE_V2_USER_STRICT) - (spectre_v2_user_ibpb == SPECTRE_V2_USER_STRICT && spectre_v2_user_stibp == SPECTRE_V2_USER_NONE) are the only possible combinations of STRICT. But also, if 'spectre_v2_user=seccomp,ibpb' (or prctl,ibpb), then spectre_v2_user_ibpb == SPECTRE_V2_USER_SECCOMP even though it is logically SPECTRE_V2_USER_STRICT. It took a bit of time to wrap my head around this, hence I'm a bit hesitant about this change (even though I think it's right). > > Thanks, > Tom > > > > > > > Anand K Mistry (1): > > x86/speculation: Allow IBPB to be conditionally enabled on CPUs with > > always-on STIBP > > > > arch/x86/kernel/cpu/bugs.c | 41 +++++++++++++++++++++----------------- > > 1 file changed, 23 insertions(+), 18 deletions(-) > > ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-11-05 1:13 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-10-29 6:51 [PATCH 0/1] x86/speculation: Allow IBPB to be conditionally enabled on CPUs with always-on STIBP Anand K Mistry 2020-10-29 6:51 ` [PATCH 1/1] " Anand K Mistry 2020-10-31 15:05 ` Tom Lendacky 2020-11-02 0:02 ` Anand K. Mistry 2020-11-03 10:57 ` Borislav Petkov 2020-11-04 23:31 ` Joel Fernandes 2020-11-05 1:13 ` Anand K. Mistry 2020-10-31 14:50 ` [PATCH 0/1] " Tom Lendacky 2020-11-01 23:57 ` Anand K. Mistry
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).