Re: [RFC v6 19/62] powerpc: ability to create execute-disabled pkeys

From: Michael Ellerman
Date: Mon Jul 31 2017 - 08:59:25 EST


Ram Pai <linuxram@xxxxxxxxxx> writes:

> On Thu, Jul 27, 2017 at 11:54:31AM -0300, Thiago Jung Bauermann wrote:
>>
>> Ram Pai <linuxram@xxxxxxxxxx> writes:
>>
>> > --- a/arch/powerpc/include/asm/pkeys.h
>> > +++ b/arch/powerpc/include/asm/pkeys.h
>> > @@ -2,6 +2,18 @@
>> > #define _ASM_PPC64_PKEYS_H
>> >
>> > extern bool pkey_inited;
>> > +/* override any generic PKEY Permission defines */
>> > +#undef PKEY_DISABLE_ACCESS
>> > +#define PKEY_DISABLE_ACCESS 0x1
>> > +#undef PKEY_DISABLE_WRITE
>> > +#define PKEY_DISABLE_WRITE 0x2
>> > +#undef PKEY_DISABLE_EXECUTE
>> > +#define PKEY_DISABLE_EXECUTE 0x4
>> > +#undef PKEY_ACCESS_MASK
>> > +#define PKEY_ACCESS_MASK (PKEY_DISABLE_ACCESS |\
>> > + PKEY_DISABLE_WRITE |\
>> > + PKEY_DISABLE_EXECUTE)
>> > +
>>
>> Is it ok to #undef macros from another header? Especially since said
>> header is in uapi (include/uapi/asm-generic/mman-common.h).
>>
>> Also, it's unnecessary to undef the _ACCESS and _WRITE macros since they
>> are identical to the original definition. And since these macros are
>> originally defined in an uapi header, the powerpc-specific ones should
>> be in an uapi header as well, if I understand it correctly.
>
> The architectural neutral code allows the implementation to define the
> macros to its taste. powerpc headers due to legacy reason includes the
> include/uapi/asm-generic/mman-common.h header. That header includes the
> generic definitions of only PKEY_DISABLE_ACCESS and PKEY_DISABLE_WRITE.
> Unfortunately we end up importing them. I dont want to depend on them.
> Any changes there could effect us. Example if the generic uapi header
> changed PKEY_DISABLE_ACCESS to 0x4, we will have a conflict with
> PKEY_DISABLE_EXECUTE. Hence I undef them and define the it my way.

Don't do that.

The generic header can't change the values, it's an ABI.

Doing it this way risks the uapi value diverging from the value used in
the powerpc code (due to a change in the powerpc version), which would
mean userspace and the kernel wouldn't agree on what the values meant
... which would be exciting.

cheers