From mboxrd@z Thu Jan  1 00:00:00 1970
From: David Miller <davem@davemloft.net>
Subject: Re: [PATCH net-next] net/tls: Corrected enabling of zero-copy mode
Date: Wed, 25 Jul 2018 13:28:36 -0700 (PDT)
Message-ID: <20180725.132836.2058461941832003627.davem@davemloft.net>
References: <20180723153006.24601-1-vakul.garg@nxp.com>
Mime-Version: 1.0
Content-Type: Text/Plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Cc: netdev@vger.kernel.org, borisp@mellanox.com, aviadye@mellanox.com,
        davejwatson@fb.com
To: vakul.garg@nxp.com
Return-path: <netdev-owner@vger.kernel.org>
Received: from shards.monkeyblade.net ([23.128.96.9]:55522 "EHLO
        shards.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1730260AbeGYVl5 (ORCPT
        <rfc822;netdev@vger.kernel.org>); Wed, 25 Jul 2018 17:41:57 -0400
In-Reply-To: <20180723153006.24601-1-vakul.garg@nxp.com>
Sender: netdev-owner@vger.kernel.org
List-ID: <netdev.vger.kernel.org>

From: Vakul Garg <vakul.garg@nxp.com>
Date: Mon, 23 Jul 2018 21:00:06 +0530

> @@ -787,7 +787,7 @@ int tls_sw_recvmsg(struct sock *sk,
>  	target = sock_rcvlowat(sk, flags & MSG_WAITALL, len);
>  	timeo = sock_rcvtimeo(sk, flags & MSG_DONTWAIT);
>  	do {
> -		bool zc = false;
> +		bool zc;
>  		int chunk = 0;
>  
>  		skb = tls_wait_data(sk, flags, timeo, &err);
 ...
> @@ -836,6 +835,7 @@ int tls_sw_recvmsg(struct sock *sk,
>  				if (err < 0)
>  					goto fallback_to_reg_recv;
>  
> +				zc = true;
>  				err = decrypt_skb_update(sk, skb, sgin, &zc);
>  				for (; pages > 0; pages--)
>  					put_page(sg_page(&sgin[pages]));
> @@ -845,6 +845,7 @@ int tls_sw_recvmsg(struct sock *sk,
>  				}
>  			} else {
>  fallback_to_reg_recv:
> +				zc = false;
>  				err = decrypt_skb_update(sk, skb, NULL, &zc);
>  				if (err < 0) {
>  					tls_err_abort(sk, EBADMSG);
> -- 
> 2.13.6
> 

This will leave a code path where 'zc' is evaluated but not initialized to
any value.

And that's the path taken when ctx->decrypted is true.  The code after
your changes looks like:

		bool zc;
 ...
		if (!ctx->decrypted) {

 ... assignments to 'zc' happen in this code block

			ctx->decrypted = true;
		}

		if (!zc) {

So when ctx->decrypted it true, the if(!zc) condition runs on an
uninitialized value.

I have to say that your TLS changes are becomming quite a time sink
for two reasons.

First, you are making a lot of changes that seem not so needed, and
whose value is purely determined by taste.  I'd put the
msg_data_left() multiple evaluation patch into this category.

The rest require deep review and understanding of the complicated
details of the TLS code, and many of them turn out to be incorrect.

As I find more errors in your submissions, I begin to scrutinize your
patches even more.  Thus, review of your changes takes even more time.

And it isn't helping that there are not a lot of other developers
helping actively to review your changes.

I would like to just make a small request to you, that you concentrate
on fixing clear bugs and clear issues that need to be resolved.

Thank you.