[PATCH] fix unsafe operation in high resolution timer

[PATCH] fix unsafe operation in high resolution timer

[Hillf Danton]

After calling the callback function of hrtimer, the timer could become
unreliable in corner cases where the timer will no longer be queued
and the mm segment, in which the timer is embedded, could be reclaimed
in the callback.

The unreliability is fixed by checking the result of callback before
operating the timer again.

Signed-off-by: Hillf Danton <dhillf@gmail.com>
---

--- a/kernel/hrtimer.c	2010-11-01 19:54:12.000000000 +0800
+++ b/kernel/hrtimer.c	2010-12-23 21:17:02.000000000 +0800
@@ -1225,6 +1225,7 @@ static void __run_hrtimer(struct hrtimer
 	raw_spin_unlock(&cpu_base->lock);
 	trace_hrtimer_expire_entry(timer, now);
 	restart = fn(timer);
+	if (restart != HRTIMER_NORESTART)
 	trace_hrtimer_expire_exit(timer);
 	raw_spin_lock(&cpu_base->lock);

@@ -1236,11 +1237,8 @@ static void __run_hrtimer(struct hrtimer
 	if (restart != HRTIMER_NORESTART) {
 		BUG_ON(timer->state != HRTIMER_STATE_CALLBACK);
 		enqueue_hrtimer(timer, base);
+		timer->state &= ~HRTIMER_STATE_CALLBACK;
 	}
-
-	WARN_ON_ONCE(!(timer->state & HRTIMER_STATE_CALLBACK));
-
-	timer->state &= ~HRTIMER_STATE_CALLBACK;
 }

 #ifdef CONFIG_HIGH_RES_TIMERS

Re: [PATCH] fix unsafe operation in high resolution timer

[Yong Zhang]

On Thu, Dec 23, 2010 at 9:29 PM, Hillf Danton <dhillf@gmail.com> wrote:
> After calling the callback function of hrtimer, the timer could become
> unreliable in corner cases where the timer will no longer be queued
> and the mm segment, in which the timer is embedded, could be reclaimed
> in the callback.
>
> The unreliability is fixed by checking the result of callback before
> operating the timer again.

Though the patch is buggy. But it actually explores a real problem.

We can't deference a maybe freed object. I'm not sure how
to fix it. Or we just prevent the hrtimer->fn free the object which
contains hrtimer?

Thomas, what's your comment?

Thanks,
Yong
>
> Signed-off-by: Hillf Danton <dhillf@gmail.com>
> ---
>
> --- a/kernel/hrtimer.c  2010-11-01 19:54:12.000000000 +0800
> +++ b/kernel/hrtimer.c  2010-12-23 21:17:02.000000000 +0800
> @@ -1225,6 +1225,7 @@ static void __run_hrtimer(struct hrtimer
>        raw_spin_unlock(&cpu_base->lock);
>        trace_hrtimer_expire_entry(timer, now);
>        restart = fn(timer);
> +       if (restart != HRTIMER_NORESTART)
>        trace_hrtimer_expire_exit(timer);
>        raw_spin_lock(&cpu_base->lock);
>
> @@ -1236,11 +1237,8 @@ static void __run_hrtimer(struct hrtimer
>        if (restart != HRTIMER_NORESTART) {
>                BUG_ON(timer->state != HRTIMER_STATE_CALLBACK);
>                enqueue_hrtimer(timer, base);
> +               timer->state &= ~HRTIMER_STATE_CALLBACK;
>        }
> -
> -       WARN_ON_ONCE(!(timer->state & HRTIMER_STATE_CALLBACK));
> -
> -       timer->state &= ~HRTIMER_STATE_CALLBACK;
>  }
>
>  #ifdef CONFIG_HIGH_RES_TIMERS
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

-- 
Only stand for myself.

Re: [PATCH] fix unsafe operation in high resolution timer

[Hillf Danton]

On Fri, Dec 24, 2010 at 3:17 PM, Yong Zhang <yong.zhang0@gmail.com> wrote:
> On Thu, Dec 23, 2010 at 9:29 PM, Hillf Danton <dhillf@gmail.com> wrote:
>> After calling the callback function of hrtimer, the timer could become
>> unreliable in corner cases where the timer will no longer be queued
>> and the mm segment, in which the timer is embedded, could be reclaimed
>> in the callback.
>>
>> The unreliability is fixed by checking the result of callback before
>> operating the timer again.
>
> Though the patch is buggy. But it actually explores a real problem.
>

Would you please finger out why the patch is buggy?

Hillf

> We can't deference a maybe freed object. I'm not sure how
> to fix it. Or we just prevent the hrtimer->fn free the object which
> contains hrtimer?
>
> Thomas, what's your comment?
>
> Thanks,
> Yong
>>
>> Signed-off-by: Hillf Danton <dhillf@gmail.com>
>> ---
>>
>> --- a/kernel/hrtimer.c  2010-11-01 19:54:12.000000000 +0800
>> +++ b/kernel/hrtimer.c  2010-12-23 21:17:02.000000000 +0800
>> @@ -1225,6 +1225,7 @@ static void __run_hrtimer(struct hrtimer
>>        raw_spin_unlock(&cpu_base->lock);
>>        trace_hrtimer_expire_entry(timer, now);
>>        restart = fn(timer);
>> +       if (restart != HRTIMER_NORESTART)
>>        trace_hrtimer_expire_exit(timer);
>>        raw_spin_lock(&cpu_base->lock);
>>
>> @@ -1236,11 +1237,8 @@ static void __run_hrtimer(struct hrtimer
>>        if (restart != HRTIMER_NORESTART) {
>>                BUG_ON(timer->state != HRTIMER_STATE_CALLBACK);
>>                enqueue_hrtimer(timer, base);
>> +               timer->state &= ~HRTIMER_STATE_CALLBACK;
>>        }
>> -
>> -       WARN_ON_ONCE(!(timer->state & HRTIMER_STATE_CALLBACK));
>> -
>> -       timer->state &= ~HRTIMER_STATE_CALLBACK;
>>  }
>>
>>  #ifdef CONFIG_HIGH_RES_TIMERS
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/
>>
>
> --
> Only stand for myself.
>

Re: [PATCH] fix unsafe operation in high resolution timer

[Yong Zhang]

On Fri, Dec 24, 2010 at 10:28:52PM +0800, Hillf Danton wrote:
> On Fri, Dec 24, 2010 at 3:17 PM, Yong Zhang <yong.zhang0@gmail.com> wrote:
> > On Thu, Dec 23, 2010 at 9:29 PM, Hillf Danton <dhillf@gmail.com> wrote:
> >> After calling the callback function of hrtimer, the timer could become
> >> unreliable in corner cases where the timer will no longer be queued
> >> and the mm segment, in which the timer is embedded, could be reclaimed
> >> in the callback.
> >>
> >> The unreliability is fixed by checking the result of callback before
> >> operating the timer again.
> >
> > Though the patch is buggy. But it actually explores a real problem.
> >
> 
> Would you please finger out why the patch is buggy?

No problem.

Actually the patch change the behavior of current hrtimer.
See comments below :)

> >> ---
> >>
> >> --- a/kernel/hrtimer.c  2010-11-01 19:54:12.000000000 +0800
> >> +++ b/kernel/hrtimer.c  2010-12-23 21:17:02.000000000 +0800
> >> @@ -1225,6 +1225,7 @@ static void __run_hrtimer(struct hrtimer
> >>        raw_spin_unlock(&cpu_base->lock);
> >>        trace_hrtimer_expire_entry(timer, now);
> >>        restart = fn(timer);
> >> +       if (restart != HRTIMER_NORESTART)
> >>        trace_hrtimer_expire_exit(timer);
> >>        raw_spin_lock(&cpu_base->lock);
> >>
> >> @@ -1236,11 +1237,8 @@ static void __run_hrtimer(struct hrtimer
> >>        if (restart != HRTIMER_NORESTART) {
> >>                BUG_ON(timer->state != HRTIMER_STATE_CALLBACK);
> >>                enqueue_hrtimer(timer, base);
> >> +               timer->state &= ~HRTIMER_STATE_CALLBACK;

HRTIMER_STATE_CALLBACK is only cleared for RESTART hrtimer with
your modification.

> >>        }
> >> -
> >> -       WARN_ON_ONCE(!(timer->state & HRTIMER_STATE_CALLBACK));
> >> -
> >> -       timer->state &= ~HRTIMER_STATE_CALLBACK;

But for a hrtimer which is not free in its callback, like a
static defined one. the hrtimer could be referenced at the same
time. So here you cann't just delete the two lines.

Thanks,
Yong

Re: [PATCH] fix unsafe operation in high resolution timer

[Hillf Danton]

On Sat, Dec 25, 2010 at 10:12 AM, Yong Zhang <yong.zhang0@gmail.com> wrote:
> On Fri, Dec 24, 2010 at 10:28:52PM +0800, Hillf Danton wrote:
>> On Fri, Dec 24, 2010 at 3:17 PM, Yong Zhang <yong.zhang0@gmail.com> wrote:
>> > On Thu, Dec 23, 2010 at 9:29 PM, Hillf Danton <dhillf@gmail.com> wrote:
>> >> After calling the callback function of hrtimer, the timer could become
>> >> unreliable in corner cases where the timer will no longer be queued
>> >> and the mm segment, in which the timer is embedded, could be reclaimed
>> >> in the callback.
>> >>
>> >> The unreliability is fixed by checking the result of callback before
>> >> operating the timer again.
>> >
>> > Though the patch is buggy. But it actually explores a real problem.
>> >
>>
>> Would you please finger out why the patch is buggy?
>
> No problem.

thanks, Yong, for your explanation, and merry Christmas.

>
> Actually the patch change the behavior of current hrtimer.
> See comments below :)
>
>> >> ---
>> >>
>> >> --- a/kernel/hrtimer.c  2010-11-01 19:54:12.000000000 +0800
>> >> +++ b/kernel/hrtimer.c  2010-12-23 21:17:02.000000000 +0800
>> >> @@ -1225,6 +1225,7 @@ static void __run_hrtimer(struct hrtimer
>> >>        raw_spin_unlock(&cpu_base->lock);
>> >>        trace_hrtimer_expire_entry(timer, now);
>> >>        restart = fn(timer);
>> >> +       if (restart != HRTIMER_NORESTART)
>> >>        trace_hrtimer_expire_exit(timer);
>> >>        raw_spin_lock(&cpu_base->lock);
>> >>
>> >> @@ -1236,11 +1237,8 @@ static void __run_hrtimer(struct hrtimer
>> >>        if (restart != HRTIMER_NORESTART) {
>> >>                BUG_ON(timer->state != HRTIMER_STATE_CALLBACK);
>> >>                enqueue_hrtimer(timer, base);
>> >> +               timer->state &= ~HRTIMER_STATE_CALLBACK;
>
> HRTIMER_STATE_CALLBACK is only cleared for RESTART hrtimer with
> your modification.
>
>> >>        }
>> >> -
>> >> -       WARN_ON_ONCE(!(timer->state & HRTIMER_STATE_CALLBACK));
>> >> -
>> >> -       timer->state &= ~HRTIMER_STATE_CALLBACK;
>
> But for a hrtimer which is not free in its callback, like a
> static defined one. the hrtimer could be referenced at the same
> time. So here you cann't just delete the two lines.

After callback, as you agree, it is hard to determine in the current
implementation if the hrtimer is static defined, though another bit
could be added, say, in the flag word of hrtimer, so cutting the two
lines off is deserved.
And more, who will take care of the NORESTART again after callback?

Cheers
Hillf

Re: [PATCH] fix unsafe operation in high resolution timer

[Yong Zhang]

On Sat, Dec 25, 2010 at 10:19:11PM +0800, Hillf Danton wrote:
> > But for a hrtimer which is not free in its callback, like a
> > static defined one. the hrtimer could be referenced at the same
> > time. So here you cann't just delete the two lines.
> 
> After callback, as you agree, it is hard to determine in the current
> implementation if the hrtimer is static defined, though another bit
> could be added, say, in the flag word of hrtimer

Yeah, that is an option, like a flag FREE_IN_CALLBACK/ONESHOT which
indicate that. Or just let the callback return another value like
HRTIMER_FREED. But as I said before I'm not sure what's the best way
to fix that. And maybe there's more suitable method.

BTW, is there any user who free the hrtimer in its callback?

> , so cutting the two
> lines off is deserved.
> And more, who will take care of the NORESTART again after callback?

It's not related to NORESTART, just HRTIMER_STATE_CALLBACK.
hrtimer'strategy somehow depends on HRTIMER_STATE_CALLBACK.
You can take a look at the caller of hrtimer_callback_running().

Thanks,
Yong

Re: [PATCH] fix unsafe operation in high resolution timer

[Hillf Danton]

On Sun, Dec 26, 2010 at 9:12 PM, Yong Zhang <yong.zhang0@gmail.com> wrote:
> On Sat, Dec 25, 2010 at 10:19:11PM +0800, Hillf Danton wrote:
>> > But for a hrtimer which is not free in its callback, like a
>> > static defined one. the hrtimer could be referenced at the same
>> > time. So here you cann't just delete the two lines.
>>
>> After callback, as you agree, it is hard to determine in the current
>> implementation if the hrtimer is static defined, though another bit
>> could be added, say, in the flag word of hrtimer
>
> Yeah, that is an option, like a flag FREE_IN_CALLBACK/ONESHOT which
> indicate that. Or just let the callback return another value like

The result of callback sounds fine.
Hillf

> HRTIMER_FREED. But as I said before I'm not sure what's the best way
> to fix that. And maybe there's more suitable method.
>
> BTW, is there any user who free the hrtimer in its callback?
>
>> , so cutting the two
>> lines off is deserved.
>> And more, who will take care of the NORESTART again after callback?
>
> It's not related to NORESTART, just HRTIMER_STATE_CALLBACK.
> hrtimer'strategy somehow depends on HRTIMER_STATE_CALLBACK.
> You can take a look at the caller of hrtimer_callback_running().
>
> Thanks,
> Yong
>

Re: [PATCH] fix unsafe operation in high resolution timer

[Hillf Danton]

On Sun, Dec 26, 2010 at 9:12 PM, Yong Zhang <yong.zhang0@gmail.com> wrote:
> On Sat, Dec 25, 2010 at 10:19:11PM +0800, Hillf Danton wrote:
>> > But for a hrtimer which is not free in its callback, like a
>> > static defined one. the hrtimer could be referenced at the same
>> > time. So here you cann't just delete the two lines.
>>
>> After callback, as you agree, it is hard to determine in the current
>> implementation if the hrtimer is static defined, though another bit
>> could be added, say, in the flag word of hrtimer
>
> Yeah, that is an option, like a flag FREE_IN_CALLBACK/ONESHOT which
> indicate that. Or just let the callback return another value like
> HRTIMER_FREED. But as I said before I'm not sure what's the best way
> to fix that. And maybe there's more suitable method.
>
> BTW, is there any user who free the hrtimer in its callback?
>
>> , so cutting the two
>> lines off is deserved.
>> And more, who will take care of the NORESTART again after callback?
>
> It's not related to NORESTART, just HRTIMER_STATE_CALLBACK.
> hrtimer'strategy somehow depends on HRTIMER_STATE_CALLBACK.
> You can take a look at the caller of hrtimer_callback_running().
>

Then the callback is responsible for signaling correctly.
Hillf
---

--- a/include/linux/hrtimer.h	2010-11-01 19:54:12.000000000 +0800
+++ b/include/linux/hrtimer.h	2010-12-29 23:43:16.000000000 +0800
@@ -44,6 +44,7 @@ enum hrtimer_mode {
 enum hrtimer_restart {
 	HRTIMER_NORESTART,	/* Timer is not restarted */
 	HRTIMER_RESTART,	/* Timer must be restarted */
+	HRTIMER_RECLAIM,	/* Timer is recycled */
 };

 /*
--- a/kernel/hrtimer.c	2010-11-01 19:54:12.000000000 +0800
+++ b/kernel/hrtimer.c	2010-12-29 23:45:48.000000000 +0800
@@ -1225,9 +1225,13 @@ static void __run_hrtimer(struct hrtimer
 	raw_spin_unlock(&cpu_base->lock);
 	trace_hrtimer_expire_entry(timer, now);
 	restart = fn(timer);
+	if (restart != HRTIMER_RECLAIM)
 	trace_hrtimer_expire_exit(timer);
 	raw_spin_lock(&cpu_base->lock);

+	if (restart == HRTIMER_RECLAIM)
+		return;
+
 	/*
 	 * Note: We clear the CALLBACK bit after enqueue_hrtimer and
 	 * we do not reprogramm the event hardware. Happens either in