Re: [PATCH 13/13] tools/nolibc: sys_gettimeofday: riscv: use __NR_clock_gettime64 for rv32
From: Zhangjin Wu
Date: Fri May 26 2023 - 23:43:55 EST
Hi, Thomas, Willy
> > On 2023-05-25 02:03:32+0800, Zhangjin Wu wrote:
> > > rv32 uses the generic include/uapi/asm-generic/unistd.h and it has no
> > > __NR_gettimeofday and __NR_clock_gettime after kernel commit d4c08b9776b3
> > > ("riscv: Use latest system call ABI"), use __NR_clock_gettime64 instead.
> > >
> > > This code is based on src/time/gettimeofday.c of musl and
> > > sysdeps/unix/sysv/linux/clock_gettime.c of glibc.
> > >
> > > Both __NR_clock_gettime and __NR_clock_gettime64 are added for
> > > sys_gettimeofday() for they share most of the code.
> > >
> > > Notes:
> > >
> > > * Both tv and tz are not directly passed to kernel clock_gettime*
> > > syscalls, so, it isn't able to check the pointer automatically with the
> > > get_user/put_user helpers just like kernel gettimeofday syscall does.
> > > instead, we emulate (but not completely) such checks in our new
> > > __NR_clock_gettime* branch of nolibc.
> > >
> > > * kernel clock_gettime* syscalls can not get tz info, just like musl and
> > > glibc do, we set tz to zero to avoid a random number.
> > >
> > > Signed-off-by: Zhangjin Wu <falcon@xxxxxxxxxxx>
> > > ---
> > > tools/include/nolibc/sys.h | 46 ++++++++++++++++++++++++++++++++++++++
> > > 1 file changed, 46 insertions(+)
> > >
> > > diff --git a/tools/include/nolibc/sys.h b/tools/include/nolibc/sys.h
> > > index 2642b380c6aa..ad38cc3856be 100644
> > > --- a/tools/include/nolibc/sys.h
> > > +++ b/tools/include/nolibc/sys.h
> > > @@ -26,6 +26,7 @@
> > >
> > > #include "arch.h"
> > > #include "errno.h"
> > > +#include "string.h"
> > > #include "types.h"
> > >
> > >
> > > @@ -51,6 +52,11 @@
> > > * should not be placed here.
> > > */
> > >
> > > +/*
> > > + * This is the first address past the end of the text segment (the program code).
> > > + */
> > > +
> > > +extern char etext;
> > >
> > > /*
> > > * int brk(void *addr);
> > > @@ -554,7 +560,47 @@ long getpagesize(void)
> > > static __attribute__((unused))
> > > int sys_gettimeofday(struct timeval *tv, struct timezone *tz)
> > > {
> > > +#ifdef __NR_gettimeofday
> > > return my_syscall2(__NR_gettimeofday, tv, tz);
> > > +#elif defined(__NR_clock_gettime) || defined(__NR_clock_gettime64)
> > > +#ifdef __NR_clock_gettime
> > > + struct timespec ts;
> > > +#else
> > > + struct timespec64 ts;
> > > +#define __NR_clock_gettime __NR_clock_gettime64
> > > +#endif
> > > + int ret;
> > > +
> > > + /* make sure tv pointer is at least after code segment */
> > > + if (tv != NULL && (char *)tv <= &etext)
> > > + return -EFAULT;
> >
> > To me the weird etext comparisions don't seem to be worth it, to be
> > honest.
> >
>
> This is the issue we explained in commit message:
>
> * Both tv and tz are not directly passed to kernel clock_gettime*
> syscalls, so, it isn't able to check the pointer automatically with the
> get_user/put_user helpers just like kernel gettimeofday syscall does.
> instead, we emulate (but not completely) such checks in our new
> __NR_clock_gettime* branch of nolibc.
>
> but not that deeply described the direct cause, the direct cause is that the
> test case passes a '(void *)1' and the kernel space of gettimeofday can simply
> 'fixup' this issue by the get_user/put_user helpers, but our user-space tv and
> tz code has no such function, just emulate such 'fixup' by a stupid etext
> compare to at least make sure the data pointer is in data range. Welcome better
> solution.
>
> CASE_TEST(gettimeofday_bad1); EXPECT_SYSER(1, gettimeofday((void *)1, NULL), -1, EFAULT); break;
> CASE_TEST(gettimeofday_bad2); EXPECT_SYSER(1, gettimeofday(NULL, (void *)1), -1, EFAULT); break;
>
> Without this ugly check, the above cases would get such error:
>
> 35 gettimeofday_bad1init[1]: unhandled signal 11 code 0x1 at 0x00000002 in init[10000+5000]
> CPU: 0 PID: 1 Comm: init Not tainted 6.4.0-rc1-00134-gf929c7b7184f-dirty #20
> Hardware name: riscv-virtio,qemu (DT)
> epc : 00012ccc ra : 00012ca8 sp : 9d254d90
> gp : 00016800 tp : 00000000 t0 : 00000000
> t1 : 0000000a t2 : 00000000 s0 : 00000001
> s1 : 00016008 a0 : 00000000 a1 : 9d254da8
> a2 : 00000014 a3 : 00000000 a4 : 00000000
> a5 : 00000000 a6 : 00000001 a7 : 00000193
> s2 : 00000023 s3 : 9d254da4 s4 : 00000000
> s5 : 00000000 s6 : 0000541b s7 : 00000007
> s8 : 9d254dcc s9 : 000144e8 s10: 00016000
> s11: 00000006 t3 : 00000000 t4 : ffffffff
> t5 : 00000000 t6 : 00000000
> status: 00000020 badaddr: 00000002 cause: 0000000f
>
> Will at least append this test error in the commit message of the coming new
> revision of this patch.
>
> Hi, Willy, this also require your discussion, simply remove the above
> two test cases may be not a good idea too, the check for gettimeofday is
> perfectly ok.
>
What about this? Just like Willy did in 1da02f51088 ("selftests/nolibc:
support glibc as well"), Let's only limit the test case under the
__NR_gettimeofday #ifdef:
diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c
index 702bf449f8d7..d52f3720918e 100644
--- a/tools/testing/selftests/nolibc/nolibc-test.c
+++ b/tools/testing/selftests/nolibc/nolibc-test.c
@@ -563,7 +563,7 @@ int run_syscall(int min, int max)
CASE_TEST(getdents64_root); EXPECT_SYSNE(1, test_getdents64("/"), -1); break;
CASE_TEST(getdents64_null); EXPECT_SYSER(1, test_getdents64("/dev/null"), -1, ENOTDIR); break;
CASE_TEST(gettimeofday_null); EXPECT_SYSZR(1, gettimeofday(NULL, NULL)); break;
-#ifdef NOLIBC
+#if defined(NOLIBC) && defined(__NR_gettimeofday)
CASE_TEST(gettimeofday_bad1); EXPECT_SYSER(1, gettimeofday((void *)1, NULL), -1, EFAULT); break;
CASE_TEST(gettimeofday_bad2); EXPECT_SYSER(1, gettimeofday(NULL, (void *)1), -1, EFAULT); break;
#endif
With the above change, we can simply remove the ugly etext check like this:
diff --git a/tools/include/nolibc/sys.h b/tools/include/nolibc/sys.h
index d1d26da306b7..ebe8ed018db6 100644
--- a/tools/include/nolibc/sys.h
+++ b/tools/include/nolibc/sys.h
@@ -572,17 +572,9 @@ int sys_gettimeofday(struct timeval *tv, struct timezone *tz)
#endif
int ret;
- /* make sure tv pointer is at least after code segment */
- if (tv != NULL && (char *)tv <= &etext)
- return -EFAULT;
-
/* set tz to zero to avoid random number */
- if (tz != NULL) {
- if ((char *)tz > &etext)
- memset(tz, 0, sizeof(struct timezone));
- else
- return -EFAULT;
- }
+ if (tz != NULL)
+ memset(tz, 0, sizeof(struct timezone));
if (tv == NULL)
return 0;
If agree, will apply this method in the next revision.
> The same 'emulate' method is used in the waitid patch, but that only
> requires to compare 'pid == INT_MIN', which is not that weird.
>
> > > +
> > > + /* set tz to zero to avoid random number */
> > > + if (tz != NULL) {
> > > + if ((char *)tz > &etext)
> > > + memset(tz, 0, sizeof(struct timezone));
> > > + else
> > > + return -EFAULT;
> > > + }
> > > +
>
> The same issue here.
>
And the one for waitid may work like this:
@@ -1390,10 +1382,6 @@ pid_t sys_wait4(pid_t pid, int *status, int options, struct rusage *rusage)
int idtype = P_PID;
int ret;
- /* emulate the 'pid == INT_MIN' path of wait4 */
- if (pid == INT_MIN)
- return -ESRCH;
-
if (pid < -1) {
idtype = P_PGID;
pid *= -1;
@@ -593,7 +593,9 @@ int run_syscall(int min, int max)
CASE_TEST(unlink_root); EXPECT_SYSER(1, unlink("/"), -1, EISDIR); break;
CASE_TEST(unlink_blah); EXPECT_SYSER(1, unlink("/proc/self/blah"), -1, ENOENT); break;
CASE_TEST(wait_child); EXPECT_SYSER(1, wait(&tmp), -1, ECHILD); break;
+#ifdef __NR_wait4
CASE_TEST(waitpid_min); EXPECT_SYSER(1, waitpid(INT_MIN, &tmp, WNOHANG), -1, ESRCH); break;
+#endif
CASE_TEST(waitpid_child); EXPECT_SYSER(1, waitpid(getpid(), &tmp, WNOHANG), -1, ECHILD); break;
CASE_TEST(write_badf); EXPECT_SYSER(1, write(-1, &tmp, 1), -1, EBADF); break;
CASE_TEST(write_zero); EXPECT_SYSZR(1, write(1, &tmp, 0)); break;
Best regards,
Zhangjin