diff mbox series

libgcc patch committed: Avoid hooks in split-stack code

Message ID CAOyqgcU-iKLhYQGU51QzfHnhbydea9FDcQyNzVguFigx0=4L=w@mail.gmail.com
State New
Headers show
Series libgcc patch committed: Avoid hooks in split-stack code | expand

Commit Message

Li, Pan2 via Gcc-patches April 3, 2020, 9:58 p.m. UTC 
The split-stack code invokes mmap and munmap with a limited amount of
stack space.  That is fine when the functions just make a system call,
but it's not fine when programs use LD_PRELOAD or linker tricks to add
hooks to mmap/munmap.  Those hooks may use too much stack space,
leading to an obscure program crash.

Avoid that at least on GNU/Linux by calling __mmap/__munmap instead.

Bootstrapped and ran Go and split-stack tests on x86_64-pc-linux-gnu.
Committed to mainline.

Ian

2020-04-03  Ian Lance Taylor  <iant@golang.org>

* generic-morestack.c: On GNU/Linux use __mmap/__munmap rather
than mmap/munmap, to avoid hooks.

Comments

Eric Botcazou April 4, 2020, 5:10 p.m. UTC | #1 
> 2020-04-03  Ian Lance Taylor  <iant@golang.org>
> 
> * generic-morestack.c: On GNU/Linux use __mmap/__munmap rather
> than mmap/munmap, to avoid hooks.

This breaks the build of gotools for me with undefined references to __mmap 
and __munmap from libgcc/generic-morestack.c (it is with glibc 2.22).
Li, Pan2 via Gcc-patches April 4, 2020, 8:23 p.m. UTC | #2 
On Sat, Apr 4, 2020 at 10:10 AM Eric Botcazou <botcazou@adacore.com> wrote:
>
> > 2020-04-03  Ian Lance Taylor  <iant@golang.org>
> >
> > * generic-morestack.c: On GNU/Linux use __mmap/__munmap rather
> > than mmap/munmap, to avoid hooks.
>
> This breaks the build of gotools for me with undefined references to __mmap
> and __munmap from libgcc/generic-morestack.c (it is with glibc 2.22).

Hmmm, sorry about that.  Which target?  x86_64?  It does seem that
glibc consolidated mmap implementations in 2.26, but even in 2.22 I
see definitions for __mmap.

Ian
Eric Botcazou April 4, 2020, 8:28 p.m. UTC | #3 
> Hmmm, sorry about that.  Which target?  x86_64?  It does seem that
> glibc consolidated mmap implementations in 2.26, but even in 2.22 I
> see definitions for __mmap.

GNU C Library (GNU libc) stable release version 2.22 (git bbab82c25da9), by 
Roland McGrath et al.
Copyright (C) 2015 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.
There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A
PARTICULAR PURPOSE.
Configured for x86_64-suse-linux.
Compiled by GNU CC version 4.8.5.
Available extensions:
        crypt add-on version 2.1 by Michael Glad and others
        GNU Libidn by Simon Josefsson
        Native POSIX Threads Library by Ulrich Drepper et al
        BIND-8.2.3-T5B
libc ABIs: UNIQUE IFUNC
For bug reporting instructions, please see:
<http://bugs.opensuse.org>.

eric@polaris:~/build/gcc/native> readelf -s /lib64/libc.so.6 | grep mmap
   317: 00000000000e8ac0    36 FUNC    WEAK   DEFAULT   13 mmap64@@GLIBC_2.2.5
   657: 00000000000e8ac0    36 FUNC    WEAK   DEFAULT   13 mmap@@GLIBC_2.2.5
   332: 0000000000070f00   320 FUNC    LOCAL  DEFAULT   13 
_IO_wfile_underflow_mmap
   364: 0000000000075310   453 FUNC    LOCAL  DEFAULT   13 mmap_remap_check
   365: 00000000000754f0    90 FUNC    LOCAL  DEFAULT   13 _IO_file_sync_mmap
   366: 0000000000075550   306 FUNC    LOCAL  DEFAULT   13 decide_maybe_mmap
   368: 00000000000757a0   238 FUNC    LOCAL  DEFAULT   13 
_IO_file_xsgetn_mmap
  1727: 000000000039e5c0   168 OBJECT  LOCAL  DEFAULT   28 _IO_file_jumps_mmap
  2012: 00000000000e8ac0    36 FUNC    LOCAL  DEFAULT   13 __GI_mmap
  2065: 0000000000075a80   224 FUNC    LOCAL  DEFAULT   13 
_IO_file_seekoff_mmap
  2449: 00000000000752c0    74 FUNC    LOCAL  DEFAULT   13 _IO_file_close_mmap
  2476: 00000000000e8ac0    36 FUNC    LOCAL  DEFAULT   13 __GI_mmap64
  2520: 000000000006b040    63 FUNC    LOCAL  DEFAULT   13 __fopen_maybe_mmap
  2605: 00000000000e8ac0    36 FUNC    LOCAL  DEFAULT   13 __GI___mmap64
  2742: 00000000000e8ac0    36 FUNC    LOCAL  DEFAULT   13 __GI___mmap
  2993: 00000000000e8ac0    36 FUNC    LOCAL  DEFAULT   13 __mmap64
  3161: 00000000000e8ac0    36 FUNC    LOCAL  DEFAULT   13 __mmap
  3304: 0000000000074d10    93 FUNC    LOCAL  DEFAULT   13 
_IO_file_setbuf_mmap
  3351: 0000000000075b60    81 FUNC    LOCAL  DEFAULT   13 
_IO_file_underflow_mmap
  3365: 000000000039e500   168 OBJECT  LOCAL  DEFAULT   28 
_IO_file_jumps_maybe_mmap
  3483: 000000000039e140   168 OBJECT  LOCAL  DEFAULT   28 
_IO_wfile_jumps_mmap
  5919: 00000000000e8ac0    36 FUNC    WEAK   DEFAULT   13 mmap64
  6128: 00000000000e8ac0    36 FUNC    WEAK   DEFAULT   13 mmap
Li, Pan2 via Gcc-patches April 4, 2020, 8:35 p.m. UTC | #4 
On Sat, Apr 4, 2020 at 1:28 PM Eric Botcazou <botcazou@adacore.com> wrote:
>
> > Hmmm, sorry about that.  Which target?  x86_64?  It does seem that
> > glibc consolidated mmap implementations in 2.26, but even in 2.22 I
> > see definitions for __mmap.
>

commit fa872e1b6210e81e60d6029429f0a083b8eab26e
Author: Adhemerval Zanella <adhemerval.zanella@linaro.org>
Date:   Fri Dec 2 16:32:28 2016 -0200

    Clean pthread functions namespaces for C11 threads
...
            * misc/Versions (libc): Export __mmap, __munmap, __mprotect,
            __sched_get_priority_min, and __sched_get_priority_max under
            GLIBC_PRIVATE.

was checked into glibc 2.26.
Li, Pan2 via Gcc-patches April 4, 2020, 8:48 p.m. UTC | #5 
On Sat, Apr 4, 2020 at 1:35 PM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> On Sat, Apr 4, 2020 at 1:28 PM Eric Botcazou <botcazou@adacore.com> wrote:
> >
> > > Hmmm, sorry about that.  Which target?  x86_64?  It does seem that
> > > glibc consolidated mmap implementations in 2.26, but even in 2.22 I
> > > see definitions for __mmap.
> >
>
> commit fa872e1b6210e81e60d6029429f0a083b8eab26e
> Author: Adhemerval Zanella <adhemerval.zanella@linaro.org>
> Date:   Fri Dec 2 16:32:28 2016 -0200
>
>     Clean pthread functions namespaces for C11 threads
> ...
>             * misc/Versions (libc): Export __mmap, __munmap, __mprotect,
>             __sched_get_priority_min, and __sched_get_priority_max under
>             GLIBC_PRIVATE.
>
> was checked into glibc 2.26.

Thanks.

I committed this patch.

Ian
diff --git a/libgcc/generic-morestack.c b/libgcc/generic-morestack.c
index bb9f67a7366..fa2062e2bb3 100644
--- a/libgcc/generic-morestack.c
+++ b/libgcc/generic-morestack.c
@@ -60,7 +60,7 @@ see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
    glibc on GNU/Linux we can avoid the problem by calling __mmap and
    __munmap.  */
 
-#ifdef __gnu_linux__
+#if defined(__gnu_linux__) && (__GLIBC__ > 2 || (__GLIBC__ == 2 && __GLIBC_MINOR__ >= 26))
 
 extern void *__mmap (void *, size_t, int, int, int, off_t);
 extern int __munmap (void *, size_t);
Eric Botcazou April 4, 2020, 9:16 p.m. UTC | #6 
> I committed this patch.

Works for me, thanks for the quick fix!
Li, Pan2 via Gcc-patches April 7, 2020, 10:19 a.m. UTC | #7 
On Fri, Apr 3, 2020 at 11:59 PM Ian Lance Taylor via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> The split-stack code invokes mmap and munmap with a limited amount of
> stack space.  That is fine when the functions just make a system call,
> but it's not fine when programs use LD_PRELOAD or linker tricks to add
> hooks to mmap/munmap.  Those hooks may use too much stack space,
> leading to an obscure program crash.
>
> Avoid that at least on GNU/Linux by calling __mmap/__munmap instead.
>
> Bootstrapped and ran Go and split-stack tests on x86_64-pc-linux-gnu.
> Committed to mainline.

This causes references to GLIBC_PRIVATE exported symbols which is
highly undesirable for system integrators (no ABI stability is guaranteed
for those).  I opened the regression PR94513 for this.

Richard.

> Ian
>
> 2020-04-03  Ian Lance Taylor  <iant@golang.org>
>
> * generic-morestack.c: On GNU/Linux use __mmap/__munmap rather
> than mmap/munmap, to avoid hooks.
Li, Pan2 via Gcc-patches April 7, 2020, 10:33 a.m. UTC | #8 
On Tue, Apr 07, 2020 at 12:19:45PM +0200, Richard Biener via Gcc-patches wrote:
> On Fri, Apr 3, 2020 at 11:59 PM Ian Lance Taylor via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
> >
> > The split-stack code invokes mmap and munmap with a limited amount of
> > stack space.  That is fine when the functions just make a system call,
> > but it's not fine when programs use LD_PRELOAD or linker tricks to add
> > hooks to mmap/munmap.  Those hooks may use too much stack space,
> > leading to an obscure program crash.
> >
> > Avoid that at least on GNU/Linux by calling __mmap/__munmap instead.
> >
> > Bootstrapped and ran Go and split-stack tests on x86_64-pc-linux-gnu.
> > Committed to mainline.
> 
> This causes references to GLIBC_PRIVATE exported symbols which is
> highly undesirable for system integrators (no ABI stability is guaranteed
> for those).  I opened the regression PR94513 for this.

Yeah, GLIBC_PRIVATE symbols may only be used by glibc itself, not by
anything else.
I'd suggest to use syscall function instead (though, that can be interposed
too).

	Jakub
Li, Pan2 via Gcc-patches April 7, 2020, 6:31 p.m. UTC | #9 
On Tue, Apr 7, 2020 at 3:33 AM Jakub Jelinek <jakub@redhat.com> wrote:
>
> On Tue, Apr 07, 2020 at 12:19:45PM +0200, Richard Biener via Gcc-patches wrote:
> > On Fri, Apr 3, 2020 at 11:59 PM Ian Lance Taylor via Gcc-patches
> > <gcc-patches@gcc.gnu.org> wrote:
> > >
> > > The split-stack code invokes mmap and munmap with a limited amount of
> > > stack space.  That is fine when the functions just make a system call,
> > > but it's not fine when programs use LD_PRELOAD or linker tricks to add
> > > hooks to mmap/munmap.  Those hooks may use too much stack space,
> > > leading to an obscure program crash.
> > >
> > > Avoid that at least on GNU/Linux by calling __mmap/__munmap instead.
> > >
> > > Bootstrapped and ran Go and split-stack tests on x86_64-pc-linux-gnu.
> > > Committed to mainline.
> >
> > This causes references to GLIBC_PRIVATE exported symbols which is
> > highly undesirable for system integrators (no ABI stability is guaranteed
> > for those).  I opened the regression PR94513 for this.
>
> Yeah, GLIBC_PRIVATE symbols may only be used by glibc itself, not by
> anything else.
> I'd suggest to use syscall function instead (though, that can be interposed
> too).

Sorry about that.  OK, let's try syscall.  I committed this patch.
Bootstrapped and tested on x86_64-pc-linux-gnu, did a small amount of
testing on s390x (but I don't have full access to an s390x system).

Ian

2020-04-07  Ian Lance Taylor  <iant@golang.org>

PR libgcc/94513
* generic-morestack.c: Give up trying to use __mmap/__munmap, use
syscall instead.
diff --git a/libgcc/generic-morestack.c b/libgcc/generic-morestack.c
index fa2062e2bb3..35764a848f6 100644
--- a/libgcc/generic-morestack.c
+++ b/libgcc/generic-morestack.c
@@ -56,17 +56,56 @@ see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
 /* Some systems use LD_PRELOAD or similar tricks to add hooks to
    mmap/munmap.  That breaks this code, because when we call mmap
    there is enough stack space for the system call but there is not,
-   in general, enough stack space to run a hook.  At least when using
-   glibc on GNU/Linux we can avoid the problem by calling __mmap and
-   __munmap.  */
+   in general, enough stack space to run a hook.  Try to avoid the
+   problem by calling syscall directly.  We only do this on GNU/Linux
+   for now, but it should be easy to add support for more systems with
+   testing.  */
 
-#if defined(__gnu_linux__) && (__GLIBC__ > 2 || (__GLIBC__ == 2 && __GLIBC_MINOR__ >= 26))
+#if defined(__gnu_linux__)
 
-extern void *__mmap (void *, size_t, int, int, int, off_t);
-extern int __munmap (void *, size_t);
+#include <sys/syscall.h>
 
-#define mmap __mmap
-#define munmap __munmap
+#if defined(SYS_mmap) || defined(SYS_mmap2)
+
+#ifdef SYS_mmap2
+#define MORESTACK_MMAP SYS_mmap2
+#define MORESTACK_ADJUST_OFFSET(x) ((x) / 4096ULL)
+#else
+#define MORESTACK_MMAP SYS_mmap
+#define MORESTACK_ADJUST_OFFSET(x) (x)
+#endif
+
+static void *
+morestack_mmap (void *addr, size_t length, int prot, int flags, int fd,
+		off_t offset)
+{
+  offset = MORESTACK_ADJUST_OFFSET (offset);
+
+#ifdef __s390__
+  long args[6] = { (long) addr, (long) length, (long) prot, (long) flags,
+		   (long) fd, (long) offset };
+  return (void *) syscall (MORESTACK_MMAP, args);
+#else
+  return (void *) syscall (MORESTACK_MMAP, addr, length, prot, flags, fd,
+			   offset);
+#endif
+}
+
+#define mmap morestack_mmap
+
+#endif /* defined(SYS_MMAP) || defined(SYS_mmap2) */
+
+#if defined(SYS_munmap)
+
+static int
+morestack_munmap (void * addr, size_t length)
+{
+  return (int) syscall (SYS_munmap, addr, length);
+}
+
+#define munmap morestack_munmap
+
+#endif /* defined(SYS_munmap) */
 
 #endif /* defined(__gnu_linux__) */
diff mbox series

Patch

diff --git a/libgcc/generic-morestack.c b/libgcc/generic-morestack.c
index c26dba1ae4a..bb9f67a7366 100644
--- a/libgcc/generic-morestack.c
+++ b/libgcc/generic-morestack.c
@@ -53,6 +53,23 @@  see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
 
 #include "generic-morestack.h"
 
+/* Some systems use LD_PRELOAD or similar tricks to add hooks to
+   mmap/munmap.  That breaks this code, because when we call mmap
+   there is enough stack space for the system call but there is not,
+   in general, enough stack space to run a hook.  At least when using
+   glibc on GNU/Linux we can avoid the problem by calling __mmap and
+   __munmap.  */
+
+#ifdef __gnu_linux__
+
+extern void *__mmap (void *, size_t, int, int, int, off_t);
+extern int __munmap (void *, size_t);
+
+#define mmap __mmap
+#define munmap __munmap
+
+#endif /* defined(__gnu_linux__) */
+
 typedef unsigned uintptr_type __attribute__ ((mode (pointer)));
 
 /* This file contains subroutines that are used by code compiled with