[patch 1/3] rtc: Handle errors correctly in rtc_irq_set_state()

[patch 1/3] rtc: Handle errors correctly in rtc_irq_set_state()

[Thomas Gleixner]

[-- Attachment #1: rtc-deal-with-errors-correctly.patch --]
[-- Type: text/plain, Size: 875 bytes --]

The code checks the correctness of the parameters, but unconditionally
arms/disarms the hrtimer.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: stable@kernel.org
---
 drivers/rtc/interface.c |    3 +++
 1 file changed, 3 insertions(+)

Index: linux-2.6/drivers/rtc/interface.c
===================================================================
--- linux-2.6.orig/drivers/rtc/interface.c
+++ linux-2.6/drivers/rtc/interface.c
@@ -656,6 +656,8 @@ int rtc_irq_set_state(struct rtc_device 
 		err = -EBUSY;
 	if (rtc->irq_task != task)
 		err = -EACCES;
+	if (err)
+		goto out;
 
 	if (enabled) {
 		ktime_t period = ktime_set(0, NSEC_PER_SEC/rtc->irq_freq);
@@ -664,6 +666,7 @@ int rtc_irq_set_state(struct rtc_device 
 		hrtimer_cancel(&rtc->pie_timer);
 	}
 	rtc->pie_enabled = enabled;
+out:
 	spin_unlock_irqrestore(&rtc->irq_task_lock, flags);
 
 	return err;

[patch 0/3] rtc: Assorted bug fixes

[Thomas Gleixner]

The following series fixes a few interesting bugs in the RTC code.

Thanks,

	tglx

[patch 2/3] rtc: Fix hrtimer deadlock

[Thomas Gleixner]

[-- Attachment #1: rtc-fix-hrtimer-deadlock.patch --]
[-- Type: text/plain, Size: 3473 bytes --]

Ben reported a lockup related to rtc. The lockup happens due to:

CPU0                                        CPU1

rtc_irq_set_state()			    __run_hrtimer()	
  spin_lock_irqsave(&rtc->irq_task_lock)    rtc_handle_legacy_irq();
					      spin_lock(&rtc->irq_task_lock);
  hrtimer_cancel()
    while (callback_running);

So the running callback never finishes as it's blocked on
rtc->irq_task_lock.  

Use hrtimer_try_to_cancel() instead and drop rtc->irq_task_lock while
waiting for the callback. Fix this for both rtc_irq_set_state() and
rtc_irq_set_freq().

Reported-by: Ben Greear <greearb@candelatech.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: stable@kernel.org
---
 drivers/rtc/interface.c |   56 +++++++++++++++++++++++++++++++-----------------
 1 file changed, 37 insertions(+), 19 deletions(-)

Index: linux-2.6/drivers/rtc/interface.c
===================================================================
--- linux-2.6.orig/drivers/rtc/interface.c
+++ linux-2.6/drivers/rtc/interface.c
@@ -636,6 +636,29 @@ void rtc_irq_unregister(struct rtc_devic
 }
 EXPORT_SYMBOL_GPL(rtc_irq_unregister);
 
+static int rtc_update_hrtimer(struct rtc_device *rtc, int enabled)
+{
+	/*
+	 * We unconditionally cancel the timer here, because otherwise
+	 * we could run into BUG_ON(timer->state != HRTIMER_STATE_CALLBACK);
+	 * when we manage to start the timer before the callback
+	 * returns HRTIMER_RESTART.
+	 *
+	 * We cannot use hrtimer_cancel() here as a running callback
+	 * could be blocked on rtc->irq_task_lock and hrtimer_cancel()
+	 * would spin forever.
+	 */
+	if (hrtimer_try_to_cancel(&rtc->pie_timer) < 0)
+		return -1;
+
+	if (enabled) {
+		ktime_t period = ktime_set(0, NSEC_PER_SEC / rtc->irq_freq);
+
+		hrtimer_start(&rtc->pie_timer, period, HRTIMER_MODE_REL);
+	}
+	return 0;
+}
+
 /**
  * rtc_irq_set_state - enable/disable 2^N Hz periodic IRQs
  * @rtc: the rtc device
@@ -651,24 +674,21 @@ int rtc_irq_set_state(struct rtc_device 
 	int err = 0;
 	unsigned long flags;
 
+retry:
 	spin_lock_irqsave(&rtc->irq_task_lock, flags);
 	if (rtc->irq_task != NULL && task == NULL)
 		err = -EBUSY;
 	if (rtc->irq_task != task)
 		err = -EACCES;
-	if (err)
-		goto out;
-
-	if (enabled) {
-		ktime_t period = ktime_set(0, NSEC_PER_SEC/rtc->irq_freq);
-		hrtimer_start(&rtc->pie_timer, period, HRTIMER_MODE_REL);
-	} else {
-		hrtimer_cancel(&rtc->pie_timer);
+	if (!err) {
+		if (rtc_update_hrtimer(rtc, enabled) < 0) {
+			spin_unlock_irqrestore(&rtc->irq_task_lock, flags);
+			cpu_relax();
+			goto retry;
+		}
+		rtc->pie_enabled = enabled;
 	}
-	rtc->pie_enabled = enabled;
-out:
 	spin_unlock_irqrestore(&rtc->irq_task_lock, flags);
-
 	return err;
 }
 EXPORT_SYMBOL_GPL(rtc_irq_set_state);
@@ -690,20 +710,18 @@ int rtc_irq_set_freq(struct rtc_device *
 
 	if (freq <= 0)
 		return -EINVAL;
-
+retry:
 	spin_lock_irqsave(&rtc->irq_task_lock, flags);
 	if (rtc->irq_task != NULL && task == NULL)
 		err = -EBUSY;
 	if (rtc->irq_task != task)
 		err = -EACCES;
-	if (err == 0) {
+	if (!err) {
 		rtc->irq_freq = freq;
-		if (rtc->pie_enabled) {
-			ktime_t period;
-			hrtimer_cancel(&rtc->pie_timer);
-			period = ktime_set(0, NSEC_PER_SEC/rtc->irq_freq);
-			hrtimer_start(&rtc->pie_timer, period,
-					HRTIMER_MODE_REL);
+		if (rtc->pie_enabled && rtc_update_hrtimer(rtc, 1) < 0) {
+			spin_unlock_irqrestore(&rtc->irq_task_lock, flags);
+			cpu_relax();
+			goto retry;
 		}
 	}
 	spin_unlock_irqrestore(&rtc->irq_task_lock, flags);

[patch 3/3] rtc: Limit frequency

[Thomas Gleixner]

[-- Attachment #1: rtc-limit-frequency.patch --]
[-- Type: text/plain, Size: 675 bytes --]

The RTC hrtimer is self rearming. We really need to limit the
frequency to something sensible.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: stable@kernel.org
---
 drivers/rtc/interface.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux-2.6/drivers/rtc/interface.c
===================================================================
--- linux-2.6.orig/drivers/rtc/interface.c
+++ linux-2.6/drivers/rtc/interface.c
@@ -708,7 +708,7 @@ int rtc_irq_set_freq(struct rtc_device *
 	int err = 0;
 	unsigned long flags;
 
-	if (freq <= 0)
+	if (freq <= 0 || freq > 5000)
 		return -EINVAL;
 retry:
 	spin_lock_irqsave(&rtc->irq_task_lock, flags);

Re: [patch 1/3] rtc: Handle errors correctly in rtc_irq_set_state()

[Andrew Morton]

On Fri, 22 Jul 2011 09:12:50 -0000
Thomas Gleixner <tglx@linutronix.de> wrote:

> The code checks the correctness of the parameters, but unconditionally
> arms/disarms the hrtimer.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: stable@kernel.org

The -stable maintainers will want to know why they should merge this.

The maintainers of other trees will wonder whether they should backport
it too.

To help them make these decision we should always provide a description
of the user-visible effects of the bug, please.

Re: [patch 3/3] rtc: Limit frequency

[Andrew Morton]

On Fri, 22 Jul 2011 09:12:51 -0000
Thomas Gleixner <tglx@linutronix.de> wrote:

> The RTC hrtimer is self rearming. We really need to limit the
> frequency to something sensible.

> Cc: stable@kernel.org

Why?  What failures does the current code cause?  What effect do these
failures have upon users?

Re: [patch 2/3] rtc: Fix hrtimer deadlock

[Andrew Morton]

On Fri, 22 Jul 2011 09:12:51 -0000
Thomas Gleixner <tglx@linutronix.de> wrote:

> Ben reported a lockup related to rtc. The lockup happens due to:
> 
> CPU0                                        CPU1
> 
> rtc_irq_set_state()			    __run_hrtimer()	
>   spin_lock_irqsave(&rtc->irq_task_lock)    rtc_handle_legacy_irq();
> 					      spin_lock(&rtc->irq_task_lock);
>   hrtimer_cancel()
>     while (callback_running);
> 
> So the running callback never finishes as it's blocked on
> rtc->irq_task_lock.  
> 
> Use hrtimer_try_to_cancel() instead and drop rtc->irq_task_lock while
> waiting for the callback. Fix this for both rtc_irq_set_state() and
> rtc_irq_set_freq().
> 
> ...
>
> +static int rtc_update_hrtimer(struct rtc_device *rtc, int enabled)
> +{
> +	/*
> +	 * We unconditionally cancel the timer here, because otherwise

The comment seems wrong.  If hrtimer_try_to_cancel() fails, we simply
bale out so we did not "unconditionally cancel the timer"?

> +	 * we could run into BUG_ON(timer->state != HRTIMER_STATE_CALLBACK);
> +	 * when we manage to start the timer before the callback
> +	 * returns HRTIMER_RESTART.
> +	 *
> +	 * We cannot use hrtimer_cancel() here as a running callback
> +	 * could be blocked on rtc->irq_task_lock and hrtimer_cancel()
> +	 * would spin forever.
> +	 */
> +	if (hrtimer_try_to_cancel(&rtc->pie_timer) < 0)
> +		return -1;
> +
> +	if (enabled) {
> +		ktime_t period = ktime_set(0, NSEC_PER_SEC / rtc->irq_freq);
> +
> +		hrtimer_start(&rtc->pie_timer, period, HRTIMER_MODE_REL);
> +	}
> +	return 0;
> +}
> +
>  /**
>   * rtc_irq_set_state - enable/disable 2^N Hz periodic IRQs
>   * @rtc: the rtc device
> @@ -651,24 +674,21 @@ int rtc_irq_set_state(struct rtc_device 
>  	int err = 0;
>  	unsigned long flags;
>  
> +retry:
>  	spin_lock_irqsave(&rtc->irq_task_lock, flags);
>  	if (rtc->irq_task != NULL && task == NULL)
>  		err = -EBUSY;
>  	if (rtc->irq_task != task)
>  		err = -EACCES;
> -	if (err)
> -		goto out;
> -
> -	if (enabled) {
> -		ktime_t period = ktime_set(0, NSEC_PER_SEC/rtc->irq_freq);
> -		hrtimer_start(&rtc->pie_timer, period, HRTIMER_MODE_REL);
> -	} else {
> -		hrtimer_cancel(&rtc->pie_timer);
> +	if (!err) {
> +		if (rtc_update_hrtimer(rtc, enabled) < 0) {
> +			spin_unlock_irqrestore(&rtc->irq_task_lock, flags);
> +			cpu_relax();
> +			goto retry;
> +		}
> +		rtc->pie_enabled = enabled;

Well this is rather nasty.  Sort of an open-coded expensive spinlock. 
All rather pointless on SMP=n builds, too.

Is there no better way, such as fixing up the locking properly?

Re: [stable] [patch 3/3] rtc: Limit frequency

[Willy Tarreau]

On Fri, Jul 22, 2011 at 03:05:50PM -0700, Andrew Morton wrote:
> On Fri, 22 Jul 2011 09:12:51 -0000
> Thomas Gleixner <tglx@linutronix.de> wrote:
> 
> > The RTC hrtimer is self rearming. We really need to limit the
> > frequency to something sensible.
> 
> > Cc: stable@kernel.org
> 
> Why?  What failures does the current code cause?  What effect do these
> failures have upon users?

I would add that if we go that route, we should at least accept values
that were documented as possible till now. Man rtc says 2 Hz to 8192 Hz,
but Thomas' proposed patch limits it to 5000 Hz, so some breakage is to
be expected.

Regards,
Willy

Re: [patch 1/3] rtc: Handle errors correctly in rtc_irq_set_state()

[Thomas Gleixner]

On Fri, 22 Jul 2011, Andrew Morton wrote:

> On Fri, 22 Jul 2011 09:12:50 -0000
> Thomas Gleixner <tglx@linutronix.de> wrote:
> 
> > The code checks the correctness of the parameters, but unconditionally
> > arms/disarms the hrtimer.
> > 
> > Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> > Cc: stable@kernel.org
> 
> The -stable maintainers will want to know why they should merge this.
> 
> The maintainers of other trees will wonder whether they should backport
> it too.
> 
> To help them make these decision we should always provide a description
> of the user-visible effects of the bug, please.

Fair enough. 

The result is that a random task might arm/disarm rtc timer and
surprise the real owner by either generating events or by stopping
them. Both undesired behaviour :)

Re: [patch 3/3] rtc: Limit frequency

[Thomas Gleixner]

On Fri, 22 Jul 2011, Andrew Morton wrote:

> On Fri, 22 Jul 2011 09:12:51 -0000
> Thomas Gleixner <tglx@linutronix.de> wrote:
> 
> > The RTC hrtimer is self rearming. We really need to limit the
> > frequency to something sensible.
> 
> > Cc: stable@kernel.org
> 
> Why?  What failures does the current code cause?  What effect do these
> failures have upon users?

Due to the hrtimer self rearming mode a user can DoS the machine
simply because it's starved by hrtimer events.

Re: [patch 2/3] rtc: Fix hrtimer deadlock

[Thomas Gleixner]

On Fri, 22 Jul 2011, Andrew Morton wrote:

> On Fri, 22 Jul 2011 09:12:51 -0000
> Thomas Gleixner <tglx@linutronix.de> wrote:
> 
> > Ben reported a lockup related to rtc. The lockup happens due to:
> > 
> > CPU0                                        CPU1
> > 
> > rtc_irq_set_state()			    __run_hrtimer()	
> >   spin_lock_irqsave(&rtc->irq_task_lock)    rtc_handle_legacy_irq();
> > 					      spin_lock(&rtc->irq_task_lock);
> >   hrtimer_cancel()
> >     while (callback_running);
> > 
> > So the running callback never finishes as it's blocked on
> > rtc->irq_task_lock.  
> > 
> > Use hrtimer_try_to_cancel() instead and drop rtc->irq_task_lock while
> > waiting for the callback. Fix this for both rtc_irq_set_state() and
> > rtc_irq_set_freq().
> > 
> > ...
> >
> > +static int rtc_update_hrtimer(struct rtc_device *rtc, int enabled)
> > +{
> > +	/*
> > +	 * We unconditionally cancel the timer here, because otherwise
> 
> The comment seems wrong.  If hrtimer_try_to_cancel() fails, we simply
> bale out so we did not "unconditionally cancel the timer"?

Well, what I meant is that we cancel it before we start it. That's
required for self rearming timers. Will reword.
 
> > +	 * we could run into BUG_ON(timer->state != HRTIMER_STATE_CALLBACK);
> > +	 * when we manage to start the timer before the callback
> > +	 * returns HRTIMER_RESTART.
> > +	 *
> > +	 * We cannot use hrtimer_cancel() here as a running callback
> > +	 * could be blocked on rtc->irq_task_lock and hrtimer_cancel()
> > +	 * would spin forever.
> > +	 */
> > +	if (hrtimer_try_to_cancel(&rtc->pie_timer) < 0)
> > +		return -1;
> > +
> > +	if (enabled) {
> > +		ktime_t period = ktime_set(0, NSEC_PER_SEC / rtc->irq_freq);
> > +
> > +		hrtimer_start(&rtc->pie_timer, period, HRTIMER_MODE_REL);
> > +	}
> > +	return 0;
> > +}
> > +
> >  /**
> >   * rtc_irq_set_state - enable/disable 2^N Hz periodic IRQs
> >   * @rtc: the rtc device
> > @@ -651,24 +674,21 @@ int rtc_irq_set_state(struct rtc_device 
> >  	int err = 0;
> >  	unsigned long flags;
> >  
> > +retry:
> >  	spin_lock_irqsave(&rtc->irq_task_lock, flags);
> >  	if (rtc->irq_task != NULL && task == NULL)
> >  		err = -EBUSY;
> >  	if (rtc->irq_task != task)
> >  		err = -EACCES;
> > -	if (err)
> > -		goto out;
> > -
> > -	if (enabled) {
> > -		ktime_t period = ktime_set(0, NSEC_PER_SEC/rtc->irq_freq);
> > -		hrtimer_start(&rtc->pie_timer, period, HRTIMER_MODE_REL);
> > -	} else {
> > -		hrtimer_cancel(&rtc->pie_timer);
> > +	if (!err) {
> > +		if (rtc_update_hrtimer(rtc, enabled) < 0) {
> > +			spin_unlock_irqrestore(&rtc->irq_task_lock, flags);
> > +			cpu_relax();
> > +			goto retry;
> > +		}
> > +		rtc->pie_enabled = enabled;
> 
> Well this is rather nasty.  Sort of an open-coded expensive spinlock. 
> All rather pointless on SMP=n builds, too.
> 
> Is there no better way, such as fixing up the locking properly?

Probably there is, but that requires a rather large patch and a
complete locking rewrite, nothing you want to push back into
stable. And we want this as the deadlock has been observed and
reported already.

Re: [stable] [patch 3/3] rtc: Limit frequency

[Joshua Kinard]

On 07/22/2011 18:39, Willy Tarreau wrote:
> On Fri, Jul 22, 2011 at 03:05:50PM -0700, Andrew Morton wrote:
>> On Fri, 22 Jul 2011 09:12:51 -0000
>> Thomas Gleixner <tglx@linutronix.de> wrote:
>>
>>> The RTC hrtimer is self rearming. We really need to limit the
>>> frequency to something sensible.
>>
>>> Cc: stable@kernel.org
>>
>> Why?  What failures does the current code cause?  What effect do these
>> failures have upon users?
> 
> I would add that if we go that route, we should at least accept values
> that were documented as possible till now. Man rtc says 2 Hz to 8192 Hz,
> but Thomas' proposed patch limits it to 5000 Hz, so some breakage is to
> be expected.
> 
> Regards,
> Willy

To me, it would make sense to lock it at 8192Hz.  I re-wrote a driver for the DS1685 family of Dallas chips that I need
to cleanup and re-submit, but in researching that specific RTC, I really could not find an RTC out there that went above
8192.

Arguably, 32768Hz might also work.  The DS1685 runs at this mode normally, but it disables PIE when it does.

My vote is 8192Hz.

Also, can we get a wrapper of some kind in the RTC core to allow an RTC driver to override the hrtimer /PIE emulation if
needed?  I have in the DS1685 driver full support for its PIE mode and really do not want to gut it as part of the
cleanup.  Some kind of override API would be nice in case anyone ever runs into this chip (or its family members).

-- 
Joshua Kinard
Gentoo/MIPS
kumba@gentoo.org

"The past tempts us, the present confuses us, the future frightens us.  And our lives slip away, moment by moment, lost
in that vast, terrible in-between."

--Emperor Turhan, Centauri Republic

Re: [stable] [patch 3/3] rtc: Limit frequency

[John Stultz]

On Thu, 2011-08-04 at 23:39 -0400, Joshua Kinard wrote:
> On 07/22/2011 18:39, Willy Tarreau wrote:
> > On Fri, Jul 22, 2011 at 03:05:50PM -0700, Andrew Morton wrote:
> >> On Fri, 22 Jul 2011 09:12:51 -0000
> >> Thomas Gleixner <tglx@linutronix.de> wrote:
> >>
> >>> The RTC hrtimer is self rearming. We really need to limit the
> >>> frequency to something sensible.
> >>
> >>> Cc: stable@kernel.org
> >>
> >> Why?  What failures does the current code cause?  What effect do these
> >> failures have upon users?
> > 
> > I would add that if we go that route, we should at least accept values
> > that were documented as possible till now. Man rtc says 2 Hz to 8192 Hz,
> > but Thomas' proposed patch limits it to 5000 Hz, so some breakage is to
> > be expected.
> > 
> > Regards,
> > Willy
> 
> To me, it would make sense to lock it at 8192Hz.  I re-wrote a driver for the DS1685 family of Dallas chips that I need
> to cleanup and re-submit, but in researching that specific RTC, I really could not find an RTC out there that went above
> 8192.
> 
> Arguably, 32768Hz might also work.  The DS1685 runs at this mode normally, but it disables PIE when it does.
> 
> My vote is 8192Hz.

Yea, I submitted a slightly different version of Thomas' patch which set
it to 8192Hz via the tip tree, but the original one went upstream first.
I'm away from home right now, so I'll be re-submitting just that change
probably next week.

> Also, can we get a wrapper of some kind in the RTC core to allow an
> RTC driver to override the hrtimer /PIE emulation if
> needed?  I have in the DS1685 driver full support for its PIE mode and
> really do not want to gut it as part of the
> cleanup.  Some kind of override API would be nice in case anyone ever
> runs into this chip (or its family members).

I'm torn here. It would be good to have the functionality listed in some
way so it is documented. However as we abstract the RTC to be more of an
internal tool for the kernel, rather then hardware directly addressed
from userland, the PIE mode is really not as useful (and quite messier
to multiplex).  I'd much rather try to extend the alarm interfaces to
allow for higher resolution, so it could be used as a alarmed highres
oneshot timer if the hardware supports it.

thanks
-john

Re: [stable] [patch 3/3] rtc: Limit frequency

[Joshua Kinard]

On 08/05/2011 05:04, John Stultz wrote:
> Yea, I submitted a slightly different version of Thomas' patch which set
> it to 8192Hz via the tip tree, but the original one went upstream first.
> I'm away from home right now, so I'll be re-submitting just that change
> probably next week.

Awesome, I'll look out for it if I get free time to look at the RTC driver I have.  It's broken now due to a loss of
__symbol_get(), as I was trying to find a way to make the driver determine if it was a module or not.


> I'm torn here. It would be good to have the functionality listed in some
> way so it is documented. However as we abstract the RTC to be more of an
> internal tool for the kernel, rather then hardware directly addressed
> from userland, the PIE mode is really not as useful (and quite messier
> to multiplex).  I'd much rather try to extend the alarm interfaces to
> allow for higher resolution, so it could be used as a alarmed highres
> oneshot timer if the hardware supports it.

That's why I think a wrapper would be best.  By default, it would use the hrtimer code and so, none of the existing
drivers would need to be modified.  Any driver that wants to, however, could override the base functions if they wanted
to provide that functionality separately.  It should probably be a CONFIG_* option, though.  Right now, consumers of
DS1685 are rare: old SGI O2 systems and some embedded board from 'electronic systems Gmbh', one of whose employees wrote
the original driver about 2 years ago.  The driver I have now handles older versions (DS1689/1693) and the more common
DS17x8y versions, and those I think are the most modern.  Not sure what systems use those, however.

-- 
Joshua Kinard
Gentoo/MIPS
kumba@gentoo.org
4096R/D25D95E3 2011-03-28

"The past tempts us, the present confuses us, the future frightens us.  And our lives slip away, moment by moment, lost
in that vast, terrible in-between."

--Emperor Turhan, Centauri Republic