Skip to content

Commit

Permalink
Reduce bouncing on the cache lock in ssl_update_cache.
Browse files Browse the repository at this point in the history 
ssl_update_cache takes the cache lock to add to the session cache,
releases it, and then immediately takes and releases the lock to
increment handshakes_since_cache_flush. Then, in 1/255 connections, does
the same thing again to flush stale sessions.

Merge the first two into one lock. In doing so, move ssl_update_cache to
ssl_session.cc, so it can access a newly-extracted add_session_lock.
Also remove the mode parameter (the SSL knows if it's a client or
server), and move the established_session != session check to the
caller, which more directly knows whether there was a new session.

Also add some TSan coverage for this path in the tests. In an earlier
iteration of this patch, I managed to introduce a double-locking bug
because we weren't testing it at all. Confirmed this test catches both
double-locking and insufficient locking. (It doesn't seem able to catch
using a read lock instead of a write lock in SSL_CTX_flush_sessions,
however. I suspect the hash table is distributing the cells each thread
touches.)

Update-Note: This reshuffles some locks around the session cache.
(Hopefully for the better.)

Change-Id: I78dca53fda74e036b90110cca7fbcc306a5c8ebe
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48133
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
  • Loading branch information
@davidben
davidben authored and Boringssl LUCI CQ committed Jun 24, 2021
1 parent 10a76ac commit a10017c
Show file tree
Hide file tree
Showing 6 changed files with 193 additions and 121 deletions.
13 changes: 8 additions & 5 deletions ssl/handshake_client.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1801,10 +1801,8 @@ static enum ssl_hs_wait_t do_finish_client_handshake(SSL_HANDSHAKE *hs) {

// Note TLS 1.2 resumptions with ticket renewal have both |ssl->session| (the
// resumed session) and |hs->new_session| (the session with the new ticket).
if (hs->new_session == nullptr) {
assert(ssl->session != nullptr);
ssl->s3->established_session = UpRef(ssl->session);
} else {
bool has_new_session = hs->new_session != nullptr;
if (has_new_session) {
// When False Start is enabled, the handshake reports completion early. The
// caller may then have passed the (then unresuable) |hs->new_session| to
// another thread via |SSL_get0_session| for resumption. To avoid potential
Expand All @@ -1821,11 +1819,16 @@ static enum ssl_hs_wait_t do_finish_client_handshake(SSL_HANDSHAKE *hs) {
}

hs->new_session.reset();
} else {
assert(ssl->session != nullptr);
ssl->s3->established_session = UpRef(ssl->session);
}

hs->handshake_finalized = true;
ssl->s3->initial_handshake_complete = true;
ssl_update_cache(hs, SSL_SESS_CACHE_CLIENT);
if (has_new_session) {
ssl_update_cache(ssl);
}

hs->state = state_done;
return ssl_hs_ok;
Expand Down
13 changes: 8 additions & 5 deletions ssl/handshake_server.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1791,18 +1791,21 @@ static enum ssl_hs_wait_t do_finish_server_handshake(SSL_HANDSHAKE *hs) {
ssl->ctx->x509_method->session_clear(hs->new_session.get());
}

if (hs->new_session == nullptr) {
assert(ssl->session != nullptr);
ssl->s3->established_session = UpRef(ssl->session);
} else {
bool has_new_session = hs->new_session != nullptr;
if (has_new_session) {
assert(ssl->session == nullptr);
ssl->s3->established_session = std::move(hs->new_session);
ssl->s3->established_session->not_resumable = false;
} else {
assert(ssl->session != nullptr);
ssl->s3->established_session = UpRef(ssl->session);
}

hs->handshake_finalized = true;
ssl->s3->initial_handshake_complete = true;
ssl_update_cache(hs, SSL_SESS_CACHE_SERVER);
if (has_new_session) {
ssl_update_cache(ssl);
}

hs->state = state12_done;
return ssl_hs_ok;
Expand Down
2 changes: 1 addition & 1 deletion ssl/internal.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3147,7 +3147,7 @@ void ssl_session_rebase_time(SSL *ssl, SSL_SESSION *session);
void ssl_session_renew_timeout(SSL *ssl, SSL_SESSION *session,
uint32_t timeout);

void ssl_update_cache(SSL_HANDSHAKE *hs, int mode);
void ssl_update_cache(SSL *ssl);

void ssl_send_alert(SSL *ssl, int level, int desc);
int ssl_send_alert_impl(SSL *ssl, int level, int desc);
Expand Down
44 changes: 0 additions & 44 deletions ssl/ssl_lib.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -272,50 +272,6 @@ ssl_open_record_t ssl_open_app_data(SSL *ssl, Span<uint8_t> *out,
return ret;
}

void ssl_update_cache(SSL_HANDSHAKE *hs, int mode) {
SSL *const ssl = hs->ssl;
SSL_CTX *ctx = ssl->session_ctx.get();
if (!SSL_SESSION_is_resumable(ssl->s3->established_session.get()) ||
(ctx->session_cache_mode & mode) != mode) {
return;
}

// Clients never use the internal session cache.
int use_internal_cache = ssl->server && !(ctx->session_cache_mode &
SSL_SESS_CACHE_NO_INTERNAL_STORE);
if (ssl->s3->established_session.get() != ssl->session.get()) {
if (use_internal_cache) {
SSL_CTX_add_session(ctx, ssl->s3->established_session.get());
}
if (ctx->new_session_cb != NULL) {
UniquePtr<SSL_SESSION> ref = UpRef(ssl->s3->established_session);
if (ctx->new_session_cb(ssl, ref.get())) {
// |new_session_cb|'s return value signals whether it took ownership.
ref.release();
}
}
}

if (use_internal_cache &&
!(ctx->session_cache_mode & SSL_SESS_CACHE_NO_AUTO_CLEAR)) {
// Automatically flush the internal session cache every 255 connections.
int flush_cache = 0;
CRYPTO_MUTEX_lock_write(&ctx->lock);
ctx->handshakes_since_cache_flush++;
if (ctx->handshakes_since_cache_flush >= 255) {
flush_cache = 1;
ctx->handshakes_since_cache_flush = 0;
}
CRYPTO_MUTEX_unlock_write(&ctx->lock);

if (flush_cache) {
struct OPENSSL_timeval now;
ssl_get_current_time(ssl, &now);
SSL_CTX_flush_sessions(ctx, now.tv_sec);
}
}
}

static bool cbb_add_hex(CBB *cbb, Span<const uint8_t> in) {
static const char hextable[] = "0123456789abcdef";
uint8_t *out;
Expand Down
186 changes: 121 additions & 65 deletions ssl/ssl_session.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,6 @@ static CRYPTO_EX_DATA_CLASS g_ex_data_class =

static void SSL_SESSION_list_remove(SSL_CTX *ctx, SSL_SESSION *session);
static void SSL_SESSION_list_add(SSL_CTX *ctx, SSL_SESSION *session);
static int remove_session_lock(SSL_CTX *ctx, SSL_SESSION *session, int lock);

UniquePtr<SSL_SESSION> ssl_session_new(const SSL_X509_METHOD *x509_method) {
return MakeUnique<SSL_SESSION>(x509_method);
Expand Down Expand Up @@ -754,34 +753,36 @@ enum ssl_hs_wait_t ssl_get_prev_session(SSL_HANDSHAKE *hs,
return ssl_hs_ok;
}

static int remove_session_lock(SSL_CTX *ctx, SSL_SESSION *session, int lock) {
int ret = 0;
static bool remove_session(SSL_CTX *ctx, SSL_SESSION *session, bool lock) {
if (session == nullptr || session->session_id_length == 0) {
return false;
}

if (session != NULL && session->session_id_length != 0) {
if (lock) {
CRYPTO_MUTEX_lock_write(&ctx->lock);
}
SSL_SESSION *found_session = lh_SSL_SESSION_retrieve(ctx->sessions,
session);
if (found_session == session) {
ret = 1;
found_session = lh_SSL_SESSION_delete(ctx->sessions, session);
SSL_SESSION_list_remove(ctx, session);
}
if (lock) {
CRYPTO_MUTEX_lock_write(&ctx->lock);
}

if (lock) {
CRYPTO_MUTEX_unlock_write(&ctx->lock);
}
SSL_SESSION *found_session = lh_SSL_SESSION_retrieve(ctx->sessions, session);
bool found = found_session == session;
if (found) {
found_session = lh_SSL_SESSION_delete(ctx->sessions, session);
SSL_SESSION_list_remove(ctx, session);
}

if (ret) {
if (ctx->remove_session_cb != NULL) {
ctx->remove_session_cb(ctx, found_session);
}
SSL_SESSION_free(found_session);
if (lock) {
CRYPTO_MUTEX_unlock_write(&ctx->lock);
}

if (found) {
// TODO(https://crbug.com/boringssl/251): Callbacks should not be called
// under a lock.
if (ctx->remove_session_cb != nullptr) {
ctx->remove_session_cb(ctx, found_session);
}
SSL_SESSION_free(found_session);
}

return ret;
return found;
}

void ssl_set_session(SSL *ssl, SSL_SESSION *session) {
Expand Down Expand Up @@ -839,6 +840,98 @@ static void SSL_SESSION_list_add(SSL_CTX *ctx, SSL_SESSION *session) {
}
}

static bool add_session_locked(SSL_CTX *ctx, UniquePtr<SSL_SESSION> session) {
SSL_SESSION *new_session = session.get();
SSL_SESSION *old_session;
if (!lh_SSL_SESSION_insert(ctx->sessions, &old_session, new_session)) {
return false;
}
// |ctx->sessions| took ownership of |new_session| and gave us back a
// reference to |old_session|. (|old_session| may be the same as
// |new_session|, in which case we traded identical references with
// |ctx->sessions|.)
session.release();
session.reset(old_session);

if (old_session != nullptr) {
if (old_session == new_session) {
// |session| was already in the cache. There are no linked list pointers
// to update.
return false;
}

// There was a session ID collision. |old_session| was replaced with
// |session| in the hash table, so |old_session| must be removed from the
// linked list to match.
SSL_SESSION_list_remove(ctx, old_session);
}

// This does not increment the reference count. Although |session| is inserted
// into two structures (a doubly-linked list and the hash table), |ctx| only
// takes one reference.
SSL_SESSION_list_add(ctx, new_session);

// Enforce any cache size limits.
if (SSL_CTX_sess_get_cache_size(ctx) > 0) {
while (lh_SSL_SESSION_num_items(ctx->sessions) >
SSL_CTX_sess_get_cache_size(ctx)) {
if (!remove_session(ctx, ctx->session_cache_tail,
/*lock=*/false)) {
break;
}
}
}

return true;
}

void ssl_update_cache(SSL *ssl) {
SSL_CTX *ctx = ssl->session_ctx.get();
SSL_SESSION *session = ssl->s3->established_session.get();
int mode = SSL_is_server(ssl) ? SSL_SESS_CACHE_SERVER : SSL_SESS_CACHE_CLIENT;
if (!SSL_SESSION_is_resumable(session) ||
(ctx->session_cache_mode & mode) != mode) {
return;
}

// Clients never use the internal session cache.
if (ssl->server &&
!(ctx->session_cache_mode & SSL_SESS_CACHE_NO_INTERNAL_STORE)) {
UniquePtr<SSL_SESSION> ref = UpRef(session);
bool remove_expired_sessions = false;
{
MutexWriteLock lock(&ctx->lock);
add_session_locked(ctx, std::move(ref));

if (!(ctx->session_cache_mode & SSL_SESS_CACHE_NO_AUTO_CLEAR)) {
// Automatically flush the internal session cache every 255 connections.
ctx->handshakes_since_cache_flush++;
if (ctx->handshakes_since_cache_flush >= 255) {
remove_expired_sessions = true;
ctx->handshakes_since_cache_flush = 0;
}
}
}

if (remove_expired_sessions) {
// |SSL_CTX_flush_sessions| takes the lock we just released. We could
// merge the critical sections, but we'd then call user code under a
// lock, or compute |now| earlier, even when not flushing.
OPENSSL_timeval now;
ssl_get_current_time(ssl, &now);
SSL_CTX_flush_sessions(ctx, now.tv_sec);
}
}

if (ctx->new_session_cb != nullptr) {
UniquePtr<SSL_SESSION> ref = UpRef(session);
if (ctx->new_session_cb(ssl, ref.get())) {
// |new_session_cb|'s return value signals whether it took ownership.
ref.release();
}
}
}

BSSL_NAMESPACE_END

using namespace bssl;
Expand Down Expand Up @@ -1121,51 +1214,13 @@ void *SSL_SESSION_get_ex_data(const SSL_SESSION *session, int idx) {
}

int SSL_CTX_add_session(SSL_CTX *ctx, SSL_SESSION *session) {
// Although |session| is inserted into two structures (a doubly-linked list
// and the hash table), |ctx| only takes one reference.
UniquePtr<SSL_SESSION> owned_session = UpRef(session);

SSL_SESSION *old_session;
MutexWriteLock lock(&ctx->lock);
if (!lh_SSL_SESSION_insert(ctx->sessions, &old_session, session)) {
return 0;
}
// |ctx->sessions| took ownership of |session| and gave us back a reference to
// |old_session|. (|old_session| may be the same as |session|, in which case
// we traded identical references with |ctx->sessions|.)
owned_session.release();
owned_session.reset(old_session);

if (old_session != NULL) {
if (old_session == session) {
// |session| was already in the cache. There are no linked list pointers
// to update.
return 0;
}

// There was a session ID collision. |old_session| was replaced with
// |session| in the hash table, so |old_session| must be removed from the
// linked list to match.
SSL_SESSION_list_remove(ctx, old_session);
}

SSL_SESSION_list_add(ctx, session);

// Enforce any cache size limits.
if (SSL_CTX_sess_get_cache_size(ctx) > 0) {
while (lh_SSL_SESSION_num_items(ctx->sessions) >
SSL_CTX_sess_get_cache_size(ctx)) {
if (!remove_session_lock(ctx, ctx->session_cache_tail, 0)) {
break;
}
}
}

return 1;
return add_session_locked(ctx, std::move(owned_session));
}

int SSL_CTX_remove_session(SSL_CTX *ctx, SSL_SESSION *session) {
return remove_session_lock(ctx, session, 1);
return remove_session(ctx, session, /*lock=*/true);
}

int SSL_set_session(SSL *ssl, SSL_SESSION *session) {
Expand Down Expand Up @@ -1219,10 +1274,11 @@ static void timeout_doall_arg(SSL_SESSION *session, void *void_param) {
if (param->time == 0 ||
session->time + session->timeout < session->time ||
param->time > (session->time + session->timeout)) {
// The reason we don't call SSL_CTX_remove_session() is to
// save on locking overhead
// TODO(davidben): This can probably just call |remove_session|.
(void) lh_SSL_SESSION_delete(param->cache, session);
SSL_SESSION_list_remove(param->ctx, session);
// TODO(https://crbug.com/boringssl/251): Callbacks should not be called
// under a lock.
if (param->ctx->remove_session_cb != NULL) {
param->ctx->remove_session_cb(param->ctx, session);
}
Expand Down
Loading

0 comments on commit a10017c

Please sign in to comment.