[PATCH] Enforce valid shift values in clocksource_register()

[PATCH] Enforce valid shift values in clocksource_register()

[John Stultz]

Thomas noted that some of the timekeeping core assumes 
clocksource shift values will be less then 32. However, 
we do nothing to enforce such limits.

This patch prints a warning if a clocksource with an invalid
shift value is attempted to be registered, and will refuse to
register the clocksource

I could not find any clocksources that use 32 or higher for a 
shift value, so this should be only a preventive warning for
future clocksource writers.

Signed-off-by: John Stultz <johnstul@us.ibm.com>

diff --git a/kernel/time/clocksource.c b/kernel/time/clocksource.c
index 9ed2eec..4a3bb13 100644
--- a/kernel/time/clocksource.c
+++ b/kernel/time/clocksource.c
@@ -318,7 +318,7 @@ static int clocksource_enqueue(struct clocksource *c)
  * clocksource_register - Used to install new clocksources
  * @t:		clocksource to be registered
  *
- * Returns -EBUSY if registration fails, zero otherwise.
+ * Returns -EBUSY or -EINVAL if registration fails, zero otherwise.
  */
 int clocksource_register(struct clocksource *c)
 {
@@ -327,7 +327,14 @@ int clocksource_register(struct clocksource *c)
 
 	/* save mult_orig on registration */
 	c->mult_orig = c->mult;
-
+	if (c->shift >= 32) {
+		printk(KERN_WARNING "===============================\n");
+		printk(KERN_WARNING "WARNING: Cannot register %s clocksource\n",
+					c->name);
+		printk(KERN_WARNING "The shift value must be less then 32\n");
+		printk(KERN_WARNING "===============================\n");
+		return -EINVAL;
+	}
 	spin_lock_irqsave(&clocksource_lock, flags);
 	ret = clocksource_enqueue(c);
 	if (!ret)

Re: [PATCH] Enforce valid shift values in clocksource_register()

[Thomas Gleixner]

On Wed, 26 Nov 2008, John Stultz wrote:

> Thomas noted that some of the timekeeping core assumes 
> clocksource shift values will be less then 32. However, 
> we do nothing to enforce such limits.
> 
> This patch prints a warning if a clocksource with an invalid
> shift value is attempted to be registered, and will refuse to
> register the clocksource
> 
> I could not find any clocksources that use 32 or higher for a 
> shift value, so this should be only a preventive warning for
> future clocksource writers.
> 
> Signed-off-by: John Stultz <johnstul@us.ibm.com>
> 
> diff --git a/kernel/time/clocksource.c b/kernel/time/clocksource.c
> index 9ed2eec..4a3bb13 100644
> --- a/kernel/time/clocksource.c
> +++ b/kernel/time/clocksource.c
> @@ -318,7 +318,7 @@ static int clocksource_enqueue(struct clocksource *c)
>   * clocksource_register - Used to install new clocksources
>   * @t:		clocksource to be registered
>   *
> - * Returns -EBUSY if registration fails, zero otherwise.
> + * Returns -EBUSY or -EINVAL if registration fails, zero otherwise.
>   */
>  int clocksource_register(struct clocksource *c)
>  {
> @@ -327,7 +327,14 @@ int clocksource_register(struct clocksource *c)
>  
>  	/* save mult_orig on registration */
>  	c->mult_orig = c->mult;
> -
> +	if (c->shift >= 32) {
> +		printk(KERN_WARNING "===============================\n");
> +		printk(KERN_WARNING "WARNING: Cannot register %s clocksource\n",
> +					c->name);
> +		printk(KERN_WARNING "The shift value must be less then 32\n");
> +		printk(KERN_WARNING "===============================\n");
> +		return -EINVAL;

Just setting the shift value to 31 along with a WARN_ON() should be
enough. We don't need to kill the clocksource in that case, as this
can be nasty when it happens in the early boot code where we dont have
any output.

Thanks,

	tglx

Re: [PATCH] Enforce valid shift values in clocksource_register()

[John Stultz]

On Thu, 2008-11-27 at 01:08 +0100, Thomas Gleixner wrote:
> On Wed, 26 Nov 2008, John Stultz wrote:
> 
> > Thomas noted that some of the timekeeping core assumes 
> > clocksource shift values will be less then 32. However, 
> > we do nothing to enforce such limits.
> > 
> > This patch prints a warning if a clocksource with an invalid
> > shift value is attempted to be registered, and will refuse to
> > register the clocksource
> > 
> > I could not find any clocksources that use 32 or higher for a 
> > shift value, so this should be only a preventive warning for
> > future clocksource writers.
> > 
> > Signed-off-by: John Stultz <johnstul@us.ibm.com>
> > 
> > diff --git a/kernel/time/clocksource.c b/kernel/time/clocksource.c
> > index 9ed2eec..4a3bb13 100644
> > --- a/kernel/time/clocksource.c
> > +++ b/kernel/time/clocksource.c
> > @@ -318,7 +318,7 @@ static int clocksource_enqueue(struct clocksource *c)
> >   * clocksource_register - Used to install new clocksources
> >   * @t:		clocksource to be registered
> >   *
> > - * Returns -EBUSY if registration fails, zero otherwise.
> > + * Returns -EBUSY or -EINVAL if registration fails, zero otherwise.
> >   */
> >  int clocksource_register(struct clocksource *c)
> >  {
> > @@ -327,7 +327,14 @@ int clocksource_register(struct clocksource *c)
> >  
> >  	/* save mult_orig on registration */
> >  	c->mult_orig = c->mult;
> > -
> > +	if (c->shift >= 32) {
> > +		printk(KERN_WARNING "===============================\n");
> > +		printk(KERN_WARNING "WARNING: Cannot register %s clocksource\n",
> > +					c->name);
> > +		printk(KERN_WARNING "The shift value must be less then 32\n");
> > +		printk(KERN_WARNING "===============================\n");
> > +		return -EINVAL;
> 
> Just setting the shift value to 31 along with a WARN_ON() should be
> enough. We don't need to kill the clocksource in that case, as this
> can be nasty when it happens in the early boot code where we dont have
> any output.

Well, we can't tweak the shift value without changing the mult. And that
seemed a bit overreaching (but might be safe). I'll give it a shot.

Also early boot we should have the jiffies clocksource around, so unless
I'm forgetting something, I don't think it has early boot concerns.

thanks
-john

Re: [PATCH] Enforce valid shift values in clocksource_register()

[Thomas Gleixner]

On Wed, 26 Nov 2008, John Stultz wrote:
> On Thu, 2008-11-27 at 01:08 +0100, Thomas Gleixner wrote:
> > On Wed, 26 Nov 2008, John Stultz wrote:
> > > +	if (c->shift >= 32) {
> > > +		printk(KERN_WARNING "===============================\n");
> > > +		printk(KERN_WARNING "WARNING: Cannot register %s clocksource\n",
> > > +					c->name);
> > > +		printk(KERN_WARNING "The shift value must be less then 32\n");
> > > +		printk(KERN_WARNING "===============================\n");
> > > +		return -EINVAL;
> > 
> > Just setting the shift value to 31 along with a WARN_ON() should be
> > enough. We don't need to kill the clocksource in that case, as this
> > can be nasty when it happens in the early boot code where we dont have
> > any output.
> 
> Well, we can't tweak the shift value without changing the mult. And that
> seemed a bit overreaching (but might be safe). I'll give it a shot.
> 
> Also early boot we should have the jiffies clocksource around, so unless
> I'm forgetting something, I don't think it has early boot concerns.

Fair enough. I forgot that we do a late handover from jiffies to the
real clocksource. Still a WARN_ON is more prominent and more useful
for developers to get to the root cause. Setting the shift to 31 will
result in wrong timer values but not brick the box. So espcially for
code, which calculates the values this might be worthwhile.

Thanks,

	tglx