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

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

[Andi Kleen]

Andrew Morton <akpm@osdl.org> writes:

> Rusty Russell <rusty@rustcorp.com.au> wrote:
>>
>> 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().
>
> mutter.  Are we likely to see any ia32 or x86_64 hotplug-cpu machines?

Yes. Think vmware and virtualization, where partitions could get CPUs 
added and removed at runtime.

-Andi

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

[Rusty Russell]

On Thu, 2004-05-20 at 18:51, Andrew Morton wrote:
> Rusty Russell <rusty@rustcorp.com.au> wrote:
> > 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.
> 
> Too many options for me!

And this would be my preferred approach to the "cpu goes offline" race. 
Two patches in one mail is bad form, but it's late, they're short, and
I'm lazy.

Compile tested.

Rusty.

Name: on_one_cpu function
Author: Rusty Russell
Status: Booted on 2.6.5

Similar to on_each_cpu, this implements on_one_cpu.  For archs which
don't do it natively, it's implemented in terms of smp_call_function().

diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .21074-linux-2.6.5/include/linux/smp.h .21074-linux-2.6.5.updated/include/linux/smp.h
--- .21074-linux-2.6.5/include/linux/smp.h	2004-03-12 07:57:26.000000000 +1100
+++ .21074-linux-2.6.5.updated/include/linux/smp.h	2004-04-05 16:21:20.000000000 +1000
@@ -8,6 +8,10 @@
 
 #include <linux/config.h>
 
+#define get_cpu()		({ preempt_disable(); smp_processor_id(); })
+#define put_cpu()		preempt_enable()
+#define put_cpu_no_resched()	preempt_enable_no_resched()
+
 #ifdef CONFIG_SMP
 
 #include <linux/preempt.h>
@@ -69,6 +73,24 @@ static inline int on_each_cpu(void (*fun
 	return ret;
 }
 
+#ifndef __HAVE_ARCH_ON_ONE_CPU
+extern int __on_one_cpu(unsigned cpu, void (*func)(void *info), void *info);
+static inline int on_one_cpu(unsigned cpu,
+			     void (*func)(void *info), void *info)
+{
+	int ret;
+
+	if (cpu == get_cpu()) {
+		func(info);
+		ret = 0;
+	} else
+		ret = __on_one_cpu(cpu, func, info);
+	put_cpu();
+	return ret;
+}
+#endif
+
+
 /*
  * True once the per process idle is forked
  */
@@ -123,6 +123,7 @@ void smp_prepare_boot_cpu(void);
 #define smp_threads_ready			1
 #define smp_call_function(func,info,retry,wait)	({ 0; })
 #define on_each_cpu(func,info,retry,wait)	({ func(info); 0; })
+#define on_one_cpu(cpu,func,info)		({ func(info); 0; })
 static inline void smp_send_reschedule(int cpu) { }
 #define num_booting_cpus()			1
 #define smp_prepare_boot_cpu()			do {} while (0)
@@ -107,8 +129,4 @@ static inline void smp_send_reschedule(i
 
 #endif /* !SMP */
 
-#define get_cpu()		({ preempt_disable(); smp_processor_id(); })
-#define put_cpu()		preempt_enable()
-#define put_cpu_no_resched()	preempt_enable_no_resched()
-
 #endif /* __LINUX_SMP_H */
diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .21074-linux-2.6.5/kernel/sched.c .21074-linux-2.6.5.updated/kernel/sched.c
--- .21074-linux-2.6.5/kernel/sched.c	2004-04-05 09:04:48.000000000 +1000
+++ .21074-linux-2.6.5.updated/kernel/sched.c	2004-04-05 16:21:45.000000000 +1000
@@ -631,7 +631,29 @@ void kick_process(task_t *p)
 
 EXPORT_SYMBOL_GPL(kick_process);
 
-#endif
+#ifndef __HAVE_ARCH_ON_ONE_CPU
+struct which_cpu
+{
+	unsigned int cpu;
+	void (*func)(void *info);
+	void *info;
+};
+
+static void maybe_on_cpu(void *_which)
+{
+	struct which_cpu *which = _which;
+
+	if (smp_processor_id() == which->cpu)
+		which->func(which->info);
+}
+
+int __on_one_cpu(unsigned cpu, void (*func)(void *info), void *info)
+{
+	struct which_cpu which = { cpu, info };
+	return smp_call_function(maybe_on_cpu, &which, 1, 1);
+}
+#endif /* __HAVE_ARCH_ON_ONE_CPU */
+#endif /* CONFIG_SMP */
 
 /***
  * try_to_wake_up - wake up a thread


Name: Neater returns for i386/x86_64 cpuid/msr on Offline CPUs
Status: Untested
Depends: Misc/i386-cpu_online-BUG.patch.gz
Depends: Misc/on_one_cpu.patch.gz

You can open an MSR or CPUID file on a cpu, and then it can go down.
You'll get junk/noop; nicer to return an error on read/write in this
case.

Use on_one_cpu() as well.  It's just easier to read.

diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .8202-linux-2.6.6-bk6/arch/i386/kernel/cpuid.c .8202-linux-2.6.6-bk6.updated/arch/i386/kernel/cpuid.c
--- .8202-linux-2.6.6-bk6/arch/i386/kernel/cpuid.c	2004-05-20 19:40:03.000000000 +1000
+++ .8202-linux-2.6.6-bk6.updated/arch/i386/kernel/cpuid.c	2004-05-20 19:52:36.000000000 +1000
@@ -42,48 +42,33 @@
 #include <asm/uaccess.h>
 #include <asm/system.h>
 
-#ifdef CONFIG_SMP
-
 struct cpuid_command {
-	int cpu;
+	int err;
 	u32 reg;
 	u32 *data;
 };
 
-static void cpuid_smp_cpuid(void *cmd_block)
+static inline void __do_cpuid(void *cmd_block)
 {
 	struct cpuid_command *cmd = (struct cpuid_command *)cmd_block;
 
-	if (cmd->cpu == smp_processor_id())
-		cpuid(cmd->reg, &cmd->data[0], &cmd->data[1], &cmd->data[2],
-		      &cmd->data[3]);
+	cpuid(cmd->reg, &cmd->data[0], &cmd->data[1], &cmd->data[2],
+	      &cmd->data[3]);
+	cmd->err = 0;
 }
 
-static inline void do_cpuid(int cpu, u32 reg, u32 * data)
+static inline int do_cpuid(int cpu, u32 reg, u32 * data)
 {
 	struct cpuid_command cmd;
 
-	preempt_disable();
-	if (cpu == smp_processor_id()) {
-		cpuid(reg, &data[0], &data[1], &data[2], &data[3]);
-	} else {
-		cmd.cpu = cpu;
-		cmd.reg = reg;
-		cmd.data = data;
-
-		smp_call_function(cpuid_smp_cpuid, &cmd, 1, 1);
-	}
-	preempt_enable();
-}
-#else				/* ! CONFIG_SMP */
+	cmd.reg = reg;
+	cmd.data = data;
+	cmd.err = -ENOENT;
 
-static inline void do_cpuid(int cpu, u32 reg, u32 * data)
-{
-	cpuid(reg, &data[0], &data[1], &data[2], &data[3]);
+	on_one_cpu(cpu, __do_cpuid, &cmd);
+	return cmd.err;
 }
 
-#endif				/* ! CONFIG_SMP */
-
 static loff_t cpuid_seek(struct file *file, loff_t offset, int orig)
 {
 	loff_t ret;
@@ -114,13 +99,15 @@ static ssize_t cpuid_read(struct file *f
 	u32 data[4];
 	size_t rv;
 	u32 reg = *ppos;
-	int cpu = iminor(file->f_dentry->d_inode);
+	int err, cpu = iminor(file->f_dentry->d_inode);
 
 	if (count % 16)
 		return -EINVAL;	/* Invalid chunk size */
 
 	for (rv = 0; count; count -= 16) {
-		do_cpuid(cpu, reg, data);
+		err = do_cpuid(cpu, reg, data);
+		if (err)
+			return err;
 		if (copy_to_user(tmp, &data, 16))
 			return -EFAULT;
 		tmp += 4;
diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .8202-linux-2.6.6-bk6/arch/i386/kernel/msr.c .8202-linux-2.6.6-bk6.updated/arch/i386/kernel/msr.c
--- .8202-linux-2.6.6-bk6/arch/i386/kernel/msr.c	2004-05-20 19:40:03.000000000 +1000
+++ .8202-linux-2.6.6-bk6.updated/arch/i386/kernel/msr.c	2004-05-20 19:52:49.000000000 +1000
@@ -86,89 +86,53 @@ static inline int rdmsr_eio(u32 reg, u32
   return err;
 }
 
-#ifdef CONFIG_SMP
-
 struct msr_command {
-  int cpu;
   int err;
   u32 reg;
   u32 data[2];
 };
 
-static void msr_smp_wrmsr(void *cmd_block)
+static inline void __msr_wrmsr(void *cmd_block)
 {
   struct msr_command *cmd = (struct msr_command *) cmd_block;
   
-  if ( cmd->cpu == smp_processor_id() )
-    cmd->err = wrmsr_eio(cmd->reg, cmd->data[0], cmd->data[1]);
+  cmd->err = wrmsr_eio(cmd->reg, cmd->data[0], cmd->data[1]);
 }
 
-static void msr_smp_rdmsr(void *cmd_block)
+static inline void __msr_rdmsr(void *cmd_block)
 {
   struct msr_command *cmd = (struct msr_command *) cmd_block;
   
-  if ( cmd->cpu == smp_processor_id() )
-    cmd->err = rdmsr_eio(cmd->reg, &cmd->data[0], &cmd->data[1]);
+  cmd->err = rdmsr_eio(cmd->reg, &cmd->data[0], &cmd->data[1]);
 }
 
 static inline int do_wrmsr(int cpu, u32 reg, u32 eax, u32 edx)
 {
   struct msr_command cmd;
-  int ret;
 
-  preempt_disable();
-  if ( cpu == smp_processor_id() ) {
-    ret = wrmsr_eio(reg, eax, edx);
-  } else {
-    cmd.cpu = cpu;
-    cmd.reg = reg;
-    cmd.data[0] = eax;
-    cmd.data[1] = edx;
-    
-    smp_call_function(msr_smp_wrmsr, &cmd, 1, 1);
-    ret = cmd.err;
-  }
-  preempt_enable();
-  return ret;
+  cmd.reg = reg;
+  cmd.data[0] = eax;
+  cmd.data[1] = edx;
+  cmd.err = -ENOENT;
+
+  on_one_cpu(cpu, __msr_wrmsr, &cmd);
+  return cmd.err;
 }
 
 static inline int do_rdmsr(int cpu, u32 reg, u32 *eax, u32 *edx)
 {
   struct msr_command cmd;
-  int ret;
-
-  preempt_disable();
-  if ( cpu == smp_processor_id() ) {
-    ret = rdmsr_eio(reg, eax, edx);
-  } else {
-    cmd.cpu = cpu;
-    cmd.reg = reg;
-
-    smp_call_function(msr_smp_rdmsr, &cmd, 1, 1);
-    
-    *eax = cmd.data[0];
-    *edx = cmd.data[1];
 
-    ret = cmd.err;
-  }
-  preempt_enable();
-  return ret;
-}
-
-#else /* ! CONFIG_SMP */
+  cmd.reg = reg;
+  cmd.err = -ENOENT;
 
-static inline int do_wrmsr(int cpu, u32 reg, u32 eax, u32 edx)
-{
-  return wrmsr_eio(reg, eax, edx);
-}
+  on_one_cpu(cpu, __msr_rdmsr, &cmd);
 
-static inline int do_rdmsr(int cpu, u32 reg, u32 *eax, u32 *edx)
-{
-  return rdmsr_eio(reg, eax, edx);
+  *eax = cmd.data[0];
+  *edx = cmd.data[1];
+  return cmd.err;
 }
 
-#endif /* ! CONFIG_SMP */
-
 static loff_t msr_seek(struct file *file, loff_t offset, int orig)
 {
   loff_t ret = -EINVAL;


-- 
Anyone who quotes me in their signature is an idiot -- Rusty Russell

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

[Paul Jackson]

Jeff wrote:
> but on nosmp machine if the cpu!=0 this procude a BUG();

The line you refer to here is, I believe, in include/linux/cpumask.h:

  #ifdef CONFIG_SMP
    ...
  #else
    ...
  #define cpu_online(cpu)    ({ BUG_ON((cpu) != 0); 1; })
    ...
  #endif

Any thoughts on whether that BUG() is serving any useful purpose?

I'm of a mind to have the non-SMP case of cpu_online(cpu) simply
respond true (1) hardcoded:

  #define cpu_online(cpu)    (1)

Or at least remove the BUG and have it respond true iff cpu == 0:

  #define cpu_online(cpu)    ((cpu) == 0)

While theoretically correct, that BUG() is, so far as I know, of
no redeeming social value.  And it wastes a few bytes of kernel
text space each time cpu_online() is used on an SMP, and causes
the occassional confusion, as in this case.

-- 
                          I won't rest till it's the best ...
                          Programmer, Linux Scalability
                          Paul Jackson <pj@sgi.com> 1.650.933.1373

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

[Rusty Russell]

On Thu, 2004-05-20 at 18:51, Andrew Morton wrote:
> How about this?

Complicated *and* doesn't solve the original problem.

How about this?

Name: Fix i386/x86_64 cpuid/msr BUG() on Impossible CPUs
Status: Trivial

Matthieu Castet <castet.matthieu@free.fr> pointed out that testing
cpu_online(cpu) on a UP system goes BUG().

That's because you're never supposed to ask cpu_online() about a CPU
which is >= NR_CPUS.  msr and cpuid devices use the minor to indicate
the CPU number.  Oops.

Fix is to explicitly test cpu < NR_CPUS.  Using cpu_online() is OK;
although the CPU might go down before you actually read the file, that
will simply cause junk to be returned.

diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .7878-linux-2.6.6-bk6/arch/i386/kernel/cpuid.c .7878-linux-2.6.6-bk6.updated/arch/i386/kernel/cpuid.c
--- .7878-linux-2.6.6-bk6/arch/i386/kernel/cpuid.c	2004-05-20 10:35:22.000000000 +1000
+++ .7878-linux-2.6.6-bk6.updated/arch/i386/kernel/cpuid.c	2004-05-20 19:26:47.000000000 +1000
@@ -132,10 +132,10 @@ 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);
+	unsigned int cpu = iminor(file->f_dentry->d_inode);
 	struct cpuinfo_x86 *c = &(cpu_data)[cpu];
 
-	if (!cpu_online(cpu))
+	if (cpu >= NR_CPUS || !cpu_online(cpu))
 		return -ENXIO;	/* No such CPU */
 	if (c->cpuid_level < 0)
 		return -EIO;	/* CPUID not supported */
diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .7878-linux-2.6.6-bk6/arch/i386/kernel/msr.c .7878-linux-2.6.6-bk6.updated/arch/i386/kernel/msr.c
--- .7878-linux-2.6.6-bk6/arch/i386/kernel/msr.c	2004-02-18 23:54:12.000000000 +1100
+++ .7878-linux-2.6.6-bk6.updated/arch/i386/kernel/msr.c	2004-05-20 19:27:01.000000000 +1000
@@ -238,10 +238,10 @@ 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);
+  unsigned int cpu = iminor(file->f_dentry->d_inode);
   struct cpuinfo_x86 *c = &(cpu_data)[cpu];
   
-  if (!cpu_online(cpu))
+  if (cpu >= NR_CPUS || !cpu_online(cpu))
     return -ENXIO;		/* No such 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

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

[Andrew Morton]

Rusty Russell <rusty@rustcorp.com.au> wrote:
>
> 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().

mutter.  Are we likely to see any ia32 or x86_64 hotplug-cpu machines?

> 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.

Too many options for me!

Doing lock_cpu_hotplug() in open() would allow an app to take that lock
permanently.

How about this?

 25-akpm/arch/i386/kernel/cpuid.c |   38 +++++++++++++++-----
 25-akpm/arch/i386/kernel/msr.c   |   74 ++++++++++++++++++++++++++++-----------
 2 files changed, 85 insertions(+), 27 deletions(-)

diff -puN arch/i386/kernel/cpuid.c~cpuid-msr-range-checking-fix arch/i386/kernel/cpuid.c
--- 25/arch/i386/kernel/cpuid.c~cpuid-msr-range-checking-fix	2004-05-20 01:51:19.395701600 -0700
+++ 25-akpm/arch/i386/kernel/cpuid.c	2004-05-20 01:51:19.400700840 -0700
@@ -115,9 +115,19 @@ static ssize_t cpuid_read(struct file *f
 	size_t rv;
 	u32 reg = *ppos;
 	int cpu = iminor(file->f_dentry->d_inode);
+	ssize_t ret = 0;
 
-	if (count % 16)
-		return -EINVAL;	/* Invalid chunk size */
+	lock_cpu_hotplug();
+
+	if (!cpu_possible(cpu) || !cpu_online(cpu)) {
+		ret = -ENXIO;	/* No such CPU */
+		goto out;
+	}
+
+	if (count % 16) {
+		ret = -EINVAL;	/* Invalid chunk size */
+		goto out;
+	}
 
 	for (rv = 0; count; count -= 16) {
 		do_cpuid(cpu, reg, data);
@@ -127,20 +137,32 @@ static ssize_t cpuid_read(struct file *f
 		*ppos = reg++;
 	}
 
-	return ((char *)tmp) - buf;
+	ret = ((char *)tmp) - buf;
+out:
+	lock_cpu_hotplug();
+	return ret;
 }
 
 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;
+	int ret = 0;
 
-	if (!cpu_online(cpu))
-		return -ENXIO;	/* No such CPU */
+	lock_cpu_hotplug();
+
+	if (!cpu_possible(cpu) || !cpu_online(cpu)) {
+		ret = -ENXIO;	/* No such CPU */
+		goot out;
+	}
+
+	c = &(cpu_data)[cpu];
 	if (c->cpuid_level < 0)
-		return -EIO;	/* CPUID not supported */
+		ret = -EIO;	/* CPUID not supported */
 
-	return 0;
+out:
+	unlock_cpu_hotplug();
+	return ret;
 }
 
 /*
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 01:51:19.396701448 -0700
+++ 25-akpm/arch/i386/kernel/msr.c	2004-05-20 01:51:19.401700688 -0700
@@ -195,20 +195,37 @@ static ssize_t msr_read(struct file * fi
   u32 reg = *ppos;
   int cpu = iminor(file->f_dentry->d_inode);
   int err;
+  ssize_t ret = 0;
 
-  if ( count % 8 )
-    return -EINVAL; /* Invalid chunk size */
+  lock_cpu_hotplug();
+
+  if (!cpu_possible(cpu) || !cpu_online(cpu)) {
+    ret = -ENXIO;	/* No such CPU */
+    goto out;
+  }
+
+  if ( count % 8 ) {
+    ret = -EINVAL; /* Invalid chunk size */
+    goto out;
+  }
   
   for ( rv = 0 ; count ; count -= 8 ) {
     err = do_rdmsr(cpu, reg, &data[0], &data[1]);
-    if ( err )
-      return err;
-    if ( copy_to_user(tmp,&data,8) )
-      return -EFAULT;
+    if ( err ) {
+      ret = err;
+      goto out;
+    }
+    if ( copy_to_user(tmp,&data,8) ) {
+      ret = -EFAULT;
+      goto out;
+    }
     tmp += 2;
   }
 
-  return ((char *)tmp) - buf;
+  ret = ((char *)tmp) - buf;
+out:
+  unlock_cpu_hotplug();
+  return ret;
 }
 
 static ssize_t msr_write(struct file * file, const char __user * buf,
@@ -220,29 +237,48 @@ static ssize_t msr_write(struct file * f
   u32 reg = *ppos;
   int cpu = iminor(file->f_dentry->d_inode);
   int err;
+  int ret = 0;
+
+  lock_cpu_hotplug();
+
+  if (!cpu_possible(cpu) || !cpu_online(cpu)) {
+    ret = -ENXIO;	/* No such CPU */
+    goto out;
+  }
+
+  if ( count % 8 ) {
+    ret = -EINVAL; /* Invalid chunk size */
+    goto out;
+  }
 
-  if ( count % 8 )
-    return -EINVAL; /* Invalid chunk size */
-  
   for ( rv = 0 ; count ; count -= 8 ) {
-    if ( copy_from_user(&data,tmp,8) )
-      return -EFAULT;
+    if ( copy_from_user(&data,tmp,8) ) {
+      ret = -EFAULT;
+      goto out;
+    }
     err = do_wrmsr(cpu, reg, data[0], data[1]);
-    if ( err )
-      return err;
+    if ( err ) {
+      ret = err;
+      goto out;
+    }
     tmp += 2;
   }
 
-  return ((char *)tmp) - buf;
+  ret = ((char *)tmp) - buf;
+out:
+  unlock_cpu_hotplug();
+  return ret;
 }
 
 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 */
   

_

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

[Rusty Russell]

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

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

[Andrew Morton]

matthieu castet <castet.matthieu@free.fr> wrote:
>
> on monocpu machine (and maybe even on smp machine), when you try to 
>  acces to a cpu that don't exist (/dev/cpu/1/cpuid), cpuid (msr) call 
>  cpu_online, but on nosmp machine if the cpu!=0 this procude a BUG();
>  So I add a check that verify if the cpu can exist before calling cpu_online.
> 
>  Matthieu CASTET
> 
> 
> [cpuid.patch  text/x-patch (589 bytes)]
>  --- linux/arch/i386/kernel/cpuid.c	2004-05-18 20:47:05.000000000 +0200
>  +++ linux-2.6.5/arch/i386/kernel/cpuid.c	2004-04-04 05:36:12.000000000 +0200
>  @@ -135,7 +135,7 @@
>     int cpu = iminor(file->f_dentry->d_inode);
>     struct cpuinfo_x86 *c = &(cpu_data)[cpu];
>   
>  -  if (cpu >= num_possible_cpus() || !cpu_online(cpu))
>  +  if (!cpu_online(cpu))
>       return -ENXIO;		/* No such CPU */
>     if ( c->cpuid_level < 0 )
>       return -EIO;		/* CPUID not supported */
> 

I think what you want here is

	if (!cpu_possible(cpu) || !cpu_online(cpu))
		return -ENXIO;

Like the below.  Rusty agree?


--- 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 */
   

_

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

[Jeff Sipek]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On Wednesday 19 May 2004 12:35, matthieu castet wrote:
> hi,
>
> on monocpu machine (and maybe even on smp machine), when you try to
> acces to a cpu that don't exist (/dev/cpu/1/cpuid), cpuid (msr) call
> cpu_online, but on nosmp machine if the cpu!=0 this procude a BUG();
> So I add a check that verify if the cpu can exist before calling
> cpu_online.

Added a check? I think your patch is reversed.

Jeff.
 
- -  if (cpu >= num_possible_cpus() || !cpu_online(cpu))
+  if (!cpu_online(cpu))
     return -ENXIO;             /* No such CPU */

- -- 
Hegh QaQ law'
quvHa'ghach QaQ puS
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.4 (GNU/Linux)

iD8DBQFArBuwwFP0+seVj/4RArcUAJwNIuZ6RhDYaHfnLIjKkiyXrXjfgwCgytvY
hpqUVuBbNwragYDMkjNOm4w=
=8DZk
-----END PGP SIGNATURE-----

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

[matthieu castet]

[-- Attachment #1: Type: text/plain, Size: 379 bytes --]

matthieu castet wrote:
> hi,
> 
> on monocpu machine (and maybe even on smp machine), when you try to 
> acces to a cpu that don't exist (/dev/cpu/1/cpuid), cpuid (msr) call 
> cpu_online, but on nosmp machine if the cpu!=0 this procude a BUG();
> So I add a check that verify if the cpu can exist before calling 
> cpu_online.
> 
> Matthieu CASTET
oups i send the wrong patch



[-- Attachment #2: cpuid.patch --]
[-- Type: text/x-patch, Size: 454 bytes --]

--- linux-2.6.5/arch/i386/kernel/cpuid.c	2004-04-04 05:36:12.000000000 +0200
+++ linux/arch/i386/kernel/cpuid.c	2004-05-18 20:47:05.000000000 +0200
@@ -135,7 +135,7 @@
   int cpu = iminor(file->f_dentry->d_inode);
   struct cpuinfo_x86 *c = &(cpu_data)[cpu];
 
-  if (!cpu_online(cpu))
+  if (cpu >= num_possible_cpus() || !cpu_online(cpu))
     return -ENXIO;		/* No such CPU */
   if ( c->cpuid_level < 0 )
     return -EIO;		/* CPUID not supported */

[-- Attachment #3: msr.patch --]
[-- Type: text/x-patch, Size: 460 bytes --]

--- linux-2.6.5/arch/i386/kernel/msr.c	2004-04-04 05:36:57.000000000 +0200
+++ linux/arch/i386/kernel/msr.c	2004-05-19 18:25:09.000000000 +0200
@@ -241,7 +241,7 @@
   int cpu = iminor(file->f_dentry->d_inode);
   struct cpuinfo_x86 *c = &(cpu_data)[cpu];
   
-  if (!cpu_online(cpu))
+  if (cpu >= num_possible_cpus() || !cpu_online(cpu))
     return -ENXIO;		/* No such CPU */
   if ( !cpu_has(c, X86_FEATURE_MSR) )
     return -EIO;		/* MSR not supported */

[patch] bug in cpuid & msr on nosmp machine

[matthieu castet]

[-- Attachment #1: Type: text/plain, Size: 307 bytes --]

hi,

on monocpu machine (and maybe even on smp machine), when you try to 
acces to a cpu that don't exist (/dev/cpu/1/cpuid), cpuid (msr) call 
cpu_online, but on nosmp machine if the cpu!=0 this procude a BUG();
So I add a check that verify if the cpu can exist before calling cpu_online.

Matthieu CASTET

[-- Attachment #2: cpuid.patch --]
[-- Type: text/x-patch, Size: 454 bytes --]

--- linux/arch/i386/kernel/cpuid.c	2004-05-18 20:47:05.000000000 +0200
+++ linux-2.6.5/arch/i386/kernel/cpuid.c	2004-04-04 05:36:12.000000000 +0200
@@ -135,7 +135,7 @@
   int cpu = iminor(file->f_dentry->d_inode);
   struct cpuinfo_x86 *c = &(cpu_data)[cpu];
 
-  if (cpu >= num_possible_cpus() || !cpu_online(cpu))
+  if (!cpu_online(cpu))
     return -ENXIO;		/* No such CPU */
   if ( c->cpuid_level < 0 )
     return -EIO;		/* CPUID not supported */

[-- Attachment #3: msr.patch --]
[-- Type: text/x-patch, Size: 460 bytes --]

--- linux/arch/i386/kernel/msr.c	2004-05-19 18:25:09.000000000 +0200
+++ linux-2.6.5/arch/i386/kernel/msr.c	2004-04-04 05:36:57.000000000 +0200
@@ -241,7 +241,7 @@
   int cpu = iminor(file->f_dentry->d_inode);
   struct cpuinfo_x86 *c = &(cpu_data)[cpu];
   
-  if (cpu >= num_possible_cpus() || !cpu_online(cpu))
+  if (!cpu_online(cpu))
     return -ENXIO;		/* No such CPU */
   if ( !cpu_has(c, X86_FEATURE_MSR) )
     return -EIO;		/* MSR not supported */