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

Message ID 20180712110350.25011-1-vakul.garg@nxp.com
State Changes Requested
Delegated to: David Miller
Headers show
Series
  • [net-next] net/tls: Removed redundant variable from 'struct tls_sw_context_rx'
Related show

Commit Message

Vakul Garg July 12, 2018, 11:03 a.m.
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 <vakul.garg@nxp.com>
---
 include/net/tls.h |  1 -
 net/tls/tls_sw.c  | 87 ++++++++++++++++++++++++-------------------------------
 2 files changed, 38 insertions(+), 50 deletions(-)

Comments

Boris Pismenny July 12, 2018, 10:41 a.m. | #1
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.

> Signed-off-by: Vakul Garg <vakul.garg@nxp.com>
> ---
>   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);
>   
>
Vakul Garg July 12, 2018, 11:14 a.m. | #2
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 <vakul.garg@nxp.com>; 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:
> > 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.




> 
> > Signed-off-by: Vakul Garg <vakul.garg@nxp.com>
> > ---
> >   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);
> >
> >
Dave Watson July 12, 2018, 2:31 p.m. | #3
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.

Patch

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);