[v5,10/14] tools/nolibc: __sysret: support syscalls who return a pointer

  To support syscalls (e.g. mmap()) who return a pointer and to allow the
pointer as big as possible, we should convert the negated errno value to
unsigned long (uintptr_t), otherwise, in signed long, a potential big
pointer (whose highest bit is 1) will be treated as a failure.

tools/include/nolibc/errno.h defines the MAX_ERRNO, let's use it
directly. after converting to unsigned long, the negative errno value
from -1 to -MAX_ERRNO becomes something like '~1 + 1' (every bit is 1)
to '~MAX_ERRNO + 1', '~1 + 1' is the biggest, '~MAX_ERRNO + 1' is the
smallest, so, the check becomes:

    if (ret <= (unsigned long)-1 && ret >= (unsigned long)-MAX_ERRNO) {
        ...
    }

Since (unsigned long)-1 is the biggest unsigned long value, it is always
true if bigger than (unsigned long)-MAX_ERRNO, so, just reserve the
following check is enough:

    if (ret >= (unsigned long)-MAX_ERRNO) {
        ...
    }

Suggested-by: David Laight <David.Laight@ACULAB.COM>
Link: https://lore.kernel.org/linux-riscv/94dd5170929f454fbc0a10a2eb3b108d@AcuMS.aculab.com/
Signed-off-by: Zhangjin Wu <falcon@tinylab.org>
---
 tools/include/nolibc/sys.h | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)
  

On Wed, Jun 28, 2023 at 09:39:56PM +0800, Zhangjin Wu wrote:
> To support syscalls (e.g. mmap()) who return a pointer and to allow the
> pointer as big as possible, we should convert the negated errno value to
> unsigned long (uintptr_t), otherwise, in signed long, a potential big
> pointer (whose highest bit is 1) will be treated as a failure.
> 
> tools/include/nolibc/errno.h defines the MAX_ERRNO, let's use it
> directly.

It might or might not work, it's an ABI change that, if validated, at
least needs a much more detailed explanation. What matters is not what
errno values we're willing to consider as an error, but what the
*syscalls* themselves return as an error. If a syscall says "< 0 is an
error equal to -errno", it means that we must treat it as an error,
and extract its value to get errno. If that errno is larger than
MAX_ERRNO it just means we don't know what the error is.

Syscalls that return pointer use that -MAX_ERRNO range to encode errors
(such as mmap()). I just do not know if there is a convention saying that
other ones also restrict themselves to that range or not. If you find
some info which guarantees that it's the case for all of them, then by
all means let's proceed like this, but in this case it should be mentioned
in the comment why we think it's valid to do this. For now it's presented
as an opportunity only.

Also, the rest of the commit message regarding uintptr_t (which we don't
use), bit values and modular arithmetics is extremely confusing and not
needed at all. What matters is only to know if we need to consider only
values -MAX_ERRNO..-1 as error or all negative ones. If so, then it's
obvious that ret >= (unsigned long)-MAX_ERRNO catches them all, as the
current mmap() function already does with -4095UL.

I just don't know where to check if we can generalize that test. In the
worst case we could have two __sys_ret(), the current one and a second
one for pointers. But I would suspect we could generalize due to ptrace,
as there it makes sense to be able to detect failures, even unknown ones.
I just need something more convincing than an intuition for a commit
message and to take such a change :-/

Thanks!
Willy
  

Hi, Willy

> On Wed, Jun 28, 2023 at 09:39:56PM +0800, Zhangjin Wu wrote:
> > To support syscalls (e.g. mmap()) who return a pointer and to allow the
> > pointer as big as possible, we should convert the negated errno value to
> > unsigned long (uintptr_t), otherwise, in signed long, a potential big
> > pointer (whose highest bit is 1) will be treated as a failure.
> > 
> > tools/include/nolibc/errno.h defines the MAX_ERRNO, let's use it
> > directly.
> 
> It might or might not work, it's an ABI change that, if validated, at
> least needs a much more detailed explanation. What matters is not what
> errno values we're willing to consider as an error, but what the
> *syscalls* themselves return as an error. If a syscall says "< 0 is an
> error equal to -errno", it means that we must treat it as an error,
> and extract its value to get errno. If that errno is larger than
> MAX_ERRNO it just means we don't know what the error is.
>

Yes, we do need to find a 'spec' or 'standard' to follow.

welcome suggestions from Arnd, Thomas and also David.

> Syscalls that return pointer use that -MAX_ERRNO range to encode errors
> (such as mmap()). I just do not know if there is a convention saying that
> other ones also restrict themselves to that range or not. If you find
> some info which guarantees that it's the case for all of them, then by
> all means let's proceed like this, but in this case it should be mentioned
> in the comment why we think it's valid to do this. For now it's presented
> as an opportunity only.

Currently, I only found a prove-in-use case in musl:

    https://elixir.bootlin.com/musl/latest/source/src/internal/syscall_ret.c:

    #include <errno.h>
    #include "syscall.h"

    long __syscall_ret(unsigned long r)
    {
    	if (r > -4096UL) {
    		errno = -r;
    		return -1;
    	}
    	return r;
    }

Our new implementation (based on the one used by mmap()) is almostly the same
as musl. Not sure if this is enough. I have tried to 'git blame' on
__syscall_ret() of musl to find some clue, but failed, because the function has
been added before importing into its git repo.

> 
> Also, the rest of the commit message regarding uintptr_t (which we don't
> use), bit values and modular arithmetics is extremely confusing and not
> needed at all. What matters is only to know if we need to consider only
> values -MAX_ERRNO..-1 as error or all negative ones. If so, then it's
> obvious that ret >= (unsigned long)-MAX_ERRNO catches them all, as the
> current mmap() function already does with -4095UL.
>

Yes, will clean up the commit message, but at first, let's continue get
more information about which one is ok:

- -MAX_ERRNO..-1 as error, for sys_mmap (we know in nolibc) currently

- all negative ones, for others currently

> I just don't know where to check if we can generalize that test. In the
> worst case we could have two __sys_ret(), the current one and a second
> one for pointers. But I would suspect we could generalize due to ptrace,
> as there it makes sense to be able to detect failures, even unknown ones.
> I just need something more convincing than an intuition for a commit
> message and to take such a change :-/
>

Of course, must be clear enough.

Best regards,
Zhangjin

> Thanks!
> Willy
  

On Mon, Jul 03, 2023 at 04:36:51PM +0800, Zhangjin Wu wrote:
> > Syscalls that return pointer use that -MAX_ERRNO range to encode errors
> > (such as mmap()). I just do not know if there is a convention saying that
> > other ones also restrict themselves to that range or not. If you find
> > some info which guarantees that it's the case for all of them, then by
> > all means let's proceed like this, but in this case it should be mentioned
> > in the comment why we think it's valid to do this. For now it's presented
> > as an opportunity only.
> 
> Currently, I only found a prove-in-use case in musl:
> 
>     https://elixir.bootlin.com/musl/latest/source/src/internal/syscall_ret.c:
> 
>     #include <errno.h>
>     #include "syscall.h"
> 
>     long __syscall_ret(unsigned long r)
>     {
>     	if (r > -4096UL) {
>     		errno = -r;
>     		return -1;
>     	}
>     	return r;
>     }
> 
> Our new implementation (based on the one used by mmap()) is almostly the same
> as musl. Not sure if this is enough. I have tried to 'git blame' on
> __syscall_ret() of musl to find some clue, but failed, because the function has
> been added before importing into its git repo.

OK, we already used the glibc-saved registers in the past to determine
the official list of clobbered registers (and the ABI spec was even
updated based on this). Here, musl is sufficiently deployed to consider
this as valid. You can simply go that route and mention in the commit
message that while you found no official reference stating that this is
valid for int/long returns, you found at least one other implementation
relying on this (i.e. if the kernel ever changes it will cause breakage).

> > Also, the rest of the commit message regarding uintptr_t (which we don't
> > use), bit values and modular arithmetics is extremely confusing and not
> > needed at all. What matters is only to know if we need to consider only
> > values -MAX_ERRNO..-1 as error or all negative ones. If so, then it's
> > obvious that ret >= (unsigned long)-MAX_ERRNO catches them all, as the
> > current mmap() function already does with -4095UL.
> >
> 
> Yes, will clean up the commit message, but at first, let's continue get
> more information about which one is ok:
> 
> - -MAX_ERRNO..-1 as error, for sys_mmap (we know in nolibc) currently
> 
> - all negative ones, for others currently

You can double-check in glibc for example, but I'm starting to guess
you'll find the same test as above, i.e. errors are exclusively >-4096,
regardless of the expected return type.

Thanks!
Willy
  

Hi, Willy

> On Mon, Jul 03, 2023 at 04:36:51PM +0800, Zhangjin Wu wrote:
> > > Syscalls that return pointer use that -MAX_ERRNO range to encode errors
> > > (such as mmap()). I just do not know if there is a convention saying that
> > > other ones also restrict themselves to that range or not. If you find
> > > some info which guarantees that it's the case for all of them, then by
> > > all means let's proceed like this, but in this case it should be mentioned
> > > in the comment why we think it's valid to do this. For now it's presented
> > > as an opportunity only.
> > 
> > Currently, I only found a prove-in-use case in musl:
> > 
> >     https://elixir.bootlin.com/musl/latest/source/src/internal/syscall_ret.c:
> > 
> >     #include <errno.h>
> >     #include "syscall.h"
> > 
> >     long __syscall_ret(unsigned long r)
> >     {
> >     	if (r > -4096UL) {
> >     		errno = -r;
> >     		return -1;
> >     	}
> >     	return r;
> >     }
> > 
> > Our new implementation (based on the one used by mmap()) is almostly the same
> > as musl. Not sure if this is enough. I have tried to 'git blame' on
> > __syscall_ret() of musl to find some clue, but failed, because the function has
> > been added before importing into its git repo.
> 
> OK, we already used the glibc-saved registers in the past to determine
> the official list of clobbered registers (and the ABI spec was even
> updated based on this). Here, musl is sufficiently deployed to consider
> this as valid. You can simply go that route and mention in the commit
> message that while you found no official reference stating that this is
> valid for int/long returns, you found at least one other implementation
> relying on this (i.e. if the kernel ever changes it will cause breakage).
>

ok.

> > > Also, the rest of the commit message regarding uintptr_t (which we don't
> > > use), bit values and modular arithmetics is extremely confusing and not
> > > needed at all. What matters is only to know if we need to consider only
> > > values -MAX_ERRNO..-1 as error or all negative ones. If so, then it's
> > > obvious that ret >= (unsigned long)-MAX_ERRNO catches them all, as the
> > > current mmap() function already does with -4095UL.
> > >
> > 
> > Yes, will clean up the commit message, but at first, let's continue get
> > more information about which one is ok:
> > 
> > - -MAX_ERRNO..-1 as error, for sys_mmap (we know in nolibc) currently
> > 
> > - all negative ones, for others currently
> 
> You can double-check in glibc for example, but I'm starting to guess
> you'll find the same test as above, i.e. errors are exclusively >-4096,
> regardless of the expected return type.
>

Your guest is definitely true ;-)

Glibc has the same logic in its INLINE_SYSCALL() macro:

    https://elixir.bootlin.com/glibc/latest/source/sysdeps/unix/sysv/linux/sysdep.h

    #undef INTERNAL_SYSCALL_ERROR_P
    #define INTERNAL_SYSCALL_ERROR_P(val) \
      ((unsigned long int) (val) > -4096UL)
    
    #ifndef SYSCALL_ERROR_LABEL
    # define SYSCALL_ERROR_LABEL(sc_err)					\
      ({									\
        __set_errno (sc_err);						\
        -1L;								\
      })
    #endif
    
    /* Define a macro which expands into the inline wrapper code for a system
       call.  It sets the errno and returns -1 on a failure, or the syscall
       return value otherwise.  */
    #undef INLINE_SYSCALL
    #define INLINE_SYSCALL(name, nr, args...)				\
      ({									\
        long int sc_ret = INTERNAL_SYSCALL (name, nr, args);		\
        __glibc_unlikely (INTERNAL_SYSCALL_ERROR_P (sc_ret))		\
        ? SYSCALL_ERROR_LABEL (INTERNAL_SYSCALL_ERRNO (sc_ret))		\
        : sc_ret;								\
      })

Nothing differs.

But 'git blame' has no clue to any 'spec' or 'standard' either.

- fcb78a55058fd, linux: Consolidate INLINE_SYSCALL

  Moved all of the arch specific INTERNAL_SYSCALL_ERROR_P() to common
  header

- 369b849f1a382, sysdeps/unix/sysv/linux/s390/s390-32/sysdep.h (INTERNAL_SYSCALL,...

  Firstly defined this macro: INTERNAL_SYSCALL_ERROR_P()

    $ git show 369b849f1a3 | grep "define.*INTERNAL_SYSCALL_ERROR_P"
    +#define INTERNAL_SYSCALL_ERROR_P(val)	((unsigned int) (val) >= 0xfffff001u)
    +#define INTERNAL_SYSCALL_ERROR_P(val)	((unsigned int) (val) >= 0xfffff001u)
    +#define INTERNAL_SYSCALL_ERROR_P(val)	((unsigned long) (val) >= -515L)
    +#define INTERNAL_SYSCALL_ERROR_P(val)	((unsigned long) (val) >= -4095L)

Willy, I plan to further use something like, is it ok for you?

    tools/include/nolibc/errno.h:

    -#define MAX_ERRNO 4095
    +#define MAX_ERRNO 4095UL

    tools/include/nolibc/sys.h:

    /* Syscall return helper for library routines
     * set errno as -ret when ret in [-MAX_ERRNO, -1]
     *
     * Note, No official reference states the errno range
     * here aligns with musl (src/internal/syscall_ret.c)
     * and glibc (sysdeps/unix/sysv/linux/sysdep.h)
     */
    static __inline__ __attribute__((unused, always_inline))
    long __sysret(unsigned long ret)
    {
            if (ret >= -MAX_ERRNO) {
                    SET_ERRNO(-(long)ret);
                    return -1;
            }
            return ret;
    }

Or we also directly use 4096UL here.

    static __inline__ __attribute__((unused, always_inline))
    long __sysret(unsigned long ret)
    {
            if (ret > -4096UL) {
                    SET_ERRNO(-(long)ret);
                    return -1;
            }
            return ret;
    }

Best regards,
Zhangjin

> Thanks!
> Willy