2011-03-11 13:59:25

by Oleg Nesterov

Subject: Re: timerfd-add-tfd_notify_clock_set-to-watch-for-clock-changes.patch added to -mm tree

> @@ -218,10 +266,12 @@ SYSCALL_DEFINE4(timerfd_settime, int, uf
> * it to the new values.
> */
> for (;;) {
> + spin_lock(&notifiers_lock);
> spin_lock_irq(&ctx->wqh.lock);
> - if (hrtimer_try_to_cancel(&ctx->tmr) >= 0)
> + if (!list_empty(&notifiers_list) || hrtimer_try_to_cancel(&ctx->tmr) >= 0)
> break;

Confused. Why do we check the global notifiers_list?

IOW. Suppose that this list is not empty and timerfd_settime() is called
without TFD_NOTIFY_CLOCK_SET. Now we are going to reprogramm the timer
without stopping it?

And. What if timerfd_settime(TFD_NOTIFY_CLOCK_SET, utmr => NULL) is called
twice? timerfd_setup() blindly does list_add(), and we corrupt the list, no?

Oleg.

2011-03-11 15:09:03

by Alexander Shishkin

Subject: Re: timerfd-add-tfd_notify_clock_set-to-watch-for-clock-changes.patch added to -mm tree

On Fri, Mar 11, 2011 at 02:50:23PM +0100, Oleg Nesterov wrote:
> > @@ -218,10 +266,12 @@ SYSCALL_DEFINE4(timerfd_settime, int, uf
> > * it to the new values.
> > */
> > for (;;) {
> > + spin_lock(&notifiers_lock);
> > spin_lock_irq(&ctx->wqh.lock);
> > - if (hrtimer_try_to_cancel(&ctx->tmr) >= 0)
> > + if (!list_empty(&notifiers_list) || hrtimer_try_to_cancel(&ctx->tmr) >= 0)
> > break;
>
> Confused. Why do we check the global notifiers_list?

You're right, it's a bug, should be &ctx->notifiers_list.

> IOW. Suppose that this list is not empty and timerfd_settime() is called
> without TFD_NOTIFY_CLOCK_SET. Now we are going to reprogramm the timer
> without stopping it?
>
> And. What if timerfd_settime(TFD_NOTIFY_CLOCK_SET, utmr => NULL) is called
> twice? timerfd_setup() blindly does list_add(), and we corrupt the list, no?

Another catch.

There will be a quite a bit of rework required, though, considering tglx's
comments.

Thanks!
--
Alex

2011-03-11 16:24:19

by Thomas Gleixner

Subject: Re: timerfd-add-tfd_notify_clock_set-to-watch-for-clock-changes.patch added to -mm tree

On Fri, 11 Mar 2011, Oleg Nesterov wrote:

> > @@ -218,10 +266,12 @@ SYSCALL_DEFINE4(timerfd_settime, int, uf
> > * it to the new values.
> > */
> > for (;;) {
> > + spin_lock(&notifiers_lock);
> > spin_lock_irq(&ctx->wqh.lock);
> > - if (hrtimer_try_to_cancel(&ctx->tmr) >= 0)
> > + if (!list_empty(&notifiers_list) || hrtimer_try_to_cancel(&ctx->tmr) >= 0)
> > break;
>
> Confused. Why do we check the global notifiers_list?
>
> IOW. Suppose that this list is not empty and timerfd_settime() is called
> without TFD_NOTIFY_CLOCK_SET. Now we are going to reprogramm the timer
> without stopping it?
>
> And. What if timerfd_settime(TFD_NOTIFY_CLOCK_SET, utmr => NULL) is called
> twice? timerfd_setup() blindly does list_add(), and we corrupt the list, no?

And why is this hack in -mm at all ?

Thanks,

tglx