Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1694912 > unrolled thread

[PATCH] alarmtimer: don't rate limit one-shot timers

Back to article view | Back to linux.kernel

Contents

  [PATCH] alarmtimer: don't rate limit one-shot timers Greg Hackmann <ghackmann@google.com> - 2017-07-24 19:30 +0200
    Re: [PATCH] alarmtimer: don't rate limit one-shot timers Greg KH <greg@kroah.com> - 2017-07-24 20:30 +0200
      Re: [PATCH] alarmtimer: don't rate limit one-shot timers Greg Hackmann <ghackmann@google.com> - 2017-07-24 23:50 +0200
        Re: [PATCH] alarmtimer: don't rate limit one-shot timers Greg KH <greg@kroah.com> - 2017-07-25 00:00 +0200
          Re: [PATCH] alarmtimer: don't rate limit one-shot timers Thomas Gleixner <tglx@linutronix.de> - 2017-07-25 09:10 +0200
            Re: [PATCH] alarmtimer: don't rate limit one-shot timers Greg KH <greg@kroah.com> - 2017-07-25 20:10 +0200
              [PATCH] alarmtimer: don't rate limit one-shot timers Greg Hackmann <ghackmann@google.com> - 2017-07-25 21:50 +0200
                Re: [PATCH] alarmtimer: don't rate limit one-shot timers Greg KH <greg@kroah.com> - 2017-07-25 23:50 +0200

#1694912 — [PATCH] alarmtimer: don't rate limit one-shot timers

Commit ff86bf0c65f1 ("alarmtimer: Rate limit periodic intervals") sets a
minimum bound on the alarm timer interval.  This minimum bound shouldn't
be applied if the interval is 0.  Otherwise, one-shot timers will be
converted into periodic ones.

This patch is against 4.9.39, and is only needed in -stable trees.
4.13-rc2 isn't impacted due to a later refactoring.

Fixes: ff86bf0c65f1 ("alarmtimer: Rate limit periodic intervals")
Reported-by: Ben Fennema <fennema@google.com>
Signed-off-by: Greg Hackmann <ghackmann@google.com>
Cc: stable@vger.kernel.org
---
 kernel/time/alarmtimer.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/time/alarmtimer.c b/kernel/time/alarmtimer.c
index 9ba04aa740b9..d67ef56ca9bc 100644
--- a/kernel/time/alarmtimer.c
+++ b/kernel/time/alarmtimer.c
@@ -629,7 +629,8 @@ static int alarm_timer_set(struct k_itimer *timr, int flags,
 	 * Rate limit to the tick as a hot fix to prevent DOS. Will be
 	 * mopped up later.
 	 */
-	if (ktime_to_ns(timr->it.alarm.interval) < TICK_NSEC)
+	if (timr->it.alarm.interval.tv64 &&
+			ktime_to_ns(timr->it.alarm.interval) < TICK_NSEC)
 		timr->it.alarm.interval = ktime_set(0, TICK_NSEC);
 
 	exp = timespec_to_ktime(new_setting->it_value);
-- 
2.14.0.rc0.284.gd933b75aa4-goog

[toc] | [next] | [standalone]

#1694979

On Mon, Jul 24, 2017 at 10:19:24AM -0700, Greg Hackmann wrote:
> Commit ff86bf0c65f1 ("alarmtimer: Rate limit periodic intervals") sets a
> minimum bound on the alarm timer interval.  This minimum bound shouldn't
> be applied if the interval is 0.  Otherwise, one-shot timers will be
> converted into periodic ones.
> 
> This patch is against 4.9.39, and is only needed in -stable trees.
> 4.13-rc2 isn't impacted due to a later refactoring.

What refactoring patch fixed this up?

> Fixes: ff86bf0c65f1 ("alarmtimer: Rate limit periodic intervals")

As this was a 4.12 patch, 4.12-stable needs this fix as well, right?

Also, was there some test-case that you caught this with that perhaps
could be added to LTP or kselftests?

thanks,

greg k-h

[toc] | [prev] | [next] | [standalone]

#1695152

On 07/24/2017 11:21 AM, Greg KH wrote:
> On Mon, Jul 24, 2017 at 10:19:24AM -0700, Greg Hackmann wrote:
>> Commit ff86bf0c65f1 ("alarmtimer: Rate limit periodic intervals") sets a
>> minimum bound on the alarm timer interval.  This minimum bound shouldn't
>> be applied if the interval is 0.  Otherwise, one-shot timers will be
>> converted into periodic ones.
>>
>> This patch is against 4.9.39, and is only needed in -stable trees.
>> 4.13-rc2 isn't impacted due to a later refactoring.
> 
> What refactoring patch fixed this up?

f2c45807d399 ("alarmtimer: Switch over to generic set/get/rearm routine")

> As this was a 4.12 patch, 4.12-stable needs this fix as well, right?

Looks like it, but I haven't actually tried 4.12 yet to confirm.

> Also, was there some test-case that you caught this with that perhaps
> could be added to LTP or kselftests?

Unfortunately not a direct testcase.  This first showed up as a 
regression in AOSP's userspace Bluetooth stack, which uses 
CLOCK_BOOTTIME_ALARM internally.

I'm working on a patch to add one-shot timer testcases to 
set-timer-lat.c, which would have caught this.  (I wrote a very rough 
test program to make sure this patch fixes the regression, but 
set-timer-lat.c already exists and is more comprehensive.)

[toc] | [prev] | [next] | [standalone]

#1695155

On Mon, Jul 24, 2017 at 02:41:10PM -0700, Greg Hackmann wrote:
> On 07/24/2017 11:21 AM, Greg KH wrote:
> > On Mon, Jul 24, 2017 at 10:19:24AM -0700, Greg Hackmann wrote:
> > > Commit ff86bf0c65f1 ("alarmtimer: Rate limit periodic intervals") sets a
> > > minimum bound on the alarm timer interval.  This minimum bound shouldn't
> > > be applied if the interval is 0.  Otherwise, one-shot timers will be
> > > converted into periodic ones.
> > > 
> > > This patch is against 4.9.39, and is only needed in -stable trees.
> > > 4.13-rc2 isn't impacted due to a later refactoring.
> > 
> > What refactoring patch fixed this up?
> 
> f2c45807d399 ("alarmtimer: Switch over to generic set/get/rearm routine")

Ick, yeah, that's not a stable patch :)

> > As this was a 4.12 patch, 4.12-stable needs this fix as well, right?
> 
> Looks like it, but I haven't actually tried 4.12 yet to confirm.
> 
> > Also, was there some test-case that you caught this with that perhaps
> > could be added to LTP or kselftests?
> 
> Unfortunately not a direct testcase.  This first showed up as a regression
> in AOSP's userspace Bluetooth stack, which uses CLOCK_BOOTTIME_ALARM
> internally.
> 
> I'm working on a patch to add one-shot timer testcases to set-timer-lat.c,
> which would have caught this.  (I wrote a very rough test program to make
> sure this patch fixes the regression, but set-timer-lat.c already exists and
> is more comprehensive.)

Ok, thanks for the information.

John and Thomas, any objection for me to take the original patch here in
the stable trees to fix this issue?

thanks,

greg k-h

[toc] | [prev] | [next] | [standalone]

#1695422

On Mon, 24 Jul 2017, Greg KH wrote:

> On Mon, Jul 24, 2017 at 02:41:10PM -0700, Greg Hackmann wrote:
> > On 07/24/2017 11:21 AM, Greg KH wrote:
> > > On Mon, Jul 24, 2017 at 10:19:24AM -0700, Greg Hackmann wrote:
> > > > Commit ff86bf0c65f1 ("alarmtimer: Rate limit periodic intervals") sets a
> > > > minimum bound on the alarm timer interval.  This minimum bound shouldn't
> > > > be applied if the interval is 0.  Otherwise, one-shot timers will be
> > > > converted into periodic ones.
> > > > 
> > > > This patch is against 4.9.39, and is only needed in -stable trees.
> > > > 4.13-rc2 isn't impacted due to a later refactoring.
> > > 
> > > What refactoring patch fixed this up?
> > 
> > f2c45807d399 ("alarmtimer: Switch over to generic set/get/rearm routine")
> 
> Ick, yeah, that's not a stable patch :)
> 
> > > As this was a 4.12 patch, 4.12-stable needs this fix as well, right?
> > 
> > Looks like it, but I haven't actually tried 4.12 yet to confirm.
> > 
> > > Also, was there some test-case that you caught this with that perhaps
> > > could be added to LTP or kselftests?
> > 
> > Unfortunately not a direct testcase.  This first showed up as a regression
> > in AOSP's userspace Bluetooth stack, which uses CLOCK_BOOTTIME_ALARM
> > internally.
> > 
> > I'm working on a patch to add one-shot timer testcases to set-timer-lat.c,
> > which would have caught this.  (I wrote a very rough test program to make
> > sure this patch fixes the regression, but set-timer-lat.c already exists and
> > is more comprehensive.)
> 
> Ok, thanks for the information.
> 
> John and Thomas, any objection for me to take the original patch here in
> the stable trees to fix this issue?

No. I borked that when I was 'fixing' that DoS issue :(

Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

[toc] | [prev] | [next] | [standalone]

#1695990

On Tue, Jul 25, 2017 at 09:00:42AM +0200, Thomas Gleixner wrote:
> On Mon, 24 Jul 2017, Greg KH wrote:
> 
> > On Mon, Jul 24, 2017 at 02:41:10PM -0700, Greg Hackmann wrote:
> > > On 07/24/2017 11:21 AM, Greg KH wrote:
> > > > On Mon, Jul 24, 2017 at 10:19:24AM -0700, Greg Hackmann wrote:
> > > > > Commit ff86bf0c65f1 ("alarmtimer: Rate limit periodic intervals") sets a
> > > > > minimum bound on the alarm timer interval.  This minimum bound shouldn't
> > > > > be applied if the interval is 0.  Otherwise, one-shot timers will be
> > > > > converted into periodic ones.
> > > > > 
> > > > > This patch is against 4.9.39, and is only needed in -stable trees.
> > > > > 4.13-rc2 isn't impacted due to a later refactoring.
> > > > 
> > > > What refactoring patch fixed this up?
> > > 
> > > f2c45807d399 ("alarmtimer: Switch over to generic set/get/rearm routine")
> > 
> > Ick, yeah, that's not a stable patch :)
> > 
> > > > As this was a 4.12 patch, 4.12-stable needs this fix as well, right?
> > > 
> > > Looks like it, but I haven't actually tried 4.12 yet to confirm.
> > > 
> > > > Also, was there some test-case that you caught this with that perhaps
> > > > could be added to LTP or kselftests?
> > > 
> > > Unfortunately not a direct testcase.  This first showed up as a regression
> > > in AOSP's userspace Bluetooth stack, which uses CLOCK_BOOTTIME_ALARM
> > > internally.
> > > 
> > > I'm working on a patch to add one-shot timer testcases to set-timer-lat.c,
> > > which would have caught this.  (I wrote a very rough test program to make
> > > sure this patch fixes the regression, but set-timer-lat.c already exists and
> > > is more comprehensive.)
> > 
> > Ok, thanks for the information.
> > 
> > John and Thomas, any objection for me to take the original patch here in
> > the stable trees to fix this issue?
> 
> No. I borked that when I was 'fixing' that DoS issue :(
> 
> Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

Thanks for the review.

Greg, can you provide a version of this for 4.12-stable?  It doesn't
apply there as-is.

thanks,

greg k-h

[toc] | [prev] | [next] | [standalone]

#1696178

Commit ff86bf0c65f1 ("alarmtimer: Rate limit periodic intervals") sets a
minimum bound on the alarm timer interval.  This minimum bound shouldn't
be applied if the interval is 0.  Otherwise, one-shot timers will be
converted into periodic ones.

This patch is specific to 4.11.y and 4.12.y.  Older -stable trees have a
slightly different patch, and 4.13-rc2 isn't impacted due to a later
refactoring.

Fixes: ff86bf0c65f1 ("alarmtimer: Rate limit periodic intervals")
Reported-by: Ben Fennema <fennema@google.com>
Signed-off-by: Greg Hackmann <ghackmann@google.com>
Cc: stable@vger.kernel.org
---
 kernel/time/alarmtimer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/time/alarmtimer.c b/kernel/time/alarmtimer.c
index ee2f4202d82a..11e70a38497c 100644
--- a/kernel/time/alarmtimer.c
+++ b/kernel/time/alarmtimer.c
@@ -665,7 +665,7 @@ static int alarm_timer_set(struct k_itimer *timr, int flags,
 	 * Rate limit to the tick as a hot fix to prevent DOS. Will be
 	 * mopped up later.
 	 */
-	if (timr->it.alarm.interval < TICK_NSEC)
+	if (timr->it.alarm.interval && timr->it.alarm.interval < TICK_NSEC)
 		timr->it.alarm.interval = TICK_NSEC;
 
 	exp = timespec64_to_ktime(new_setting->it_value);
-- 
2.14.0.rc0.284.gd933b75aa4-goog

[toc] | [prev] | [next] | [standalone]

#1696617

On Tue, Jul 25, 2017 at 12:42:46PM -0700, Greg Hackmann wrote:
> Commit ff86bf0c65f1 ("alarmtimer: Rate limit periodic intervals") sets a
> minimum bound on the alarm timer interval.  This minimum bound shouldn't
> be applied if the interval is 0.  Otherwise, one-shot timers will be
> converted into periodic ones.
> 
> This patch is specific to 4.11.y and 4.12.y.  Older -stable trees have a
> slightly different patch, and 4.13-rc2 isn't impacted due to a later
> refactoring.
> 
> Fixes: ff86bf0c65f1 ("alarmtimer: Rate limit periodic intervals")
> Reported-by: Ben Fennema <fennema@google.com>
> Signed-off-by: Greg Hackmann <ghackmann@google.com>
> Cc: stable@vger.kernel.org
> ---
>  kernel/time/alarmtimer.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Thanks, now queued up.

greg k-h

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web