On Fri, 28 May 2021 15:40:38 +0300 Maxim Mikityanskiy wrote:

quoted

quoted

@@ -961,6 +964,17 @@ int tls_device_decrypted(struct sock *sk, struct tls_context *tls_ctx,
  
  	ctx->sw.decrypted |= is_decrypted;
  
+	if (unlikely(test_bit(TLS_RX_DEV_DEGRADED, &tls_ctx->flags))) {  

Why not put the check in tls_device_core_ctrl_rx_resync()?
Would be less code, right?  

I see what you mean, and I considered this option, but I think my option 
has better readability and is more future-proof. By doing an early 
return, I skip all code irrelevant to the degraded mode, and even though 
changing ctx->resync_nh_reset won't have effect in the degraded mode, it 
will be easier for readers to understand that this part of code is not 
relevant. Furthermore, if someone decides to add more code to 
!is_encrypted branches in the future, there is a chance that the 
degraded mode will be missed from consideration. With the early return 
there is not problem, but if I follow your suggestion and do the check 
only under is_encrypted, a future contributor unfamiliar with this 
"degraded flow" might fail to add that check where it will be needed.

This was the reason I implemented it this way. What do you think?

In general "someone may miss this in the future" is better served by
adding a test case than code duplication. But we don't have infra to 
fake-offload TLS so I don't feel strongly. You can keep as is if that's
your preference.