Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 29 Mar 2023 16:59:16 -0700
From:   "H. Peter Anvin" <hpa@zytor.com>
To:     "Li, Xin3" <xin3.li@intel.com>,
        "Hansen, Dave" <dave.hansen@intel.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "x86@kernel.org" <x86@kernel.org>
CC:     "tglx@linutronix.de" <tglx@linutronix.de>,
        "mingo@redhat.com" <mingo@redhat.com>,
        "bp@alien8.de" <bp@alien8.de>,
        "dave.hansen@linux.intel.com" <dave.hansen@linux.intel.com>,
        "bigeasy@linutronix.de" <bigeasy@linutronix.de>,
        "Liu, Yujie" <yujie.liu@intel.com>,
        "Kang, Shan" <shan.kang@intel.com>
Subject: =?US-ASCII?Q?RE=3A_=5BPATCH_1/1=5D_x86/vdso=3A_use_the_right_GDT=5FENT?= =?US-ASCII?Q?RY=5FCPUNODE_for_32-bit_getcpu=28=29_on_64-bit_kernel?=
User-Agent: K-9 Mail for Android
In-Reply-To: <SA1PR11MB673439BB3F5C1BE2CDEF91A1A8899@SA1PR11MB6734.namprd11.prod.outlook.com>
References: <20230311084824.2340-1-xin3.li@intel.com> <5e84fd48-459d-8850-d26f-860c6d34a9ad@intel.com> <SA1PR11MB6734CF1607F263CB8389F5F2A8B99@SA1PR11MB6734.namprd11.prod.outlook.com> <SA1PR11MB6734735DD3637E302DC155FFA8899@SA1PR11MB6734.namprd11.prod.outlook.com> <SA1PR11MB673439BB3F5C1BE2CDEF91A1A8899@SA1PR11MB6734.namprd11.prod.outlook.com>
Message-ID: <149866DF-508B-426D-9D76-90B3257C1756@zytor.com>
MIME-Version: 1.0
Content-Type: text/plain;
 charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On March 29, 2023 4:11:09 PM PDT, "Li, Xin3" <xin3=2Eli@intel=2Ecom> 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=2E
>> > >
>> > > Second, if any other users of fake_32bit_build=2Eh for the VDSO sho=
w
>> > > up, they'll need a similar #ifdef=2E
>> > >
>> > > 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=
=2E
>> >
>> > Sounds a better way, let me try=2E
>> >
>> > > Also, about the *silent* failure=2E=2E=2E  Do we not have a selftes=
t for this somewhere?
>> >
>> > When lsl is used, we should check ZF which indicates whether the
>> > segment limit is loaded successfully=2E  Seems we need to refactor
>> vdso_read_cpunode() a bit=2E
>>=20
>> Hi Dave,
>>=20
>> How about the following patch to address the silent failure?
>>=20
>> diff --git a/arch/x86/include/asm/segment=2Eh b/arch/x86/include/asm/se=
gment=2Eh
>> index 794f69625780=2E=2Ed75ce4afff5b 100644
>> --- a/arch/x86/include/asm/segment=2Eh
>> +++ b/arch/x86/include/asm/segment=2Eh
>> @@ -254,7 +254,10 @@ static inline void vdso_read_cpunode(unsigned *cpu=
,
>> unsigned *node)
>>          *
>>          * If RDPID is available, use it=2E
>>          */
>> -       alternative_io ("lsl %[seg],%[p]",
>> +       alternative_io ("lsl %[seg],%[p]\n"
>> +                       "jz 1f\n"
>> +                       "mov $-1,%[p]\n"
>> +                       "1:",
>>                         "=2Ebyte 0xf3,0x0f,0xc7,0xf8", /* RDPID %eax/ra=
x */
>>                         X86_FEATURE_RDPID,
>>                         [p] "=3Da" (p), [seg] "r" (__CPUNODE_SEG));
>>=20
>>=20
>> And the test then reports CPU id 4095 (not a big enough #), which can b=
e 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=
=2E
>But there is also no reasonable errno for getcpu() to return to its calle=
r,
>saying "we had a problem in the syscall's kernel implementation"=2E
>
>>=20
>> ~/selftests$ sudo =2E/run_kselftest=2Esh -t x86:test_vsyscall_32 TAP ve=
rsion 13
>> 1=2E=2E1
>> # selftests: x86: test_vsyscall_32
>> # [RUN] test gettimeofday()
>> #       vDSO time offsets: 0=2E000028 0=2E000000
>> # [OK]  vDSO gettimeofday()'s timeval was okay # [RUN] test time() # [O=
K]  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=3D1
>>=20
>> Thanks!
>>   Xin
>

I don't think we should put a test in the vdso implementation=2E We need a=
 self test, to be sure, and we should check that LSL works standalone (beca=
use of Oracle et al), but putting an assert like this in the vdso is like o=
f odd at best=2E

If we *do* put in a test, it should trap to the kernel, not return an errn=
o, because it is the equivalent of putting a BUG() in the kernel=2E In this=
 case we can even do better, because we can execute the getcpu system call =
at that point=2E

But=2E=2E=2E why? We generally trust the kernel implementation=2E