RE: [PATCH 1/1] x86/vdso: use the right GDT_ENTRY_CPUNODE for 32-bit getcpu() on 64-bit kernel

[Li, Xin3]

> -----Original Message-----
> From: Li, Xin3 <xin3.li@xxxxxxxxx>
> Sent: Monday, March 13, 2023 10:43 AM
> To: Hansen, Dave <dave.hansen@xxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx;
> x86@xxxxxxxxxx
> Cc: tglx@xxxxxxxxxxxxx; mingo@xxxxxxxxxx; bp@xxxxxxxxx;
> dave.hansen@xxxxxxxxxxxxxxx; hpa@xxxxxxxxx; bigeasy@xxxxxxxxxxxxx; Liu, Yujie
> <yujie.liu@xxxxxxxxx>; Kang, Shan <shan.kang@xxxxxxxxx>
> Subject: RE: [PATCH 1/1] x86/vdso: use the right GDT_ENTRY_CPUNODE for 32-bit
> getcpu() on 64-bit kernel
> 
> > > +#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:

~/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