[PATCH 0/1] x86/speculation: Allow IBPB to be conditionally enabled on CPUs with always-on STIBP

[PATCH 0/1] x86/speculation: Allow IBPB to be conditionally enabled on CPUs with always-on STIBP

[Anand K Mistry]

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

[PATCH 1/1] x86/speculation: Allow IBPB to be conditionally enabled on CPUs with always-on STIBP

[Anand K Mistry]

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

Re: [PATCH 0/1] x86/speculation: Allow IBPB to be conditionally enabled on CPUs with always-on STIBP

[Tom Lendacky]

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(-)
> 

Re: [PATCH 1/1] x86/speculation: Allow IBPB to be conditionally enabled on CPUs with always-on STIBP

[Tom Lendacky]

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;
>   }
>   
> 

Re: [PATCH 0/1] x86/speculation: Allow IBPB to be conditionally enabled on CPUs with always-on STIBP

[Anand K. Mistry]

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(-)
> >

Re: [PATCH 1/1] x86/speculation: Allow IBPB to be conditionally enabled on CPUs with always-on STIBP

[Anand K. Mistry]

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

Re: [PATCH 1/1] x86/speculation: Allow IBPB to be conditionally enabled on CPUs with always-on STIBP

[Borislav Petkov]

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

Re: [PATCH 1/1] x86/speculation: Allow IBPB to be conditionally enabled on CPUs with always-on STIBP

[Joel Fernandes]

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

Re: [PATCH 1/1] x86/speculation: Allow IBPB to be conditionally enabled on CPUs with always-on STIBP

[Anand K. Mistry]

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