Messages in this thread | | | Subject | Re: [PATCH] pidfd: fix test failure due to stack overflow on some arches | From | Shuah Khan <> | Date | Wed, 2 Feb 2022 08:52:47 -0700 |
| |
On 1/28/22 1:56 AM, Christian Brauner wrote: > On Thu, Jan 27, 2022 at 01:29:51PM -0800, Axel Rasmussen wrote: >> When running the pidfd_fdinfo_test on arm64, it fails for me. After some >> digging, the reason is that the child exits due to SIGBUS, because it >> overflows the 1024 byte stack we've reserved for it. >> >> To fix the issue, increase the stack size to 8192 bytes (this number is >> somewhat arbitrary, and was arrived at through experimentation -- I kept >> doubling until the failure no longer occurred). >> >> Also, let's make the issue easier to debug. wait_for_pid() returns an >> ambiguous value: it may return -1 in all of these cases: >> >> 1. waitpid() itself returned -1 >> 2. waitpid() returned success, but we found !WIFEXITED(status). >> 3. The child process exited, but it did so with a -1 exit code. >> >> There's no way for the caller to tell the difference. So, at least log >> which occurred, so the test runner can debug things. >> >> While debugging this, I found that we had !WIFEXITED(), because the >> child exited due to a signal. This seems like a reasonably common case, >> so also print out whether or not we have WIFSIGNALED(), and the >> associated WTERMSIG() (if any). This lets us see the SIGBUS I'm fixing >> clearly when it occurs. >> >> Finally, I'm suspicious of allocating the child's stack on our stack. >> man clone(2) suggests that the correct way to do this is with mmap(), >> and in particular by setting MAP_STACK. So, switch to doing it that way >> instead. > > Heh, yes. :) > > commit 99c3a000279919cc4875c9dfa9c3ebb41ed8773e > Author: Michael Kerrisk <mtk.manpages@gmail.com> > Date: Thu Nov 14 12:19:21 2019 +0100 > > clone.2: Allocate child's stack using mmap(2) rather than malloc(3) > > Christian Brauner suggested mmap(MAP_STACKED), rather than > malloc(), as the canonical way of allocating a stack for the > child of clone(), and Jann Horn noted some reasons why: > > Not on Linux, but on OpenBSD, they do use MAP_STACK now > AFAIK; this was announced here: > <http://openbsd-archive.7691.n7.nabble.com/stack-register-checking-td338238.html>. > Basically they periodically check whether the userspace > stack pointer points into a MAP_STACK region, and if not, > they kill the process. So even if it's a no-op on Linux, it > might make sense to advise people to use the flag to improve > portability? I'm not sure if that's something that belongs > in Linux manpages. > > Another reason against malloc() is that when setting up > thread stacks in proper, reliable software, you'll probably > want to place a guard page (in other words, a 4K PROT_NONE > VMA) at the bottom of the stack to reliably catch stack > overflows; and you probably don't want to do that with > malloc, in particular with non-page-aligned allocations. > > And the OpenBSD 6.5 manual pages says: > > MAP_STACK > Indicate that the mapping is used as a stack. This > flag must be used in combination with MAP_ANON and > MAP_PRIVATE. > > And I then noticed that MAP_STACK seems already to be on > FreeBSD for a long time: > > MAP_STACK > Map the area as a stack. MAP_ANON is implied. > Offset should be 0, fd must be -1, and prot should > include at least PROT_READ and PROT_WRITE. This > option creates a memory region that grows to at > most len bytes in size, starting from the stack > top and growing down. The stack top is the start‐ > ing address returned by the call, plus len bytes. > The bottom of the stack at maximum growth is the > starting address returned by the call. > > The entire area is reserved from the point of view > of other mmap() calls, even if not faulted in yet. > > Reported-by: Jann Horn <jannh@google.com> > Reported-by: Christian Brauner <christian.brauner@ubuntu.com> > Signed-off-by: Michael Kerrisk <mtk.manpages@gmail.com> > > >> >> Signed-off-by: Axel Rasmussen <axelrasmussen@google.com> >> --- > > Yeah, stack handling - especially with legacy clone() - is yucky on the > best of days. Thank you for the fix. > > Acked-by: Christian Brauner <brauner@kernel.org> >
Thank you both. Will apply for 5.17-rc4 or so.
thanks, -- Shuah
| |