linux-kernel - Re: [PATCH] fix __percpu annotation in asm-generic

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]

Date:   Sat, 30 Nov 2019 01:00:37 +0100
From:   Luc Van Oostenryck <luc.vanoostenryck@...il.com>
To:     Christopher Lameter <cl@...ux.com>
Cc:     Dennis Zhou <dennis@...nel.org>,
        Ben Dooks <ben.dooks@...ethink.co.uk>,
        linux-arch@...r.kernel.org, linux-kernel@...r.kernel.org,
        Tejun Heo <tj@...nel.org>, Nicholas Piggin <npiggin@...il.com>,
        Arnd Bergmann <arnd@...db.de>
Subject: Re: [PATCH] fix __percpu annotation in asm-generic

On Fri, Nov 29, 2019 at 06:11:59PM +0000, Christopher Lameter wrote:
> On Wed, 27 Nov 2019, Luc Van Oostenryck wrote:
> 
> > 1) it would strip any address space, not just __percpu, so:
> >    it would need to be combined with __verify_pcpu_ptr() or,
> >    * a better name should be used,
> 
> typeof_cast_kernel() to express the fact that it creates a kernel pointer
> and ignored the attributes??

typeof_strip_address_space() would, I think, express this better. 
It's not obvious at all to me that 'kernel' in 'typeof_cast_kernel()'
relates to the (default) kernel address space.
Maybe it's just me. I don't know.

> >    * it should be defined in a generic header, any idea where?
> 
> include/linux/compiler-types.h

Yes, OK.

> > 2) while I find the current solution:
> > 	typeof(T) __kernel __force *ptr = ...;
> 
> It would be
> 
>    typeof_cast_kernel(&T) *xx = xxx
> 
> or so?

No, it would not. __percpu, and more generally, the address space
is a property of the object, not of its address.
For example, let's say T is a __percpu object:
	int __percpu obj;
then '&T' is just a 'normal'/__kernel pointer to it:
	int __percpu *;
There is nothing to strip (it would be if the __percpu
would be 'on the other side of the *': int * __percpu).
It's exactly the same as with 'const': a 'const char *'
is not const, only a pointer to const.

The situation with raw_cpu_generic_add_return() is:
- pcp is a lvalue of of a __percpu object of type T, so:
	typeof(pcp)  := T __percpu
- pcp's address is given to raw_cpu_ptr(), so
	typeof(&pcp) := T __percpu *
- raw_cpu_ptr() return the corresponding __kernel pointer
  (adjusted for the current percu offset), so:
	typeof(raw_cpu_ptr(&pcp)) := T *
- so, the macro needs to declare a variable __p of type T*
  hence:
	typeof(pcp) __kernel __force *__p;
  or, with this new macro:
	typeof_cast_kernel(pcp) *__p;

Maybe a better solution would be to directly play at pointer
level and thus have something like this:
	typeof_<some good name>(&pcp) __p = raw_cpu_ptr(&pcp);
or even:
	__kernel_pointer(&pcp) __p = raw_cpu_ptr(&pcp);
I dunno.

Note: at implementation level, it complicates things slightly
      to want this 'strip_percpu' macro to behaves like typeof()
      because it means that it can take in argument either an
      expression or a type. And if it's a type, you can't do a
      simple cast on it, you need to declare an intermediate
      variable, hence the horrible:
	  typeof(({ typeof(T) __kernel __force __fakename; __fakename; }))

Note: it would be much much nicer to do all these type generic
      macros with '__auto_type' (only supported in GCC 4.9 IIUC
      and supported in sparse but it shouldn't be very hard to do)..


-- Luc