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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Sat, 25 Dec 2010 22:19:11 +0800
From:	Hillf Danton <dhillf@...il.com>
To:	Yong Zhang <yong.zhang0@...il.com>
Cc:	Thomas Gleixner <tglx@...utronix.de>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] fix unsafe operation in high resolution timer

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