Re: [PATCH v2 2/2] Extend struct r_debug to support multiple namespaces

On Tue, Aug 17, 2021 at 10:44 AM Daniel Walker <[email protected]> wrote:
>
> On Mon, Aug 16, 2021 at 06:06:29PM -0700, H.J. Lu wrote:
> > Glibc does not provide an interface for debugger to access libraries
> > loaded in multiple namespaces via dlmopen.
> >
> > The current rtld-debugger interface is described in the file:
> >
> > elf/rtld-debugger-interface.txt
> >
> > under the "Standard debugger interface" heading.  This interface only
> > provides access to the first link-map (LM_ID_BASE).
> >
> > Based on the patch from Conan C Huang <[email protected]>:
> >
> > https://sourceware.org/pipermail/libc-alpha/2020-June/115448.html
> >
> > 1. Bump r_version to 2.  This triggers the GDB bug:
> >
> > https://sourceware.org/bugzilla/show_bug.cgi?id=28236
> >
> > 2. Add struct r_debug_extended to extend struct r_debug into a linked-list,
> > where each element correlates to an unique namespace.
> > 3. Add a hidden symbol, _r_debug_extended, for struct r_debug_extended.
> > 4. Provide the compatibility symbol, _r_debug, with size of struct r_debug,
> > as an alise of _r_debug_extended, for programs which reference _r_debug.
>
> It's very similar to patches we have already created internally.
>
> One comment below,
>
> > ---
> >  csu/Makefile                    |  3 ++
> >  csu/rtld-sizes.sym              |  4 ++
> >  elf/Makefile                    | 22 ++++++++++-
> >  elf/dl-close.c                  |  2 +-
> >  elf/dl-debug-symbols-gen.c      | 24 +++++++++++
> >  elf/dl-debug-symbols.S          | 31 +++++++++++++++
> >  elf/dl-debug.c                  | 29 +++++++-------
> >  elf/dl-load.c                   |  2 +-
> >  elf/dl-open.c                   |  2 +-
> >  elf/dl-reloc-static-pie.c       |  2 +-
> >  elf/link.h                      | 70 +++++++++++++++++++++++++--------
> >  elf/rtld-debugger-interface.txt | 14 +++++++
> >  elf/rtld.c                      |  4 +-
> >  elf/tst-dlmopen4.c              | 68 ++++++++++++++++++++++++++++++++
> >  include/link.h                  |  4 ++
> >  sysdeps/generic/ldsodefs.h      |  5 ++-
> >  16 files changed, 245 insertions(+), 41 deletions(-)
> >  create mode 100644 csu/rtld-sizes.sym
> >  create mode 100644 elf/dl-debug-symbols-gen.c
> >  create mode 100644 elf/dl-debug-symbols.S
> >  create mode 100644 elf/tst-dlmopen4.c
> >
> > diff --git a/csu/Makefile b/csu/Makefile
> > index 3054329cea..e2390e4a7d 100644
> > --- a/csu/Makefile
> > +++ b/csu/Makefile
> > @@ -88,6 +88,9 @@ endif
> >  before-compile += $(objpfx)abi-tag.h
> >  generated += abi-tag.h
> >
> > +# Put it here to generate it earlier.
> > +gen-as-const-headers += rtld-sizes.sym
> > +
> >  # These are the special initializer/finalizer files.  They are always the
> >  # first and last file in the link.  crti.o ... crtn.o define the global
> >  # "functions" _init and _fini to run the .init and .fini sections.
> > diff --git a/csu/rtld-sizes.sym b/csu/rtld-sizes.sym
> > new file mode 100644
> > index 0000000000..40dd8edaec
> > --- /dev/null
> > +++ b/csu/rtld-sizes.sym
> > @@ -0,0 +1,4 @@
> > +#include <link.h>
> > +
> > +--
> > +COMPAT_R_DEBUG_SIZE  sizeof (struct r_debug)
> > diff --git a/elf/Makefile b/elf/Makefile
> > index 725537c40b..1444a53405 100644
> > --- a/elf/Makefile
> > +++ b/elf/Makefile
> > @@ -35,7 +35,8 @@ dl-routines = $(addprefix dl-,load lookup object reloc deps \
> >                                 execstack open close trampoline \
> >                                 exception sort-maps lookup-direct \
> >                                 call-libc-early-init write \
> > -                               thread_gscope_wait tls_init_tp)
> > +                               thread_gscope_wait tls_init_tp \
> > +                               debug-symbols)
> >  ifeq (yes,$(use-ldconfig))
> >  dl-routines += dl-cache
> >  endif
> > @@ -203,7 +204,7 @@ tests += restest1 preloadtest loadfail multiload origtest resolvfail \
> >        tst-tls16 tst-tls17 tst-tls18 tst-tls19 tst-tls-dlinfo \
> >        tst-align tst-align2 \
> >        tst-dlmodcount tst-dlopenrpath tst-deep1 \
> > -      tst-dlmopen1 tst-dlmopen3 \
> > +      tst-dlmopen1 tst-dlmopen3 tst-dlmopen4 \
> >        unload3 unload4 unload5 unload6 unload7 unload8 tst-global1 order2 \
> >        tst-audit1 tst-audit2 tst-audit8 tst-audit9 \
> >        tst-addr1 tst-thrlock \
> > @@ -672,6 +673,21 @@ LC_ALL=C sed $(ldd-rewrite) < $< \
> >  endef
> >  endif
> >
> > +ifeq ($(build-shared),yes)
> > +generated += dl-debug-compat-symbols.os dl-debug-compat-symbols.o
> > +
> > +libof-dl-debug-compat-symbols = rtld
> > +
> > +$(objpfx)dl-debug-compat-symbols.os: dl-debug-symbols-gen.c
> > +     $(compile-command.c) -S
> > +
> > +$(objpfx)dl-debug-compat-symbols.o: dl-debug-symbols-gen.c
> > +     $(compile-command.c) -S
> > +
> > +$(objpfx)dl-debug-symbols.os: $(objpfx)dl-debug-compat-symbols.os
> > +$(objpfx)dl-debug-symbols.o: $(objpfx)dl-debug-compat-symbols.o
> > +endif
> > +
> >  $(objpfx)ldd: ldd.bash.in $(common-objpfx)soversions.mk \
> >             $(common-objpfx)config.make
> >       $(gen-ldd)
> > @@ -1242,6 +1258,8 @@ $(objpfx)tst-dlmopen2.out: $(objpfx)tst-dlmopen1mod.so
> >
> >  $(objpfx)tst-dlmopen3.out: $(objpfx)tst-dlmopen1mod.so
> >
> > +$(objpfx)tst-dlmopen4.out: $(objpfx)tst-dlmopen1mod.so
> > +
> >  $(objpfx)tst-audit1.out: $(objpfx)tst-auditmod1.so
> >  tst-audit1-ENV = LD_AUDIT=$(objpfx)tst-auditmod1.so
> >
> > diff --git a/elf/dl-close.c b/elf/dl-close.c
> > index f39001cab9..f59ffdd666 100644
> > --- a/elf/dl-close.c
> > +++ b/elf/dl-close.c
> > @@ -500,7 +500,7 @@ _dl_close_worker (struct link_map *map, bool force)
> >  #endif
> >
> >    /* Notify the debugger we are about to remove some loaded objects.  */
> > -  struct r_debug *r = _dl_debug_initialize (0, nsid);
> > +  struct r_debug_extended *r = _dl_debug_initialize (0, nsid);
> >    r->r_state = RT_DELETE;
> >    _dl_debug_state ();
> >    LIBC_PROBE (unmap_start, 2, nsid, r);
> > diff --git a/elf/dl-debug-symbols-gen.c b/elf/dl-debug-symbols-gen.c
> > new file mode 100644
> > index 0000000000..2406260bcb
> > --- /dev/null
> > +++ b/elf/dl-debug-symbols-gen.c
> > @@ -0,0 +1,24 @@
> > +/* Generate the _r_debug_extended symbol used to communicate dynamic
> > +   linker state to the debugger at runtime.
> > +   Copyright (C) 2021 Free Software Foundation, Inc.
> > +   This file is part of the GNU C Library.
> > +
> > +   The GNU C Library is free software; you can redistribute it and/or
> > +   modify it under the terms of the GNU Lesser General Public
> > +   License as published by the Free Software Foundation; either
> > +   version 2.1 of the License, or (at your option) any later version.
> > +
> > +   The GNU C Library is distributed in the hope that it will be useful,
> > +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > +   Lesser General Public License for more details.
> > +
> > +   You should have received a copy of the GNU Lesser General Public
> > +   License along with the GNU C Library; if not, see
> > +   <https://www.gnu.org/licenses/>.  */
> > +
> > +#include <link.h>
> > +
> > +/* This structure communicates dl state to the debugger.  The debugger
> > +   finds it via the DT_DEBUG entry in the dynamic section.  */
> > +struct r_debug_extended _r_debug_extended;
> > diff --git a/elf/dl-debug-symbols.S b/elf/dl-debug-symbols.S
> > new file mode 100644
> > index 0000000000..0966b172ab
> > --- /dev/null
> > +++ b/elf/dl-debug-symbols.S
> > @@ -0,0 +1,31 @@
> > +/* Define symbols used to communicate dynamic linker state to the
> > +   debugger at runtime.
> > +   Copyright (C) 2021 Free Software Foundation, Inc.
> > +   This file is part of the GNU C Library.
> > +
> > +   The GNU C Library is free software; you can redistribute it and/or
> > +   modify it under the terms of the GNU Lesser General Public
> > +   License as published by the Free Software Foundation; either
> > +   version 2.1 of the License, or (at your option) any later version.
> > +
> > +   The GNU C Library is distributed in the hope that it will be useful,
> > +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > +   Lesser General Public License for more details.
> > +
> > +   You should have received a copy of the GNU Lesser General Public
> > +   License along with the GNU C Library; if not, see
> > +   <https://www.gnu.org/licenses/>.  */
> > +
> > +#include <rtld-sizes.h>
> > +
> > +#ifdef SHARED
> > +# include "dl-debug-compat-symbols.os"
> > +#else
> > +# include "dl-debug-compat-symbols.o"
> > +#endif
> > +
> > +/* Define the compatibility symbol, _r_debug, with size of struct r_debug,
> > +   as an alias of _r_debug_extended.  */
> > +declare_object_symbol_alias (_r_debug, _r_debug_extended,
> > +                          COMPAT_R_DEBUG_SIZE);
> > diff --git a/elf/dl-debug.c b/elf/dl-debug.c
> > index 2cd5f09753..9e884a5648 100644
> > --- a/elf/dl-debug.c
> > +++ b/elf/dl-debug.c
> > @@ -30,34 +30,35 @@ extern const int verify_link_map_members[(VERIFY_MEMBER (l_addr)
> >                                         && VERIFY_MEMBER (l_prev))
> >                                        ? 1 : -1];
> >
> > -/* This structure communicates dl state to the debugger.  The debugger
> > -   normally finds it via the DT_DEBUG entry in the dynamic section, but in
> > -   a statically-linked program there is no dynamic section for the debugger
> > -   to examine and it looks for this particular symbol name.  */
> > -struct r_debug _r_debug;
> > -
> > -
> >  /* Initialize _r_debug if it has not already been done.  The argument is
> >     the run-time load address of the dynamic linker, to be put in
> >     _r_debug.r_ldbase.  Returns the address of _r_debug.  */
> >
> > -struct r_debug *
> > +struct r_debug_extended *
> >  _dl_debug_initialize (ElfW(Addr) ldbase, Lmid_t ns)
> >  {
> > -  struct r_debug *r;
> > +  struct r_debug_extended *r, *rp;
> >
> >    if (ns == LM_ID_BASE)
> > -    r = &_r_debug;
> > +    r = &_r_debug_extended;
> >    else
> > -    r = &GL(dl_ns)[ns]._ns_debug;
> > +    {
> > +      r = &GL(dl_ns)[ns]._ns_debug;
> > +      rp = &GL(dl_ns)[ns - 1]._ns_debug;
> > +      rp->r_next = r;
> > +      if (ns - 1 == LM_ID_BASE)
> > +        _r_debug_extended.r_next = r;
> > +    }
>
> I'm not sure why, but we have an issue assigning the dl_ns[ns - 1] when ns -1
> was equal to LM_ID_BASE. I don't know what the issue was but we had tests fail
> as a result of it. In my case I add an else clause and only set r_next when it
> wasn't LM_ID_BASE, and for debugging it didn't matter since the structure was
> all that mattered.
>
> Daniel

_dl_debug_initialize can be called multiple times like

 _dl_debug_initialize (0, nsid)->r_state = RT_CONSISTENT;

I will update my patch to fix it.

-- 
H.J.