Skip to content

Commit

Permalink
mutex: force serialization on mutex_exit() to fix races
Browse files Browse the repository at this point in the history
It is known that mutex in Linux is not safe when using it to synchronize
the freeing of object in which the mutex is embedded:
http://lwn.net/Articles/575477/

The known places in ZFS which are suspected to suffer from the race condition
are zio->io_lock and dbuf->db_mtx:
  zio uses zio->io_lock and zio->io_cv to synchronize freeing between zio_wait()
  and zio_done().
  dbuf uses dbuf->db_mtx to protect reference counting.

Add it's been reported multiple time that zio->io_lock and dbuf->db_mtx suffer
from corruption, which is exactly the symptom of the race.
openzfs/zfs#2523
openzfs/zfs#2897

This patch fix this kind of race by forcing serializaion on mutex_exit() with
a spinlock, making the mutex safer by sacrificing a bit of performance and
memory overhead.

Signed-off-by: Chunwei Chen <tuxoko@gmail.com>
  • Loading branch information
tuxoko committed Dec 19, 2014
1 parent 52479ec commit a5249cc
Showing 1 changed file with 31 additions and 1 deletion.
32 changes: 31 additions & 1 deletion include/sys/mutex.h
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ typedef enum {
*/
typedef struct {
struct mutex m;
spinlock_t m_lock; /* used for serializing mutex_exit */
} kmutex_t;

static inline kthread_t *
Expand All @@ -70,6 +71,7 @@ mutex_owner(kmutex_t *mp)
ASSERT(type == MUTEX_DEFAULT); \
\
__mutex_init(&(mp)->m, #mp, &__key); \
spin_lock_init(&(mp)->m_lock); \
})

#undef mutex_destroy
Expand All @@ -84,12 +86,37 @@ mutex_owner(kmutex_t *mp)
ASSERT3P(mutex_owner(mp), !=, current); \
mutex_lock(&(mp)->m); \
})
#define mutex_exit(mp) mutex_unlock(&(mp)->m)
/*
* The reason for the spinlock:
*
* The Linux mutex is designed with a fast-path/slow-path design such that it
* does not guarantee serialization upon itself, allowing a race where later
* aquirers finish mutex_unlock before former ones.
*
* The race renders it unsafe to be used for serializing the freeing of object
* in which the mutex is embedded, where the later aquirer could go on to free
* the object while the former one still doing mutex_unlock and causing memory
* corruption.
*
* However, there are many places in ZFS where the mutex is used for
* serializing object freeing, and the code is shared among other OSes without
* this issue. Thus, we need the spinlock to force the serialization on
* mutex_exit().
*
* See http://lwn.net/Articles/575477/ for the information about the race.
*/
#define mutex_exit(mp) \
({ \
spin_lock(&(mp)->m_lock); \
mutex_unlock(&(mp)->m); \
spin_unlock(&(mp)->m_lock); \
})

#else /* HAVE_MUTEX_OWNER */

typedef struct {
struct mutex m_mutex;
spinlock_t m_lock;
kthread_t *m_owner;
} kmutex_t;

Expand Down Expand Up @@ -125,6 +152,7 @@ spl_mutex_clear_owner(kmutex_t *mp)
ASSERT(type == MUTEX_DEFAULT); \
\
__mutex_init(MUTEX(mp), #mp, &__key); \
spin_lock_init(&(mp)->m_lock); \
spl_mutex_clear_owner(mp); \
})

Expand Down Expand Up @@ -153,8 +181,10 @@ spl_mutex_clear_owner(kmutex_t *mp)

#define mutex_exit(mp) \
({ \
spin_lock(&(mp)->m_lock); \
spl_mutex_clear_owner(mp); \
mutex_unlock(MUTEX(mp)); \
spin_unlock(&(mp)->m_lock); \
})

#endif /* HAVE_MUTEX_OWNER */
Expand Down

0 comments on commit a5249cc

Please sign in to comment.