From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754191AbYK0AqV (ORCPT ); Wed, 26 Nov 2008 19:46:21 -0500 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1752599AbYK0AqM (ORCPT ); Wed, 26 Nov 2008 19:46:12 -0500 Received: from e33.co.us.ibm.com ([32.97.110.151]:58436 "EHLO e33.co.us.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752566AbYK0AqM (ORCPT ); Wed, 26 Nov 2008 19:46:12 -0500 Subject: Re: [PATCH] Enforce valid shift values in clocksource_register() From: John Stultz To: Thomas Gleixner Cc: LKML In-Reply-To: References: <1227743196.18967.36.camel@jstultz-laptop> Content-Type: text/plain Date: Wed, 26 Nov 2008 16:39:18 -0800 Message-Id: <1227746358.18967.41.camel@jstultz-laptop> Mime-Version: 1.0 X-Mailer: Evolution 2.24.1 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > > > > 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