From: Linus Torvalds <torvalds@linux-foundation.org>
Newsgroups: fa.linux.kernel
Subject: RE: [PATCH 03/16] IA64: use ACCESS_ONCE for rlimits
Date: Wed, 18 Nov 2009 19:49:18 UTC
Message-ID: <fa.B+72DUd0WNNBw/qvxd2zb//l6p4@ifi.uio.no>

On Wed, 18 Nov 2009, Luck, Tony wrote:
>
> > Make sure compiler won't do weird things with limits. E.g. fetching
> > them twice may return 2 different values after writable limits are
> > implemented.
>
> -	if (size > task->signal->rlim[RLIMIT_MEMLOCK].rlim_cur)
> +	if (size > ACCESS_ONCE(task->signal->rlim[RLIMIT_MEMLOCK].rlim_cur))
>
> I don't see how this helps.  If someone else is changing limits while
> we are looking at them, then there is a race.  We either get the old
> or the new value.  Using ACCESS_ONCE (which on ia64 forces a "volatile"
> access, which will make the compiler generate "ld.acq" rather than a
> plain "ld") won't make any difference to this race.
>
> Please explain what issue you see with the current code.

The problem may not be in _that_ particular code, but imagine code like
this:

	if (a > MEMORY) {
		do1;
		do2;
		do3;
	} else {
		do2;
	}

where the compiler could actually turn this into (having noticed that
neither "do1" nor "do2" can alias with MEMORY):

	if (a > MEMORY)
		do1;
	do2;
	if (a > MEMORY)
		do3;

and now what you end up having is a situation where it's possible that
"do1" gets executed but "do3" does not (or vice versa).

Notice how when you look at the code, it looks impossible, and then you
get subtle security bugs.

Now, you may say that "but _my_ code doesn't have that "else" statement",
and maybe you're right. In fact, maybe the source code was really just

	if (a > MEMORY)
		return something();
	return do_something_else();

and you are _sure_ that the ACCESS_ONCE() cannot possibly be needed. But
what if those 'something()' and 'do_something_else()' were inlines, and
the compiler internally turns it into

	if (a > MEMORY) {
		ret = something();
	} else {
		ret = do_something_else();
	}
	return ret;

and you now hit the case above where part of it was shared after all, and
the compiler for some strange reason (register reload, whatever) ends up
doing it as two conditionals after all?

The thing is, you can't _prove_ that the compiler won't do it, especially
if you end up changing the code later (without thinking about the fact
that you're loading things without locking).

So the rule is: if you access unlocked values, you use ACCESS_ONCE(). You
don't say "but it can't matter". Because you simply don't know.

			Linus

From: Linus Torvalds <torvalds@linux-foundation.org>
Newsgroups: fa.linux.kernel
Subject: Re: [PATCH 06/16] SPARC: use ACCESS_ONCE for rlimits
Date: Wed, 18 Nov 2009 18:10:25 UTC
Message-ID: <fa.uZuvIumKMns0BsRVWAMSUQyleX0@ifi.uio.no>

On Wed, 18 Nov 2009, David Miller wrote:
>
> But I wonder have we really seen the compiler create this
> kind of situation?  Or is this patch series based upon the
> fact that it "could happen"?

We have seen things like that in practice - where the compiler re-loads a
value twice, rather than use a copy like the source code did.

That said, it's rare, to the point of being _almost_ unheard of. It's much
more common that gcc generates bad code by doing the reverse (trying to
keep things in registers and spilling, instead of just re-generating the
value). There are very very few cases where ACCESS_ONCE() actually matters
for correctness.

Because in practice, the value is either modified some way (and spilling
it is cheaper than re-computing the modification), or there's just some
operation that might act as a memory barrier and alias the original memory
location so gcc wouldn't dare re-load anyway.

However, one of the nice things about ACCESS_ONCE() is that it's also a
big flag for "this value is loaded without locking, on purpose".

So even if it doesn't then actually change code generation significantly
(most common end result especially on x86 that has most ALU instructions
taking memory operations: gcc generates slightly worse code due to getting
nervous about 'volatile' and not combining instructions), it's a big
honking piece of programmer documentation: look out!

It's basically a heads-up for lockless programming like RCU. As such, it
can be something scary, but when it's done right, it's a good thing. And I
think that for rlimits, we do have a good reason to say "sure, somebody
else may change the limit values concurrently, but we don't really care:
we just want _one_ value, whether it's the old or the new one".

That said, the patch you Ack'ed is in the series of patches that I hated,
and Nak'ed for other reasons (namely "-EEXPRESSIONTOOCOMPLICATEDTOLIVE").

			Linus

From: Linus Torvalds <torvalds@linux-foundation.org>
Newsgroups: fa.linux.kernel
Subject: Re: [PATCH 1/1] sched: prevent divide by zero error in
Date: Sat, 29 Nov 2008 19:21:27 UTC
Message-ID: <fa.hCRd5eLdiNNcB6sWMGMEYc/e+cw@ifi.uio.no>

On Wed, 26 Nov 2008, Steven Rostedt wrote:
>  {
>  	struct rq *rq = cpu_rq(cpu);
> +	unsigned long nr_running = rq->nr_running;
>
> -	if (rq->nr_running)
> -		rq->avg_load_per_task = rq->load.weight / rq->nr_running;
> +	if (nr_running)
> +		rq->avg_load_per_task = rq->load.weight / nr_running;
>  	else
>  		rq->avg_load_per_task = 0;

I don't think this necessarily fixes it.

There's nothing that keeps gcc from deciding not to reload rq->nr_running.

Of course, in _practice_, I don't think gcc ever will (if it decides that
it will spill, gcc is likely going to decide that it will literally spill
the local variable to the stack rather than decide to reload off the
pointer), but it's a valid compiler optimization, and it even has a name
(rematerialization).

So I suspect that your patch does fix the bug, but it still leaves the
fairly unlikely _potential_ for it to re-appear at some point.

We have ACCESS_ONCE() as a macro to guarantee that the compiler doesn't
rematerialize a pointer access. That also would clarify the fact that we
access something unsafe outside a lock.

			Linus