Re: [PATCH 11/13] tools/nolibc: sys_select: riscv: use __NR_pselect6_time64 for rv32

On 2023-05-25 15:10:21+0800, Zhangjin Wu wrote:

...

Hi, Thomas

...

On 2023-05-25 01:59:55+0800, Zhangjin Wu wrote:

...

rv32 uses the generic include/uapi/asm-generic/unistd.h and it has no __NR_pselect6 after kernel commit d4c08b9776b3 ("riscv: Use latest system call ABI"), use __NR_pselect6_time64 instead.

Signed-off-by: Zhangjin Wu falcon@tinylab.org

tools/include/nolibc/sys.h | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/tools/include/nolibc/sys.h b/tools/include/nolibc/sys.h index c0335a84f880..00c7197dcd50 100644 --- a/tools/include/nolibc/sys.h +++ b/tools/include/nolibc/sys.h @@ -1041,8 +1041,13 @@ int sys_select(int nfds, fd_set *rfds, fd_set *wfds, fd_set *efds, struct timeva struct timeval *t; } arg = { .n = nfds, .r = rfds, .w = wfds, .e = efds, .t = timeout }; return my_syscall1(__NR_select, &arg); -#elif defined(__ARCH_WANT_SYS_PSELECT6) && defined(__NR_pselect6) +#elif defined(__ARCH_WANT_SYS_PSELECT6) && (defined(__NR_pselect6) || defined(__NR_pselect6_time64)) +#ifdef __NR_pselect6 struct timespec t; +#else

• struct timespec64 t;

+#define __NR_pselect6 __NR_pselect6_time64

Wouldn't this #define leak to the users of nolibc and lead to calls to pselect6_time64 with the ABI of the __NR_pselect6 if userspace is doing its own raw syscalls?

Yeah, it would break the user-side raw __NR_pselect6 syscall for nolibc is a header-only libc, so, it is not safe to use such method like glibc.

Something like this will let the syscall call to pselect6_time64 instead of the user-required __NR_pselect6 and pass the wrong type of argument.

#include "nolibc.h"  // If no __NR_pselect6 defined, __NR_pselect6 = __NR_pselect6_time64

#ifdef __NR_pselect6
    struct timespec t;  // come here for __NR_pselect6_time64, but t is not timespec64, broken
    syscall(__NR_pselect6, nfds, rfds, wfds, efds, timeout ? &t : NULL, NULL);
#else
    struct timespec64 t;
    syscall(__NR_pselect6, nfds, rfds, wfds, efds, timeout ? &t : NULL, NULL);
#endif

I have used something like __NR_pselect6_time3264 locally, before sending the patchset, I found a cleaner method already used in sys.h:

#ifndef __NR__newselect
#define __NR__newselect __NR_select
#endif

But I forgot the arguments mixing issue, __NR__newselect and __NR_select share the same type of arguments, but __NR_pselect6 and __NR_pselect6_time64 not, so, I will use back the old method but still need to find a better string, just like __NR__newselect, __NR__pselect6 may be used in kernel space in the future, and __NR_pselect6_time3264 is too long, what about this?

#ifdef __NR_pselect6
        struct timespec t;
#define __NR_pselect6__ __NR_pselect6
#else
        struct timespec64 t;
#define __NR_pselect6__ __NR_pselect6_time64
#endif

Or even ___NR_pselect6?

What about:

#ifdef __NR_pselect6 struct timespec t; const long nr_pselect = __NR_pselect6; #else struct timespec64 t; const long nr_pselect = __NR_pselect6_time64; #endif

It looks better and cleaner, will apply this method, thanks!

...

The same issue is in this patch:

[PATCH 13/13] tools/nolibc: sys_gettimeofday: riscv: use __NR_clock_gettime64

will solve it with the same method.

And also this one:

[PATCH 09/13] tools/nolibc: sys_poll: riscv: use __NR_ppoll_time64

Have tested all of them, will send a v2 later.

Best regards, Zhangjin

Thanks!