Date: Tue, 5 May 2015 17:11:40 +0200 (CEST)
From: Thomas Gleixner <tglx@linutronix.de>
To: Jiri Bohac <jbohac@suse.cz>
cc: linux-kernel@vger.kernel.org, mhocko@suse.cz
Subject: Re: running hrtimer_start on an already active hrtimer?
In-Reply-To: <20150505133054.GA20643@midget.suse.cz>
Message-ID: <alpine.DEB.2.11.1505051657130.4225@nanos>
References: <20150505133054.GA20643@midget.suse.cz>
User-Agent: Alpine 2.11 (DEB 23 2013-08-11)
MIME-Version: 1.0
Content-Type: TEXT/PLAIN; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2061
Lines: 70

On Tue, 5 May 2015, Jiri Bohac wrote:
> Hi,
> 
> 
> I came across a strange bug (in a very old kernel) that triggers
> the
> 	BUG_ON(timer->state != HRTIMER_STATE_CALLBACK);
> in __run_hrtimer().
> 
> The code runs hrtimer_start() on an already started hrtimer. 
> Looking at the description of hrtimer_start() it looks
> like something that is allowed:
> 	/**
> 	 * hrtimer_start - (re)start an hrtimer on the current CPU
> 	...
> 	 * Returns:
> 	 *  0 on success
> 	 *  1 when the timer was active
> 
> Is this really supposed to work?
> 
> I think it's not immune to this race condition:
> 
> CPU0						CPU1
> __run_hrtimer()
>    __remove_hrtimer(...HRTIMER_STATE_CALLBACK)
>       //clears HRTIMER_STATE_ENQUEUED
>    ...
>    raw_spin_unlock(&cpu_base->lock);
>    restart = fn(timer);
> 						hrtimer_start()
> 						   __hrtimer_start_range_ns()
> 						      //remove_hrtimer() does nothing because
> 						      //  HRTIMER_STATE_ENQUEUED is not set
> 						      enqueue_hrtimer()
>    raw_spin_lock(&cpu_base->lock);
>    ...
>    BUG_ON(timer->state != HRTIMER_STATE_CALLBACK);
>    // state has HRTIMER_STATE_ENQUEUED set
>    

That's in the conditional path:

       if (restart != HRTIMER_NORESTART) {
       	  BUG_ON(timer->state != HRTIMER_STATE_CALLBACK);
	  ....

Which was intentional when we implemented hrtimers in the very
beginning. We wanted to enforce that restart from the callback is not
mixed with a start from some other place.


We removed that restriction recently (queued for 4.2 in
tip/timers/core)

> Should __hrtimer_start_range_ns() do something like
> hrtimer_cancel - i.e. explicitly check for ...
> HRTIMER_STATE_CALLBACK?

No, you cannot do anything about it other than lifting the restriction
or preventing the site which handles the hrtimer to start it.

Thanks,

	tglx
--
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/