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

[Zhangjin Wu]

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