[PATCH] powerpc/vdso: Drop -mstack-protector-guard flags in 32-bit files with clang

[Nathan Chancellor]

Hi Christophe and Segher,

On Wed, Nov 06, 2024 at 07:37:52AM -0600, Segher Boessenkool wrote:
> On Wed, Nov 06, 2024 at 09:55:58AM +0100, Christophe Leroy wrote:
> > Le 30/10/2024 à 19:41, Nathan Chancellor a écrit :
> > >Under certain conditions, the 64-bit '-mstack-protector-guard' flags may
> > >end up in the 32-bit vDSO flags, resulting in build failures due to the
> > >structure of clang's argument parsing of the stack protector options,
> > >which validates the arguments of the stack protector guard flags
> > >unconditionally in the frontend, choking on the 64-bit values when
> > >targeting 32-bit:
> > >
> > >   clang: error: invalid value 'r13' in 'mstack-protector-guard-reg=', 
> > >   expected one of: r2
> > >   clang: error: invalid value 'r13' in 'mstack-protector-guard-reg=', 
> > >   expected one of: r2
> > >   make[3]: *** [arch/powerpc/kernel/vdso/Makefile:85: 
> > >   arch/powerpc/kernel/vdso/vgettimeofday-32.o] Error 1
> > >   make[3]: *** [arch/powerpc/kernel/vdso/Makefile:87: 
> > >   arch/powerpc/kernel/vdso/vgetrandom-32.o] Error 1
> > >
> > >Remove these flags by adding them to the CC32FLAGSREMOVE variable, which
> > >already handles situations similar to this. Additionally, reformat and
> > >align a comment better for the expanding CONFIG_CC_IS_CLANG block.
> > 
> > Is the problem really exclusively for 32-bit VDSO on 64-bit kernel ?

As far as I can tell, yes, as I do not think there are any other places
where flags for targeting one word size were being used when targeting
the other word size.

> > In any case, it is just wrong to have anything related to stack 
> > protection in VDSO, for this reason we have the following in Makefile:
> > 
> > ccflags-y += $(call cc-option, -fno-stack-protector)
> > 
> > If it is not enough, should we have more complete ?

That should be enough to disable the stack protector from my
understanding. It is just that clang's argument validation happens even
with -fno-stack-protector, so the flags need to contain valid values for
the target. This is true for GCC as well, it just supports any base
register like Segher mentions below so it does not hit any issue here:

  $ powerpc64-linux-gcc -fno-stack-protector -mstack-protector-guard=tls -mstack-protector-guard-reg=r50 -c -o /dev/null -x c /dev/null
  cc1: error: 'r50' is not a valid base register in '-mstack-protector-guard-reg='
  cc1: error: '-mstack-protector-guard=tls' needs a valid base register

> The -mstack-protector-guard-reg= doesn't do anything if you aren't
> doing stack protection.  It allows any base register (so, r1..r31).
> Setting it to any valid reg should be fine and not do anything harmful,
> unless perhaps you *do* enable stack protector, then it better be the
> expected stuff ;-)
> 
> Apparently clang does not implement it correctly?  This is just a clang
> bug, please report it with them?
> 
> (r2 is the default for -m32, r13 is the default for -m64, it appears
> that clang does not implement this option at all, it simply checks if
> you set the default, and explodes if not).

Not sure that I would say it has not been implemented correctly, more
that it has not been implemented in the same manner as GCC. Keith chose
not to open up support for arbitrary registers to keep the
implementation of this option in LLVM simple:

https://lore.kernel.org/linuxppc-dev/87o73uvaq5.fsf@keithp.com/

Cheers,
Nathan