x86/vdso: A bugfix and a selftest
Subsystems (5)
Andy Lutomirski
maintainercover x86/vdso: A bugfix and a selftest
Hi Thomas-
In preparation for your big rewrite, here is a bugfix and a selftest. I marked both for -stable. If you commit them, it might be nice to insert the commit hash for the first patch into the changelog for the second patch. I obviously can't do that myself :)
Andy Lutomirski (2):
x86/vdso: Fix asm constraints on vDSO syscall fallbacks selftests/x86: Add clock_gettime() tests to test_vdso
arch/x86/entry/vdso/vclock_gettime.c | 18 +++-- tools/testing/selftests/x86/test_vdso.c | 99 +++++++++++++++++++++++++ 2 files changed, 109 insertions(+), 8 deletions(-)
Signature · 1 line
2.17.1
Andy Lutomirski
maintainerPATCH 1/2 x86/vdso: Fix asm constraints on vDSO syscall fallbacks
The syscall fallbacks in the vDSO have incorrect asm constraints. They are not marked as writing to their outputs -- instead, they are marked as clobbering "memory", which is useless. In particular, gcc is smart enough to know that the timespec parameter hasn't escaped, so a memory clobber doesn't clobber it. And passing a pointer as an asm *input* does not tell gcc that the pointed-to value is changed.
Add in the fact that the asm instructions weren't volatile, and gcc was free to omit them entirely unless their sole output (the return value) is used. Which it is (phew!), but that stops happening with some upcoming patches.
As a trivial example, the following code:
void test_fallback(struct timespec *ts)
{
vdso_fallback_gettime(CLOCK_MONOTONIC, ts);
}compiles to:
00000000000000c0 <test_fallback>:
c0: c3 retq
To add insult to injury, the RCX and R11 clobbers on 64-bit builds were missing.
The "memory" clobber is also unnecessary -- no ordering with respect to other memory operations is needed, but I'll fix that in a separate not-for-stable patch.
--- arch/x86/entry/vdso/vclock_gettime.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-)
arch/x86/entry/vdso/vclock_gettime.c +10 -8
@@ -43,8 +43,9 @@ extern u8 hvclock_page notrace static long vdso_fallback_gettime(long clock, struct timespec *ts) {long ret; - asm("syscall" :"=a" (ret) : -"0" (__NR_clock_gettime),"D" (clock),"S" (ts) :"memory" ); + asm ("syscall" :"=a" (ret),"=m" (*ts) : +"0" (__NR_clock_gettime),"D" (clock),"S" (ts) : +"memory" ,"rcx" ,"r11" );return ret; }
@@ -52,8 +53,9 @@ notrace static long vdso_fallback_gtod(struct timeval *tv, struct timezone *tz) {long ret; - asm("syscall" :"=a" (ret) : -"0" (__NR_gettimeofday),"D" (tv),"S" (tz) :"memory" ); + asm ("syscall" :"=a" (ret),"=m" (*tv),"=m" (*tz) : +"0" (__NR_gettimeofday),"D" (tv),"S" (tz) : +"memory" ,"rcx" ,"r11" );return ret; }
@@ -64,12 +66,12 @@ notrace static long vdso_fallback_gettime(long clock, struct timespec *ts) {long ret; - asm( + asm ("mov %%ebx, %%edx \n" "mov %2, %%ebx \n" "call __kernel_vsyscall \n" "mov %%edx, %%ebx \n" - :"=a" (ret) + :"=a" (ret),"=m" (*ts) :"0" (__NR_clock_gettime),"g" (clock),"c" (ts) :"memory" ,"edx" );return ret;
@@ -79,12 +81,12 @@ notrace static long vdso_fallback_gtod(struct timeval *tv, struct timezone *tz) {long ret; - asm( + asm ("mov %%ebx, %%edx \n" "mov %2, %%ebx \n" "call __kernel_vsyscall \n" "mov %%edx, %%ebx \n" - :"=a" (ret) + :"=a" (ret),"=m" (*tv),"=m" (*tz) :"0" (__NR_gettimeofday),"g" (tv),"c" (tz) :"memory" ,"edx" );return ret;
Signature · 1 line
2.17.1
tip-bot for Andy Lutomirski
merge bot[tip:x86/urgent] x86/vdso: Fix asm constraints on vDSO syscall fallbacks
x86/vdso: Fix asm constraints on vDSO syscall fallbacks
Commit-ID: 715bd9d12f84d8f5cc8ad21d888f9bc304a8eb0b Gitweb: https://git.kernel.org/tip/715bd9d12f84d8f5cc8ad21d888f9bc304a8eb0b Author: Andy Lutomirski <[email protected]> AuthorDate: Mon, 1 Oct 2018 12:52:15 -0700 Committer: Thomas Gleixner <[email protected]> CommitDate: Tue, 2 Oct 2018 08:28:15 +0200
x86/vdso: Fix asm constraints on vDSO syscall fallbacks
The syscall fallbacks in the vDSO have incorrect asm constraints. They are not marked as writing to their outputs -- instead, they are marked as clobbering "memory", which is useless. In particular, gcc is smart enough to know that the timespec parameter hasn't escaped, so a memory clobber doesn't clobber it. And passing a pointer as an asm *input* does not tell gcc that the pointed-to value is changed.
Add in the fact that the asm instructions weren't volatile, and gcc was free to omit them entirely unless their sole output (the return value) is used. Which it is (phew!), but that stops happening with some upcoming patches.
As a trivial example, the following code:
void test_fallback(struct timespec *ts)
{
vdso_fallback_gettime(CLOCK_MONOTONIC, ts);
}compiles to:
00000000000000c0 <test_fallback>:
c0: c3 retq
To add insult to injury, the RCX and R11 clobbers on 64-bit builds were missing.
The "memory" clobber is also unnecessary -- no ordering with respect to other memory operations is needed, but that's going to be fixed in a separate not-for-stable patch.
--- arch/x86/entry/vdso/vclock_gettime.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-)
arch/x86/entry/vdso/vclock_gettime.c +10 -8
@@ -43,8 +43,9 @@ extern u8 hvclock_page notrace static long vdso_fallback_gettime(long clock, struct timespec *ts) {long ret; - asm("syscall" :"=a" (ret) : -"0" (__NR_clock_gettime),"D" (clock),"S" (ts) :"memory" ); + asm ("syscall" :"=a" (ret),"=m" (*ts) : +"0" (__NR_clock_gettime),"D" (clock),"S" (ts) : +"memory" ,"rcx" ,"r11" );return ret; }
@@ -52,8 +53,9 @@ notrace static long vdso_fallback_gtod(struct timeval *tv, struct timezone *tz) {long ret; - asm("syscall" :"=a" (ret) : -"0" (__NR_gettimeofday),"D" (tv),"S" (tz) :"memory" ); + asm ("syscall" :"=a" (ret),"=m" (*tv),"=m" (*tz) : +"0" (__NR_gettimeofday),"D" (tv),"S" (tz) : +"memory" ,"rcx" ,"r11" );return ret; }
@@ -64,12 +66,12 @@ notrace static long vdso_fallback_gettime(long clock, struct timespec *ts) {long ret; - asm( + asm ("mov %%ebx, %%edx \n" "mov %2, %%ebx \n" "call __kernel_vsyscall \n" "mov %%edx, %%ebx \n" - :"=a" (ret) + :"=a" (ret),"=m" (*ts) :"0" (__NR_clock_gettime),"g" (clock),"c" (ts) :"memory" ,"edx" );return ret;
@@ -79,12 +81,12 @@ notrace static long vdso_fallback_gtod(struct timeval *tv, struct timezone *tz) {long ret; - asm( + asm ("mov %%ebx, %%edx \n" "mov %2, %%ebx \n" "call __kernel_vsyscall \n" "mov %%edx, %%ebx \n" - :"=a" (ret) + :"=a" (ret),"=m" (*tv),"=m" (*tz) :"0" (__NR_gettimeofday),"g" (tv),"c" (tz) :"memory" ,"edx" );return ret;
Andy Lutomirski
maintainerRe: [tip:x86/urgent] x86/vdso: Fix asm constraints on vDSO syscall fallbacks
Quoted 9 lines · tip-bot for Andy Lutomirski
Commit-ID: 715bd9d12f84d8f5cc8ad21d888f9bc304a8eb0b Gitweb: https://git.kernel.org/tip/715bd9d12f84d8f5cc8ad21d888f9bc304a8eb0b Author: Andy Lutomirski <[email protected]> AuthorDate: Mon, 1 Oct 2018 12:52:15 -0700 Committer: Thomas Gleixner <[email protected]> CommitDate: Tue, 2 Oct 2018 08:28:15 +0200x86/vdso: Fix asm constraints on vDSO syscall fallbacks
Ugh, I broke the 32-bit code and failed to test it right. Incremental fix coming.
Andy Lutomirski
maintainerPATCH 2/2 selftests/x86: Add clock_gettime() tests to test_vdso
In reply to Andy LutomirskiNow that the vDSO implementation of clock_gettime() is getting reworked, add a selftest for it. This tests that its output is consistent with the syscall version.
This is marked for stable to serve as a test for commit "x86/vdso:
Fix asm constraints on vDSO syscall fallbacks".
--- tools/testing/selftests/x86/test_vdso.c | 99 +++++++++++++++++++++++++ 1 file changed, 99 insertions(+)
tools/testing/selftests/x86/test_vdso.c +99 -0
@@ -17,6 +17,7 @@#include <errno.h> #include <sched.h> #include <stdbool.h> +#include <limits.h> #ifndef SYS_getcpu# ifdef __x86_64__
@@ -31,6 +32,10 @@int nerrs =0 ; +typedef int (*vgettime_t )(clockid_t ,struct timespec *); + +vgettime_t vdso_clock_gettime; +typedef long (*getcpu_t )(unsigned *,unsigned *,void *);getcpu_t vgetcpu;
@@ -95,6 +100,10 @@ static void fill_function_pointers()printf ("Warning: failed to find getcpu in vDSO\n" ); vgetcpu = (getcpu_t )vsyscall_getcpu (); + + vdso_clock_gettime = (vgettime_t )dlsym (vdso,"__vdso_clock_gettime" ); +if (!vdso_clock_gettime) +printf ("Warning: failed to find clock_gettime in vDSO\n" ); } static long sys_getcpu(unsigned * cpu, unsigned * node,
@@ -103,6 +112,11 @@ static long sys_getcpu(unsigned * cpu, unsigned * node,return syscall (__NR_getcpu, cpu, node, cache); } +static inline int sys_clock_gettime (clockid_t id,struct timespec *ts) +{ +return syscall (__NR_clock_gettime, id, ts); +} +static void test_getcpu (void ) {printf ("[RUN]\tTesting getcpu...\n" );
@@ -155,10 +169,95 @@ static void test_getcpu(void) } } +static bool ts_leq (const struct timespec *a,const struct timespec *b) +{ +if (a->tv_sec != b->tv_sec) +return a->tv_sec < b->tv_sec; +else +return a->tv_nsec <= b->tv_nsec; +} + +static char const *const clocknames[] = { + [0 ] ="CLOCK_REALTIME" , + [1 ] ="CLOCK_MONOTONIC" , + [2 ] ="CLOCK_PROCESS_CPUTIME_ID" , + [3 ] ="CLOCK_THREAD_CPUTIME_ID" , + [4 ] ="CLOCK_MONOTONIC_RAW" , + [5 ] ="CLOCK_REALTIME_COARSE" , + [6 ] ="CLOCK_MONOTONIC_COARSE" , + [7 ] ="CLOCK_BOOTTIME" , + [8 ] ="CLOCK_REALTIME_ALARM" , + [9 ] ="CLOCK_BOOTTIME_ALARM" , + [10 ] ="CLOCK_SGI_CYCLE" , + [11 ] ="CLOCK_TAI" , +}; + +static void test_one_clock_gettime (int clock,const char *name) +{ +struct timespec start, vdso, end; +int vdso_ret, end_ret; + +printf ("[RUN]\tTesting clock_gettime for clock %s (%d)...\n" , name, clock); + +if (sys_clock_gettime (clock, &start) <0 ) { +if (errno ==EINVAL ) { + vdso_ret =vdso_clock_gettime (clock, &vdso); +if (vdso_ret == -EINVAL ) { +printf ("[OK]\tNo such clock.\n" ); + }else { +printf ("[FAIL]\tNo such clock, but __vdso_clock_gettime returned %d\n" , vdso_ret); + nerrs++; + } + }else { +printf ("[WARN]\t clock_gettime(%d) syscall returned error %d\n" , clock, errno); + } +return ; + } + + vdso_ret =vdso_clock_gettime (clock, &vdso); + end_ret =sys_clock_gettime (clock, &end); + +if (vdso_ret !=0 || end_ret !=0 ) { +printf ("[FAIL]\tvDSO returned %d, syscall errno=%d\n" , + vdso_ret, errno); + nerrs++; +return ; + } + +printf ("\t%llu.%09ld %llu.%09ld %llu.%09ld\n" , + (unsigned long long )start.tv_sec, start.tv_nsec, + (unsigned long long )vdso.tv_sec, vdso.tv_nsec, + (unsigned long long )end.tv_sec, end.tv_nsec); + +if (!ts_leq (&start, &vdso) || !ts_leq (&vdso, &end)) { +printf ("[FAIL]\tTimes are out of sequence\n" ); + nerrs++; + } +} + +static void test_clock_gettime (void ) +{ +for (int clock =0 ; clock <sizeof (clocknames) /sizeof (clocknames[0 ]); + clock++) { +test_one_clock_gettime (clock, clocknames[clock]); + } + +/* Also test some invalid clock ids */ +test_one_clock_gettime (-1 ,"invalid" ); +test_one_clock_gettime (INT_MIN ,"invalid" ); +test_one_clock_gettime (INT_MAX ,"invalid" ); +} +int main (int argc,char **argv) {fill_function_pointers (); +test_clock_gettime (); + +/* +* Test getcpu() last so that, if something goes wrong setting affinity, +* we still run the other tests. +*/ test_getcpu ();return nerrs ?1 :0 ;
Signature · 1 line
2.17.1
tip-bot for Andy Lutomirski
merge bot[tip:x86/urgent] selftests/x86: Add clock_gettime() tests to test_vdso
selftests/x86: Add clock_gettime() tests to test_vdso
Commit-ID: 7c03e7035ac1cf2a6165754e4f3a49c2f1977838 Gitweb: https://git.kernel.org/tip/7c03e7035ac1cf2a6165754e4f3a49c2f1977838 Author: Andy Lutomirski <[email protected]> AuthorDate: Mon, 1 Oct 2018 12:52:16 -0700 Committer: Thomas Gleixner <[email protected]> CommitDate: Tue, 2 Oct 2018 08:28:32 +0200
selftests/x86: Add clock_gettime() tests to test_vdso
Now that the vDSO implementation of clock_gettime() is getting reworked, add a selftest for it. This tests that its output is consistent with the syscall version.
This is marked for stable to serve as a test for commit
715bd9d12f84 ("x86/vdso: Fix asm constraints on vDSO syscall fallbacks")
--- tools/testing/selftests/x86/test_vdso.c | 99 +++++++++++++++++++++++++++++++++ 1 file changed, 99 insertions(+)
tools/testing/selftests/x86/test_vdso.c +99 -0
@@ -17,6 +17,7 @@#include <errno.h> #include <sched.h> #include <stdbool.h> +#include <limits.h> #ifndef SYS_getcpu# ifdef __x86_64__
@@ -31,6 +32,10 @@int nerrs =0 ; +typedef int (*vgettime_t )(clockid_t ,struct timespec *); + +vgettime_t vdso_clock_gettime; +typedef long (*getcpu_t )(unsigned *,unsigned *,void *);getcpu_t vgetcpu;
@@ -95,6 +100,10 @@ static void fill_function_pointers()printf ("Warning: failed to find getcpu in vDSO\n" ); vgetcpu = (getcpu_t )vsyscall_getcpu (); + + vdso_clock_gettime = (vgettime_t )dlsym (vdso,"__vdso_clock_gettime" ); +if (!vdso_clock_gettime) +printf ("Warning: failed to find clock_gettime in vDSO\n" ); } static long sys_getcpu(unsigned * cpu, unsigned * node,
@@ -103,6 +112,11 @@ static long sys_getcpu(unsigned * cpu, unsigned * node,return syscall (__NR_getcpu, cpu, node, cache); } +static inline int sys_clock_gettime (clockid_t id,struct timespec *ts) +{ +return syscall (__NR_clock_gettime, id, ts); +} +static void test_getcpu (void ) {printf ("[RUN]\tTesting getcpu...\n" );
@@ -155,10 +169,95 @@ static void test_getcpu(void) } } +static bool ts_leq (const struct timespec *a,const struct timespec *b) +{ +if (a->tv_sec != b->tv_sec) +return a->tv_sec < b->tv_sec; +else +return a->tv_nsec <= b->tv_nsec; +} + +static char const *const clocknames[] = { + [0 ] ="CLOCK_REALTIME" , + [1 ] ="CLOCK_MONOTONIC" , + [2 ] ="CLOCK_PROCESS_CPUTIME_ID" , + [3 ] ="CLOCK_THREAD_CPUTIME_ID" , + [4 ] ="CLOCK_MONOTONIC_RAW" , + [5 ] ="CLOCK_REALTIME_COARSE" , + [6 ] ="CLOCK_MONOTONIC_COARSE" , + [7 ] ="CLOCK_BOOTTIME" , + [8 ] ="CLOCK_REALTIME_ALARM" , + [9 ] ="CLOCK_BOOTTIME_ALARM" , + [10 ] ="CLOCK_SGI_CYCLE" , + [11 ] ="CLOCK_TAI" , +}; + +static void test_one_clock_gettime (int clock,const char *name) +{ +struct timespec start, vdso, end; +int vdso_ret, end_ret; + +printf ("[RUN]\tTesting clock_gettime for clock %s (%d)...\n" , name, clock); + +if (sys_clock_gettime (clock, &start) <0 ) { +if (errno ==EINVAL ) { + vdso_ret =vdso_clock_gettime (clock, &vdso); +if (vdso_ret == -EINVAL ) { +printf ("[OK]\tNo such clock.\n" ); + }else { +printf ("[FAIL]\tNo such clock, but __vdso_clock_gettime returned %d\n" , vdso_ret); + nerrs++; + } + }else { +printf ("[WARN]\t clock_gettime(%d) syscall returned error %d\n" , clock, errno); + } +return ; + } + + vdso_ret =vdso_clock_gettime (clock, &vdso); + end_ret =sys_clock_gettime (clock, &end); + +if (vdso_ret !=0 || end_ret !=0 ) { +printf ("[FAIL]\tvDSO returned %d, syscall errno=%d\n" , + vdso_ret, errno); + nerrs++; +return ; + } + +printf ("\t%llu.%09ld %llu.%09ld %llu.%09ld\n" , + (unsigned long long )start.tv_sec, start.tv_nsec, + (unsigned long long )vdso.tv_sec, vdso.tv_nsec, + (unsigned long long )end.tv_sec, end.tv_nsec); + +if (!ts_leq (&start, &vdso) || !ts_leq (&vdso, &end)) { +printf ("[FAIL]\tTimes are out of sequence\n" ); + nerrs++; + } +} + +static void test_clock_gettime (void ) +{ +for (int clock =0 ; clock <sizeof (clocknames) /sizeof (clocknames[0 ]); + clock++) { +test_one_clock_gettime (clock, clocknames[clock]); + } + +/* Also test some invalid clock ids */ +test_one_clock_gettime (-1 ,"invalid" ); +test_one_clock_gettime (INT_MIN ,"invalid" ); +test_one_clock_gettime (INT_MAX ,"invalid" ); +} +int main (int argc,char **argv) {fill_function_pointers (); +test_clock_gettime (); + +/* +* Test getcpu() last so that, if something goes wrong setting affinity, +* we still run the other tests. +*/ test_getcpu ();return nerrs ?1 :0 ;