Prevent overflow in the computation of timer expiry time inside apply_slack(). Signed-off-by: Jiri Bohac <jbohac@suse.cz> Suggested-by: Deborah Townsend <dstownse@us.ibm.com> diff --git a/kernel/timer.c b/kernel/timer.c index 87bd529..4c36c91 100644 --- a/kernel/timer.c +++ b/kernel/timer.c @@ -838,7 +838,7 @@ unsigned long apply_slack(struct timer_list *timer, unsigned long expires) bit = find_last_bit(&mask, BITS_PER_LONG); - mask = (1 << bit) - 1; + mask = (1LL << bit) - 1; expires_limit = expires_limit & ~(mask); -- Jiri Bohac <jbohac@suse.cz> SUSE Labs, SUSE CZ
On Thu, 17 Apr 2014, Jiri Bohac wrote:
> Prevent overflow in the computation of timer expiry time inside
> apply_slack().
What's the impact of this overflow?
We don't want changelogs which describe what they do, we want them to
describe WHY and WHAT kind of problem the patch fixes.
Thanks,
tglx
On architectures with sizeof(int) < sizeof (long), the computation of mask inside apply_slack() can be undefined if the computed bit is > 32. E.g. with: expires = 0xffffe6f5 and slack = 25, we get: expires_limit = 0x20000000e bit = 33 mask = (1 << 33) - 1 /* undefined */ On x86, mask becomes 1 and and the slack is not applied properly. On s390, mask is -1, expires is set to 0 and the timer fires immediately. Signed-off-by: Jiri Bohac <jbohac@suse.cz> Suggested-by: Deborah Townsend <dstownse@us.ibm.com> diff --git a/kernel/timer.c b/kernel/timer.c index 87bd529..4c36c91 100644 --- a/kernel/timer.c +++ b/kernel/timer.c @@ -838,7 +838,7 @@ unsigned long apply_slack(struct timer_list *timer, unsigned long expires) bit = find_last_bit(&mask, BITS_PER_LONG); - mask = (1 << bit) - 1; + mask = (1LL << bit) - 1; expires_limit = expires_limit & ~(mask); -- -- Jiri Bohac <jbohac@suse.cz> SUSE Labs, SUSE CZ
Thomas, does this make sense now, with the new description?
On Fri, Apr 18, 2014 at 05:23:11PM +0200, Jiri Bohac wrote:
> On architectures with sizeof(int) < sizeof (long), the
> computation of mask inside apply_slack() can be undefined if the
> computed bit is > 32.
>
> E.g. with: expires = 0xffffe6f5 and slack = 25, we get:
>
> expires_limit = 0x20000000e
> bit = 33
> mask = (1 << 33) - 1 /* undefined */
>
> On x86, mask becomes 1 and and the slack is not applied properly.
> On s390, mask is -1, expires is set to 0 and the timer fires immediately.
>
> Signed-off-by: Jiri Bohac <jbohac@suse.cz>
> Suggested-by: Deborah Townsend <dstownse@us.ibm.com>
>
> diff --git a/kernel/timer.c b/kernel/timer.c
> index 87bd529..4c36c91 100644
> --- a/kernel/timer.c
> +++ b/kernel/timer.c
> @@ -838,7 +838,7 @@ unsigned long apply_slack(struct timer_list *timer, unsigned long expires)
>
> bit = find_last_bit(&mask, BITS_PER_LONG);
>
> - mask = (1 << bit) - 1;
> + mask = (1LL << bit) - 1;
>
> expires_limit = expires_limit & ~(mask);
>
Thanks,
--
Jiri Bohac <jbohac@suse.cz>
SUSE Labs, SUSE CZ
On Tue, 29 Apr 2014, Jiri Bohac wrote: > Thomas, does this make sense now, with the new description? Yep, except > On Fri, Apr 18, 2014 at 05:23:11PM +0200, Jiri Bohac wrote: > > On architectures with sizeof(int) < sizeof (long), the > > computation of mask inside apply_slack() can be undefined if the > > computed bit is > 32. > > > > E.g. with: expires = 0xffffe6f5 and slack = 25, we get: > > > > expires_limit = 0x20000000e > > bit = 33 > > mask = (1 << 33) - 1 /* undefined */ > > > > On x86, mask becomes 1 and and the slack is not applied properly. > > On s390, mask is -1, expires is set to 0 and the timer fires immediately. > > > > Signed-off-by: Jiri Bohac <jbohac@suse.cz> > > Suggested-by: Deborah Townsend <dstownse@us.ibm.com> > > > > diff --git a/kernel/timer.c b/kernel/timer.c > > index 87bd529..4c36c91 100644 > > --- a/kernel/timer.c > > +++ b/kernel/timer.c > > @@ -838,7 +838,7 @@ unsigned long apply_slack(struct timer_list *timer, unsigned long expires) > > > > bit = find_last_bit(&mask, BITS_PER_LONG); > > > > - mask = (1 << bit) - 1; > > + mask = (1LL << bit) - 1; This should be 1UL, shouldn't it? > > expires_limit = expires_limit & ~(mask); > > > > Thanks, > > -- > Jiri Bohac <jbohac@suse.cz> > SUSE Labs, SUSE CZ > >
On Tue, Apr 29, 2014 at 07:22:25PM +0200, Thomas Gleixner wrote: > > > + mask = (1LL << bit) - 1; > > This should be 1UL, shouldn't it? yes, good catch, thanks! On architectures with sizeof(int) < sizeof (long), the computation of mask inside apply_slack() can be undefined if the computed bit is > 32. E.g. with: expires = 0xffffe6f5 and slack = 25, we get: expires_limit = 0x20000000e bit = 33 mask = (1 << 33) - 1 /* undefined */ On x86, mask becomes 1 and and the slack is not applied properly. On s390, mask is -1, expires is set to 0 and the timer fires immediately. Signed-off-by: Jiri Bohac <jbohac@suse.cz> Suggested-by: Deborah Townsend <dstownse@us.ibm.com> diff --git a/kernel/timer.c b/kernel/timer.c index 87bd529..4c36c91 100644 --- a/kernel/timer.c +++ b/kernel/timer.c @@ -838,7 +838,7 @@ unsigned long apply_slack(struct timer_list *timer, unsigned long expires) bit = find_last_bit(&mask, BITS_PER_LONG); - mask = (1 << bit) - 1; + mask = (1UL << bit) - 1; expires_limit = expires_limit & ~(mask); -- Jiri Bohac <jbohac@suse.cz> SUSE Labs, SUSE CZ
Commit-ID: 98a01e779f3c66b0b11cd7e64d531c0e41c95762 Gitweb: http://git.kernel.org/tip/98a01e779f3c66b0b11cd7e64d531c0e41c95762 Author: Jiri Bohac <jbohac@suse.cz> AuthorDate: Fri, 18 Apr 2014 17:23:11 +0200 Committer: Thomas Gleixner <tglx@linutronix.de> CommitDate: Wed, 30 Apr 2014 13:46:17 +0200 timer: Prevent overflow in apply_slack On architectures with sizeof(int) < sizeof (long), the computation of mask inside apply_slack() can be undefined if the computed bit is > 32. E.g. with: expires = 0xffffe6f5 and slack = 25, we get: expires_limit = 0x20000000e bit = 33 mask = (1 << 33) - 1 /* undefined */ On x86, mask becomes 1 and and the slack is not applied properly. On s390, mask is -1, expires is set to 0 and the timer fires immediately. Use 1UL << bit to solve that issue. Suggested-by: Deborah Townsend <dstownse@us.ibm.com> Signed-off-by: Jiri Bohac <jbohac@suse.cz> Cc: stable@vger.kernel.org Link: http://lkml.kernel.org/r/20140418152310.GA13654@midget.suse.cz Signed-off-by: Thomas Gleixner <tglx@linutronix.de> --- kernel/timer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kernel/timer.c b/kernel/timer.c index 87bd529..3bb01a3 100644 --- a/kernel/timer.c +++ b/kernel/timer.c @@ -838,7 +838,7 @@ unsigned long apply_slack(struct timer_list *timer, unsigned long expires) bit = find_last_bit(&mask, BITS_PER_LONG); - mask = (1 << bit) - 1; + mask = (1UL << bit) - 1; expires_limit = expires_limit & ~(mask);