Skip to content

Commit

Permalink
lockdep: fix races with concurrent lockdep teardown
Browse files Browse the repository at this point in the history
If the cct is unregistered while other threads are flogging mutexes,
then we can hit all sorts of bugs. Ensure that we handle that
situation sanely, by checking that g_lockdep is still set after
we take the lockdep_mutex.

Also, remove an assertion from lockdep_unregister, and just turn it into
an immediate return. It's possible to have a call to
lockdep_unregister_ceph_context, and then a call to
lockdep_register_ceph_context while a mutex is being held by another
task.

In that case, it's possible the lock does not exist in the map
when we go to unregister it. That's not a bug though, just a natural
consequence of that series of actions.

Tracker: http://tracker.ceph.com/issues/20988
Signed-off-by: Jeff Layton <jlayton@redhat.com>
  • Loading branch information
jtlayton committed Sep 16, 2017
1 parent 01863bb commit 75f41a9
Showing 1 changed file with 26 additions and 4 deletions.
30 changes: 26 additions & 4 deletions src/common/lockdep.cc
Expand Up @@ -113,6 +113,8 @@ void lockdep_unregister_ceph_context(CephContext *cct)
int lockdep_dump_locks()
{
pthread_mutex_lock(&lockdep_mutex);
if (!g_lockdep)
goto out;

for (ceph::unordered_map<pthread_t, map<int,BackTrace*> >::iterator p = held.begin();
p != held.end();
Expand All @@ -127,7 +129,7 @@ int lockdep_dump_locks()
*_dout << dendl;
}
}

out:
pthread_mutex_unlock(&lockdep_mutex);
return 0;
}
Expand Down Expand Up @@ -165,8 +167,10 @@ int lockdep_get_free_id(void)

static int _lockdep_register(const char *name)
{
int id;
int id = -1;

if (!g_lockdep)
return id;
ceph::unordered_map<std::string, int>::iterator p = lock_ids.find(name);
if (p == lock_ids.end()) {
id = lockdep_get_free_id();
Expand Down Expand Up @@ -211,9 +215,16 @@ void lockdep_unregister(int id)
}

pthread_mutex_lock(&lockdep_mutex);
if (!g_lockdep) {
pthread_mutex_unlock(&lockdep_mutex);
return;
}

map<int, std::string>::iterator p = lock_names.find(id);
assert(p != lock_names.end());
if (p == lock_names.end()) {
pthread_mutex_unlock(&lockdep_mutex);
return;
}

int &refs = lock_refs[id];
if (--refs == 0) {
Expand Down Expand Up @@ -279,8 +290,14 @@ int lockdep_will_lock(const char *name, int id, bool force_backtrace)
pthread_t p = pthread_self();

pthread_mutex_lock(&lockdep_mutex);
if (!g_lockdep) {
pthread_mutex_unlock(&lockdep_mutex);
return id;
}

if (id < 0)
id = _lockdep_register(name);

lockdep_dout(20) << "_will_lock " << name << " (" << id << ")" << dendl;

// check dependency graph
Expand Down Expand Up @@ -343,7 +360,6 @@ int lockdep_will_lock(const char *name, int id, bool force_backtrace)
}
}
}

pthread_mutex_unlock(&lockdep_mutex);
return id;
}
Expand All @@ -353,6 +369,8 @@ int lockdep_locked(const char *name, int id, bool force_backtrace)
pthread_t p = pthread_self();

pthread_mutex_lock(&lockdep_mutex);
if (!g_lockdep)
goto out;
if (id < 0)
id = _lockdep_register(name);

Expand All @@ -361,6 +379,7 @@ int lockdep_locked(const char *name, int id, bool force_backtrace)
held[p][id] = new BackTrace(BACKTRACE_SKIP);
else
held[p][id] = 0;
out:
pthread_mutex_unlock(&lockdep_mutex);
return id;
}
Expand All @@ -376,6 +395,8 @@ int lockdep_will_unlock(const char *name, int id)
}

pthread_mutex_lock(&lockdep_mutex);
if (!g_lockdep)
goto out;
lockdep_dout(20) << "_will_unlock " << name << dendl;

// don't assert.. lockdep may be enabled at any point in time
Expand All @@ -384,6 +405,7 @@ int lockdep_will_unlock(const char *name, int id)

delete held[p][id];
held[p].erase(id);
out:
pthread_mutex_unlock(&lockdep_mutex);
return id;
}
Expand Down

0 comments on commit 75f41a9

Please sign in to comment.