netdev - Re: [PATCH net-next v1] net/tls: Add support for async decryption of tls records

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20180817221238.b4napcwedbwup22q@davejwatson-mba.local.dhcp.thefacebook.com>
Date:   Fri, 17 Aug 2018 15:12:38 -0700
From:   Dave Watson <davejwatson@...com>
To:     Vakul Garg <vakul.garg@....com>
CC:     <netdev@...r.kernel.org>, <borisp@...lanox.com>,
        <aviadye@...lanox.com>, <davem@...emloft.net>
Subject: Re: [PATCH net-next v1] net/tls: Add support for async decryption of
 tls records

On 08/16/18 08:49 PM, Vakul Garg wrote:
> Changes since RFC version:
> 	1) Improved commit message.
> 	2) Fixed dequeued record offset handling because of which few of
> 	   tls selftests 'recv_partial, recv_peek, recv_peek_multiple' were failing.

Thanks! Commit message much more clear, tests work great for me also,
only minor comments on clarity

> -			if (tls_sw_advance_skb(sk, skb, chunk)) {
> +			if (async) {
> +				/* Finished with current record, pick up next */
> +				ctx->recv_pkt = NULL;
> +				__strp_unpause(&ctx->strp);
> +				goto mark_eor_chk_ctrl;

Control flow is a little hard to follow here, maybe just pass an async
flag to tls_sw_advance_skb?  It already does strp_unpause and recv_pkt
= NULL.  

> +			} else if (tls_sw_advance_skb(sk, skb, chunk)) {
>  				/* Return full control message to
>  				 * userspace before trying to parse
>  				 * another message type
>  				 */
> +mark_eor_chk_ctrl:
>  				msg->msg_flags |= MSG_EOR;
>  				if (control != TLS_RECORD_TYPE_DATA)
>  					goto recv_end;
> +			} else {
> +				break;

I don't see the need for the else { break; }, isn't this already
covered by while(len); below as before?