[PATCH net-next] net/tls: Corrected enabling of zero-copy mode

[PATCH net-next] net/tls: Corrected enabling of zero-copy mode

[Vakul Garg]

Zero-copy mode was left enabled even when zerocopy_from_iter() failed.
In this case, control falls back to regular non zero-copy mode. This
caused skipping call of datagram copy function skb_copy_datagram_msg()
and required an extra iteration of record decryption loop for decrypted
record to be copied into user space provided buffer. Hence zero-copy
mode should be enabled/disabled as per the success/failure of
zerocopy_from_iter().

Fixes: c46234ebb4d1 ("tls: RX path for ktls")
Signed-off-by: Vakul Garg <vakul.garg@nxp.com>
---

The patch does not need to be applied to 'net' branch as it does not fix
any functional bug. The patch achieves better run-time efficiency and
code readability.

 net/tls/tls_sw.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index 0c2d029c9d4c..9ae57bec4927 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -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);
@@ -824,7 +824,6 @@ int tls_sw_recvmsg(struct sock *sk,
 				struct scatterlist sgin[MAX_SKB_FRAGS + 1];
 				int pages = 0;
 
-				zc = true;
 				sg_init_table(sgin, MAX_SKB_FRAGS + 1);
 				sg_set_buf(&sgin[0], ctx->rx_aad_plaintext,
 					   TLS_AAD_SPACE_SIZE);
@@ -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

Re: [PATCH net-next] net/tls: Corrected enabling of zero-copy mode

[David Miller]

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.

RE: [PATCH net-next] net/tls: Corrected enabling of zero-copy mode

[Vakul Garg]

> -----Original Message-----
> From: netdev-owner@vger.kernel.org [mailto:netdev-
> owner@vger.kernel.org] On Behalf Of David Miller
> Sent: Thursday, July 26, 2018 1:59 AM
> To: Vakul Garg <vakul.garg@nxp.com>
> Cc: netdev@vger.kernel.org; borisp@mellanox.com;
> aviadye@mellanox.com; davejwatson@fb.com
> Subject: Re: [PATCH net-next] net/tls: Corrected enabling of zero-copy mode
> 
> 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.

My apologies for sending incorrect patches. I would be more careful next time. 


> 
> 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.