LTTng » LTTng-UST

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

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

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

Thanks,

Mathieu

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

Mathieu Desnoyers wrote:

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

• 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

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.

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.

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

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

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.

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

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

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

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