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] [day] [month] [year] [list]

Message-ID: <DB7PR04MB425234DA9B42B272EB523CBE8B3C0@DB7PR04MB4252.eurprd04.prod.outlook.com>
Date:   Sat, 18 Aug 2018 05:55:15 +0000
From:   Vakul Garg <vakul.garg@....com>
To:     Dave Watson <davejwatson@...com>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "borisp@...lanox.com" <borisp@...lanox.com>,
        "aviadye@...lanox.com" <aviadye@...lanox.com>,
        "davem@...emloft.net" <davem@...emloft.net>
Subject: RE: [PATCH net-next v1] net/tls: Add support for async decryption of
 tls records



> -----Original Message-----
> From: Dave Watson <davejwatson@...com>
> Sent: Saturday, August 18, 2018 3:43 AM
> 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.
> 

I improved it but in a slightly different way. Please see in v2.
As net-next is closed right now, I would send the patch to you privately &
later post it on list when David gives a green signal.
Is it ok?


> > +			} 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?
 
When tls_sw_advance_skb() returns false, it is certain that we cannot 
continue in the loop. So putting a break here avoids having to execute
'if' checks and while (len) checks down below.