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

Subsystems: networking [general], networking [tls], the rest

6 messages, 2 authors, 2018-07-18 · open the first message on its own page

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

From: Vakul Garg <hidden>
Date: 2018-07-16 11:04:29

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 <redacted>
---
 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 v2 2/5] net/tls: Use socket data_ready callback on record availability

From: Vakul Garg <hidden>
Date: 2018-07-16 11:04:32

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

Signed-off-by: Vakul Garg <redacted>
---
 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 v2 3/5] net/tls: Remove redundant variable assignments and wakeup

From: Vakul Garg <hidden>
Date: 2018-07-16 11:04:36

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 <redacted>
---
 net/tls/tls_sw.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index 186152dced25..6eaaa587db71 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c

@@ -675,8 +675,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 v2 4/5] net/tls: Remove redundant array allocation.

From: Vakul Garg <hidden>
Date: 2018-07-16 11:04:38

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 <redacted>
---
 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 6eaaa587db71..55960088baea 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c

@@ -700,7 +700,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;
 	}

@@ -721,9 +720,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 v2 5/5] net/tls: Rework error checking after decrypt_skb_update()

From: Vakul Garg <hidden>
Date: 2018-07-16 11:04:40

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 <redacted>
---
 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 55960088baea..0e915d4a30bc 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c

@@ -825,18 +825,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

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

From: David Miller <davem@davemloft.net>
Date: 2018-07-18 21:02:57

From: Vakul Garg <redacted>
Date: Mon, 16 Jul 2018 21:27:11 +0530

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

This adds a warning:

net/tls/tls_sw.c: In function ‘decrypt_skb_update’:
net/tls/tls_sw.c:662:28: warning: unused variable ‘ctx’ [-Wunused-variable]
  struct tls_sw_context_rx *ctx = tls_sw_ctx_rx(tls_ctx);

Also, you must submit a patch series with a proper cover letter
"[net-next vX 0/N] net/tls: ..." explaining what the patch series
is doing, how it is doing it, and why it doing it that way.

You should also provide appropriate "Fixes: " tags to your
patches.

Thank you.

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help