[Xenomai-core] [PATCH] Check frequency in xnarch_init_timeconv()

[Xenomai-core] [PATCH] Check frequency in xnarch_init_timeconv()

[Bernhard Walle]

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

In some conditions, while testing Xenomai in VirtualBox, I had the error that
xnarch_init_timeconv() is called from init_32.h with a frequency of 0. That
leads to a division by zero, followed by a system oops.

Of course that is a bug in the virtualisation and Linux reports a CPU frequency
of 0 in /proc/cpuinfo. However, there's no harm if Xenomai checks that instead
of crashing the whole system.


Signed-off-by: Bernhard Walle <bernhard@domain.hid>


 include/asm-arm/bits/init.h         |   4 +++-
 include/asm-blackfin/bits/init.h    |   4 +++-
 include/asm-generic/bits/timeconv.h |  18 +++++++++++++++++-
 include/asm-nios2/bits/init.h       |   4 +++-
 include/asm-powerpc/bits/init.h     |   4 +++-
 include/asm-x86/bits/init_32.h      |   4 +++-
 include/asm-x86/bits/init_64.h      |   4 +++-
 src/skins/native/timer.c            |   7 ++++++-
 src/skins/posix/clock.c             |   8 +++++++-
 9 files changed, 48 insertions(+), 9 deletions(-)



[-- Attachment #2: warn-about-broken-tsc.diff --]
[-- Type: text/x-patch, Size: 4757 bytes --]

In some conditions, while testing Xenomai in VirtualBox, I had the error that
xnarch_init_timeconv() is called from init_32.h with a frequency of 0. That
leads to a division by zero, followed by a system oops.

Of course that is a bug in the virtualisation and Linux reports a CPU frequency
of 0 in /proc/cpuinfo. However, there's no harm if Xenomai checks that instead
of crashing the whole system.


Signed-off-by: Bernhard Walle <bernhard@domain.hid>

diff --git a/include/asm-arm/bits/init.h b/include/asm-arm/bits/init.h
--- a/include/asm-arm/bits/init.h
+++ b/include/asm-arm/bits/init.h
@@ -82,7 +82,9 @@
 	set_cpus_allowed(current, cpumask_of_cpu(0));
 #endif /* CONFIG_SMP && MODULE */
 
-	xnarch_init_timeconv(RTHAL_CPU_FREQ);
+	ret = xnarch_init_timeconv(RTHAL_CPU_FREQ);
+	if (ret)
+		return ret;
 
 	ret = xnarch_calibrate_sched();
 	if (ret)
diff --git a/include/asm-blackfin/bits/init.h b/include/asm-blackfin/bits/init.h
--- a/include/asm-blackfin/bits/init.h
+++ b/include/asm-blackfin/bits/init.h
@@ -83,7 +83,9 @@
 	set_cpus_allowed(current, cpumask_of_cpu(0));
 #endif /* CONFIG_SMP && MODULE */
 
-	xnarch_init_timeconv(RTHAL_CPU_FREQ);
+	ret = xnarch_init_timeconv(RTHAL_CPU_FREQ);
+	if (ret)
+		return ret;
 
 	ret = xnarch_calibrate_sched();
 	if (ret)
diff --git a/include/asm-generic/bits/timeconv.h b/include/asm-generic/bits/timeconv.h
--- a/include/asm-generic/bits/timeconv.h
+++ b/include/asm-generic/bits/timeconv.h
@@ -100,8 +100,23 @@
 }
 #endif /* !XNARCH_HAVE_NODIV_LLIMD */
 
-static inline void xnarch_init_timeconv(unsigned long long freq)
+#ifndef xnarch_logerr
+#define xnarch_logerr(fmt, args...)
+#endif
+
+static inline int xnarch_init_timeconv(unsigned long long freq)
 {
+	/*
+	 * Division by zero is not defined (and in kernel context, it
+	 * especially leads to an oops)
+	 */
+	if (freq == 0) {
+		xnarch_logerr("xnarch_init_timeconv called with a "
+			      "frequency of 0. Probably this indicates a "
+			      "broken TSC.");
+		return -EINVAL;
+	}
+
 	clockfreq = freq;
 #ifdef XNARCH_HAVE_LLMULSHFT
 	xnarch_init_llmulshft(1000000000, freq, &tsc_scale, &tsc_shift);
@@ -110,6 +125,7 @@
 	xnarch_init_u32frac(&bln_frac, 1, 1000000000);
 #endif
 #endif
+	return 0;
 }
 
 #ifdef __KERNEL__
diff --git a/include/asm-nios2/bits/init.h b/include/asm-nios2/bits/init.h
--- a/include/asm-nios2/bits/init.h
+++ b/include/asm-nios2/bits/init.h
@@ -72,7 +72,9 @@
 	if (ret)
 		return ret;
 
-	xnarch_init_timeconv(RTHAL_CLOCK_FREQ);
+	ret = xnarch_init_timeconv(RTHAL_CLOCK_FREQ);
+	if (ret)
+		return ret;
 
 	ret = xnarch_calibrate_sched();
 	if (ret)
diff --git a/include/asm-powerpc/bits/init.h b/include/asm-powerpc/bits/init.h
--- a/include/asm-powerpc/bits/init.h
+++ b/include/asm-powerpc/bits/init.h
@@ -82,7 +82,9 @@
 	set_cpus_allowed(current, cpumask_of_cpu(0));
 #endif /* CONFIG_SMP && MODULE */
 
-	xnarch_init_timeconv(RTHAL_CPU_FREQ);
+	ret = xnarch_init_timeconv(RTHAL_CPU_FREQ);
+	if (ret)
+		return ret;
 
 	ret = xnarch_calibrate_sched();
 	if (ret)
diff --git a/include/asm-x86/bits/init_32.h b/include/asm-x86/bits/init_32.h
--- a/include/asm-x86/bits/init_32.h
+++ b/include/asm-x86/bits/init_32.h
@@ -88,7 +88,9 @@
 	set_cpus_allowed(current, cpumask_of_cpu(0));
 #endif /* CONFIG_SMP && MODULE */
 
-	xnarch_init_timeconv(RTHAL_CPU_FREQ);
+	ret = xnarch_init_timeconv(RTHAL_CPU_FREQ);
+	if (ret)
+		return ret;
 
 	ret = xnarch_calibrate_sched();
 	if (ret)
diff --git a/include/asm-x86/bits/init_64.h b/include/asm-x86/bits/init_64.h
--- a/include/asm-x86/bits/init_64.h
+++ b/include/asm-x86/bits/init_64.h
@@ -85,7 +85,9 @@
 	set_cpus_allowed(current, cpumask_of_cpu(0));
 #endif /* CONFIG_SMP && MODULE */
 
-	xnarch_init_timeconv(RTHAL_CPU_FREQ);
+	ret = xnarch_init_timeconv(RTHAL_CPU_FREQ);
+	if (ret)
+		return ret;
 
 	ret = xnarch_calibrate_sched();
 	if (ret)
diff --git a/src/skins/native/timer.c b/src/skins/native/timer.c
--- a/src/skins/native/timer.c
+++ b/src/skins/native/timer.c
@@ -35,7 +35,12 @@
 		exit(EXIT_FAILURE);
 	}
 
-	xnarch_init_timeconv(sysinfo.cpufreq);
+	err = xnarch_init_timeconv(sysinfo.cpufreq);
+	if (err) {
+		fprintf(stderr, "Native skin init: "
+			"xnarch_init_timeconv: %s\n", strerror(-err));
+		exit(EXIT_FAILURE);
+	}
 }
 
 int rt_timer_set_mode(RTIME tickval)
diff --git a/src/skins/posix/clock.c b/src/skins/posix/clock.c
--- a/src/skins/posix/clock.c
+++ b/src/skins/posix/clock.c
@@ -38,7 +38,13 @@
 			"sys_info: %s\n", strerror(err));
 		exit(EXIT_FAILURE);
 	}
-	xnarch_init_timeconv(sysinfo.cpufreq);
+
+	err = xnarch_init_timeconv(sysinfo.cpufreq);
+	if (err) {
+		fprintf(stderr, "Xenomai Posix skin init: "
+			"xnarch_init_timeconv: %s\n", strerror(err));
+		exit(EXIT_FAILURE);
+	}
 }
 #endif /* XNARCH_HAVE_NONPRIV_TSC */
 

Re: [Xenomai-core] [PATCH] Check frequency in xnarch_init_timeconv()

[Philippe Gerum]

On Sun, 2009-11-01 at 19:58 +0000, Bernhard Walle wrote:
> In some conditions, while testing Xenomai in VirtualBox, I had the error that
> xnarch_init_timeconv() is called from init_32.h with a frequency of 0. That
> leads to a division by zero, followed by a system oops.
> 
> Of course that is a bug in the virtualisation and Linux reports a CPU frequency
> of 0 in /proc/cpuinfo. However, there's no harm if Xenomai checks that instead
> of crashing the whole system.
> 
> 

I agree with the conclusion, not with the fix.

- the userland part reuses the frequency value returned by the nucleus,
so testing the issue up front when the nucleus initializes would be
enough. No need to propagate error detection to userland.

- the kernel side should rather test RTHAL_CPU_FREQ for consistency in
the generic hal init code, and bail out with an error if 0 is detected,
since there is no way for the nucleus to operate properly with such
setting anyway.

It turns out that no change have to be done in xnarch_init_timeconv()
which should remain a void routine, but its argument - RTHAL_CPU_FREQ -
should rather be tested as early as possible for consistency, directly
from kernel space.

> Signed-off-by: Bernhard Walle <bernhard@domain.hid>
> 
> 
>  include/asm-arm/bits/init.h         |   4 +++-
>  include/asm-blackfin/bits/init.h    |   4 +++-
>  include/asm-generic/bits/timeconv.h |  18 +++++++++++++++++-
>  include/asm-nios2/bits/init.h       |   4 +++-
>  include/asm-powerpc/bits/init.h     |   4 +++-
>  include/asm-x86/bits/init_32.h      |   4 +++-
>  include/asm-x86/bits/init_64.h      |   4 +++-
>  src/skins/native/timer.c            |   7 ++++++-
>  src/skins/posix/clock.c             |   8 +++++++-
>  9 files changed, 48 insertions(+), 9 deletions(-)
> 
> 
> _______________________________________________
> Xenomai-core mailing list
> Xenomai-core@domain.hid
> https://mail.gna.org/listinfo/xenomai-core


-- 
Philippe.

Re: [Xenomai-core] [PATCH] Check frequency in xnarch_init_timeconv()

[Bernhard Walle]

Hi,

Philippe Gerum schrieb:
> 
> I agree with the conclusion, not with the fix.

Thanks for reviewing the patch and sorry for the late answer.

> - the kernel side should rather test RTHAL_CPU_FREQ for consistency in
> the generic hal init code, and bail out with an error if 0 is detected,
> since there is no way for the nucleus to operate properly with such
> setting anyway.
> 
> It turns out that no change have to be done in xnarch_init_timeconv()
> which should remain a void routine, but its argument - RTHAL_CPU_FREQ -
> should rather be tested as early as possible for consistency, directly
> from kernel space.

I'll post a new patch right now.


Regards,
Bernhard

[Xenomai-core] [PATCH] Check CPU frequency

[Bernhard Walle]

In some conditions, while testing Xenomai in VirtualBox, I had the error that
xnarch_init_timeconv() is called from init_32.h with a frequency of 0. That
leads to a division by zero, followed by a system oops.

Of course that is a bug in the virtualisation and Linux reports a CPU frequency
of 0 in /proc/cpuinfo. However, there's no harm if Xenomai checks that instead
of crashing the whole system.

Signed-off-by: Bernhard Walle <bernhard@domain.hid>
---
 ksrc/arch/generic/hal.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/ksrc/arch/generic/hal.c b/ksrc/arch/generic/hal.c
index 4d2fd38..8a5ec39 100644
--- a/ksrc/arch/generic/hal.c
+++ b/ksrc/arch/generic/hal.c
@@ -822,6 +822,13 @@ int rthal_init(void)
      * The arch-dependent support must have updated the various
      * frequency args as required.
      */
+
+    /* check the CPU frequency first and abort if it's invalid */
+    if (rthal_cpufreq_arg == 0) {
+        printk(KERN_ERR "Xenomai has detected a CPU frequency of 0. Aborting.\n");
+        return -ENODEV;
+    }
+
     rthal_tunables.cpu_freq = rthal_cpufreq_arg;
     rthal_tunables.timer_freq = rthal_timerfreq_arg;
     rthal_tunables.clock_freq = rthal_clockfreq_arg;
-- 
1.6.3.3