Project

General

Profile

Bug #1199

close() implementation of ust-fd is not async-signal-safe causing child processes to hang forever after fork()

Added by Tai Dinh 5 months ago. Updated 4 months ago.

Status:
Resolved
Priority:
High
Assignee:
-
Target version:
Start date:
10/03/2019
Due date:
% Done:

100%

Estimated time:

Description

According to below link, all implementation of close() should be async-signal-safe. But the implementation of ust-fd is absolutely not a safe one since it use a lots of none safe functions, especially the pthread_mutex_lock.

http://man7.org/linux/man-pages/man7/signal-safety.7.html

This causes a serious problem for any application that do a fork() followed by close() of file descriptor in the child before calling any of exec* family function, which is a common pattern.
With ust-fd preload, the lttng_ust_safe_close_fd will be called and if the fork() happens right after the ust_safe_guard_fd_mutex is locked in the parent (due to some trace events), the child will end up in a deadlock since it will be no longer can lock/unlock the mutex again.

(gdb) bt
#0 lll_lock_wait () at ../sysdeps/unix/sysv/linux/x86_64/lowlevellock.S:135
#1 0x00007f2f3ae4c023 in _GI
_pthread_mutex_lock (mutex=mutex@entry=0x7f2f3a843940 <ust_safe_guard_fd_mutex>) at ../nptl/pthread_mutex_lock.c:78
#2 0x00007f2f3a5e6eaa in lttng_ust_lock_fd_tracker () at lttng-ust-fd-tracker.c:133
#3 0x00007f2f3a5e71a5 in lttng_ust_safe_close_fd (fd=1, close_cb=0x7f2f3ae53410 <
_close>) at lttng-ust-fd-tracker.c:300
#4 0x000055b014499c72 in do_thread ()
#5 0x00007f2f3ae496db in start_thread (arg=0x7f2f38891700) at pthread_create.c:463
#6 0x00007f2f3a96e88f in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95

It is OK that this can be an exception and we cannot make a POSIX conforming implementation but at least the dead lock should be fixed since the problem can always be reproduced and is blocking those kind of application.

/Tai


Files

1199.c (2.31 KB) 1199.c Tai Dinh, 10/03/2019 03:43 PM
ust_pipe_and_fork.h (631 Bytes) ust_pipe_and_fork.h Tai Dinh, 10/03/2019 04:05 PM
tp.c (64 Bytes) tp.c Tai Dinh, 10/03/2019 04:05 PM
patch-1199-1.diff (609 Bytes) patch-1199-1.diff Mathieu Desnoyers, 10/03/2019 04:41 PM
#1

Updated by Mathieu Desnoyers 5 months ago

Can you provide a small reproducer program along with instructions on how to launch it ?

Also, it's unclear to me why the fork() is issued with ust_safe_guard_fd_mutex locked in your scenario. You hint that this is due to some trace events. What is the causality link between tracing an event and locking this mutex ?

Thanks,

Mathieu

#2

Updated by Tai Dinh 5 months ago

Hi Mathieu,

You can find in attachment a sample program to reproduce this issue.
Just open two terminals.
At the first one:

LD_PRELOAD=./ust_pipe_and_fork.so:/usr/local/lib/liblttng-ust-fd.so ./1199

At the second one:

while /bin/true; do lttng start; lttng stop; done

Really soon, you can see that the program at the first terminal will hang waiting for its children which never exits.

By events I meant it can be any, just want to send something to the lttng listening socket so that the ustcomm_recv_unix_sock receives some events and break.
I use start/stop for the reproduction.
Later on, the ust_listener_thread will start to process the message and trigger some calls to lttng_ust_lock_fd_tracker.
At the same time, main thread doing fork before the lttng_ust_unlock_fd_tracker can be called at ust_listener_thread.

I guess the only way to fix this is to implement the mutex reinit inside ust_after_fork_child.

/Tai

#3

Updated by Mathieu Desnoyers 5 months ago

Is ust_pipe_and_fork.so in your reproducer instruction different from upstream's liblttng-ust-fork.so ?

#4

Updated by Mathieu Desnoyers 5 months ago

Actually, I think it would be best if you can attach the ust_pipe_and_fork.{ch} used with the reproducer.

Thanks,

Mathieu

#5

Updated by Tai Dinh 5 months ago

Mathieu Desnoyers wrote:

Is ust_pipe_and_fork.so in your reproducer instruction different from upstream's liblttng-ust-fork.so ?

No, the ust_pipe_and_fork.so is just a dynamic part of my instrumented application. It has nothing to do with the upstream liblttng-ust-fork.so

#6

Updated by Tai Dinh 5 months ago

Mathieu Desnoyers wrote:

Actually, I think it would be best if you can attach the ust_pipe_and_fork.{ch} used with the reproducer.

Thanks,

Mathieu

Done.

#7

Updated by Mathieu Desnoyers 5 months ago

Can you test the attached patch ? So far it seems to run fine here.

#8

Updated by Tai Dinh 5 months ago

Mathieu Desnoyers wrote:

Can you test the attached patch ? So far it seems to run fine here.

Sure, I'll spend sometimes to test it today.
But I have some comments regarding to it in the meanwhile:
  • I'm not quite sure that it would be OK or not but since our mutex is a normal type one, unlocking it after forking in the child would result in an undefined behavior since we are unlocking a mutex that is not owned by ourselves.
    https://linux.die.net/man/3/pthread_mutex_lock
    If the mutex type is PTHREAD_MUTEX_NORMAL, deadlock detection shall not be provided. Attempting to relock the mutex causes deadlock. If a thread attempts to unlock a mutex that it has not locked or a mutex which is unlocked, undefined behavior results.
    

    Maybe we don't see any issues for now but it is not guaranteed that we won't have any later. And I don't know if we test this scenario comprehensively enough, e.g: post fork() tracing.
    Anyway, I can see that we use the same method for other mutex as well for a long time with no issue so I guess it should be OK at least from experimentation point of view? Maybe one of the reason is because we always have one single thread right after fork.
  • The documentation is also need to be updated so that it is no longer about application that doing fork() without following exec*(), but also it is now applicable for all fork() application (or at least application that doing close() after fork and preload our ust-fd).

/Tai

#9

Updated by Mathieu Desnoyers 5 months ago

About the question related to holding the mutex across fork:

You cite:

"Attempting to relock the mutex causes deadlock."

-> We are not relocking the mutex here. The mutex is locked in the parent's thread doing the fork, before fork, and unlocked in both the parent and the child, which each have their own address space at that point (after fork).

"If a thread attempts to unlock a mutex that it has not locked or a mutex which is unlocked, undefined behavior results."

-> We are not unlocking a mutex that has not been locked here, because the parent has locked the mutex before doing the fork.

This is the same strategy used by lttng-ust for the ust_lock(), which handles fork.

#10

Updated by Mathieu Desnoyers 5 months ago

About your documentation question:

"The documentation is also need to be updated so that it is no longer about application that doing fork() without following exec*(), but also it is now applicable for all fork() application (or at least application that doing close() after fork and preload our ust-fd)."

What is meant here is that if you have an application which is not immediately doing exec() after fork(), the liblttng-ust-fork.so helper is needed. My initial assumption was that closing file descriptors before doing the exec() falls in that category.

But we'd need to clearly state what we mean by "without a following exec(3)", IOW what side-effects are allowed, and which are not.

I think you point here is that close(3) is async-signal-safe, and should be allowed to be done after fork/before exec without issues. I'll have to think about it some more.

#11

Updated by Tai Dinh 5 months ago

Mathieu Desnoyers wrote:

About the question related to holding the mutex across fork:

You cite:

"Attempting to relock the mutex causes deadlock."

-> We are not relocking the mutex here. The mutex is locked in the parent's thread doing the fork, before fork, and unlocked in both the parent and the child, which each have their own address space at that point (after fork).

"If a thread attempts to unlock a mutex that it has not locked or a mutex which is unlocked, undefined behavior results."

-> We are not unlocking a mutex that has not been locked here, because the parent has locked the mutex before doing the fork.

This is the same strategy used by lttng-ust for the ust_lock(), which handles fork.

There are there cases mentioned in the manpage:
1. Attempting to relock the mutex
2. attempts to unlock a mutex that it has not locked
3. attempts to unlock a mutex that is unlocked

You're talking about 1 and 3, but what I meant is 2, ' unlocking it after forking in the child would result in an undefined behavior since we are unlocking a mutex that is not owned by ourselves '.
Sorry for the confusion, I should be clearer.

If I interpret the mutex manpage correctly, then the second cases is talking about the scenario where a thread unlocks a mutex that was not locked by itself before (e.g: thread 1 do lock and thread 2 do unlock). And this is applicable for the new single thread right after fork at the child process (it is OK for after fork at parent since it is still on the same thread). Please correct me if I'm wrong.

I agree that this the same strategy which has been used for a while, but I'm not so sure if it is ok or not.

/Tai

#12

Updated by Tai Dinh 5 months ago

Mathieu Desnoyers wrote:

About your documentation question:

"The documentation is also need to be updated so that it is no longer about application that doing fork() without following exec*(), but also it is now applicable for all fork() application (or at least application that doing close() after fork and preload our ust-fd)."

What is meant here is that if you have an application which is not immediately doing exec() after fork(), the liblttng-ust-fork.so helper is needed. My initial assumption was that closing file descriptors before doing the exec() falls in that category.

But we'd need to clearly state what we mean by "without a following exec(3)", IOW what side-effects are allowed, and which are not.

I think you point here is that close(3) is async-signal-safe, and should be allowed to be done after fork/before exec without issues. I'll have to think about it some more.

Yes, please consider it if possible.
At least my first interpretation for that sentence was mostly applicable for application that do fork() but without any following exec().
This even makes more sense for application that using daemon().
Anyway, should we mention daemon() there as well?

/Tai

#13

Updated by Mathieu Desnoyers 4 months ago

Indeed, unlocking a "fast" mutex which has been locked by a different thread is an undefined behavior:

https://pubs.opengroup.org/onlinepubs/007904975/functions/pthread_mutex_lock.html

"If the mutex type is PTHREAD_MUTEX_NORMAL, deadlock detection shall not be provided. Attempting to relock the mutex causes deadlock. If a thread attempts to unlock a mutex that it has not locked or a mutex which is unlocked, undefined behavior results."

And pthread_mutex_lock(3):

``Fast'' mutexes perform no such checks, thus allowing a locked mutex to be unlocked by a thread other than its owner. This is non-portable behavior and must not be relied upon.

However, given that we already rely on this behavior for ust_lock(), I am tempted to use the same strategy here. If this happens to be an issue in the future, we will want to consider replacing the pthread_mutex_unlock() by a pthread_mutex_init() in the after-fork-child handler. That would be a separate discussion though.

#14

Updated by Mathieu Desnoyers 4 months ago

With respect to the documentation of fork, daemon, clone(!(flags & CLONE_VM)) vs exec(), I will open a separate ticket to track this separately.

#15

Updated by Tai Dinh 4 months ago

Mathieu Desnoyers wrote:

Indeed, unlocking a "fast" mutex which has been locked by a different thread is an undefined behavior:

https://pubs.opengroup.org/onlinepubs/007904975/functions/pthread_mutex_lock.html

"If the mutex type is PTHREAD_MUTEX_NORMAL, deadlock detection shall not be provided. Attempting to relock the mutex causes deadlock. If a thread attempts to unlock a mutex that it has not locked or a mutex which is unlocked, undefined behavior results."

And pthread_mutex_lock(3):

``Fast'' mutexes perform no such checks, thus allowing a locked mutex to be unlocked by a thread other than its owner. This is non-portable behavior and must not be relied upon.

However, given that we already rely on this behavior for ust_lock(), I am tempted to use the same strategy here. If this happens to be an issue in the future, we will want to consider replacing the pthread_mutex_unlock() by a pthread_mutex_init() in the after-fork-child handler. That would be a separate discussion though.

This is exactly what I wanted to address. Also I agree with you that we can go with that approach. If it happens to be an issue, we can reconsider it later.

/Tai

#16

Updated by Tai Dinh 4 months ago

Mathieu Desnoyers wrote:

With respect to the documentation of fork, daemon, clone(!(flags & CLONE_VM)) vs exec(), I will open a separate ticket to track this separately.

Sure. No more comments from me then.

/Tai

#17

Updated by Mathieu Desnoyers 4 months ago

  • Status changed from New to Resolved
  • % Done changed from 0 to 100
#18

Updated by Mathieu Desnoyers 4 months ago

Relevant commits:

commit c1be081a2f016fb6dcaef1d471389ede3aa00103
Author: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Date:   Fri Oct 4 10:07:03 2019 -0400

    Fix: Lock FD tracker across fork

commit 96a6162e1925d4bd1dfc6f4167d75f396c0dbe4c
Author: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Date:   Fri Oct 4 15:04:13 2019 -0400

    Fix: fd tracker: provide async-signal-safety for close wrapper

commit 48a143c09cc97bf7a2ace811277e7d60b294b5f6
Author: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Date:   Mon Oct 7 15:41:10 2019 -0400

    Fix: fd tracker: do not allow signal handlers to close lttng-ust FDs

Also available in: Atom PDF