| Message ID | 1517600891-23632-7-git-send-email-francis.deslauriers@efficios.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | Support provider duplicates and unloading | expand |
----- On Feb 2, 2018, at 2:48 PM, Francis Deslauriers francis.deslauriers at efficios.com wrote: > dlopen() increments the refcount of the library thus preventing the > refcount to reach zero in the case of dlclose; The changelog and comment do not explain _why_ this is needed. The scenario is: - an application is _not_ linked against liblttng-ust.so - the application dlopen() a tracepoint probe, - the tracepoint probe .so is linked against liblttng-ust, and this is what ends up getting lttng-ust loaded, Given that our goal is to allow dlclose() of tracepoint probes (new features), we don't want the side effect of this dlclose() to also try to unload liblttng-ust. Because liblttng-ust does _not_ support being dlclose'd, we need to "pin" it in memory by grabbing an extra reference on the library, with a NODELETE RTLD flag. Thanks, Mathieu > > Signed-off-by: Francis Deslauriers <francis.deslauriers at efficios.com> > --- > configure.ac | 1 + > liblttng-ust/Makefile.am | 2 ++ > liblttng-ust/lttng-ust-comm.c | 22 ++++++++++++++++++++++ > 3 files changed, 25 insertions(+) > > diff --git a/configure.ac b/configure.ac > index b0b4157..4fc6f9c 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -25,6 +25,7 @@ m4_define([UST_LIB_V_MINOR], [0]) > m4_define([UST_LIB_V_PATCH], [0]) > > AC_SUBST([LTTNG_UST_LIBRARY_VERSION], > [UST_LIB_V_MAJOR:UST_LIB_V_MINOR:UST_LIB_V_PATCH]) > +AC_SUBST([LTTNG_UST_LIBRARY_VERSION_MAJOR], [UST_LIB_V_MAJOR]) > # note: remember to update tracepoint.h dlopen() to match this version > # number. TODO: eventually automate by exporting the major number. > > diff --git a/liblttng-ust/Makefile.am b/liblttng-ust/Makefile.am > index 982be69..a7edfd5 100644 > --- a/liblttng-ust/Makefile.am > +++ b/liblttng-ust/Makefile.am > @@ -60,6 +60,8 @@ liblttng_ust_runtime_la_SOURCES = \ > string-utils.c \ > string-utils.h > > +liblttng_ust_runtime_la_CFLAGS = > -DLTTNG_UST_LIBRARY_VERSION_MAJOR=\"$(LTTNG_UST_LIBRARY_VERSION_MAJOR)\" > + > if HAVE_PERF_EVENT > liblttng_ust_runtime_la_SOURCES += \ > lttng-context-perf-counters.c \ > diff --git a/liblttng-ust/lttng-ust-comm.c b/liblttng-ust/lttng-ust-comm.c > index 511b9cf..ed912b8 100644 > --- a/liblttng-ust/lttng-ust-comm.c > +++ b/liblttng-ust/lttng-ust-comm.c > @@ -27,6 +27,7 @@ > #include <sys/stat.h> > #include <sys/types.h> > #include <sys/wait.h> > +#include <dlfcn.h> > #include <fcntl.h> > #include <unistd.h> > #include <errno.h> > @@ -59,6 +60,9 @@ > #include "../libringbuffer/getcpu.h" > #include "getenv.h" > > +/* Concatenate lttng ust shared library name with its major version number. */ > +#define LTTNG_UST_LIB_SO_NAME "liblttng-ust.so." > LTTNG_UST_LIBRARY_VERSION_MAJOR > + > /* > * Has lttng ust comm constructor been called ? > */ > @@ -1648,6 +1652,7 @@ void __attribute__((constructor)) lttng_ust_init(void) > pthread_attr_t thread_attr; > int timeout_mode; > int ret; > + void *handle; > > if (uatomic_xchg(&initialized, 1) == 1) > return; > @@ -1662,6 +1667,23 @@ void __attribute__((constructor)) lttng_ust_init(void) > lttng_ust_loaded = 1; > > /* > + * Manually load liblttng-ust.so to increment the dynamic loader's internal > + * refcount for this library so it never becomes zero, thus never gets > + * unloaded from the address space of the process. Since we are already > + * running in the constructor of the LTTNG_UST_LIB_SO_NAME library, calling > + * dlopen will simply increment the refcount and no additionnal work is > + * needed by the dynamic loader as the shared library is already loaded in > + * the address space. As a safe guard, we use the RTLD_NODELETE flag to > + * prevent unloading of the UST library if its refcount becomes zero > + * (which should never happen). Do the return value check but discard the > + * handle at the end of the function as it's not needed. > + */ > + handle = dlopen(LTTNG_UST_LIB_SO_NAME, RTLD_LAZY | RTLD_NODELETE); > + if (!handle) { > + ERR("dlopen of liblttng-ust shared library (%s).", LTTNG_UST_LIB_SO_NAME); > + } > + > + /* > * We want precise control over the order in which we construct > * our sub-libraries vs starting to receive commands from > * sessiond (otherwise leading to errors when trying to create > -- > 2.7.4
2018-02-07 15:59 GMT-05:00 Mathieu Desnoyers <mathieu.desnoyers at efficios.com>: > ----- On Feb 2, 2018, at 2:48 PM, Francis Deslauriers francis.deslauriers at efficios.com wrote: > >> dlopen() increments the refcount of the library thus preventing the >> refcount to reach zero in the case of dlclose; > > The changelog and comment do not explain _why_ this is needed. > > The scenario is: > > - an application is _not_ linked against liblttng-ust.so > - the application dlopen() a tracepoint probe, > - the tracepoint probe .so is linked against liblttng-ust, and this is what ends up > getting lttng-ust loaded, > > Given that our goal is to allow dlclose() of tracepoint probes (new features), we don't > want the side effect of this dlclose() to also try to unload liblttng-ust. Because > liblttng-ust does _not_ support being dlclose'd, we need to "pin" it in memory by > grabbing an extra reference on the library, with a NODELETE RTLD flag. > I agree, here is a revised version of the commit message and comment: Manually dlopen() liblttng-ust.so to prevent unloading The support of dlclose() allows for the following problematic scenario: - Application is not linked against the liblttng-ust.so - Application dlopen() a probe provider library that is linked against liblttng-ust.so - Application dlclose() the probe provider In this scenario, the probe provider has a dependency on liblttng-ust.so, so when it's loaded by the application, liblttng-ust.so is loaded too. The probe provider library now has the only reference to the liblttng-ust.so library. When the application calls dlclose() on it, all its references are dropped, thus triggering the unloading of both the probe provider library and liblttng-ust.so. This scenario is problematic because the probe provider needs the liblttng-ust.so library to complete its own unloading. As we can't control the order in which libraries are unloaded, we must ensure that liblttng-ust.so is not unloaded before the probe provider. To prevent that unloading, we pin it in memory by grabbing an extra reference on the library, with a RTLD_NODELETE flag. This will prevent the dynamic loader from ever removing the liblttng-ust.so library from the process' address space. Thank you, Francis > Thanks, > > Mathieu > > >> >> Signed-off-by: Francis Deslauriers <francis.deslauriers at efficios.com> >> --- >> configure.ac | 1 + >> liblttng-ust/Makefile.am | 2 ++ >> liblttng-ust/lttng-ust-comm.c | 22 ++++++++++++++++++++++ >> 3 files changed, 25 insertions(+) >> >> diff --git a/configure.ac b/configure.ac >> index b0b4157..4fc6f9c 100644 >> --- a/configure.ac >> +++ b/configure.ac >> @@ -25,6 +25,7 @@ m4_define([UST_LIB_V_MINOR], [0]) >> m4_define([UST_LIB_V_PATCH], [0]) >> >> AC_SUBST([LTTNG_UST_LIBRARY_VERSION], >> [UST_LIB_V_MAJOR:UST_LIB_V_MINOR:UST_LIB_V_PATCH]) >> +AC_SUBST([LTTNG_UST_LIBRARY_VERSION_MAJOR], [UST_LIB_V_MAJOR]) >> # note: remember to update tracepoint.h dlopen() to match this version >> # number. TODO: eventually automate by exporting the major number. >> >> diff --git a/liblttng-ust/Makefile.am b/liblttng-ust/Makefile.am >> index 982be69..a7edfd5 100644 >> --- a/liblttng-ust/Makefile.am >> +++ b/liblttng-ust/Makefile.am >> @@ -60,6 +60,8 @@ liblttng_ust_runtime_la_SOURCES = \ >> string-utils.c \ >> string-utils.h >> >> +liblttng_ust_runtime_la_CFLAGS = >> -DLTTNG_UST_LIBRARY_VERSION_MAJOR=\"$(LTTNG_UST_LIBRARY_VERSION_MAJOR)\" >> + >> if HAVE_PERF_EVENT >> liblttng_ust_runtime_la_SOURCES += \ >> lttng-context-perf-counters.c \ >> diff --git a/liblttng-ust/lttng-ust-comm.c b/liblttng-ust/lttng-ust-comm.c >> index 511b9cf..ed912b8 100644 >> --- a/liblttng-ust/lttng-ust-comm.c >> +++ b/liblttng-ust/lttng-ust-comm.c >> @@ -27,6 +27,7 @@ >> #include <sys/stat.h> >> #include <sys/types.h> >> #include <sys/wait.h> >> +#include <dlfcn.h> >> #include <fcntl.h> >> #include <unistd.h> >> #include <errno.h> >> @@ -59,6 +60,9 @@ >> #include "../libringbuffer/getcpu.h" >> #include "getenv.h" >> >> +/* Concatenate lttng ust shared library name with its major version number. */ >> +#define LTTNG_UST_LIB_SO_NAME "liblttng-ust.so." >> LTTNG_UST_LIBRARY_VERSION_MAJOR >> + >> /* >> * Has lttng ust comm constructor been called ? >> */ >> @@ -1648,6 +1652,7 @@ void __attribute__((constructor)) lttng_ust_init(void) >> pthread_attr_t thread_attr; >> int timeout_mode; >> int ret; >> + void *handle; >> >> if (uatomic_xchg(&initialized, 1) == 1) >> return; >> @@ -1662,6 +1667,23 @@ void __attribute__((constructor)) lttng_ust_init(void) >> lttng_ust_loaded = 1; >> >> /* >> + * Manually load liblttng-ust.so to increment the dynamic loader's internal >> + * refcount for this library so it never becomes zero, thus never gets >> + * unloaded from the address space of the process. Since we are already >> + * running in the constructor of the LTTNG_UST_LIB_SO_NAME library, calling >> + * dlopen will simply increment the refcount and no additionnal work is >> + * needed by the dynamic loader as the shared library is already loaded in >> + * the address space. As a safe guard, we use the RTLD_NODELETE flag to >> + * prevent unloading of the UST library if its refcount becomes zero >> + * (which should never happen). Do the return value check but discard the >> + * handle at the end of the function as it's not needed. >> + */ >> + handle = dlopen(LTTNG_UST_LIB_SO_NAME, RTLD_LAZY | RTLD_NODELETE); >> + if (!handle) { >> + ERR("dlopen of liblttng-ust shared library (%s).", LTTNG_UST_LIB_SO_NAME); >> + } >> + >> + /* >> * We want precise control over the order in which we construct >> * our sub-libraries vs starting to receive commands from >> * sessiond (otherwise leading to errors when trying to create >> -- >> 2.7.4 > > -- > Mathieu Desnoyers > EfficiOS Inc. > http://www.efficios.com
----- On Feb 8, 2018, at 3:24 PM, Francis Deslauriers francis.deslauriers at efficios.com wrote: > 2018-02-07 15:59 GMT-05:00 Mathieu Desnoyers <mathieu.desnoyers at efficios.com>: >> ----- On Feb 2, 2018, at 2:48 PM, Francis Deslauriers >> francis.deslauriers at efficios.com wrote: >> >>> dlopen() increments the refcount of the library thus preventing the >>> refcount to reach zero in the case of dlclose; >> >> The changelog and comment do not explain _why_ this is needed. >> >> The scenario is: >> >> - an application is _not_ linked against liblttng-ust.so >> - the application dlopen() a tracepoint probe, >> - the tracepoint probe .so is linked against liblttng-ust, and this is what ends >> up >> getting lttng-ust loaded, >> >> Given that our goal is to allow dlclose() of tracepoint probes (new features), >> we don't >> want the side effect of this dlclose() to also try to unload liblttng-ust. >> Because >> liblttng-ust does _not_ support being dlclose'd, we need to "pin" it in memory >> by >> grabbing an extra reference on the library, with a NODELETE RTLD flag. >> > I agree, here is a revised version of the commit message and comment: > > Manually dlopen() liblttng-ust.so to prevent unloading > > The support of dlclose() allows for the following problematic scenario: > - Application is not linked against the liblttng-ust.so > - Application dlopen() a probe provider library that is linked against > liblttng-ust.so > - Application dlclose() the probe provider > > In this scenario, the probe provider has a dependency on liblttng-ust.so, so > when it's loaded by the application, liblttng-ust.so is loaded too. The probe > provider library now has the only reference to the liblttng-ust.so library. > When the application calls dlclose() on it, all its references are dropped, > thus triggering the unloading of both the probe provider library and > liblttng-ust.so. > > This scenario is problematic because the probe provider needs the > liblttng-ust.so library to complete its own unloading. As we can't control the > order in which libraries are unloaded, we must ensure that liblttng-ust.so is > not unloaded before the probe provider. To prevent that unloading, we pin it in > memory by grabbing an extra reference on the library, with a RTLD_NODELETE > flag. This will prevent the dynamic loader from ever removing the > liblttng-ust.so library from the process' address space. I don't think this was the reason *why* we could not dlclose() liblttng-ust. But I do not recall how that fails exactly. Thoughts ? Thanks, Mathieu > > > Thank you, > Francis > >> Thanks, >> >> Mathieu >> >> >>> >>> Signed-off-by: Francis Deslauriers <francis.deslauriers at efficios.com> >>> --- >>> configure.ac | 1 + >>> liblttng-ust/Makefile.am | 2 ++ >>> liblttng-ust/lttng-ust-comm.c | 22 ++++++++++++++++++++++ >>> 3 files changed, 25 insertions(+) >>> >>> diff --git a/configure.ac b/configure.ac >>> index b0b4157..4fc6f9c 100644 >>> --- a/configure.ac >>> +++ b/configure.ac >>> @@ -25,6 +25,7 @@ m4_define([UST_LIB_V_MINOR], [0]) >>> m4_define([UST_LIB_V_PATCH], [0]) >>> >>> AC_SUBST([LTTNG_UST_LIBRARY_VERSION], >>> [UST_LIB_V_MAJOR:UST_LIB_V_MINOR:UST_LIB_V_PATCH]) >>> +AC_SUBST([LTTNG_UST_LIBRARY_VERSION_MAJOR], [UST_LIB_V_MAJOR]) >>> # note: remember to update tracepoint.h dlopen() to match this version >>> # number. TODO: eventually automate by exporting the major number. >>> >>> diff --git a/liblttng-ust/Makefile.am b/liblttng-ust/Makefile.am >>> index 982be69..a7edfd5 100644 >>> --- a/liblttng-ust/Makefile.am >>> +++ b/liblttng-ust/Makefile.am >>> @@ -60,6 +60,8 @@ liblttng_ust_runtime_la_SOURCES = \ >>> string-utils.c \ >>> string-utils.h >>> >>> +liblttng_ust_runtime_la_CFLAGS = >>> -DLTTNG_UST_LIBRARY_VERSION_MAJOR=\"$(LTTNG_UST_LIBRARY_VERSION_MAJOR)\" >>> + >>> if HAVE_PERF_EVENT >>> liblttng_ust_runtime_la_SOURCES += \ >>> lttng-context-perf-counters.c \ >>> diff --git a/liblttng-ust/lttng-ust-comm.c b/liblttng-ust/lttng-ust-comm.c >>> index 511b9cf..ed912b8 100644 >>> --- a/liblttng-ust/lttng-ust-comm.c >>> +++ b/liblttng-ust/lttng-ust-comm.c >>> @@ -27,6 +27,7 @@ >>> #include <sys/stat.h> >>> #include <sys/types.h> >>> #include <sys/wait.h> >>> +#include <dlfcn.h> >>> #include <fcntl.h> >>> #include <unistd.h> >>> #include <errno.h> >>> @@ -59,6 +60,9 @@ >>> #include "../libringbuffer/getcpu.h" >>> #include "getenv.h" >>> >>> +/* Concatenate lttng ust shared library name with its major version number. */ >>> +#define LTTNG_UST_LIB_SO_NAME "liblttng-ust.so." >>> LTTNG_UST_LIBRARY_VERSION_MAJOR >>> + >>> /* >>> * Has lttng ust comm constructor been called ? >>> */ >>> @@ -1648,6 +1652,7 @@ void __attribute__((constructor)) lttng_ust_init(void) >>> pthread_attr_t thread_attr; >>> int timeout_mode; >>> int ret; >>> + void *handle; >>> >>> if (uatomic_xchg(&initialized, 1) == 1) >>> return; >>> @@ -1662,6 +1667,23 @@ void __attribute__((constructor)) lttng_ust_init(void) >>> lttng_ust_loaded = 1; >>> >>> /* >>> + * Manually load liblttng-ust.so to increment the dynamic loader's >>> internal >>> + * refcount for this library so it never becomes zero, thus never gets >>> + * unloaded from the address space of the process. Since we are already >>> + * running in the constructor of the LTTNG_UST_LIB_SO_NAME library, >>> calling >>> + * dlopen will simply increment the refcount and no additionnal work is >>> + * needed by the dynamic loader as the shared library is already loaded in >>> + * the address space. As a safe guard, we use the RTLD_NODELETE flag to >>> + * prevent unloading of the UST library if its refcount becomes zero >>> + * (which should never happen). Do the return value check but discard the >>> + * handle at the end of the function as it's not needed. >>> + */ >>> + handle = dlopen(LTTNG_UST_LIB_SO_NAME, RTLD_LAZY | RTLD_NODELETE); >>> + if (!handle) { >>> + ERR("dlopen of liblttng-ust shared library (%s).", >>> LTTNG_UST_LIB_SO_NAME); >>> + } >>> + >>> + /* >>> * We want precise control over the order in which we construct >>> * our sub-libraries vs starting to receive commands from >>> * sessiond (otherwise leading to errors when trying to create >>> -- >>> 2.7.4 >> >> -- >> Mathieu Desnoyers >> EfficiOS Inc. >> http://www.efficios.com > > > > -- > Francis Deslauriers > Software developer > EfficiOS inc.
diff --git a/configure.ac b/configure.ac index b0b4157..4fc6f9c 100644 --- a/configure.ac +++ b/configure.ac @@ -25,6 +25,7 @@ m4_define([UST_LIB_V_MINOR], [0]) m4_define([UST_LIB_V_PATCH], [0]) AC_SUBST([LTTNG_UST_LIBRARY_VERSION], [UST_LIB_V_MAJOR:UST_LIB_V_MINOR:UST_LIB_V_PATCH]) +AC_SUBST([LTTNG_UST_LIBRARY_VERSION_MAJOR], [UST_LIB_V_MAJOR]) # note: remember to update tracepoint.h dlopen() to match this version # number. TODO: eventually automate by exporting the major number. diff --git a/liblttng-ust/Makefile.am b/liblttng-ust/Makefile.am index 982be69..a7edfd5 100644 --- a/liblttng-ust/Makefile.am +++ b/liblttng-ust/Makefile.am @@ -60,6 +60,8 @@ liblttng_ust_runtime_la_SOURCES = \ string-utils.c \ string-utils.h +liblttng_ust_runtime_la_CFLAGS = -DLTTNG_UST_LIBRARY_VERSION_MAJOR=\"$(LTTNG_UST_LIBRARY_VERSION_MAJOR)\" + if HAVE_PERF_EVENT liblttng_ust_runtime_la_SOURCES += \ lttng-context-perf-counters.c \ diff --git a/liblttng-ust/lttng-ust-comm.c b/liblttng-ust/lttng-ust-comm.c index 511b9cf..ed912b8 100644 --- a/liblttng-ust/lttng-ust-comm.c +++ b/liblttng-ust/lttng-ust-comm.c @@ -27,6 +27,7 @@ #include <sys/stat.h> #include <sys/types.h> #include <sys/wait.h> +#include <dlfcn.h> #include <fcntl.h> #include <unistd.h> #include <errno.h> @@ -59,6 +60,9 @@ #include "../libringbuffer/getcpu.h" #include "getenv.h" +/* Concatenate lttng ust shared library name with its major version number. */ +#define LTTNG_UST_LIB_SO_NAME "liblttng-ust.so." LTTNG_UST_LIBRARY_VERSION_MAJOR + /* * Has lttng ust comm constructor been called ? */ @@ -1648,6 +1652,7 @@ void __attribute__((constructor)) lttng_ust_init(void) pthread_attr_t thread_attr; int timeout_mode; int ret; + void *handle; if (uatomic_xchg(&initialized, 1) == 1) return; @@ -1662,6 +1667,23 @@ void __attribute__((constructor)) lttng_ust_init(void) lttng_ust_loaded = 1; /* + * Manually load liblttng-ust.so to increment the dynamic loader's internal + * refcount for this library so it never becomes zero, thus never gets + * unloaded from the address space of the process. Since we are already + * running in the constructor of the LTTNG_UST_LIB_SO_NAME library, calling + * dlopen will simply increment the refcount and no additionnal work is + * needed by the dynamic loader as the shared library is already loaded in + * the address space. As a safe guard, we use the RTLD_NODELETE flag to + * prevent unloading of the UST library if its refcount becomes zero + * (which should never happen). Do the return value check but discard the + * handle at the end of the function as it's not needed. + */ + handle = dlopen(LTTNG_UST_LIB_SO_NAME, RTLD_LAZY | RTLD_NODELETE); + if (!handle) { + ERR("dlopen of liblttng-ust shared library (%s).", LTTNG_UST_LIB_SO_NAME); + } + + /* * We want precise control over the order in which we construct * our sub-libraries vs starting to receive commands from * sessiond (otherwise leading to errors when trying to create
dlopen() increments the refcount of the library thus preventing the refcount to reach zero in the case of dlclose; Signed-off-by: Francis Deslauriers <francis.deslauriers at efficios.com> --- configure.ac | 1 + liblttng-ust/Makefile.am | 2 ++ liblttng-ust/lttng-ust-comm.c | 22 ++++++++++++++++++++++ 3 files changed, 25 insertions(+)