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

From: Willy Tarreau <w@1wt.eu>
To: Zhangjin Wu <falcon@tinylab.org>
Cc: arnd@arndb.de, david.laight@aculab.com, thomas@t-8ch.de,
	linux-kernel@vger.kernel.org, linux-kselftest@vger.kernel.org,
	linux-riscv@lists.infradead.org
Subject: Re: [PATCH v5 10/14] tools/nolibc: __sysret: support syscalls who return a pointer
Date: Mon, 3 Jul 2023 12:03:59 +0200	[thread overview]
Message-ID: <ZKKdD/p4UkEavru6@1wt.eu> (raw)
In-Reply-To: <20230703083651.491785-1-falcon@tinylab.org>

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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv