On Sat, Mar 02 2024 at 14:49, Linus Torvalds wrote:
> On Sat, 2 Mar 2024 at 14:00, Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote:
>>
>> I had commented out both. But the real reason is the EXPORT_SYMBOL,
>> which obviously wants to be EXPORT_PER_CPU_SYMBOL_GPL...
>
> Side note: while it's nice to hear that sparse kind of got this right,
> I wonder what gcc does when we start using the named address spaces
> for percpu variables.
>
> We actively make EXPORT_PER_CPU_SYMBOL_XYZ be a no-op for sparse
> exactly because sparse ended up warning about the regular
> EXPORT_SYMBOL, and we didn't have any "real" per-cpu export model.
Right.
> So EXPORT_PER_CPU_SYMBOL_GPL() is kind of an artificial "shut up
> sparse".
Aside of that it's also making it clear what this is about. So I don't
think it's purely artifical.
> But with __seg_gs/fs support for native percpu symbols with
> gcc, I wonder if we'll hit the same thing. Or is there something that
> makes gcc not warn about the named address spaces?
Right now the pending code in tip does not complain about the
EXPORT_PER_CPU_SYMBOL_GPL() part because our current macro maze is
hideous. Here is the preprocessor output.
This is DECLARE_PER_CPU() in the header:
extern __attribute__((section(".data..percpu" ""))) __typeof__(u64) x86_spec_ctrl_current;
Here is DEFINE_PER_CPU():
__attribute__((section(".data..percpu" ""))) __typeof__(u64) x86_spec_ctrl_current;
And the EXPORT:
extern typeof(x86_spec_ctrl_current) x86_spec_ctrl_current;
static void * __attribute__((__used__))
__attribute__((__section__(".discard.addressable")))
__UNIQUE_ID___addressable_x86_spec_ctrl_current804 = (void *)(uintptr_t)&x86_spec_ctrl_current;
asm(".section \".export_symbol\",\"a\" ;
__export_symbol_x86_spec_ctrl_current: ;
.asciz \"GPL\" ; .asciz \"\" ; .balign 8 ; .quad x86_spec_ctrl_current ; .previous");
And the __seg_gs magic happens only in the per CPU accessor itself:
__attribute__((__noinline__)) __attribute__((no_instrument_function))
__attribute((__section__(".noinstr.text")))
__attribute__((__no_sanitize_address__))
__attribute__((__no_profile_instrument_function__))
u64 spec_ctrl_current(void)
{
return ({
// this_cpu_read(x86_spec_ctrl_current)
typeof(x86_spec_ctrl_current) pscr_ret__;
do { const void *__vpp_verify = (typeof((&(x86_spec_ctrl_current)) + 0))((void *)0); (void)__vpp_verify;
} while (0);
switch(sizeof(x86_spec_ctrl_current))
{
case 1: pscr_ret__ = ({
*(volatile typeof(x86_spec_ctrl_current) __seg_gs *)(typeof(*&(x86_spec_ctrl_current)) __seg_gs *)(uintptr_t)(&(x86_spec_ctrl_current)); });
break;
case 2: pscr_ret__ = ({
*(volatile typeof(x86_spec_ctrl_current) __seg_gs *)(typeof(*&(x86_spec_ctrl_current)) __seg_gs *)(uintptr_t)(&(x86_spec_ctrl_current)); });
break;
case 4: pscr_ret__ = ({
*(volatile typeof(x86_spec_ctrl_current) __seg_gs *)(typeof(*&(x86_spec_ctrl_current)) __seg_gs *)(uintptr_t)(&(x86_spec_ctrl_current)); });
break;
case 8: pscr_ret__ = ({
*(volatile typeof(x86_spec_ctrl_current) __seg_gs *)(typeof(*&(x86_spec_ctrl_current)) __seg_gs *)(uintptr_t)(&(x86_spec_ctrl_current)); });
break;
default: __bad_size_call_parameter(); break;
}
pscr_ret__;
});
}
So all the export etc. just works because it all operates on a plain
data type and the __seg_gs is only bolted on via type casts in the
accessors.
As the per cpu variables are in the .data..percpu section the linker
puts them at address 0 and upwards. So the cast to a __seg_gs pointer
makes it end up at the real kernel address because of GSBASE + "offset".
The compiler converts this to RIP relative addressing:
movq $0x0,%gs:0x7e14169f(%rip) # 1ba08 <fpu_fpregs_owner_ctx>
This obviously has a downside. If I do:
u64 foo;
this_cpu_read(foo);
the compiler is just happy to build that w/o complaining and it will
only explode at runtime because foo is a kernel data address which added
to GSBASE will result in accessing some random address:
mov %gs:0x15d08d4(%rip),%rax # ffffffff834aac60 <x86_spec_ctrl_base>
This is not at all different from the inline ASM based version which is
in your tree. The only difference is that the macro maze is pure C and
the __set_gs cast allows the compiler to (micro) optimize, e.g. 'mov
%gs:...; movzbl' into a single 'movzbl'.
IOW, right now the only defense against such a mistake is actually the
sparse check. Maybe one of the coccinelle scripts has something similar,
I don't know.
I did not follow the __set_gs work closely, so I don't know whether Uros
ever tried to actually mark the per CPU variable __set_gs right away,
which would obviously catch the above 'foo' nonsense.
I think this should just work, but that would obviously require to do
the type cast magic at the EXPORT_SYMBOL side and in some other places.
Thanks,
tglx
