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

[Hillf Danton]

On Sat, Dec 25, 2010 at 10:12 AM, Yong Zhang <yong.zhang0@xxxxxxxxx> 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@xxxxxxxxx> wrote:
>> > On Thu, Dec 23, 2010 at 9:29 PM, Hillf Danton <dhillf@xxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/