linux-kernel - Re: rcutorture initrd/nolibc build on ARMv8?

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20210120145447.GC77728@C02TD0UTHF1T.local>
Date:   Wed, 20 Jan 2021 14:54:47 +0000
From:   Mark Rutland <mark.rutland@....com>
To:     Willy Tarreau <w@....eu>
Cc:     "Paul E. McKenney" <paulmck@...nel.org>,
        linux-kernel@...r.kernel.org, valentin.schneider@....com
Subject: Re: rcutorture initrd/nolibc build on ARMv8?

On Wed, Jan 20, 2021 at 03:25:00PM +0100, Willy Tarreau wrote:
> On Wed, Jan 20, 2021 at 01:45:11PM +0000, Mark Rutland wrote:
> > > Ah that's very interesting indeed because actually these ones should
> > > only be used when __NR_dup3 or __NR_clone are not defined. Thus I wanted
> > > to check the definitions that were reported in your error output but
> > > actually what was needed was to figure whether the correct ones were
> > > present, and they are, here on my machine (and yes I agree that in this
> > > case the dup2/fork above are bofus):
> > 
> > The issue is that even if a function is unused, the compiler still has
> > to parse and compile the code, so where __NR_dup2 is not defined, we'll
> > get a build error for:
> > 
> > static __attribute__((unused))
> > int sys_dup2(int old, int new)
> > {
> >        return my_syscall2(__NR_dup2, old, new);
> > }
> 
> For sure but this is supposed to be used only when __NR_dup3 is not
> defined. Ah now I understand where my mistake is: after it built
> successfully for me I inspected the most recent tree which has these
> in place. Sorry for being stupid!
> 
> In my local tree it's defined like this:
> 
>  static __attribute__((unused))
>  int sys_dup2(int old, int new)
>  {
> #ifdef __NR_dup3
>        return my_syscall3(__NR_dup3, old, new, 0);
> #else
>        return my_syscall2(__NR_dup2, old, new);
> #endif
>  }

Ah, much better!

For robustness, I think it would be worth doing:

static __attribute__((unused))
int sys_dup2(int old, int new)
{
#if defined(__NR_dup3)
	return my_syscall3(__NR_dup3, old, new, 0);
#elif defined(__NR_dup2)
	return my_syscall2(__NR_dup2, old, new);
#else
#error Cannot implement dup2
#endif
}

... and getting rid of the ARCH_WANT_* definitions (which are never
legitimate for userspace to set). That way, there's no risk that we
accidentally use a bogus syscall number in future. Where the kernel does
implement a syscall, it will have done whatever is necessary to expose
the corresponding __NR_<syscall> to userspace without userspace needing
to define anything.

> I didn't want to do that because that would break userland which needs
> dup2(), hence the mapping to dup3 instead:
> 
>   static __attribute__((unused))
>   int sys_dup2(int old, int new)
>   {
>   #ifdef __NR_dup3
>           return my_syscall3(__NR_dup3, old, new, 0);
>   #else
>           return my_syscall2(__NR_dup2, old, new);
>   #endif
>   }

Sure, makes sense, though as above it might be worth adding an explicit
check for the fallback syscall number.

> I shouldn't need since these are already fixed in my tree. At first glance
> the equivalent of the following commits is missing from the kernel version:
> 
>    https://github.com/wtarreau/nolibc/commit/2379f25073f906d0bad22c48566fcffee8fc9cde
>    https://github.com/wtarreau/nolibc/commit/fd5272ec2c66e6f5b195d41c9c8978f58eb74668
>    https://github.com/wtarreau/nolibc/commit/47cc42a79c92305f4f8bc02fb28628a4fdd63eaa
>    https://github.com/wtarreau/nolibc/commit/d2dc42fd614991c741dfdc8b984864fa3cf64c8e
>    https://github.com/wtarreau/nolibc/commit/800f75c13ede49097325f82a4cca3515c44a7939
> 
> However I'm interested in knowing if the latest version fixes everything
> for you or not :
> 
>   https://raw.githubusercontent.com/wtarreau/nolibc/master/nolibc.h

I replaced my in-tree copy with that, and it built without issues, and
the tests ran correctly.

> OK thanks! I will retry here without setting those. I'm pretty sure I
> needed these ones to find the __NR_* values but it's possible that it
> was before I had the alternate ones and that these are finally not
> nedeed at all (which I would prefer as these are ugly).

Great! I reckon they're not needed at all so long as usage of each
__NR_* is suitably guarded (such as above).

If you do spot issues when removing ARCH_WANT_*, I'm happy to take a
look, and/or to test patches handling any fallout.

Thanks,
Mark.