diff mbox series

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

Message ID 9359ab11b52ef7c1799337f475e1e27c0cb00e3b.1684949268.git.falcon@tinylab.org
State New
Headers
Series tools/nolibc: riscv: Add full rv32 support |

Commit Message

Zhangjin Wu May 24, 2023, 5:59 p.m. UTC 
  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(-)

Comments

Thomas Weißschuh May 24, 2023, 8:22 p.m. UTC | #1 
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?

> +#endif
>  
>  	if (timeout) {
>  		t.tv_sec  = timeout->tv_sec;
> -- 
> 2.25.1
>
Zhangjin Wu May 25, 2023, 7:10 a.m. UTC | #2 
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
> >
Thomas Weißschuh May 25, 2023, 7:22 a.m. UTC | #3 
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

> 
> 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 Wu May 26, 2023, 1:50 a.m. UTC | #4 
> 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!
Arnd Bergmann May 26, 2023, 9:19 a.m. UTC | #5 
On Wed, May 24, 2023, at 19:59, Zhangjin Wu wrote:
> 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
> +#endif

Overriding the __NR_pselect6 constant seems wrong here, this can
easily lead to more bugs, as pselect6 and pselect6_time64 are
not compatible because of the different arguments, so anything
using the constant after including sys.h will be broken.

I would just use __kernel_timespec64 unconditionally and then
use __NR_pselect6_time64 on all 32-bit architectures here,
but use __NR_pselect6 on 32-bit architectures.

I think we can also kill off the oldselect and newselect 
cases, using pselect6/pselect6_time64 unconditionally here,
and no longer care about building against pre-5.1 kernel
headers, at least for the copy of nolibc that ships with the
kernel.

     Arnd
Zhangjin Wu May 26, 2023, 11 a.m. UTC | #6 
Hi, Arnd

> On Wed, May 24, 2023, at 19:59, Zhangjin Wu wrote:
> > 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
> > +#endif
> 
> Overriding the __NR_pselect6 constant seems wrong here, this can
> easily lead to more bugs, as pselect6 and pselect6_time64 are
> not compatible because of the different arguments, so anything
> using the constant after including sys.h will be broken.
>

Yes, thanks, Thomas also pointed out this issue in another reply of this
message thread, it has been fixed locally with his suggestion, it looks
like this:

    #ifdef __NR_pselect6
    	struct timespec t;
    	const long nr_pselect = __NR_pselect6;
    #else
    	struct timespec64 t;
    	const long nr_pselect = __NR_pselect6_time64;
    #endif
    
    	if (timeout) {
    		t.tv_sec  = timeout->tv_sec;
    		t.tv_nsec = timeout->tv_usec * 1000;
    	}
    	return my_syscall6(nr_pselect, nfds, rfds, wfds, efds, timeout ? &t : NULL, NULL);

I have applied this method for ppoll_time64 and clock_gettime64 too,
this method can save several duplicated lines for us, I have prepared v2
patches locally for them. 

> I would just use __kernel_timespec64 unconditionally and then
> use __NR_pselect6_time64 on all 32-bit architectures here,
> but use __NR_pselect6 on 32-bit architectures.
>

The 2nd 32-bit you mean is 64-bit?

This is related to the timespec64/time64_t definitions as you commented
in another reply. I will learn how to use the one from
<linux/time_types.h>, it may require to clean up the existing files in
tools/include/nolibc/ at first.

> I think we can also kill off the oldselect and newselect 
> cases, using pselect6/pselect6_time64 unconditionally here,
> and no longer care about building against pre-5.1 kernel
> headers, at least for the copy of nolibc that ships with the
> kernel.

As Willy commented in another reply, users may want to copy the recent
one and use them with an old kernel, even if want to drop them, a
standalone patch may be preferable.

Thanks very much,
Zhangjin

> 
>      Arnd
Arnd Bergmann May 26, 2023, 11:13 a.m. UTC | #7 
On Fri, May 26, 2023, at 13:00, Zhangjin Wu wrote:
>> On Wed, May 24, 2023, at 19:59, Zhangjin Wu wrote:
>> > diff --git a/tools/include/nolibc/sys.h b/tools/include/nolibc/sys.h
>
> I have applied this method for ppoll_time64 and clock_gettime64 too,
> this method can save several duplicated lines for us, I have prepared v2
> patches locally for them. 

Ok, that addresses my concern about the possible bugs.

>> I would just use __kernel_timespec64 unconditionally and then
>> use __NR_pselect6_time64 on all 32-bit architectures here,
>> but use __NR_pselect6 on 32-bit architectures.
>>
>
> The 2nd 32-bit you mean is 64-bit?

Yes, sorry for the typo.

> This is related to the timespec64/time64_t definitions as you commented
> in another reply. I will learn how to use the one from
> <linux/time_types.h>, it may require to clean up the existing files in
> tools/include/nolibc/ at first.

Ok, thanks.

>> I think we can also kill off the oldselect and newselect 
>> cases, using pselect6/pselect6_time64 unconditionally here,
>> and no longer care about building against pre-5.1 kernel
>> headers, at least for the copy of nolibc that ships with the
>> kernel.
>
> As Willy commented in another reply, users may want to copy the recent
> one and use them with an old kernel, even if want to drop them, a
> standalone patch may be preferable.

Fair enough. I checked the old versions and see that 5.1 in May 2019
was the first one to include the time64 system call definitions, though
5.6 from March 2020 was the first version that I consider fully working
with time64.

I don't know how common it is to copy nolibc into other projects,
but requiring a three year old kernel might be a little too aggressive
here. They could copy from 6.1-stable to keep the fallback (and miss
rv32) if we do this, but a better cutoff may be Dec 2025 when
linux-5.4 has its "projected EOL" date and one last LTS with the
fallback (linux-6.16 or so) gets released.

      Arnd
diff mbox series

Patch

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
+#endif
 
 	if (timeout) {
 		t.tv_sec  = timeout->tv_sec;