linux-riscv.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
From: Zhangjin Wu <falcon@tinylab.org>
To: thomas@t-8ch.de, w@1wt.eu
Cc: falcon@tinylab.org, linux-kernel@vger.kernel.org,
	linux-kselftest@vger.kernel.org, linux-riscv@lists.infradead.org,
	palmer@dabbelt.com, paul.walmsley@sifive.com
Subject: Re: [PATCH 13/13] tools/nolibc: sys_gettimeofday: riscv: use __NR_clock_gettime64 for rv32
Date: Sat, 27 May 2023 11:39:36 +0800	[thread overview]
Message-ID: <20230527033936.15465-1-falcon@tinylab.org> (raw)
In-Reply-To: <20230527012635.19595-1-falcon@tinylab.org>

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@tinylab.org>
> > > ---
> > >  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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

  reply	other threads:[~2023-05-27  3:40 UTC|newest]

Thread overview: 60+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-05-24 17:33 [PATCH 00/13] tools/nolibc: riscv: Add full rv32 support Zhangjin Wu
2023-05-24 17:41 ` [PATCH 01/13] Revert "tools/nolibc: riscv: Support __NR_llseek for rv32" Zhangjin Wu
2023-05-24 17:44 ` [PATCH 02/13] Revert "selftests/nolibc: Fix up compile error " Zhangjin Wu
2023-05-24 17:48 ` [PATCH 04/13] selftests/nolibc: syscall_args: use __NR_statx for rv32 Zhangjin Wu
2023-05-24 19:49   ` Thomas Weißschuh
2023-05-25  7:20     ` Zhangjin Wu
2023-05-26  9:21   ` Arnd Bergmann
2023-05-26 10:06     ` Willy Tarreau
2023-05-27  0:58     ` Zhangjin Wu
2023-05-24 17:50 ` [PATCH 05/13] selftests/nolibc: riscv: customize makefile " Zhangjin Wu
2023-05-26  6:57   ` Thomas Weißschuh
2023-05-26  9:20     ` Zhangjin Wu
2023-05-24 17:52 ` [PATCH 06/13] selftests/nolibc: allow specify a bios for qemu Zhangjin Wu
2023-05-26  7:00   ` Thomas Weißschuh
2023-05-26 10:25     ` Zhangjin Wu
2023-05-26 10:36       ` Conor Dooley
2023-05-26 13:38         ` Zhangjin Wu
2023-05-26 15:08           ` Conor Dooley
2023-05-28  7:52     ` Willy Tarreau
2023-05-24 17:54 ` [PATCH 07/13] selftests/nolibc: remove the duplicated gettimeofday_bad2 Zhangjin Wu
2023-05-24 17:55 ` [PATCH 08/13] tools/nolibc: sys_lseek: riscv: use __NR_llseek for rv32 Zhangjin Wu
2023-05-24 17:57 ` [PATCH 09/13] tools/nolibc: sys_poll: riscv: use __NR_ppoll_time64 " Zhangjin Wu
2023-05-26  7:15   ` Thomas Weißschuh
2023-05-26  9:34     ` Arnd Bergmann
2023-05-28  8:25       ` Zhangjin Wu
2023-05-28  8:48         ` Arnd Bergmann
2023-05-28 10:29         ` Willy Tarreau
2023-05-28 10:55           ` Arnd Bergmann
2023-05-28 11:03             ` Willy Tarreau
2023-05-24 17:58 ` [PATCH 10/13] tools/nolibc: ppoll/ppoll_time64: add a missing argument Zhangjin Wu
2023-05-24 17:59 ` [PATCH 11/13] tools/nolibc: sys_select: riscv: use __NR_pselect6_time64 for rv32 Zhangjin Wu
2023-05-24 20:22   ` Thomas Weißschuh
2023-05-25  7:10     ` Zhangjin Wu
2023-05-25  7:22       ` Thomas Weißschuh
2023-05-26  1:50         ` Zhangjin Wu
2023-05-26  9:19   ` Arnd Bergmann
2023-05-26 11:00     ` [PATCH 00/13] tools/nolibc: riscv: Add full rv32 support Zhangjin Wu
2023-05-26 11:13       ` Arnd Bergmann
2023-05-24 18:02 ` [PATCH 12/13] tools/nolibc: sys_wait4: riscv: use __NR_waitid for rv32 Zhangjin Wu
2023-05-24 18:03 ` [PATCH 13/13] tools/nolibc: sys_gettimeofday: riscv: use __NR_clock_gettime64 " Zhangjin Wu
2023-05-26  7:38   ` Thomas Weißschuh
2023-05-27  1:26     ` Zhangjin Wu
2023-05-27  3:39       ` Zhangjin Wu [this message]
2023-05-27  5:12       ` Willy Tarreau
2023-05-24 18:24 ` [PATCH 00/13] tools/nolibc: riscv: Add full rv32 support Zhangjin Wu
2023-05-24 18:28 ` [PATCH 03/13] selftests/nolibc: print name instead of number for EOVERFLOW Zhangjin Wu
2023-05-24 17:46   ` Zhangjin Wu
2023-05-24 20:23   ` Thomas Weißschuh
2023-05-28  7:59 ` [PATCH 00/13] tools/nolibc: riscv: Add full rv32 support Willy Tarreau
2023-05-28  8:42   ` Thomas Weißschuh
2023-05-28  9:41     ` Thomas Weißschuh
2023-05-28 10:17       ` Willy Tarreau
2023-05-28 10:39   ` Zhangjin Wu
2023-05-28 11:33     ` Willy Tarreau
2023-05-28 12:52       ` Zhangjin Wu
2023-05-28 13:45     ` Thomas Weißschuh 
2023-05-28 18:39       ` Zhangjin Wu
2023-05-29  8:45         ` Thomas Weißschuh
2023-05-29 11:31           ` Willy Tarreau
2023-05-30 10:06             ` Zhangjin Wu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20230527033936.15465-1-falcon@tinylab.org \
    --to=falcon@tinylab.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-kselftest@vger.kernel.org \
    --cc=linux-riscv@lists.infradead.org \
    --cc=palmer@dabbelt.com \
    --cc=paul.walmsley@sifive.com \
    --cc=thomas@t-8ch.de \
    --cc=w@1wt.eu \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).