[PATCH net-next] net/tls: Removed redundant variable from 'struct tls_sw_context_rx'

The variable 'decrypted' in 'struct tls_sw_context_rx' is redundant and
is being set/unset without purpose. Simplified the code by removing it.

Signed-off-by: Vakul Garg <redacted>
---
 include/net/tls.h |  1 -
 net/tls/tls_sw.c  | 87 ++++++++++++++++++++++++-------------------------------
 2 files changed, 38 insertions(+), 50 deletions(-)

diff --git a/include/net/tls.h b/include/net/tls.h
index 70c273777fe9..528d0c2d6cc2 100644
--- a/include/net/tls.h
+++ b/include/net/tls.h

@@ -113,7 +113,6 @@ struct tls_sw_context_rx {
 				struct poll_table_struct *wait);
 	struct sk_buff *recv_pkt;
 	u8 control;
-	bool decrypted;
 
 	char rx_aad_ciphertext[TLS_AAD_SPACE_SIZE];
 	char rx_aad_plaintext[TLS_AAD_SPACE_SIZE];

diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index 0d670c8adf18..e5f2de2c3fd6 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c

@@ -81,8 +81,6 @@ static int tls_do_decryption(struct sock *sk,
 	rxm->full_len -= tls_ctx->rx.overhead_size;
 	tls_advance_record_sn(sk, &tls_ctx->rx);
 
-	ctx->decrypted = true;
-
 	ctx->saved_data_ready(sk);
 
 out:

@@ -756,6 +754,9 @@ int tls_sw_recvmsg(struct sock *sk,
 	bool cmsg = false;
 	int target, err = 0;
 	long timeo;
+	int page_count;
+	int to_copy;
+
 
 	flags |= nonblock;
 

@@ -792,46 +793,38 @@ int tls_sw_recvmsg(struct sock *sk,
 			goto recv_end;
 		}
 
-		if (!ctx->decrypted) {
-			int page_count;
-			int to_copy;
-
-			page_count = iov_iter_npages(&msg->msg_iter,
-						     MAX_SKB_FRAGS);
-			to_copy = rxm->full_len - tls_ctx->rx.overhead_size;
-			if (to_copy <= len && page_count < MAX_SKB_FRAGS &&
-			    likely(!(flags & MSG_PEEK)))  {
-				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);
-
-				err = zerocopy_from_iter(sk, &msg->msg_iter,
-							 to_copy, &pages,
-							 &chunk, &sgin[1],
-							 MAX_SKB_FRAGS,	false);
-				if (err < 0)
-					goto fallback_to_reg_recv;
-
-				err = decrypt_skb(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 {
+		page_count = iov_iter_npages(&msg->msg_iter, MAX_SKB_FRAGS);
+		to_copy = rxm->full_len - tls_ctx->rx.overhead_size;
+
+		if (to_copy <= len && page_count < MAX_SKB_FRAGS &&
+		    likely(!(flags & MSG_PEEK)))  {
+			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);
+			err = zerocopy_from_iter(sk, &msg->msg_iter, to_copy,
+						 &pages, &chunk, &sgin[1],
+						 MAX_SKB_FRAGS, false);
+			if (err < 0)
+				goto fallback_to_reg_recv;
+
+			err = decrypt_skb(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(sk, skb, NULL);
-				if (err < 0) {
-					tls_err_abort(sk, EBADMSG);
-					goto recv_end;
-				}
+			err = decrypt_skb(sk, skb, NULL);
+			if (err < 0) {
+				tls_err_abort(sk, EBADMSG);
+				goto recv_end;
 			}
-			ctx->decrypted = true;
 		}
 
 		if (!zc) {

@@ -895,15 +888,13 @@ ssize_t tls_sw_splice_read(struct socket *sock,  loff_t *ppos,
 		goto splice_read_end;
 	}
 
-	if (!ctx->decrypted) {
-		err = decrypt_skb(sk, skb, NULL);
+	err = decrypt_skb(sk, skb, NULL);
 
-		if (err < 0) {
-			tls_err_abort(sk, EBADMSG);
-			goto splice_read_end;
-		}
-		ctx->decrypted = true;
+	if (err < 0) {
+		tls_err_abort(sk, EBADMSG);
+		goto splice_read_end;
 	}
+
 	rxm = strp_msg(skb);
 
 	chunk = min_t(unsigned int, rxm->full_len, len);

@@ -998,8 +989,6 @@ static void tls_queue(struct strparser *strp, struct sk_buff *skb)
 	struct tls_context *tls_ctx = tls_get_ctx(strp->sk);
 	struct tls_sw_context_rx *ctx = tls_sw_ctx_rx(tls_ctx);
 
-	ctx->decrypted = false;
-
 	ctx->recv_pkt = skb;
 	strp_pause(strp);
 

-- 
2.13.6

Hi Vakul,

On 7/12/2018 7:03 AM, Vakul Garg wrote:

The variable 'decrypted' in 'struct tls_sw_context_rx' is redundant and
is being set/unset without purpose. Simplified the code by removing it.

AFAIU, this variable has an important use here. It keeps the state 
whether the current record has been decrypted between invocations of the 
recv/splice system calls. Otherwise, some records would be decrypted 
more than once if the entire record was not read.

Hi Boris

Thanks for explaining.
Few questions/observations.

1. Isn't ' ctx->decrypted = true' a redundant statement in tls_do_decryption()?
The same has been repeated in tls_recvmsg() after calling decrypt_skb()?

2. Similarly, ctx->saved_data_ready(sk) seems not required in tls_do_decryption().
This is because tls_do_decryption() is already triggered from tls_recvmsg() i.e. from user space app context.

3. In tls_queue(), I think strp->sk->sk_state_change() needs to be replaced with ctx->saved_data_ready().

Regards

Vakul

-----Original Message-----
From: Boris Pismenny [mailto:borisp@mellanox.com]
Sent: Thursday, July 12, 2018 4:11 PM
To: Vakul Garg <redacted>; davem@davemloft.net;
davejwatson@fb.com; netdev@vger.kernel.org
Cc: aviadye@mellanox.com
Subject: Re: [PATCH net-next] net/tls: Removed redundant variable from
'struct tls_sw_context_rx'

Hi Vakul,

On 7/12/2018 7:03 AM, Vakul Garg wrote:

quoted

The variable 'decrypted' in 'struct tls_sw_context_rx' is redundant
and is being set/unset without purpose. Simplified the code by removing it.

AFAIU, this variable has an important use here. It keeps the state whether
the current record has been decrypted between invocations of the
recv/splice system calls. Otherwise, some records would be decrypted more
than once if the entire record was not read.

quoted

Signed-off-by: Vakul Garg <redacted>
---
  include/net/tls.h |  1 -
  net/tls/tls_sw.c  | 87 ++++++++++++++++++++++++----------------------------

---

quoted

  2 files changed, 38 insertions(+), 50 deletions(-)

diff --git a/include/net/tls.h b/include/net/tls.h index

70c273777fe9..528d0c2d6cc2 100644

--- a/include/net/tls.h
+++ b/include/net/tls.h

@@ -113,7 +113,6 @@ struct tls_sw_context_rx {
  				struct poll_table_struct *wait);
  	struct sk_buff *recv_pkt;
  	u8 control;
-	bool decrypted;

  	char rx_aad_ciphertext[TLS_AAD_SPACE_SIZE];
  	char rx_aad_plaintext[TLS_AAD_SPACE_SIZE];

diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c index

0d670c8adf18..e5f2de2c3fd6 100644

--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c

@@ -81,8 +81,6 @@ static int tls_do_decryption(struct sock *sk,
  	rxm->full_len -= tls_ctx->rx.overhead_size;
  	tls_advance_record_sn(sk, &tls_ctx->rx);

-	ctx->decrypted = true;
-
  	ctx->saved_data_ready(sk);

  out:

@@ -756,6 +754,9 @@ int tls_sw_recvmsg(struct sock *sk,
  	bool cmsg = false;
  	int target, err = 0;
  	long timeo;
+	int page_count;
+	int to_copy;
+

  	flags |= nonblock;

@@ -792,46 +793,38 @@ int tls_sw_recvmsg(struct sock *sk,
  			goto recv_end;
  		}

-		if (!ctx->decrypted) {
-			int page_count;
-			int to_copy;
-
-			page_count = iov_iter_npages(&msg->msg_iter,
-						     MAX_SKB_FRAGS);
-			to_copy = rxm->full_len - tls_ctx->rx.overhead_size;
-			if (to_copy <= len && page_count < MAX_SKB_FRAGS

&&

quoted

-			    likely(!(flags & MSG_PEEK)))  {
-				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);
-
-				err = zerocopy_from_iter(sk, &msg-
msg_iter,
-							 to_copy, &pages,
-							 &chunk, &sgin[1],
-							 MAX_SKB_FRAGS,

	false);

quoted

-				if (err < 0)
-					goto fallback_to_reg_recv;
-
-				err = decrypt_skb(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 {
+		page_count = iov_iter_npages(&msg->msg_iter,

MAX_SKB_FRAGS);

quoted

+		to_copy = rxm->full_len - tls_ctx->rx.overhead_size;
+
+		if (to_copy <= len && page_count < MAX_SKB_FRAGS &&
+		    likely(!(flags & MSG_PEEK)))  {
+			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);
+			err = zerocopy_from_iter(sk, &msg->msg_iter,

to_copy,

quoted

+						 &pages, &chunk, &sgin[1],
+						 MAX_SKB_FRAGS, false);
+			if (err < 0)
+				goto fallback_to_reg_recv;
+
+			err = decrypt_skb(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(sk, skb, NULL);
-				if (err < 0) {
-					tls_err_abort(sk, EBADMSG);
-					goto recv_end;
-				}
+			err = decrypt_skb(sk, skb, NULL);
+			if (err < 0) {
+				tls_err_abort(sk, EBADMSG);
+				goto recv_end;
  			}
-			ctx->decrypted = true;
  		}

  		if (!zc) {

@@ -895,15 +888,13 @@ ssize_t tls_sw_splice_read(struct socket *sock,

loff_t *ppos,

quoted

  		goto splice_read_end;
  	}

-	if (!ctx->decrypted) {
-		err = decrypt_skb(sk, skb, NULL);
+	err = decrypt_skb(sk, skb, NULL);

-		if (err < 0) {
-			tls_err_abort(sk, EBADMSG);
-			goto splice_read_end;
-		}
-		ctx->decrypted = true;
+	if (err < 0) {
+		tls_err_abort(sk, EBADMSG);
+		goto splice_read_end;
  	}
+
  	rxm = strp_msg(skb);

  	chunk = min_t(unsigned int, rxm->full_len, len); @@ -998,8 +989,6

@@ static void tls_queue(struct strparser *strp, struct sk_buff *skb)
  	struct tls_context *tls_ctx = tls_get_ctx(strp->sk);
  	struct tls_sw_context_rx *ctx = tls_sw_ctx_rx(tls_ctx);

-	ctx->decrypted = false;
-
  	ctx->recv_pkt = skb;
  	strp_pause(strp);

On 07/12/18 11:14 AM, Vakul Garg wrote:

Hi Boris

Thanks for explaining.
Few questions/observations.

1. Isn't ' ctx->decrypted = true' a redundant statement in tls_do_decryption()?
The same has been repeated in tls_recvmsg() after calling decrypt_skb()?

2. Similarly, ctx->saved_data_ready(sk) seems not required in tls_do_decryption().
This is because tls_do_decryption() is already triggered from tls_recvmsg() i.e. from user space app context.

3. In tls_queue(), I think strp->sk->sk_state_change() needs to be replaced with ctx->saved_data_ready().

Yes, I think these 3 can all be changed.  #2 would be required if
do_decryption ever is called not in user context, but that's not the
case currently.