Re: [PATCH 05/16] Fix a couple of signal issues

[Bart Van Assche <bvanassche@acm.org>]

On 05/03/13 22:24, Benjamin Marzinski wrote:
> On Fri, May 03, 2013 at 08:36:19AM +0200, Bart Van Assche wrote:
>> On 05/02/13 23:46, Benjamin Marzinski wrote:
>>> The patch cleans up some signal issues.
>>> First, when the vecs locking around reconfigure got shuffled
>>> around earlier, it was removed from sighup. This patch restores
>>> that.
>>>
>>> Second, a new sigusr1 handler was created. However the existing
>>> one was never removed.  Since signal handlers are per-process, and
>>> not per-thread, the original handler will get overwritten by the
>>> new one, so this patch deletes the original handler.
>>>
>>> Third, sighup locks the vecs lock and sigusr1 locks logq_lock.
>>> However, these signals weren't being blocked before threads locked
>>> those locks.  This patch blocks those signals while those locks
>>> are being taken to avoid locking deadlocks.
>>
>> Are you aware that POSIX does not allow any locking function to be invoked
>> from inside a signal handler ? See e.g.
>> http://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_04
>> and the text that starts with "The following table defines a set of
>> functions that shall be async-signal-safe. Therefore, applications can
>> invoke them, without restriction, from signal-catching functions" for a
>> list of C library functions that may be invoked from inside a signal
>> handler.
>
> Um. no. I clearly wasn't aware of that. Nuts. So is the risk that a
> signal will come into some thread that's not blocking it, and try to
> acquire the pthread mutex at the same time as another thread that is
> blocking the signal is trying to aquire the same mutex and that this
> will corrupt the lock?
>
> The Ubuntu man page for pthread_mutex_lock says
>
> "The mutex functions are not async-signal safe. What this means is  that
>   they should not be called from a signal handler. In particular, calling
>   pthread_mutex_lock or pthread_mutex_unlock from a  signal handler  may
>   deadlock the calling thread."
>
> If that's the only risk, then blocking the signal before we lock the
> mutexes should avoid the issue.  Since the point of mutexes is do deal
> with multiple threads that are trying to acquire them at the same time,
> it seems like they should be able to handle this when one of the threads
> happens to be in a signal handler. Also, the locks in sighup were there
> for a long time before they were removed, and while I have definitely
> seen deadlocks when we don't remember to mask the signal before locking
> on that thread, I've never encountered a bug that seems to be related to
> a locking corruption like I speculated in the beginning of my reply.
>
> Now, since my patch was to fix some potential deadlocks from using
> pthread_mutex_locks in signal handlers, I do realize that the way
> we are doing things isn't the safest way around.  Also, it seems pretty
> obvious from looking at the URL you posted that we are in undefined
> behavior territory, where if we are safe, it's solely based on the
> some non-defined parts of the implementation. So I agree, we aren't
> following the specs, and I'll work on redesigning things to avoid this.
>
> But the locking I added in this patch fixes corruption that definitely
> happens, and is very much able to crash multipathd.  I still think this
> patch should go in, since those locks were previously there for a long
> time, and I can easily crash multipathd by repeadly sending it sighups
> without this patch.
>
> Does that sound reasonable?

Sorry but in my opinion the patch at the start of this thread makes the 
approach for signal handling in multipathd more complex and harder to 
maintain than strictly necessary. Will e.g. the next person who modifies 
this code be aware of all these subtleties ? The approach I follow in 
multithreaded code that I maintain myself for handling signals is to let 
each signal handler write some data into a pipe, wait on that pipe in 
the main thread and let the main thread take the appropriate action. 
This approach is easy to maintain, does not require to block and unblock 
signals at runtime, is portable between Unix systems and POSIX 
compliant. See e.g. 
http://github.com/bvanassche/srptools/commit/b6589892206ca628dbbb7fd8a1d613bf4f442ee6 
for an example. Note: a possible alternative is to use signalfd() 
instead of creating a pipe.

Bart.