On March 29, 2023 4:11:09 PM PDT, "Li, Xin3" <xin3.li@intel.com> wrote:
>> > > > +#ifndef BUILD_VDSO32_64
>> > > > #define GDT_ENTRY_CPUNODE 28
>> > > > +#else
>> > > > +#define GDT_ENTRY_CPUNODE 15
>> > > > +#endif
>> > >
>> > > Isn't this kinda a hack?
>> > >
>> > > First, it means that we'll now have two duplicate versions of this:
>> > >
>> > > #define GDT_ENTRY_CPUNODE 15
>> > >
>> > > in the same file.
>> > >
>> > > Second, if any other users of fake_32bit_build.h for the VDSO show
>> > > up, they'll need a similar #ifdef.
>> > >
>> > > I think I'd much rather if we define all of the GDT_ENTRY_* macros
>> > > in
>> > > *one* place, then make that *one* place depend on BUILD_VDSO32_64.
>> >
>> > Sounds a better way, let me try.
>> >
>> > > Also, about the *silent* failure... Do we not have a selftest for this somewhere?
>> >
>> > When lsl is used, we should check ZF which indicates whether the
>> > segment limit is loaded successfully. Seems we need to refactor
>> vdso_read_cpunode() a bit.
>>
>> Hi Dave,
>>
>> How about the following patch to address the silent failure?
>>
>> diff --git a/arch/x86/include/asm/segment.h b/arch/x86/include/asm/segment.h
>> index 794f69625780..d75ce4afff5b 100644
>> --- a/arch/x86/include/asm/segment.h
>> +++ b/arch/x86/include/asm/segment.h
>> @@ -254,7 +254,10 @@ static inline void vdso_read_cpunode(unsigned *cpu,
>> unsigned *node)
>> *
>> * If RDPID is available, use it.
>> */
>> - alternative_io ("lsl %[seg],%[p]",
>> + alternative_io ("lsl %[seg],%[p]\n"
>> + "jz 1f\n"
>> + "mov $-1,%[p]\n"
>> + "1:",
>> ".byte 0xf3,0x0f,0xc7,0xf8", /* RDPID %eax/rax */
>> X86_FEATURE_RDPID,
>> [p] "=a" (p), [seg] "r" (__CPUNODE_SEG));
>>
>>
>> And the test then reports CPU id 4095 (not a big enough #), which can be used to
>> indicate a failure of the lsl instruction:
>
>Having to say, it's a *bad* idea to use a special # to indicate an error.
>But there is also no reasonable errno for getcpu() to return to its caller,
>saying "we had a problem in the syscall's kernel implementation".
>
>>
>> ~/selftests$ sudo ./run_kselftest.sh -t x86:test_vsyscall_32 TAP version 13
>> 1..1
>> # selftests: x86: test_vsyscall_32
>> # [RUN] test gettimeofday()
>> # vDSO time offsets: 0.000028 0.000000
>> # [OK] vDSO gettimeofday()'s timeval was okay # [RUN] test time() # [OK] vDSO
>> time() is okay # [RUN] getcpu() on CPU 0
>> # [FAIL] vDSO reported CPU 4095 but should be 0
>> # [FAIL] vDSO reported node 65535 but should be 0
>> # [RUN] getcpu() on CPU 1
>> # [FAIL] vDSO reported CPU 4095 but should be 1
>> # [FAIL] vDSO reported node 65535 but should be 0
>> not ok 1 selftests: x86: test_vsyscall_32 # exit=1
>>
>> Thanks!
>> Xin
>
I don't think we should put a test in the vdso implementation. We need a self test, to be sure, and we should check that LSL works standalone (because of Oracle et al), but putting an assert like this in the vdso is like of odd at best.
If we *do* put in a test, it should trap to the kernel, not return an errno, because it is the equivalent of putting a BUG() in the kernel. In this case we can even do better, because we can execute the getcpu system call at that point.
But... why? We generally trust the kernel implementation.
>> > > > +#ifndef BUILD_VDSO32_64
>> > > > #define GDT_ENTRY_CPUNODE 28
>> > > > +#else
>> > > > +#define GDT_ENTRY_CPUNODE 15
>> > > > +#endif
>> > >
>> > > Isn't this kinda a hack?
>> > >
>> > > First, it means that we'll now have two duplicate versions of this:
>> > >
>> > > #define GDT_ENTRY_CPUNODE 15
>> > >
>> > > in the same file.
>> > >
>> > > Second, if any other users of fake_32bit_build.h for the VDSO show
>> > > up, they'll need a similar #ifdef.
>> > >
>> > > I think I'd much rather if we define all of the GDT_ENTRY_* macros
>> > > in
>> > > *one* place, then make that *one* place depend on BUILD_VDSO32_64.
>> >
>> > Sounds a better way, let me try.
>> >
>> > > Also, about the *silent* failure... Do we not have a selftest for this somewhere?
>> >
>> > When lsl is used, we should check ZF which indicates whether the
>> > segment limit is loaded successfully. Seems we need to refactor
>> vdso_read_cpunode() a bit.
>>
>> Hi Dave,
>>
>> How about the following patch to address the silent failure?
>>
>> diff --git a/arch/x86/include/asm/segment.h b/arch/x86/include/asm/segment.h
>> index 794f69625780..d75ce4afff5b 100644
>> --- a/arch/x86/include/asm/segment.h
>> +++ b/arch/x86/include/asm/segment.h
>> @@ -254,7 +254,10 @@ static inline void vdso_read_cpunode(unsigned *cpu,
>> unsigned *node)
>> *
>> * If RDPID is available, use it.
>> */
>> - alternative_io ("lsl %[seg],%[p]",
>> + alternative_io ("lsl %[seg],%[p]\n"
>> + "jz 1f\n"
>> + "mov $-1,%[p]\n"
>> + "1:",
>> ".byte 0xf3,0x0f,0xc7,0xf8", /* RDPID %eax/rax */
>> X86_FEATURE_RDPID,
>> [p] "=a" (p), [seg] "r" (__CPUNODE_SEG));
>>
>>
>> And the test then reports CPU id 4095 (not a big enough #), which can be used to
>> indicate a failure of the lsl instruction:
>
>Having to say, it's a *bad* idea to use a special # to indicate an error.
>But there is also no reasonable errno for getcpu() to return to its caller,
>saying "we had a problem in the syscall's kernel implementation".
>
>>
>> ~/selftests$ sudo ./run_kselftest.sh -t x86:test_vsyscall_32 TAP version 13
>> 1..1
>> # selftests: x86: test_vsyscall_32
>> # [RUN] test gettimeofday()
>> # vDSO time offsets: 0.000028 0.000000
>> # [OK] vDSO gettimeofday()'s timeval was okay # [RUN] test time() # [OK] vDSO
>> time() is okay # [RUN] getcpu() on CPU 0
>> # [FAIL] vDSO reported CPU 4095 but should be 0
>> # [FAIL] vDSO reported node 65535 but should be 0
>> # [RUN] getcpu() on CPU 1
>> # [FAIL] vDSO reported CPU 4095 but should be 1
>> # [FAIL] vDSO reported node 65535 but should be 0
>> not ok 1 selftests: x86: test_vsyscall_32 # exit=1
>>
>> Thanks!
>> Xin
>
I don't think we should put a test in the vdso implementation. We need a self test, to be sure, and we should check that LSL works standalone (because of Oracle et al), but putting an assert like this in the vdso is like of odd at best.
If we *do* put in a test, it should trap to the kernel, not return an errno, because it is the equivalent of putting a BUG() in the kernel. In this case we can even do better, because we can execute the getcpu system call at that point.
But... why? We generally trust the kernel implementation.