linux-kernel - Re: linux-next: percpu tree build warning

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: <20091125134058.GA9097@elte.hu>
Date:	Wed, 25 Nov 2009 14:40:58 +0100
From:	Ingo Molnar <mingo@...e.hu>
To:	Tejun Heo <tj@...nel.org>
Cc:	Stephen Rothwell <sfr@...b.auug.org.au>,
	Fr??d??ric Weisbecker <fweisbec@...il.com>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	Rusty Russell <rusty@...tcorp.com.au>,
	Christoph Lameter <cl@...ux-foundation.org>,
	linux-next@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: linux-next: percpu tree build warning

* Tejun Heo <tj@...nel.org> wrote:

> > If yes then that needs to be fixed in the percpu tree. per-cpu 
> > variables used to have a __per_cpu prefix and that should be 
> > maintained - the two namespaces are obviously separate on the 
> > logical space, so they should never overlap in the implementational 
> > space either.
> 
> If all we ever have are static variables, the prefix may be fine but 
> with dynamic percpu variables now basically being the same first class 
> citizen but prefix just doesn't cut it.  It just ends up adding more 
> confusion.  The transition will be a bit painful (but not too much, 
> how many of these reports have we had?  Only several) but after that 
> it's just plain local/global symbol collision the compiler would have 
> no problem warning about.  It behaves exactly like other global 
> symbols.
> 
> Percpu symbols and variables belong to a different address space than 
> normal symbols.  Unfortunately, C doesn't have support for such thing. 

That argument does not parse for me. Obviously no sane programming 
language should allow shadowed variables which are used in the same way 
- it's way too easy to use the wrong one.

But we have _no_ real shadowing here - it's a pure artifact of how the 
percpu symbol space is mapped back into C - and the collision (which 
does not exist in the program space) is created where none existed 
before.

In other words: you are solving a problem that does not exist - you 
cannot mix up a local C variable and a percpu variable. The two spaces 
are clearly separated via definition and APIs. A C variable is defined 
via:

  unsigned long *dr7;

and is used via:

  dr7

While a percpu variable is defined and used in completely different 
ways:

  DEFINE_PER_CPU(unsigned long, dr7);

and is used via:

  __get_cpu_var(cpu_dr7);

It's analogous as if we had a 'struct percpu' C structure, and 
dereferenced it via:

 cpu->dr7.

Note that we dont require it to be renamed to cpu->cpu_dr7.

And look at your own 'cleanup' patch - it changes the percpu name to 
'cpu_dr7'. That results in nonsensical repetition:

        dr7 = &__get_cpu_var(cpu_dr7);

I already said it's a percpu variable, via the __get_cpu_var() 
primitive. Why do i have to type cpu_ again to express this, hm?

These kinds of messy interactions between clearly disjunct name spaces 
are bad IMO. And i dont see how dynamic percpu variables change this in 
any way - none of the above is a dynamic percpu variable.

I've applied your patch to not hold things up in linux-next (shadowing 
is dangerous) but i dont see how your arguments add up.

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