Re: [PATCH net 5/5] tls: don't decrypt the next record if it's of a different type

2023-09-08, 14:06:12 +0800, Herbert Xu wrote:

On Thu, Sep 07, 2023 at 02:21:59PM +0200, Sabrina Dubroca wrote:

quoted

Herbert, WDYT? We're calling tls_do_decryption twice from the same
tls_sw_recvmsg invocation, first with darg->async = true, then with
darg->async = false. Is it ok to use ctx->async_wait for both, or do
we need a fresh one as in this patch?

Yes I think your patch makes sense and the existing code could
malfunction if two decryption requests occur during the same
tls_sw_recvmsg call, with the first being async and the second
being sync.

Thanks for confirming. I'll add it to v2 of this series.

However, I'm still unsure about the case where two async decryption
requests occur during the same tls_sw_recvmsg call.  Or perhaps this
is not possible due to other constraints that are not obvious?

tls_decrypt_done only runs the completion when decrypt_pending drops
to 0, so this should be covered.


I wonder if this situation could happen:

tls_sw_recvmsg
  process first record
    decrypt_pending = 1
                                  CB runs
                                  decrypt_pending = 0
                                  complete(&ctx->async_wait.completion);

  process second record
    decrypt_pending = 1
  tls_sw_recvmsg reaches "recv_end"
    decrypt_pending != 0
    crypto_wait_req sees the first completion of ctx->async_wait and proceeds

                                  CB runs
                                  decrypt_pending = 0
                                  complete(&ctx->async_wait.completion);


With my force_async patch I've managed to run into situations where
the CB runs before we reach the crypto_wait_req at the end of
tls_sw_recvmsg (patch 4 of this series [1]). I don't know if it's a
side-effect of my hack or if it's realistic.

[1] https://patchwork.kernel.org/project/netdevbpf/patch/e094325019f7fd960470c10efda41c1b7f9bc54f.1694018970.git.sd@queasysnail.net/

-- 
Sabrina