RE: [PATCH net 3/4] tls: Fix mixing between async capable and async

[Vakul Garg <vakul.garg@nxp.com>]

> -----Original Message-----
> From: Boris Pismenny <borisp@mellanox.com>
> Sent: Tuesday, February 26, 2019 5:43 PM
> To: aviadye@mellanox.com; davejwatson@fb.com;
> john.fastabend@gmail.com; daniel@iogearbox.net; Vakul Garg
> <vakul.garg@nxp.com>; netdev@vger.kernel.org
> Cc: eranbe@mellanox.com; borisp@mellanox.com
> Subject: [PATCH net 3/4] tls: Fix mixing between async capable and async
> 
> From: Eran Ben Elisha <eranbe@mellanox.com>
> 
> Today, tls_sw_recvmsg is capable of using asynchronous mode to handle
> application data TLS records. Moreover, it assumes that if the cipher can be
> handled asynchronously, then all packets will be processed asynchronously.
> 
> However, this assumption is not always true. 

Could you please elaborate, what happens?

> Specifically, for AES-GCM in
> TLS1.2, it causes data corruption, and breaks user applications.
> 
> This patch fixes this problem by separating the async capability from the
> decryption operation result.
> 
> Fixes: c0ab4732d4c6 ("net/tls: Do not use async crypto for non-data
> records")
> Signed-off-by: Eran Ben Elisha <eranbe@mellanox.com>
> Reviewed-by: Boris Pismenny <borisp@mellanox.com>
> ---
>  net/tls/tls_sw.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c index
> 4afa67b00aaf..f515cd7e984e 100644
> --- a/net/tls/tls_sw.c
> +++ b/net/tls/tls_sw.c
> @@ -1693,7 +1693,8 @@ int tls_sw_recvmsg(struct sock *sk,
>  		bool zc = false;
>  		int to_decrypt;
>  		int chunk = 0;
> -		bool async;
> +		bool async_capable;
> +		bool async = false;
> 
>  		skb = tls_wait_data(sk, psock, flags, timeo, &err);
>  		if (!skb) {
> @@ -1727,21 +1728,23 @@ int tls_sw_recvmsg(struct sock *sk,
> 
>  		/* Do not use async mode if record is non-data */
>  		if (ctx->control == TLS_RECORD_TYPE_DATA)
> -			async = ctx->async_capable;
> +			async_capable = ctx->async_capable;
>  		else
> -			async = false;
> +			async_capable = false;
> 
>  		err = decrypt_skb_update(sk, skb, &msg->msg_iter,
> -					 &chunk, &zc, async);
> +					 &chunk, &zc, async_capable);
>  		if (err < 0 && err != -EINPROGRESS) {
>  			tls_err_abort(sk, EBADMSG);
>  			goto recv_end;
>  		}
> 
> -		if (err == -EINPROGRESS)
> +		if (err == -EINPROGRESS && async_capable) {
Why do we need to check 'async_capable'? 
Do we get err == -EINPROGRESS even when async_capable is false?

> +			async = true;
>  			num_async++;
> -		else if (prot->version == TLS_1_3_VERSION)
> +		} else if (prot->version == TLS_1_3_VERSION) {
>  			tlm->control = ctx->control;
> +		}
> 
>  		/* If the type of records being processed is not known yet,
>  		 * set it to record type just dequeued. If it is already known,
> --
> 2.12.2