Re: [lttng-dev] [PATCH lttng-ust 6/8] Manually dlopen() liblttng-ust.so to prevent unloading

2018-02-08 Thread Mathieu Desnoyers 
- On Feb 8, 2018, at 3:24 PM, Francis Deslauriers 
francis.deslauri...@efficios.com wrote:

> 2018-02-07 15:59 GMT-05:00 Mathieu Desnoyers :
>> - On Feb 2, 2018, at 2:48 PM, Francis Deslauriers
>> francis.deslauri...@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 
>>> ---
>>> 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 
>>> #include 
>>> #include 
>>> +#include 
>>> #include 
>>> #include 
>>> #include 
>>> @@ -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(, 1) == 1)
>>>   return;
>>> @@ -1662,6 +1667,23 @@ void __attribute__((constructor)) 
>>> lttng_ust_init(void)
>>>   lttng_ust_loaded = 1;

Re: [lttng-dev] [PATCH lttng-ust 6/8] Manually dlopen() liblttng-ust.so to prevent unloading

2018-02-08 Thread Francis Deslauriers 
2018-02-07 15:59 GMT-05:00 Mathieu Desnoyers :
> - On Feb 2, 2018, at 2:48 PM, Francis Deslauriers 
> francis.deslauri...@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 
>> ---
>> 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 
>> #include 
>> #include 
>> +#include 
>> #include 
>> #include 
>> #include 
>> @@ -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(, 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

Re: [lttng-dev] [PATCH lttng-ust 6/8] Manually dlopen() liblttng-ust.so to prevent unloading

2018-02-07 Thread Mathieu Desnoyers 
- On Feb 2, 2018, at 2:48 PM, Francis Deslauriers 
francis.deslauri...@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 
> ---
> 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 
> #include 
> #include 
> +#include 
> #include 
> #include 
> #include 
> @@ -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(, 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
___
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

[lttng-dev] [PATCH lttng-ust 6/8] Manually dlopen() liblttng-ust.so to prevent unloading

2018-02-02 Thread Francis Deslauriers 
dlopen() increments the refcount of the library thus preventing the
refcount to reach zero in the case of dlclose;

Signed-off-by: Francis Deslauriers 
---
 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 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -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(, 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

___
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev