Project

General

Profile

Bug #654

Proposed patch to mutex.c

Added by Noam Lampert over 6 years ago. Updated about 6 years ago.

Status:
Closed
Priority:
Normal
Category:
libstrongswan
Target version:
Start date:
16.07.2014
Due date:
Estimated time:
Affected version:
5.2.0
Resolution:
Fixed

Description

The code in src/libstrongswan/threading/mutex.c performs (without lock):

pthread_t self = pthread_self();
if (pthread_equal(this->thread, self))

when this->thread is concurrently mutated.

This can break even on single CPU due to compiler optimizations.
Compilers assume that programs are race-free.
On top of that pthread_t is not guaranteed to be a single word,
something that this code assumes.

The proposed patch fixes this by adding an additional mutex that protects the pthread_t.

mutex.patch (3.99 KB) mutex.patch Noam Lampert, 16.07.2014 11:18

Associated revisions

Revision 65cb93bf (diff)
Added by Tobias Brunner over 6 years ago

mutex: Use atomics to set current thread in recursive mutex

Because this->thread is also read by threads that don't hold the
mutex the previous implementation was problematic (especially since
pthread_t is an opaque type of unknown length).

Fixes #654.

Revision 59d7a7f9 (diff)
Added by Tobias Brunner about 6 years ago

mutex: Use atomics to set current thread in recursive mutex

Because this->thread is also read by threads that don't hold the
mutex the previous implementation was problematic (especially since
pthread_t is an opaque type of unknown length).

Fixes #654.

Revision dbd7f4be (diff)
Added by Tobias Brunner about 6 years ago

mutex: Use atomics to set current thread in recursive mutex

Because this->thread is also read by threads that don't hold the
mutex the previous implementation was problematic (especially since
pthread_t is an opaque type of unknown length).

Fixes #654.

History

#1 Updated by Tobias Brunner over 6 years ago

  • Tracker changed from Issue to Bug
  • Category set to libstrongswan
  • Status changed from New to Feedback
  • Assignee set to Tobias Brunner
  • Target version set to 5.2.1

Thanks for the report and the patch.

Introducing a global mutex is not optimal, all threads that want to access any recursive mutex would have to compete for it. They wouldn't hold it very long, but it still might create a bottleneck.

The associated commit uses atomic compare-and-swap operations and a pointer to our thread abstraction (which is stored in thread local storage) to determine whether the mutex is currently held. On some platforms this might still require a global mutex, but on most CAS is implemented efficiently.

#2 Updated by Noam Lampert over 6 years ago

I didn't realized strongswan was already using compare-and-swap. Your patch is definitely better.

For reference only, the newly introduced mutex in the original was not global, but rather part of the recursive-mutex, so two threads locking two different mutexes don't compete.
Also, the implementation of cas_xxx when !defined(HAVE_GCC_ATOMIC_OPERATIONS) can be improved my making the cas_mutex and array of mutexes and using the pointer to chose which mutexto lock. Not sure this is relevant, if there even are platforms in which HAVE_GCC_ATOMIC_OPERATIONS is not defined.

#3 Updated by Tobias Brunner over 6 years ago

For reference only, the newly introduced mutex in the original was not global, but rather part of the recursive-mutex, so two threads locking two different mutexes don't compete.

You're right, sorry. Should have looked more closely at the patch. If you send other patches could you please send them in the unified diff format (git diff, or with diff -u). Thanks!

Also, the implementation of cas_xxx when !defined(HAVE_GCC_ATOMIC_OPERATIONS) can be improved my making the cas_mutex and array of mutexes and using the pointer to chose which mutexto lock. Not sure this is relevant, if there even are platforms in which HAVE_GCC_ATOMIC_OPERATIONS is not defined.

Agreed, probably not worth the effort.

#4 Updated by Tobias Brunner about 6 years ago

  • Status changed from Feedback to Closed
  • Resolution set to Fixed

Also available in: Atom PDF