Skip to content

Commit a0d97d5

Browse files
matthewstevenson88copybara-github
authored andcommitted
[security] Remove lock that guards the tsi_frame_protector. (grpc#39356)
This is believed to be safe following grpc#39308, but we are guarding the change with an experiment to err on the safe side. The new concurrency expectations are documented in grpc#39351. Closes grpc#39356 COPYBARA_INTEGRATE_REVIEW=grpc#39356 from matthewstevenson88:remove-mu 40c0e02 PiperOrigin-RevId: 751505414
1 parent 583f7fe commit a0d97d5

File tree

8 files changed

+109
-30
lines changed

8 files changed

+109
-30
lines changed

BUILD

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2238,6 +2238,7 @@ grpc_cc_library(
22382238
"//src/core:context",
22392239
"//src/core:error",
22402240
"//src/core:event_engine_memory_allocator",
2241+
"//src/core:experiments",
22412242
"//src/core:gpr_atm",
22422243
"//src/core:handshaker_factory",
22432244
"//src/core:handshaker_registry",

bazel/experiments.bzl

Lines changed: 1 addition & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

src/core/handshaker/security/legacy_secure_endpoint.cc

Lines changed: 34 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@
3939
#include "absl/strings/string_view.h"
4040
#include "src/core/handshaker/security/secure_endpoint.h"
4141
#include "src/core/lib/debug/trace.h"
42+
#include "src/core/lib/experiments/experiments.h"
4243
#include "src/core/lib/iomgr/closure.h"
4344
#include "src/core/lib/iomgr/endpoint.h"
4445
#include "src/core/lib/iomgr/error.h"
@@ -288,11 +289,17 @@ static void on_read(void* user_data, grpc_error_handle error) {
288289
size_t unprotected_buffer_size_written =
289290
static_cast<size_t>(end - cur);
290291
size_t processed_message_size = message_size;
291-
gpr_mu_lock(&ep->protector_mu);
292-
result = tsi_frame_protector_unprotect(
293-
ep->protector, message_bytes, &processed_message_size, cur,
294-
&unprotected_buffer_size_written);
295-
gpr_mu_unlock(&ep->protector_mu);
292+
if (grpc_core::IsTsiFrameProtectorWithoutLocksEnabled()) {
293+
result = tsi_frame_protector_unprotect(
294+
ep->protector, message_bytes, &processed_message_size, cur,
295+
&unprotected_buffer_size_written);
296+
} else {
297+
gpr_mu_lock(&ep->protector_mu);
298+
result = tsi_frame_protector_unprotect(
299+
ep->protector, message_bytes, &processed_message_size, cur,
300+
&unprotected_buffer_size_written);
301+
gpr_mu_unlock(&ep->protector_mu);
302+
}
296303
if (result != TSI_OK) {
297304
LOG(ERROR) << "Decryption error: " << tsi_result_to_string(result);
298305
break;
@@ -443,11 +450,17 @@ static void endpoint_write(grpc_endpoint* secure_ep, grpc_slice_buffer* slices,
443450
while (message_size > 0) {
444451
size_t protected_buffer_size_to_send = static_cast<size_t>(end - cur);
445452
size_t processed_message_size = message_size;
446-
gpr_mu_lock(&ep->protector_mu);
447-
result = tsi_frame_protector_protect(ep->protector, message_bytes,
448-
&processed_message_size, cur,
449-
&protected_buffer_size_to_send);
450-
gpr_mu_unlock(&ep->protector_mu);
453+
if (grpc_core::IsTsiFrameProtectorWithoutLocksEnabled()) {
454+
result = tsi_frame_protector_protect(
455+
ep->protector, message_bytes, &processed_message_size, cur,
456+
&protected_buffer_size_to_send);
457+
} else {
458+
gpr_mu_lock(&ep->protector_mu);
459+
result = tsi_frame_protector_protect(
460+
ep->protector, message_bytes, &processed_message_size, cur,
461+
&protected_buffer_size_to_send);
462+
gpr_mu_unlock(&ep->protector_mu);
463+
}
451464
if (result != TSI_OK) {
452465
LOG(ERROR) << "Encryption error: " << tsi_result_to_string(result);
453466
break;
@@ -466,11 +479,17 @@ static void endpoint_write(grpc_endpoint* secure_ep, grpc_slice_buffer* slices,
466479
size_t still_pending_size;
467480
do {
468481
size_t protected_buffer_size_to_send = static_cast<size_t>(end - cur);
469-
gpr_mu_lock(&ep->protector_mu);
470-
result = tsi_frame_protector_protect_flush(
471-
ep->protector, cur, &protected_buffer_size_to_send,
472-
&still_pending_size);
473-
gpr_mu_unlock(&ep->protector_mu);
482+
if (grpc_core::IsTsiFrameProtectorWithoutLocksEnabled()) {
483+
result = tsi_frame_protector_protect_flush(
484+
ep->protector, cur, &protected_buffer_size_to_send,
485+
&still_pending_size);
486+
} else {
487+
gpr_mu_lock(&ep->protector_mu);
488+
result = tsi_frame_protector_protect_flush(
489+
ep->protector, cur, &protected_buffer_size_to_send,
490+
&still_pending_size);
491+
gpr_mu_unlock(&ep->protector_mu);
492+
}
474493
if (result != TSI_OK) break;
475494
cur += protected_buffer_size_to_send;
476495
if (cur == end) {

src/core/handshaker/security/secure_endpoint.cc

Lines changed: 34 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@
4141
#include "absl/status/status.h"
4242
#include "absl/strings/string_view.h"
4343
#include "src/core/lib/debug/trace.h"
44+
#include "src/core/lib/experiments/experiments.h"
4445
#include "src/core/lib/iomgr/closure.h"
4546
#include "src/core/lib/iomgr/endpoint.h"
4647
#include "src/core/lib/iomgr/error.h"
@@ -218,11 +219,17 @@ class FrameProtector : public RefCounted<FrameProtector> {
218219
size_t unprotected_buffer_size_written =
219220
static_cast<size_t>(end - cur);
220221
size_t processed_message_size = message_size;
221-
protector_mu_.Lock();
222-
result = tsi_frame_protector_unprotect(
223-
protector_, message_bytes, &processed_message_size, cur,
224-
&unprotected_buffer_size_written);
225-
protector_mu_.Unlock();
222+
if (IsTsiFrameProtectorWithoutLocksEnabled()) {
223+
result = tsi_frame_protector_unprotect(
224+
protector_, message_bytes, &processed_message_size, cur,
225+
&unprotected_buffer_size_written);
226+
} else {
227+
protector_mu_.Lock();
228+
result = tsi_frame_protector_unprotect(
229+
protector_, message_bytes, &processed_message_size, cur,
230+
&unprotected_buffer_size_written);
231+
protector_mu_.Unlock();
232+
}
226233
if (result != TSI_OK) {
227234
LOG(ERROR) << "Decryption error: " << tsi_result_to_string(result);
228235
break;
@@ -343,11 +350,17 @@ class FrameProtector : public RefCounted<FrameProtector> {
343350
while (message_size > 0) {
344351
size_t protected_buffer_size_to_send = static_cast<size_t>(end - cur);
345352
size_t processed_message_size = message_size;
346-
protector_mu_.Lock();
347-
result = tsi_frame_protector_protect(protector_, message_bytes,
348-
&processed_message_size, cur,
349-
&protected_buffer_size_to_send);
350-
protector_mu_.Unlock();
353+
if (IsTsiFrameProtectorWithoutLocksEnabled()) {
354+
result = tsi_frame_protector_protect(
355+
protector_, message_bytes, &processed_message_size, cur,
356+
&protected_buffer_size_to_send);
357+
} else {
358+
protector_mu_.Lock();
359+
result = tsi_frame_protector_protect(
360+
protector_, message_bytes, &processed_message_size, cur,
361+
&protected_buffer_size_to_send);
362+
protector_mu_.Unlock();
363+
}
351364
if (result != TSI_OK) {
352365
LOG(ERROR) << "Encryption error: " << tsi_result_to_string(result);
353366
break;
@@ -366,11 +379,17 @@ class FrameProtector : public RefCounted<FrameProtector> {
366379
size_t still_pending_size;
367380
do {
368381
size_t protected_buffer_size_to_send = static_cast<size_t>(end - cur);
369-
protector_mu_.Lock();
370-
result = tsi_frame_protector_protect_flush(
371-
protector_, cur, &protected_buffer_size_to_send,
372-
&still_pending_size);
373-
protector_mu_.Unlock();
382+
if (IsTsiFrameProtectorWithoutLocksEnabled()) {
383+
result = tsi_frame_protector_protect_flush(
384+
protector_, cur, &protected_buffer_size_to_send,
385+
&still_pending_size);
386+
} else {
387+
protector_mu_.Lock();
388+
result = tsi_frame_protector_protect_flush(
389+
protector_, cur, &protected_buffer_size_to_send,
390+
&still_pending_size);
391+
protector_mu_.Unlock();
392+
}
374393
if (result != TSI_OK) break;
375394
cur += protected_buffer_size_to_send;
376395
if (cur == end) {

src/core/lib/experiments/experiments.cc

Lines changed: 24 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

src/core/lib/experiments/experiments.h

Lines changed: 8 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

src/core/lib/experiments/experiments.yaml

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -287,6 +287,11 @@
287287
expiry: 2025/06/01
288288
289289
test_tags: ["endpoint_test", "flow_control_test"]
290+
- name: tsi_frame_protector_without_locks
291+
description: Do not hold locks while using the tsi_frame_protector.
292+
expiry: 2025/09/03
293+
294+
test_tags: []
290295
- name: unconstrained_max_quota_buffer_size
291296
description: Discard the cap on the max free pool size for one memory allocator
292297
expiry: 2025/09/03

src/core/lib/experiments/rollouts.yaml

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -104,5 +104,7 @@
104104
default: false
105105
- name: tcp_rcv_lowat
106106
default: false
107+
- name: tsi_frame_protector_without_locks
108+
default: false
107109
- name: unconstrained_max_quota_buffer_size
108110
default: false

0 commit comments

Comments
 (0)