Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Zhangjin Wu <falcon@tinylab.org>
To:     thomas@t-8ch.de
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, w@1wt.eu
Subject: Re: [PATCH 11/13] tools/nolibc: sys_select: riscv: use __NR_pselect6_time64 for rv32
Date:   Thu, 25 May 2023 15:10:21 +0800
Message-Id: <20230525071021.77223-1-falcon@tinylab.org>
In-Reply-To: <83339ea6-6653-41fe-af75-e0d51a4fa920@t-8ch.de>
References: <83339ea6-6653-41fe-af75-e0d51a4fa920@t-8ch.de>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Feedback-ID: bizesmtp:tinylab.org:qybglogicsvrsz:qybglogicsvrsz3a-3
Precedence: bulk

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
> >