1. Patchew
  2. Xen
  3. View series

[PATCH] x86: harden use of calc_ler_msr()

Jan Beulich posted 1 patch 1 year, 11 months ago
Failed in applying to current master (apply log)
[PATCH] x86: harden use of calc_ler_msr()
Posted by Jan Beulich 1 year, 11 months ago 
Avoid calling the function more than once, thus making sure we won't,
under any unusual circumstances, attempt to enable XEN_LER late (which
can't work, for setup_force_cpu_cap() being __init. In turn this then
allows making the function itself __init, too.

While fiddling with section attributes in this area, also move the two
involved variables to .data.ro_after_init.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -126,11 +126,11 @@ DEFINE_PER_CPU_PAGE_ALIGNED(struct tss_p
 static int debug_stack_lines = 20;
 integer_param("debug_stack_lines", debug_stack_lines);
 
-static bool opt_ler;
+static bool __ro_after_init opt_ler;
 boolean_param("ler", opt_ler);
 
 /* LastExceptionFromIP on this hardware.  Zero if LER is not in use. */
-unsigned int __read_mostly ler_msr;
+unsigned int __ro_after_init ler_msr;
 
 const unsigned int nmi_cpu;
 
@@ -2133,7 +2133,7 @@ static void __init set_intr_gate(unsigne
     __set_intr_gate(n, 0, addr);
 }
 
-static unsigned int calc_ler_msr(void)
+static unsigned int noinline __init calc_ler_msr(void)
 {
     switch ( boot_cpu_data.x86_vendor )
     {
@@ -2171,8 +2171,17 @@ void percpu_traps_init(void)
     if ( !opt_ler )
         return;
 
-    if ( !ler_msr && (ler_msr = calc_ler_msr()) )
+    if ( !ler_msr )
+    {
+        ler_msr = calc_ler_msr();
+        if ( !ler_msr )
+        {
+            opt_ler = false;
+            return;
+        }
+
         setup_force_cpu_cap(X86_FEATURE_XEN_LBR);
+    }
 
     if ( cpu_has_xen_lbr )
         wrmsrl(MSR_IA32_DEBUGCTLMSR, IA32_DEBUGCTLMSR_LBR);
Re: [PATCH] x86: harden use of calc_ler_msr()
Posted by Roger Pau Monné 1 year, 11 months ago 
On Mon, May 30, 2022 at 05:48:51PM +0200, Jan Beulich wrote:
> Avoid calling the function more than once, thus making sure we won't,
> under any unusual circumstances, attempt to enable XEN_LER late (which
> can't work, for setup_force_cpu_cap() being __init. In turn this then
> allows making the function itself __init, too.
> 
> While fiddling with section attributes in this area, also move the two
> involved variables to .data.ro_after_init.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -126,11 +126,11 @@ DEFINE_PER_CPU_PAGE_ALIGNED(struct tss_p
>  static int debug_stack_lines = 20;
>  integer_param("debug_stack_lines", debug_stack_lines);
>  
> -static bool opt_ler;
> +static bool __ro_after_init opt_ler;
>  boolean_param("ler", opt_ler);
>  
>  /* LastExceptionFromIP on this hardware.  Zero if LER is not in use. */
> -unsigned int __read_mostly ler_msr;
> +unsigned int __ro_after_init ler_msr;
>  
>  const unsigned int nmi_cpu;
>  
> @@ -2133,7 +2133,7 @@ static void __init set_intr_gate(unsigne
>      __set_intr_gate(n, 0, addr);
>  }
>  
> -static unsigned int calc_ler_msr(void)
> +static unsigned int noinline __init calc_ler_msr(void)
>  {
>      switch ( boot_cpu_data.x86_vendor )
>      {
> @@ -2171,8 +2171,17 @@ void percpu_traps_init(void)
>      if ( !opt_ler )
>          return;
>  
> -    if ( !ler_msr && (ler_msr = calc_ler_msr()) )
> +    if ( !ler_msr )
> +    {
> +        ler_msr = calc_ler_msr();
> +        if ( !ler_msr )
> +        {

While doing this rework it might make sense to print some message
here, like: "LER option requested but no LBR support available" or
similar IMO.

The rest LGTM:

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.
Re: [PATCH] x86: harden use of calc_ler_msr()
Posted by Jan Beulich 1 year, 11 months ago 
On 30.05.2022 17:58, Roger Pau Monné wrote:
> On Mon, May 30, 2022 at 05:48:51PM +0200, Jan Beulich wrote:
>> Avoid calling the function more than once, thus making sure we won't,
>> under any unusual circumstances, attempt to enable XEN_LER late (which
>> can't work, for setup_force_cpu_cap() being __init. In turn this then
>> allows making the function itself __init, too.
>>
>> While fiddling with section attributes in this area, also move the two
>> involved variables to .data.ro_after_init.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> --- a/xen/arch/x86/traps.c
>> +++ b/xen/arch/x86/traps.c
>> @@ -126,11 +126,11 @@ DEFINE_PER_CPU_PAGE_ALIGNED(struct tss_p
>>  static int debug_stack_lines = 20;
>>  integer_param("debug_stack_lines", debug_stack_lines);
>>  
>> -static bool opt_ler;
>> +static bool __ro_after_init opt_ler;
>>  boolean_param("ler", opt_ler);
>>  
>>  /* LastExceptionFromIP on this hardware.  Zero if LER is not in use. */
>> -unsigned int __read_mostly ler_msr;
>> +unsigned int __ro_after_init ler_msr;
>>  
>>  const unsigned int nmi_cpu;
>>  
>> @@ -2133,7 +2133,7 @@ static void __init set_intr_gate(unsigne
>>      __set_intr_gate(n, 0, addr);
>>  }
>>  
>> -static unsigned int calc_ler_msr(void)
>> +static unsigned int noinline __init calc_ler_msr(void)
>>  {
>>      switch ( boot_cpu_data.x86_vendor )
>>      {
>> @@ -2171,8 +2171,17 @@ void percpu_traps_init(void)
>>      if ( !opt_ler )
>>          return;
>>  
>> -    if ( !ler_msr && (ler_msr = calc_ler_msr()) )
>> +    if ( !ler_msr )
>> +    {
>> +        ler_msr = calc_ler_msr();
>> +        if ( !ler_msr )
>> +        {
> 
> While doing this rework it might make sense to print some message
> here, like: "LER option requested but no LBR support available" or
> similar IMO.

Hmm, yes, but you look to do so in your series already. Could we
leave things silent here (as it always was) until your adding of
arch-LBR support, and then taking care of both failure conditions
with a single log message? Of course I could add a message here
just for you to then (likely) alter it again ...

> The rest LGTM:
> 
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, but I'll wait with applying this.

Jan
Re: [PATCH] x86: harden use of calc_ler_msr()
Posted by Roger Pau Monné 1 year, 11 months ago 
On Mon, May 30, 2022 at 06:07:23PM +0200, Jan Beulich wrote:
> On 30.05.2022 17:58, Roger Pau Monné wrote:
> > On Mon, May 30, 2022 at 05:48:51PM +0200, Jan Beulich wrote:
> >> Avoid calling the function more than once, thus making sure we won't,
> >> under any unusual circumstances, attempt to enable XEN_LER late (which
> >> can't work, for setup_force_cpu_cap() being __init. In turn this then
> >> allows making the function itself __init, too.
> >>
> >> While fiddling with section attributes in this area, also move the two
> >> involved variables to .data.ro_after_init.
> >>
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >>
> >> --- a/xen/arch/x86/traps.c
> >> +++ b/xen/arch/x86/traps.c
> >> @@ -126,11 +126,11 @@ DEFINE_PER_CPU_PAGE_ALIGNED(struct tss_p
> >>  static int debug_stack_lines = 20;
> >>  integer_param("debug_stack_lines", debug_stack_lines);
> >>  
> >> -static bool opt_ler;
> >> +static bool __ro_after_init opt_ler;
> >>  boolean_param("ler", opt_ler);
> >>  
> >>  /* LastExceptionFromIP on this hardware.  Zero if LER is not in use. */
> >> -unsigned int __read_mostly ler_msr;
> >> +unsigned int __ro_after_init ler_msr;
> >>  
> >>  const unsigned int nmi_cpu;
> >>  
> >> @@ -2133,7 +2133,7 @@ static void __init set_intr_gate(unsigne
> >>      __set_intr_gate(n, 0, addr);
> >>  }
> >>  
> >> -static unsigned int calc_ler_msr(void)
> >> +static unsigned int noinline __init calc_ler_msr(void)
> >>  {
> >>      switch ( boot_cpu_data.x86_vendor )
> >>      {
> >> @@ -2171,8 +2171,17 @@ void percpu_traps_init(void)
> >>      if ( !opt_ler )
> >>          return;
> >>  
> >> -    if ( !ler_msr && (ler_msr = calc_ler_msr()) )
> >> +    if ( !ler_msr )
> >> +    {
> >> +        ler_msr = calc_ler_msr();
> >> +        if ( !ler_msr )
> >> +        {
> > 
> > While doing this rework it might make sense to print some message
> > here, like: "LER option requested but no LBR support available" or
> > similar IMO.
> 
> Hmm, yes, but you look to do so in your series already. Could we
> leave things silent here (as it always was) until your adding of
> arch-LBR support, and then taking care of both failure conditions
> with a single log message? Of course I could add a message here
> just for you to then (likely) alter it again ...

Oh, so I do introduce one, sorry I didn't remember.  Then it's fine to
go as-is.

Thanks, Roger.

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.