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

Zhangjin Wu
2023-05-25T07:10:55Z

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?

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.

Thanks,
Zhangjin

>
> > +#endif
> >
> > if (timeout) {
> > t.tv_sec = timeout->tv_sec;
> > --
> > 2.25.1
> >