From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Pismenny Subject: Re: [PATCH net-next] net/tls: Removed redundant variable from 'struct tls_sw_context_rx' Date: Thu, 12 Jul 2018 06:41:00 -0400 Message-ID: <3e1dfaea-8895-69fc-476f-b80124713652@mellanox.com> References: <20180712110350.25011-1-vakul.garg@nxp.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Cc: aviadye@mellanox.com To: Vakul Garg , davem@davemloft.net, davejwatson@fb.com, netdev@vger.kernel.org Return-path: Received: from mail-db5eur01on0059.outbound.protection.outlook.com ([104.47.2.59]:63024 "EHLO EUR01-DB5-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1726476AbeGLKuU (ORCPT ); Thu, 12 Jul 2018 06:50:20 -0400 In-Reply-To: <20180712110350.25011-1-vakul.garg@nxp.com> Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: 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 > --- > 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); > >