Re: [PATCH net-next] net/tls: Removed redundant variable from 'struct tls_sw_context_rx'
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.
RE: [PATCH net-next] net/tls: Removed redundant variable from 'struct tls_sw_context_rx'
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 ; 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
> > ---
> > 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, _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_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([0], ctx->rx_aad_plaintext,
> > - TLS_AAD_SPACE_SIZE);
> > -
> > - err = zerocopy_from_iter(sk,
> >msg_iter,
> > -to_copy, ,
> > -, [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([pages]));
> > - if (err < 0) {
> > - tls_err_abo
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
---
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, _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_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([0], ctx->rx_aad_plaintext,
- TLS_AAD_SPACE_SIZE);
-
- err = zerocopy_from_iter(sk, >msg_iter,
-to_copy, ,
-, [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([pages]));
- if (err < 0) {
- tls_err_abort(sk, EBADMSG);
- goto recv_end;
- }
- } else {
+ page_count = iov_iter_npages(>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([0], ctx->rx_aad_plaintext,
+ TLS_AAD_SPACE_SIZE);
+ err = zerocopy_from_iter(sk, >msg_iter, to_copy,
+, , [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([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;
- }
+
