[PATCH lttng-ust] Bugfix for #745 deadlock with baddr statedump+fork

[PATCH lttng-ust] Bugfix for #745 deadlock with baddr statedump+fork

[Paul Woegerer]

Signed-off-by: Paul Woegerer <paul_woegerer@mentor.com>
---
 liblttng-ust/lttng-ust-comm.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/liblttng-ust/lttng-ust-comm.c b/liblttng-ust/lttng-ust-comm.c
index 5df33f5..8fa2e03 100644
--- a/liblttng-ust/lttng-ust-comm.c
+++ b/liblttng-ust/lttng-ust-comm.c
@@ -173,6 +173,7 @@ void ust_unlock(void)
  *   daemon problems).
  */
 static sem_t constructor_wait;
+static sem_t fork_child_wait;
 /*
  * Doing this for both the global and local sessiond.
  */
@@ -502,11 +503,18 @@ int handle_register_done(struct sock_info *sock_info)
 static
 void handle_pending_statedump(struct sock_info *sock_info)
 {
+	int ret;
 	int ctor_passed = sock_info->constructor_sem_posted;
 
 	if (ctor_passed && sock_info->statedump_pending) {
+		do {
+			ret = sem_wait(&fork_child_wait);
+		} while (ret < 0 && errno == EINTR);
+		assert(!ret);
 		sock_info->statedump_pending = 0;
 		lttng_handle_pending_statedump(sock_info);
+		ret = sem_post(&fork_child_wait);
+		assert(!ret);
 	}
 }
 
@@ -1436,6 +1444,9 @@ void __attribute__((constructor)) lttng_ust_init(void)
 	ret = sem_init(&constructor_wait, 0, 0);
 	assert(!ret);
 
+	ret = sem_init(&fork_child_wait, 0, 1);
+	assert(!ret);
+
 	ret = setup_local_apps();
 	if (ret) {
 		DBG("local apps setup returned %d", ret);
@@ -1627,6 +1638,12 @@ void ust_before_fork(sigset_t *save_sigset)
 	if (ret == -1) {
 		PERROR("sigprocmask");
 	}
+
+	do {
+		ret = sem_wait(&fork_child_wait);
+	} while (ret < 0 && errno == EINTR);
+	assert(!ret);
+
 	ust_lock_nocheck();
 	rcu_bp_before_fork();
 }
@@ -1637,6 +1654,10 @@ static void ust_after_fork_common(sigset_t *restore_sigset)
 
 	DBG("process %d", getpid());
 	ust_unlock();
+
+	ret = sem_post(&fork_child_wait);
+	assert(!ret);
+
 	/* Restore signals */
 	ret = sigprocmask(SIG_SETMASK, restore_sigset, NULL);
 	if (ret == -1) {
-- 
2.0.1

Ping: Re: [PATCH lttng-ust] Bugfix for http://bugs.lttng.org/issues/745

[Woegerer, Paul]

Hi Mathieu,

Since we have no other options currently (see LD_AUDIT discussion) we
really should get this merged into master. As said, it's thoroughly
tested and should not cause any ill side-effects.

Many Thanks,
Paul

On 07/04/2014 02:21 PM, Paul Woegerer wrote:
> Since (at least) in the short term LD_AUDIT will not be able to make sem_wait()
> in the static constructor go away I provide the following patch to fix
> http://bugs.lttng.org/issues/745.
> 
> It works by making the process of forking mutual exclusive with base address
> statedumping that happens from the ust_listener_thread.
> 
> I have tested this fix with several variations of (constrained) fork bombs
> (including tests/regression/ust/daemon from lttng-tools) on x86_64, ARM(imx6q)
> and powerpc(P4080DS). It works reliable. I could not find any issues with this
> approach.
> 
> Paul Woegerer (1):
>   Bugfix for #745 deadlock with baddr statedump+fork
> 
>  liblttng-ust/lttng-ust-comm.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 


-- 
Paul Woegerer, SW Development Engineer
Sourcery Analyzer <http://go.mentor.com/sourceryanalyzer>
Mentor Graphics, Embedded Software Division

Re: Ping: Re: [PATCH lttng-ust] Bugfix for http://bugs.lttng.org/issues/745

[Mathieu Desnoyers]

----- Original Message -----
> From: "Paul Woegerer" <Paul_Woegerer@mentor.com>
> To: lttng-dev@lists.lttng.org, "mathieu desnoyers" <mathieu.desnoyers@efficios.com>
> Sent: Thursday, July 10, 2014 8:49:37 AM
> Subject: Ping: Re: [PATCH lttng-ust] Bugfix for http://bugs.lttng.org/issues/745
> 
> Hi Mathieu,
> 
> Since we have no other options currently (see LD_AUDIT discussion) we
> really should get this merged into master. As said, it's thoroughly
> tested and should not cause any ill side-effects.

Hi Paul,

Sorry about the delayed answer, I was busy implementing a tool that extracts
UST buffers in the event of a system crash.

Looking at your patch below, I wonder why you use a semaphore (0, 1) rather
than a mutex ? I guess you could achieve the same result with a pthread
mutex ? If so, it would be better, because it makes it easier to understand
mutex nesting.

Thanks,

Mathieu

> 
> Many Thanks,
> Paul
> 
> On 07/04/2014 02:21 PM, Paul Woegerer wrote:
> > Since (at least) in the short term LD_AUDIT will not be able to make
> > sem_wait()
> > in the static constructor go away I provide the following patch to fix
> > http://bugs.lttng.org/issues/745.
> > 
> > It works by making the process of forking mutual exclusive with base
> > address
> > statedumping that happens from the ust_listener_thread.
> > 
> > I have tested this fix with several variations of (constrained) fork bombs
> > (including tests/regression/ust/daemon from lttng-tools) on x86_64,
> > ARM(imx6q)
> > and powerpc(P4080DS). It works reliable. I could not find any issues with
> > this
> > approach.
> > 
> > Paul Woegerer (1):
> >   Bugfix for #745 deadlock with baddr statedump+fork
> > 
> >  liblttng-ust/lttng-ust-comm.c | 21 +++++++++++++++++++++
> >  1 file changed, 21 insertions(+)
> > 
> 
> 
> --
> Paul Woegerer, SW Development Engineer
> Sourcery Analyzer <http://go.mentor.com/sourceryanalyzer>
> Mentor Graphics, Embedded Software Division
> 

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

Re: Ping: Re: [PATCH lttng-ust] Bugfix for http://bugs.lttng.org/issues/745

[Woegerer, Paul]

On 07/10/2014 03:24 PM, Mathieu Desnoyers wrote:
> ----- Original Message -----
>> From: "Paul Woegerer" <Paul_Woegerer@mentor.com>
>> To: lttng-dev@lists.lttng.org, "mathieu desnoyers" <mathieu.desnoyers@efficios.com>
>> Sent: Thursday, July 10, 2014 8:49:37 AM
>> Subject: Ping: Re: [PATCH lttng-ust] Bugfix for http://bugs.lttng.org/issues/745
>>
>> Hi Mathieu,
>>
>> Since we have no other options currently (see LD_AUDIT discussion) we
>> really should get this merged into master. As said, it's thoroughly
>> tested and should not cause any ill side-effects.
> 
> Hi Paul,
> 
> Sorry about the delayed answer, I was busy implementing a tool that extracts
> UST buffers in the event of a system crash.
> 
> Looking at your patch below, I wonder why you use a semaphore (0, 1) rather
> than a mutex ? I guess you could achieve the same result with a pthread
> mutex ? If so, it would be better, because it makes it easier to understand
> mutex nesting.

Thanks for your feedback.

I will build another variant that uses a mutex instead and run it
through the same test procedure. If that works equally well I sent an
updated patch.

--
Paul

> 
> Thanks,
> 
> Mathieu
> 
>>
>> Many Thanks,
>> Paul
>>
>> On 07/04/2014 02:21 PM, Paul Woegerer wrote:
>>> Since (at least) in the short term LD_AUDIT will not be able to make
>>> sem_wait()
>>> in the static constructor go away I provide the following patch to fix
>>> http://bugs.lttng.org/issues/745.
>>>
>>> It works by making the process of forking mutual exclusive with base
>>> address
>>> statedumping that happens from the ust_listener_thread.
>>>
>>> I have tested this fix with several variations of (constrained) fork bombs
>>> (including tests/regression/ust/daemon from lttng-tools) on x86_64,
>>> ARM(imx6q)
>>> and powerpc(P4080DS). It works reliable. I could not find any issues with
>>> this
>>> approach.
>>>
>>> Paul Woegerer (1):
>>>   Bugfix for #745 deadlock with baddr statedump+fork
>>>
>>>  liblttng-ust/lttng-ust-comm.c | 21 +++++++++++++++++++++
>>>  1 file changed, 21 insertions(+)
>>>
>>
>>
>> --
>> Paul Woegerer, SW Development Engineer
>> Sourcery Analyzer <http://go.mentor.com/sourceryanalyzer>
>> Mentor Graphics, Embedded Software Division
>>
> 


-- 
Paul Woegerer, SW Development Engineer
Sourcery Analyzer <http://go.mentor.com/sourceryanalyzer>
Mentor Graphics, Embedded Software Division

[PATCH lttng-ust 0/2] Bugfix for http://bugs.lttng.org/issues/745 (now using mutex)

[Paul Woegerer]

As discussed previously I replaced the semaphore with a mutex and retested on
all three platforms (x86_x64, ARM and PPC). Turns out using a mutex is as good
as the original implementation with the semaphore.

Paul Woegerer (2):
  Bugfix for #745 deadlock with baddr statedump+fork
  Revert "Turn base address dump into experimental feature"

 doc/man/lttng-ust.3            | 10 +++-------
 liblttng-ust/lttng-ust-baddr.c |  2 +-
 liblttng-ust/lttng-ust-comm.c  | 16 ++++++++++++++++
 3 files changed, 20 insertions(+), 8 deletions(-)

-- 
2.0.1

[PATCH lttng-ust 1/2] Bugfix for #745 deadlock with baddr statedump+fork

[Paul Woegerer]

Signed-off-by: Paul Woegerer <paul_woegerer@mentor.com>
---
 liblttng-ust/lttng-ust-comm.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/liblttng-ust/lttng-ust-comm.c b/liblttng-ust/lttng-ust-comm.c
index 5df33f5..a73808e 100644
--- a/liblttng-ust/lttng-ust-comm.c
+++ b/liblttng-ust/lttng-ust-comm.c
@@ -86,6 +86,14 @@ static DEFINE_URCU_TLS(int, ust_mutex_nest);
  */
 static pthread_mutex_t ust_exit_mutex = PTHREAD_MUTEX_INITIALIZER;
 
+/*
+ * ust_fork_mutex protects base address statedump tracing against forks. It
+ * prevents the dynamic loader lock to be taken (by base address statedump
+ * tracing) while a fork is happening, thus preventing deadlock issues with
+ * the dynamic loader lock.
+ */
+static pthread_mutex_t ust_fork_mutex = PTHREAD_MUTEX_INITIALIZER;
+
 /* Should the ust comm thread quit ? */
 static int lttng_ust_comm_should_quit;
 
@@ -505,8 +513,10 @@ void handle_pending_statedump(struct sock_info *sock_info)
 	int ctor_passed = sock_info->constructor_sem_posted;
 
 	if (ctor_passed && sock_info->statedump_pending) {
+		pthread_mutex_lock(&ust_fork_mutex);
 		sock_info->statedump_pending = 0;
 		lttng_handle_pending_statedump(sock_info);
+		pthread_mutex_unlock(&ust_fork_mutex);
 	}
 }
 
@@ -1627,6 +1637,9 @@ void ust_before_fork(sigset_t *save_sigset)
 	if (ret == -1) {
 		PERROR("sigprocmask");
 	}
+
+	pthread_mutex_lock(&ust_fork_mutex);
+
 	ust_lock_nocheck();
 	rcu_bp_before_fork();
 }
@@ -1637,6 +1650,9 @@ static void ust_after_fork_common(sigset_t *restore_sigset)
 
 	DBG("process %d", getpid());
 	ust_unlock();
+
+	pthread_mutex_unlock(&ust_fork_mutex);
+
 	/* Restore signals */
 	ret = sigprocmask(SIG_SETMASK, restore_sigset, NULL);
 	if (ret == -1) {
-- 
2.0.1

[PATCH lttng-ust 2/2] Revert "Turn base address dump into experimental feature"

[Paul Woegerer]

This reverts commit b11abb674e50c67e3410ab2bd5d0a263e88b73ba.

Signed-off-by: Paul Woegerer <paul_woegerer@mentor.com>
---
 doc/man/lttng-ust.3            | 10 +++-------
 liblttng-ust/lttng-ust-baddr.c |  2 +-
 2 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/doc/man/lttng-ust.3 b/doc/man/lttng-ust.3
index c72bd32..117fe98 100644
--- a/doc/man/lttng-ust.3
+++ b/doc/man/lttng-ust.3
@@ -397,13 +397,9 @@ Pthread identifier. Can be used on architectures where pthread_t maps
 nicely to an unsigned long type.
 .PP
 
-.SH "BASE ADDRESS STATEDUMP (Experimental feature)"
+.SH "BASE ADDRESS STATEDUMP"
 
 .PP
-Warning: This is an experimental feature known to cause deadlocks when the
-traced application uses fork, clone or daemon. Only use it for debugging and
-testing.  Do NOT use it in production.
-
 If an application that uses liblttng-ust.so becomes part of a session,
 information about its currently loaded shared objects will be traced to the
 session at session-enable time. To record this information, the following event
@@ -440,8 +436,8 @@ specified in milliseconds. The value 0 means "don't wait". The value
 recommended for applications with time constraints on the process
 startup time.
 .PP
-.IP "LTTNG_UST_WITH_EXPERIMENTAL_BADDR_STATEDUMP"
-Experimentally allow liblttng-ust to perform a base-address statedump on session-enable.
+.IP "LTTNG_UST_WITHOUT_BADDR_STATEDUMP"
+Prevent liblttng-ust to perform a base-address statedump on session-enable.
 .PP
 
 .SH "SEE ALSO"
diff --git a/liblttng-ust/lttng-ust-baddr.c b/liblttng-ust/lttng-ust-baddr.c
index dec7e82..922899b 100644
--- a/liblttng-ust/lttng-ust-baddr.c
+++ b/liblttng-ust/lttng-ust-baddr.c
@@ -176,7 +176,7 @@ int lttng_ust_baddr_statedump(void *owner)
 {
 	struct extract_data data;
 
-	if (!getenv("LTTNG_UST_WITH_EXPERIMENTAL_BADDR_STATEDUMP"))
+	if (getenv("LTTNG_UST_WITHOUT_BADDR_STATEDUMP"))
 		return 0;
 
 	data.owner = owner;
-- 
2.0.1

Re: [PATCH lttng-ust 0/2] Bugfix for http://bugs.lttng.org/issues/745 (now using mutex)

[Mathieu Desnoyers]

Both patches merged into master and will be part of
the 2.5 release. No backport to stable-2.4 planned
though, as this is fixing what was an experimental
feature in 2.4.

Thanks,

Mathieu

----- Original Message -----
> From: "Paul Woegerer" <paul_woegerer@mentor.com>
> To: lttng-dev@lists.lttng.org, "mathieu desnoyers" <mathieu.desnoyers@efficios.com>
> Cc: "Paul Woegerer" <paul_woegerer@mentor.com>
> Sent: Monday, July 14, 2014 4:38:23 AM
> Subject: [PATCH lttng-ust 0/2] Bugfix for http://bugs.lttng.org/issues/745 (now using mutex)
> 
> As discussed previously I replaced the semaphore with a mutex and retested on
> all three platforms (x86_x64, ARM and PPC). Turns out using a mutex is as
> good
> as the original implementation with the semaphore.
> 
> Paul Woegerer (2):
>   Bugfix for #745 deadlock with baddr statedump+fork
>   Revert "Turn base address dump into experimental feature"
> 
>  doc/man/lttng-ust.3            | 10 +++-------
>  liblttng-ust/lttng-ust-baddr.c |  2 +-
>  liblttng-ust/lttng-ust-comm.c  | 16 ++++++++++++++++
>  3 files changed, 20 insertions(+), 8 deletions(-)
> 
> --
> 2.0.1
> 
> 

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com