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

["H.J. Lu" <hjl.tools@gmail.com>]

On Tue, Aug 17, 2021 at 1:25 PM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> On Tue, Aug 17, 2021 at 10:44 AM Daniel Walker <danielwa@cisco.com> 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 <conhuang@cisco.com>:
> > >
> > > 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.

I sent out the v3 patch to fix it.

-- 
H.J.