linux-kernel - RE: [PATCH V3 1/2] x86/msr: add msr_set/clear_bit_on

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 28 Mar 2017 17:38:13 +0000
From:   "Liang, Kan" <kan.liang@...el.com>
To:     Thomas Gleixner <tglx@...utronix.de>
CC:     "peterz@...radead.org" <peterz@...radead.org>,
        "mingo@...hat.com" <mingo@...hat.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "bp@...en8.de" <bp@...en8.de>, "acme@...nel.org" <acme@...nel.org>,
        "eranian@...gle.com" <eranian@...gle.com>,
        "jolsa@...nel.org" <jolsa@...nel.org>,
        "ak@...ux.intel.com" <ak@...ux.intel.com>
Subject: RE: [PATCH V3 1/2] x86/msr: add msr_set/clear_bit_on_cpu/cpus
 access functions



.
> > msr_set/clear_bit() are not protected by anyhting. And in your call
> > site this is invoked from fully preemptible context. What protects
> > against context switch and interrupts fiddling with DEBUGMSR?
> 
> And thinking more about that whole interface. It's just overkill.
> 
> diff --git a/arch/x86/lib/msr.c b/arch/x86/lib/msr.c index
> d1dee753b949..35763927adaa 100644
> --- a/arch/x86/lib/msr.c
> +++ b/arch/x86/lib/msr.c
> @@ -58,7 +58,7 @@ int msr_write(u32 msr, struct msr *m)
>  	return wrmsrl_safe(msr, m->q);
>  }
> 
> -static inline int __flip_bit(u32 msr, u8 bit, bool set)
> +int msr_flip_bit(u32 msr, u8 bit, bool set)
>  {
>  	struct msr m, m1;
>  	int err = -EINVAL;
> @@ -85,6 +85,7 @@ static inline int __flip_bit(u32 msr, u8 bit, bool set)
> 
>  	return 1;
>  }
> +EXPORT_SYMBOL_GPL(msr_flip_bit);
> 
>  /**
>   * Set @bit in a MSR @msr.
> @@ -96,7 +97,7 @@ static inline int __flip_bit(u32 msr, u8 bit, bool set)
>   */
>  int msr_set_bit(u32 msr, u8 bit)
>  {
> -	return __flip_bit(msr, bit, true);
> +	return msr_flip_bit(msr, bit, true);
>  }
> 
>  /**
> @@ -109,7 +110,7 @@ int msr_set_bit(u32 msr, u8 bit)
>   */
>  int msr_clear_bit(u32 msr, u8 bit)
>  {
> -	return __flip_bit(msr, bit, false);
> +	return msr_flip_bit(msr, bit, false);
>  }
> 
>  #ifdef CONFIG_TRACEPOINTS
> 
> And in the driver:
> 
> static void flip_smm_bit(void *data)
> {
> 	int val = *(int *)data;
> 
> 	msr_flip_bit(DEBUGMSR, SMMBIT, val);
> }
> 
> And in the write function:
> 
>        smp_call_function(flip_smm_bit, &val, 1);
> 
> That avoids all the extra interfaces and requires less code and less text foot
> print when unused .....
> 

Thanks. It simplify the code very much.
I think we still need to protect the smp_call_function in the driver, right?
Would be the following code enough?

get_online_cpus();
preempt_disable();
smp_call_function(flip_smm_bit, &val, 1);
preempt_enable();
put_online_cpus();

Thanks,
Kan