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:bor...@mellanox.com] > Sent: Thursday, July 12, 2018 4:11 PM > To: Vakul Garg <vakul.g...@nxp.com>; da...@davemloft.net; > davejwat...@fb.com; netdev@vger.kernel.org > Cc: avia...@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.g...@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); > > > >
