* Re: [net-next v3 1/5] net/tls: Do not enable zero-copy prematurely
2018-07-19 11:16 ` [net-next v3 1/5] net/tls: Do not enable zero-copy prematurely Vakul Garg
@ 2018-07-19 10:28 ` Boris Pismenny
2018-07-19 10:32 ` Vakul Garg
0 siblings, 1 reply; 8+ messages in thread
From: Boris Pismenny @ 2018-07-19 10:28 UTC (permalink / raw)
To: Vakul Garg, netdev; +Cc: aviadye, davejwatson, davem
Hi Vakul,
On 7/19/2018 7:16 AM, Vakul Garg wrote:
> Zero-copy mode was left enabled even when zerocopy_from_iter() failed.
> Set the zero-copy mode only when zerocopy_from_iter() succeeds. This
> leads to removal of argument 'zc' of function decrypt_skb_update().
> Function decrypt_skb_update() does not need to check whether
> ctx->decrypted is set since it is never called if ctx->decrypted is
> true.
>
This patch breaks our tls_device code for the following 2 reasons:
1. We need to disable zerocopy if the device decrypted the record,
because decrypted data has to be copied to user buffers.
2. ctx->decrypted must be checked in decrypt_skb_update, because it
might change after calling tls_device_decrypted.
^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [net-next v3 1/5] net/tls: Do not enable zero-copy prematurely
2018-07-19 10:28 ` Boris Pismenny
@ 2018-07-19 10:32 ` Vakul Garg
0 siblings, 0 replies; 8+ messages in thread
From: Vakul Garg @ 2018-07-19 10:32 UTC (permalink / raw)
To: Boris Pismenny, netdev; +Cc: aviadye, davejwatson, davem
Thanks for the comment.
I will take this patch out of the series.
> -----Original Message-----
> From: Boris Pismenny [mailto:borisp@mellanox.com]
> Sent: Thursday, July 19, 2018 3:58 PM
> To: Vakul Garg <vakul.garg@nxp.com>; netdev@vger.kernel.org
> Cc: aviadye@mellanox.com; davejwatson@fb.com; davem@davemloft.net
> Subject: Re: [net-next v3 1/5] net/tls: Do not enable zero-copy prematurely
>
> Hi Vakul,
>
> On 7/19/2018 7:16 AM, Vakul Garg wrote:
> > Zero-copy mode was left enabled even when zerocopy_from_iter() failed.
> > Set the zero-copy mode only when zerocopy_from_iter() succeeds. This
> > leads to removal of argument 'zc' of function decrypt_skb_update().
> > Function decrypt_skb_update() does not need to check whether
> > ctx->decrypted is set since it is never called if ctx->decrypted is
> > true.
> >
>
> This patch breaks our tls_device code for the following 2 reasons:
> 1. We need to disable zerocopy if the device decrypted the record, because
> decrypted data has to be copied to user buffers.
> 2. ctx->decrypted must be checked in decrypt_skb_update, because it might
> change after calling tls_device_decrypted.
^ permalink raw reply [flat|nested] 8+ messages in thread
* [net-next v3 0/5] net/tls: Minor code cleanup patches
@ 2018-07-19 11:16 Vakul Garg
2018-07-19 11:16 ` [net-next v3 1/5] net/tls: Do not enable zero-copy prematurely Vakul Garg
` (4 more replies)
0 siblings, 5 replies; 8+ messages in thread
From: Vakul Garg @ 2018-07-19 11:16 UTC (permalink / raw)
To: netdev; +Cc: borisp, aviadye, davejwatson, davem, Vakul Garg
This patch series improves tls_sw.c code by:
1) Removing redundant function decrypt_skb_update() argument.
2) Using correct socket callback for flagging data availability.
3) Removing redundant variable assignments and wakeup callbacks.
4) Removing redundant dynamic array allocation.
5) Using common error checking code for zero-copy, non zero-copy modes.
The patches do not fix any functional bug. Hence "Fixes:" tag has not
been used.
Vakul Garg (5):
net/tls: Do not enable zero-copy prematurely
net/tls: Use socket data_ready callback on record availability
net/tls: Remove redundant variable assignments and wakeup
net/tls: Remove redundant array allocation.
net/tls: Rework error checking after decrypt_skb_update()
net/tls/tls_sw.c | 45 ++++++++++++++++-----------------------------
1 file changed, 16 insertions(+), 29 deletions(-)
--
2.13.6
^ permalink raw reply [flat|nested] 8+ messages in thread
* [net-next v3 1/5] net/tls: Do not enable zero-copy prematurely
2018-07-19 11:16 [net-next v3 0/5] net/tls: Minor code cleanup patches Vakul Garg
@ 2018-07-19 11:16 ` Vakul Garg
2018-07-19 10:28 ` Boris Pismenny
2018-07-19 11:16 ` [net-next v3 2/5] net/tls: Use socket data_ready callback on record availability Vakul Garg
` (3 subsequent siblings)
4 siblings, 1 reply; 8+ messages in thread
From: Vakul Garg @ 2018-07-19 11:16 UTC (permalink / raw)
To: netdev; +Cc: borisp, aviadye, davejwatson, davem, Vakul Garg
Zero-copy mode was left enabled even when zerocopy_from_iter() failed.
Set the zero-copy mode only when zerocopy_from_iter() succeeds. This
leads to removal of argument 'zc' of function decrypt_skb_update().
Function decrypt_skb_update() does not need to check whether
ctx->decrypted is set since it is never called if ctx->decrypted is
true.
Signed-off-by: Vakul Garg <vakul.garg@nxp.com>
---
net/tls/tls_sw.c | 22 +++++++++-------------
1 file changed, 9 insertions(+), 13 deletions(-)
diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index 7d194c0cd6cf..e94cb54a6994 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -656,7 +656,7 @@ static struct sk_buff *tls_wait_data(struct sock *sk, int flags,
}
static int decrypt_skb_update(struct sock *sk, struct sk_buff *skb,
- struct scatterlist *sgout, bool *zc)
+ struct scatterlist *sgout)
{
struct tls_context *tls_ctx = tls_get_ctx(sk);
struct tls_sw_context_rx *ctx = tls_sw_ctx_rx(tls_ctx);
@@ -668,13 +668,9 @@ static int decrypt_skb_update(struct sock *sk, struct sk_buff *skb,
if (err < 0)
return err;
#endif
- if (!ctx->decrypted) {
- err = decrypt_skb(sk, skb, sgout);
- if (err < 0)
- return err;
- } else {
- *zc = false;
- }
+ err = decrypt_skb(sk, skb, sgout);
+ if (err < 0)
+ return err;
rxm->offset += tls_ctx->rx.prepend_size;
rxm->full_len -= tls_ctx->rx.overhead_size;
@@ -819,7 +815,6 @@ int tls_sw_recvmsg(struct sock *sk,
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);
@@ -830,8 +825,10 @@ int tls_sw_recvmsg(struct sock *sk,
MAX_SKB_FRAGS, false, true);
if (err < 0)
goto fallback_to_reg_recv;
+ else
+ zc = true;
- err = decrypt_skb_update(sk, skb, sgin, &zc);
+ err = decrypt_skb_update(sk, skb, sgin);
for (; pages > 0; pages--)
put_page(sg_page(&sgin[pages]));
if (err < 0) {
@@ -840,7 +837,7 @@ int tls_sw_recvmsg(struct sock *sk,
}
} else {
fallback_to_reg_recv:
- err = decrypt_skb_update(sk, skb, NULL, &zc);
+ err = decrypt_skb_update(sk, skb, NULL);
if (err < 0) {
tls_err_abort(sk, EBADMSG);
goto recv_end;
@@ -895,7 +892,6 @@ ssize_t tls_sw_splice_read(struct socket *sock, loff_t *ppos,
int err = 0;
long timeo;
int chunk;
- bool zc;
lock_sock(sk);
@@ -912,7 +908,7 @@ ssize_t tls_sw_splice_read(struct socket *sock, loff_t *ppos,
}
if (!ctx->decrypted) {
- err = decrypt_skb_update(sk, skb, NULL, &zc);
+ err = decrypt_skb_update(sk, skb, NULL);
if (err < 0) {
tls_err_abort(sk, EBADMSG);
--
2.13.6
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [net-next v3 2/5] net/tls: Use socket data_ready callback on record availability
2018-07-19 11:16 [net-next v3 0/5] net/tls: Minor code cleanup patches Vakul Garg
2018-07-19 11:16 ` [net-next v3 1/5] net/tls: Do not enable zero-copy prematurely Vakul Garg
@ 2018-07-19 11:16 ` Vakul Garg
2018-07-19 11:16 ` [net-next v3 3/5] net/tls: Remove redundant variable assignments and wakeup Vakul Garg
` (2 subsequent siblings)
4 siblings, 0 replies; 8+ messages in thread
From: Vakul Garg @ 2018-07-19 11:16 UTC (permalink / raw)
To: netdev; +Cc: borisp, aviadye, davejwatson, davem, Vakul Garg
On receipt of a complete tls record, use socket's saved data_ready
callback instead of state_change callback.
Signed-off-by: Vakul Garg <vakul.garg@nxp.com>
---
net/tls/tls_sw.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index e94cb54a6994..186152dced25 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -1019,7 +1019,7 @@ static void tls_queue(struct strparser *strp, struct sk_buff *skb)
ctx->recv_pkt = skb;
strp_pause(strp);
- strp->sk->sk_state_change(strp->sk);
+ ctx->saved_data_ready(strp->sk);
}
static void tls_data_ready(struct sock *sk)
--
2.13.6
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [net-next v3 3/5] net/tls: Remove redundant variable assignments and wakeup
2018-07-19 11:16 [net-next v3 0/5] net/tls: Minor code cleanup patches Vakul Garg
2018-07-19 11:16 ` [net-next v3 1/5] net/tls: Do not enable zero-copy prematurely Vakul Garg
2018-07-19 11:16 ` [net-next v3 2/5] net/tls: Use socket data_ready callback on record availability Vakul Garg
@ 2018-07-19 11:16 ` Vakul Garg
2018-07-19 11:16 ` [net-next v3 4/5] net/tls: Remove redundant array allocation Vakul Garg
2018-07-19 11:16 ` [net-next v3 5/5] net/tls: Rework error checking after decrypt_skb_update() Vakul Garg
4 siblings, 0 replies; 8+ messages in thread
From: Vakul Garg @ 2018-07-19 11:16 UTC (permalink / raw)
To: netdev; +Cc: borisp, aviadye, davejwatson, davem, Vakul Garg
In function decrypt_skb_update(), the assignment to tls receive context
variable 'decrypted' is redundant as the same is being done in function
tls_sw_recvmsg() after calling decrypt_skb_update(). Also calling callback
function to wakeup processes sleeping on socket data availability is
useless as decrypt_skb_update() is invoked from user processes only. This
patch cleans these up.
Signed-off-by: Vakul Garg <vakul.garg@nxp.com>
---
v2 -> v3
Removed compilation warning.
net/tls/tls_sw.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index 186152dced25..5dcfbaf33680 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -659,7 +659,6 @@ static int decrypt_skb_update(struct sock *sk, struct sk_buff *skb,
struct scatterlist *sgout)
{
struct tls_context *tls_ctx = tls_get_ctx(sk);
- struct tls_sw_context_rx *ctx = tls_sw_ctx_rx(tls_ctx);
struct strp_msg *rxm = strp_msg(skb);
int err = 0;
@@ -675,8 +674,6 @@ static int decrypt_skb_update(struct sock *sk, struct sk_buff *skb,
rxm->offset += tls_ctx->rx.prepend_size;
rxm->full_len -= tls_ctx->rx.overhead_size;
tls_advance_record_sn(sk, &tls_ctx->rx);
- ctx->decrypted = true;
- ctx->saved_data_ready(sk);
return err;
}
--
2.13.6
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [net-next v3 4/5] net/tls: Remove redundant array allocation.
2018-07-19 11:16 [net-next v3 0/5] net/tls: Minor code cleanup patches Vakul Garg
` (2 preceding siblings ...)
2018-07-19 11:16 ` [net-next v3 3/5] net/tls: Remove redundant variable assignments and wakeup Vakul Garg
@ 2018-07-19 11:16 ` Vakul Garg
2018-07-19 11:16 ` [net-next v3 5/5] net/tls: Rework error checking after decrypt_skb_update() Vakul Garg
4 siblings, 0 replies; 8+ messages in thread
From: Vakul Garg @ 2018-07-19 11:16 UTC (permalink / raw)
To: netdev; +Cc: borisp, aviadye, davejwatson, davem, Vakul Garg
In function decrypt_skb(), array allocation in case when sgout is NULL
is unnecessary. Instead, local variable sgin_arr[] can be used.
Signed-off-by: Vakul Garg <vakul.garg@nxp.com>
---
net/tls/tls_sw.c | 4 ----
1 file changed, 4 deletions(-)
diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index 5dcfbaf33680..ae0b40d6671b 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -699,7 +699,6 @@ int decrypt_skb(struct sock *sk, struct sk_buff *skb,
memcpy(iv, tls_ctx->rx.iv, TLS_CIPHER_AES_GCM_128_SALT_SIZE);
if (!sgout) {
nsg = skb_cow_data(skb, 0, &unused) + 1;
- sgin = kmalloc_array(nsg, sizeof(*sgin), sk->sk_allocation);
sgout = sgin;
}
@@ -720,9 +719,6 @@ int decrypt_skb(struct sock *sk, struct sk_buff *skb,
rxm->full_len - tls_ctx->rx.overhead_size,
skb, sk->sk_allocation);
- if (sgin != &sgin_arr[0])
- kfree(sgin);
-
return ret;
}
--
2.13.6
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [net-next v3 5/5] net/tls: Rework error checking after decrypt_skb_update()
2018-07-19 11:16 [net-next v3 0/5] net/tls: Minor code cleanup patches Vakul Garg
` (3 preceding siblings ...)
2018-07-19 11:16 ` [net-next v3 4/5] net/tls: Remove redundant array allocation Vakul Garg
@ 2018-07-19 11:16 ` Vakul Garg
4 siblings, 0 replies; 8+ messages in thread
From: Vakul Garg @ 2018-07-19 11:16 UTC (permalink / raw)
To: netdev; +Cc: borisp, aviadye, davejwatson, davem, Vakul Garg
Error checking code after invoking decrypt_skb_update() for zero-copy
and non-zero-copy cases in tls_sw_recvmsg has been made common.
Signed-off-by: Vakul Garg <vakul.garg@nxp.com>
---
net/tls/tls_sw.c | 14 ++++++--------
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index ae0b40d6671b..da3b884bea93 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -824,18 +824,16 @@ int tls_sw_recvmsg(struct sock *sk,
err = decrypt_skb_update(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_update(sk, skb, NULL);
- if (err < 0) {
- tls_err_abort(sk, EBADMSG);
- goto recv_end;
- }
}
+
+ if (err < 0) {
+ tls_err_abort(sk, EBADMSG);
+ goto recv_end;
+ }
+
ctx->decrypted = true;
}
--
2.13.6
^ permalink raw reply related [flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-07-19 11:15 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-07-19 11:16 [net-next v3 0/5] net/tls: Minor code cleanup patches Vakul Garg
2018-07-19 11:16 ` [net-next v3 1/5] net/tls: Do not enable zero-copy prematurely Vakul Garg
2018-07-19 10:28 ` Boris Pismenny
2018-07-19 10:32 ` Vakul Garg
2018-07-19 11:16 ` [net-next v3 2/5] net/tls: Use socket data_ready callback on record availability Vakul Garg
2018-07-19 11:16 ` [net-next v3 3/5] net/tls: Remove redundant variable assignments and wakeup Vakul Garg
2018-07-19 11:16 ` [net-next v3 4/5] net/tls: Remove redundant array allocation Vakul Garg
2018-07-19 11:16 ` [net-next v3 5/5] net/tls: Rework error checking after decrypt_skb_update() Vakul Garg
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).