linux-kernel - Re: [PATCH 08/14] hrtimer: Allow hrtimer::function() to free the 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]

Message-ID: <20150608142749.GB13168@redhat.com>
Date:	Mon, 8 Jun 2015 16:27:49 +0200
From:	Oleg Nesterov <oleg@...hat.com>
To:	Peter Zijlstra <peterz@...radead.org>
Cc:	umgwanakikbuti@...il.com, mingo@...e.hu, ktkhai@...allels.com,
	rostedt@...dmis.org, tglx@...utronix.de, juri.lelli@...il.com,
	pang.xunlei@...aro.org, wanpeng.li@...ux.intel.com,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 08/14] hrtimer: Allow hrtimer::function() to free the
	timer

On 06/08, Peter Zijlstra wrote:
>
> On Mon, Jun 08, 2015 at 11:14:17AM +0200, Peter Zijlstra wrote:
> > > Finally. Suppose that timer->function() returns HRTIMER_RESTART
> > > and hrtimer_active() is called right after __run_hrtimer() sets
> > > cpu_base->running = NULL. I can't understand why hrtimer_active()
> > > can't miss ENQUEUED in this case. We have wmb() in between, yes,
> > > but then hrtimer_active() should do something like
> > >
> > > 	active = cpu_base->running == timer;
> > > 	if (!active) {
> > > 		rmb();
> > > 		active = state != HRTIMER_STATE_INACTIVE;
> > > 	}
> > >
> > > No?
> >
> > Hmm, good point. Let me think about that. It would be nice to be able to
> > avoid more memory barriers.

Yes, but otoh, can't we avoid seqcount_t altogether?

To remind, we assume that

	- "false positive" is fine. If we observe ENQUEUED or ->running
	  we can safely return true. It doesn't matter if the timer becomes
	  "inactive" right after return.

	- we need to fix migrate_hrtimer_list() and __hrtimer_start_range_ns()
	  to preserve ENQUEUED. This fixes the races with hrtimer_is_queued()
	  and hrtimer_active() we currently have.

Now, can't we simply do

	__run_hrtimer()
	{

		cpu_base->running = timer;

		wmb();				// 1

		__remove_hrtimer(INACTIVE);	// clears ENQUEUED

		fn();				// autorearm can set ENQUEUED again

		wmb();				// 2

		cpu_base->running = NULL;	// XXX
	}

	hrtimer_active(timer)
	{
		if (timer->state & ENQUEUED)
			return true;

		rmb();				// pairs with 1


		// We do not care if we race with __hrtimer_start_range_ns().
		// The running timer can't change its base.
		// If it was ENQUEUED, we rely on the previous check.

		base = timer->base->cpu_base;
		read_barrier_depends();
		if (base->running == timer)
			return true;

		rmb();				// pairs with 2

		// Avoid the race with auto-rearming timer. If we see the
		// result of XXX above we should also see ENQUEUED if it
		// was set by __run_hrtimer() or timer->function().
		//
		// We do not care if another thread does hrtimer_start()
		// and we miss ENQUEUED. In this case we can the "inactive"
		// window anyway, we can pretend that hrtimer_start() was
		// called after XXX above. So we can equally pretend that
		// hrtimer_active() was called in this window.
		//
		if (timer->state & ENQUEUED)
			return true;

		return false;
	}

Most probably I missed something... I'll try to think more, but perhaps
you see a hole immediately?

Oleg.

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