Re: [patch] bug in cpuid & msr on nosmp machine

[Rusty Russell <rusty@rustcorp.com.au>]

On Thu, 2004-05-20 at 17:32, Andrew Morton wrote:
> I think what you want here is
> 
> 	if (!cpu_possible(cpu) || !cpu_online(cpu))
> 		return -ENXIO;

It works, but it's not really correct.  cpu_possible() is correct, but
cpu_online() might no longer be true by the time do_cpu_read() calls
do_cpu_id().

One way would be to do lock_cpu_hotplug() in cpuid_open() and introduce
a cpuid_close() which would do unlock_cpu_hotplug().  Another would be
to drop the check here, and fail the actual read.  A final way is to do
no checks, in which case it becomes a noop if the cpu is offline.

> --- 25/arch/i386/kernel/cpuid.c~cpuid-msr-range-checking-fix	2004-05-20 00:30:21.812166544 -0700
> +++ 25-akpm/arch/i386/kernel/cpuid.c	2004-05-20 00:31:16.607836336 -0700
> @@ -133,10 +133,12 @@ static ssize_t cpuid_read(struct file *f
>  static int cpuid_open(struct inode *inode, struct file *file)
>  {
>  	int cpu = iminor(file->f_dentry->d_inode);
> -	struct cpuinfo_x86 *c = &(cpu_data)[cpu];
> +	struct cpuinfo_x86 *c;
>  
> -	if (!cpu_online(cpu))
> +	if (!cpu_possible(cpu) || !cpu_online(cpu))
>  		return -ENXIO;	/* No such CPU */
> +
> +	c = &(cpu_data)[cpu];
>  	if (c->cpuid_level < 0)
>  		return -EIO;	/* CPUID not supported */
>  
> diff -puN arch/i386/kernel/msr.c~cpuid-msr-range-checking-fix arch/i386/kernel/msr.c
> --- 25/arch/i386/kernel/msr.c~cpuid-msr-range-checking-fix	2004-05-20 00:30:21.836162896 -0700
> +++ 25-akpm/arch/i386/kernel/msr.c	2004-05-20 00:31:56.952702984 -0700
> @@ -239,10 +239,12 @@ static ssize_t msr_write(struct file * f
>  static int msr_open(struct inode *inode, struct file *file)
>  {
>    int cpu = iminor(file->f_dentry->d_inode);
> -  struct cpuinfo_x86 *c = &(cpu_data)[cpu];
> -  
> -  if (!cpu_online(cpu))
> -    return -ENXIO;		/* No such CPU */
> +  struct cpuinfo_x86 *c;
> +
> +  if (!cpu_possible(cpu) || !cpu_online(cpu))
> +    return -ENXIO;	/* No such CPU */
> +
> +  c = &(cpu_data)[cpu];
>    if ( !cpu_has(c, X86_FEATURE_MSR) )
>      return -EIO;		/* MSR not supported */
>    
> 
> _
-- 
Anyone who quotes me in their signature is an idiot -- Rusty Russell