Re: [net-next v3 1/5] net/tls: Do not enable zero-copy prematurely

Re: [net-next v3 1/5] net/tls: Do not enable zero-copy prematurely

[Boris Pismenny]

Hi Vakul,

On 7/19/2018 7:16 AM, Vakul Garg wrote:
> Zero-copy mode was left enabled even when zerocopy_from_iter() failed.
> Set the zero-copy mode only when zerocopy_from_iter() succeeds. This
> leads to removal of argument 'zc' of function decrypt_skb_update().
> Function decrypt_skb_update() does not need to check whether
> ctx->decrypted is set since it is never called if ctx->decrypted is
> true.
> 

This patch breaks our tls_device code for the following 2 reasons:
1. We need to disable zerocopy if the device decrypted the record, 
because decrypted data has to be copied to user buffers.
2. ctx->decrypted must be checked in decrypt_skb_update, because it 
might change after calling tls_device_decrypted.

RE: [net-next v3 1/5] net/tls: Do not enable zero-copy prematurely

[Vakul Garg]

Thanks for the comment.
I will take this patch out of the series.

> -----Original Message-----
> From: Boris Pismenny [mailto:borisp@mellanox.com]
> Sent: Thursday, July 19, 2018 3:58 PM
> To: Vakul Garg <vakul.garg@nxp.com>; netdev@vger.kernel.org
> Cc: aviadye@mellanox.com; davejwatson@fb.com; davem@davemloft.net
> Subject: Re: [net-next v3 1/5] net/tls: Do not enable zero-copy prematurely
> 
> Hi Vakul,
> 
> On 7/19/2018 7:16 AM, Vakul Garg wrote:
> > Zero-copy mode was left enabled even when zerocopy_from_iter() failed.
> > Set the zero-copy mode only when zerocopy_from_iter() succeeds. This
> > leads to removal of argument 'zc' of function decrypt_skb_update().
> > Function decrypt_skb_update() does not need to check whether
> > ctx->decrypted is set since it is never called if ctx->decrypted is
> > true.
> >
> 
> This patch breaks our tls_device code for the following 2 reasons:
> 1. We need to disable zerocopy if the device decrypted the record, because
> decrypted data has to be copied to user buffers.
> 2. ctx->decrypted must be checked in decrypt_skb_update, because it might
> change after calling tls_device_decrypted.

[net-next v3 0/5] net/tls: Minor code cleanup patches

[Vakul Garg]

This patch series improves tls_sw.c code by:

1) Removing redundant function decrypt_skb_update() argument.
2) Using correct socket callback for flagging data availability.
3) Removing redundant variable assignments and wakeup callbacks.
4) Removing redundant dynamic array allocation.
5) Using common error checking code for zero-copy, non zero-copy modes.

The patches do not fix any functional bug. Hence "Fixes:" tag has not
been used.

Vakul Garg (5):
  net/tls: Do not enable zero-copy prematurely
  net/tls: Use socket data_ready callback on record availability
  net/tls: Remove redundant variable assignments and wakeup
  net/tls: Remove redundant array allocation.
  net/tls: Rework error checking after decrypt_skb_update()

 net/tls/tls_sw.c | 45 ++++++++++++++++-----------------------------
 1 file changed, 16 insertions(+), 29 deletions(-)

-- 
2.13.6

[net-next v3 1/5] net/tls: Do not enable zero-copy prematurely

[Vakul Garg]

Zero-copy mode was left enabled even when zerocopy_from_iter() failed.
Set the zero-copy mode only when zerocopy_from_iter() succeeds. This
leads to removal of argument 'zc' of function decrypt_skb_update().
Function decrypt_skb_update() does not need to check whether
ctx->decrypted is set since it is never called if ctx->decrypted is
true.

Signed-off-by: Vakul Garg <vakul.garg@nxp.com>
---
 net/tls/tls_sw.c | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index 7d194c0cd6cf..e94cb54a6994 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -656,7 +656,7 @@ static struct sk_buff *tls_wait_data(struct sock *sk, int flags,
 }
 
 static int decrypt_skb_update(struct sock *sk, struct sk_buff *skb,
-			      struct scatterlist *sgout, bool *zc)
+			      struct scatterlist *sgout)
 {
 	struct tls_context *tls_ctx = tls_get_ctx(sk);
 	struct tls_sw_context_rx *ctx = tls_sw_ctx_rx(tls_ctx);
@@ -668,13 +668,9 @@ static int decrypt_skb_update(struct sock *sk, struct sk_buff *skb,
 	if (err < 0)
 		return err;
 #endif
-	if (!ctx->decrypted) {
-		err = decrypt_skb(sk, skb, sgout);
-		if (err < 0)
-			return err;
-	} else {
-		*zc = false;
-	}
+	err = decrypt_skb(sk, skb, sgout);
+	if (err < 0)
+		return err;
 
 	rxm->offset += tls_ctx->rx.prepend_size;
 	rxm->full_len -= tls_ctx->rx.overhead_size;
@@ -819,7 +815,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);
@@ -830,8 +825,10 @@ int tls_sw_recvmsg(struct sock *sk,
 							 MAX_SKB_FRAGS,	false, true);
 				if (err < 0)
 					goto fallback_to_reg_recv;
+				else
+					zc = true;
 
-				err = decrypt_skb_update(sk, skb, sgin, &zc);
+				err = decrypt_skb_update(sk, skb, sgin);
 				for (; pages > 0; pages--)
 					put_page(sg_page(&sgin[pages]));
 				if (err < 0) {
@@ -840,7 +837,7 @@ int tls_sw_recvmsg(struct sock *sk,
 				}
 			} else {
 fallback_to_reg_recv:
-				err = decrypt_skb_update(sk, skb, NULL, &zc);
+				err = decrypt_skb_update(sk, skb, NULL);
 				if (err < 0) {
 					tls_err_abort(sk, EBADMSG);
 					goto recv_end;
@@ -895,7 +892,6 @@ ssize_t tls_sw_splice_read(struct socket *sock,  loff_t *ppos,
 	int err = 0;
 	long timeo;
 	int chunk;
-	bool zc;
 
 	lock_sock(sk);
 
@@ -912,7 +908,7 @@ ssize_t tls_sw_splice_read(struct socket *sock,  loff_t *ppos,
 	}
 
 	if (!ctx->decrypted) {
-		err = decrypt_skb_update(sk, skb, NULL, &zc);
+		err = decrypt_skb_update(sk, skb, NULL);
 
 		if (err < 0) {
 			tls_err_abort(sk, EBADMSG);
-- 
2.13.6

[net-next v3 2/5] net/tls: Use socket data_ready callback on record availability

[Vakul Garg]

On receipt of a complete tls record, use socket's saved data_ready
callback instead of state_change callback.

Signed-off-by: Vakul Garg <vakul.garg@nxp.com>
---
 net/tls/tls_sw.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index e94cb54a6994..186152dced25 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -1019,7 +1019,7 @@ static void tls_queue(struct strparser *strp, struct sk_buff *skb)
 	ctx->recv_pkt = skb;
 	strp_pause(strp);
 
-	strp->sk->sk_state_change(strp->sk);
+	ctx->saved_data_ready(strp->sk);
 }
 
 static void tls_data_ready(struct sock *sk)
-- 
2.13.6

[net-next v3 3/5] net/tls: Remove redundant variable assignments and wakeup

[Vakul Garg]

In function decrypt_skb_update(), the assignment to tls receive context
variable 'decrypted' is redundant as the same is being done in function
tls_sw_recvmsg() after calling decrypt_skb_update(). Also calling callback
function to wakeup processes sleeping on socket data availability is
useless as decrypt_skb_update() is invoked from user processes only. This
patch cleans these up.

Signed-off-by: Vakul Garg <vakul.garg@nxp.com>
---

v2 -> v3
Removed compilation warning.

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

diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index 186152dced25..5dcfbaf33680 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -659,7 +659,6 @@ static int decrypt_skb_update(struct sock *sk, struct sk_buff *skb,
 			      struct scatterlist *sgout)
 {
 	struct tls_context *tls_ctx = tls_get_ctx(sk);
-	struct tls_sw_context_rx *ctx = tls_sw_ctx_rx(tls_ctx);
 	struct strp_msg *rxm = strp_msg(skb);
 	int err = 0;
 
@@ -675,8 +674,6 @@ static int decrypt_skb_update(struct sock *sk, struct sk_buff *skb,
 	rxm->offset += tls_ctx->rx.prepend_size;
 	rxm->full_len -= tls_ctx->rx.overhead_size;
 	tls_advance_record_sn(sk, &tls_ctx->rx);
-	ctx->decrypted = true;
-	ctx->saved_data_ready(sk);
 
 	return err;
 }
-- 
2.13.6

[net-next v3 4/5] net/tls: Remove redundant array allocation.

[Vakul Garg]

In function decrypt_skb(), array allocation in case when sgout is NULL
is unnecessary. Instead, local variable sgin_arr[] can be used.

Signed-off-by: Vakul Garg <vakul.garg@nxp.com>
---
 net/tls/tls_sw.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index 5dcfbaf33680..ae0b40d6671b 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -699,7 +699,6 @@ int decrypt_skb(struct sock *sk, struct sk_buff *skb,
 	memcpy(iv, tls_ctx->rx.iv, TLS_CIPHER_AES_GCM_128_SALT_SIZE);
 	if (!sgout) {
 		nsg = skb_cow_data(skb, 0, &unused) + 1;
-		sgin = kmalloc_array(nsg, sizeof(*sgin), sk->sk_allocation);
 		sgout = sgin;
 	}
 
@@ -720,9 +719,6 @@ int decrypt_skb(struct sock *sk, struct sk_buff *skb,
 				rxm->full_len - tls_ctx->rx.overhead_size,
 				skb, sk->sk_allocation);
 
-	if (sgin != &sgin_arr[0])
-		kfree(sgin);
-
 	return ret;
 }
 
-- 
2.13.6

[net-next v3 5/5] net/tls: Rework error checking after decrypt_skb_update()

[Vakul Garg]

Error checking code after invoking decrypt_skb_update() for zero-copy
and non-zero-copy cases in tls_sw_recvmsg has been made common.

Signed-off-by: Vakul Garg <vakul.garg@nxp.com>
---
 net/tls/tls_sw.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index ae0b40d6671b..da3b884bea93 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -824,18 +824,16 @@ int tls_sw_recvmsg(struct sock *sk,
 				err = decrypt_skb_update(sk, skb, sgin);
 				for (; pages > 0; pages--)
 					put_page(sg_page(&sgin[pages]));
-				if (err < 0) {
-					tls_err_abort(sk, EBADMSG);
-					goto recv_end;
-				}
 			} else {
 fallback_to_reg_recv:
 				err = decrypt_skb_update(sk, skb, NULL);
-				if (err < 0) {
-					tls_err_abort(sk, EBADMSG);
-					goto recv_end;
-				}
 			}
+
+			if (err < 0) {
+				tls_err_abort(sk, EBADMSG);
+				goto recv_end;
+			}
+
 			ctx->decrypted = true;
 		}
 
-- 
2.13.6