* [net v3 0/2] net/tls: fix encryption error path
@ 2020-05-20 8:41 Vadim Fedorenko
2020-05-20 8:41 ` [net v3 1/2] net/tls: fix encryption error checking Vadim Fedorenko
` (3 more replies)
0 siblings, 4 replies; 6+ messages in thread
From: Vadim Fedorenko @ 2020-05-20 8:41 UTC (permalink / raw)
To: Jakub Kicinski, Boris Pismenny, Aviad Yehezkel, John Fastabend,
Daniel Borkmann
Cc: David S. Miller, netdev, Vadim Fedorenko
The problem with data stream corruption was found in KTLS
transmit path with small socket send buffers and large
amount of data. bpf_exec_tx_verdict() frees open record
on any type of error including EAGAIN, ENOMEM and ENOSPC
while callers are able to recover this transient errors.
Also wrong error code was returned to user space in that
case. This patchset fixes the problems.
Vadim Fedorenko (2):
net/tls: fix encryption error checking
net/tls: free record only on encryption error
net/tls/tls_sw.c | 17 ++++++++++-------
1 file changed, 10 insertions(+), 7 deletions(-)
--
1.8.3.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* [net v3 1/2] net/tls: fix encryption error checking
2020-05-20 8:41 [net v3 0/2] net/tls: fix encryption error path Vadim Fedorenko
@ 2020-05-20 8:41 ` Vadim Fedorenko
2020-05-20 8:41 ` [net v3 2/2] net/tls: free record only on encryption error Vadim Fedorenko
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Vadim Fedorenko @ 2020-05-20 8:41 UTC (permalink / raw)
To: Jakub Kicinski, Boris Pismenny, Aviad Yehezkel, John Fastabend,
Daniel Borkmann
Cc: David S. Miller, netdev, Vadim Fedorenko
bpf_exec_tx_verdict() can return negative value for copied
variable. In that case this value will be pushed back to caller
and the real error code will be lost. Fix it using signed type and
checking for positive value.
Fixes: d10523d0b3d7 ("net/tls: free the record on encryption error")
Fixes: d3b18ad31f93 ("tls: add bpf support to sk_msg handling")
Signed-off-by: Vadim Fedorenko <vfedorenko@novek.ru>
---
net/tls/tls_sw.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index d8ebdfc..e61c024 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -782,7 +782,7 @@ static int tls_push_record(struct sock *sk, int flags,
static int bpf_exec_tx_verdict(struct sk_msg *msg, struct sock *sk,
bool full_record, u8 record_type,
- size_t *copied, int flags)
+ ssize_t *copied, int flags)
{
struct tls_context *tls_ctx = tls_get_ctx(sk);
struct tls_sw_context_tx *ctx = tls_sw_ctx_tx(tls_ctx);
@@ -918,7 +918,8 @@ int tls_sw_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
unsigned char record_type = TLS_RECORD_TYPE_DATA;
bool is_kvec = iov_iter_is_kvec(&msg->msg_iter);
bool eor = !(msg->msg_flags & MSG_MORE);
- size_t try_to_copy, copied = 0;
+ size_t try_to_copy;
+ ssize_t copied = 0;
struct sk_msg *msg_pl, *msg_en;
struct tls_rec *rec;
int required_size;
@@ -1120,7 +1121,7 @@ int tls_sw_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
release_sock(sk);
mutex_unlock(&tls_ctx->tx_lock);
- return copied ? copied : ret;
+ return copied > 0 ? copied : ret;
}
static int tls_sw_do_sendpage(struct sock *sk, struct page *page,
@@ -1134,7 +1135,7 @@ static int tls_sw_do_sendpage(struct sock *sk, struct page *page,
struct sk_msg *msg_pl;
struct tls_rec *rec;
int num_async = 0;
- size_t copied = 0;
+ ssize_t copied = 0;
bool full_record;
int record_room;
int ret = 0;
@@ -1236,7 +1237,7 @@ static int tls_sw_do_sendpage(struct sock *sk, struct page *page,
}
sendpage_end:
ret = sk_stream_error(sk, flags, ret);
- return copied ? copied : ret;
+ return copied > 0 ? copied : ret;
}
int tls_sw_sendpage_locked(struct sock *sk, struct page *page,
--
1.8.3.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [net v3 2/2] net/tls: free record only on encryption error
2020-05-20 8:41 [net v3 0/2] net/tls: fix encryption error path Vadim Fedorenko
2020-05-20 8:41 ` [net v3 1/2] net/tls: fix encryption error checking Vadim Fedorenko
@ 2020-05-20 8:41 ` Vadim Fedorenko
2020-05-20 20:46 ` [net v3 0/2] net/tls: fix encryption error path Jakub Kicinski
2020-05-22 0:20 ` David Miller
3 siblings, 0 replies; 6+ messages in thread
From: Vadim Fedorenko @ 2020-05-20 8:41 UTC (permalink / raw)
To: Jakub Kicinski, Boris Pismenny, Aviad Yehezkel, John Fastabend,
Daniel Borkmann
Cc: David S. Miller, netdev, Vadim Fedorenko
We cannot free record on any transient error because it leads to
losing previos data. Check socket error to know whether record must
be freed or not.
Fixes: d10523d0b3d7 ("net/tls: free the record on encryption error")
Signed-off-by: Vadim Fedorenko <vfedorenko@novek.ru>
---
net/tls/tls_sw.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index e61c024..cb72abe 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -798,9 +798,10 @@ static int bpf_exec_tx_verdict(struct sk_msg *msg, struct sock *sk,
psock = sk_psock_get(sk);
if (!psock || !policy) {
err = tls_push_record(sk, flags, record_type);
- if (err && err != -EINPROGRESS) {
+ if (err && sk->sk_err == EBADMSG) {
*copied -= sk_msg_free(sk, msg);
tls_free_open_rec(sk);
+ err = -sk->sk_err;
}
if (psock)
sk_psock_put(sk, psock);
@@ -826,9 +827,10 @@ static int bpf_exec_tx_verdict(struct sk_msg *msg, struct sock *sk,
switch (psock->eval) {
case __SK_PASS:
err = tls_push_record(sk, flags, record_type);
- if (err && err != -EINPROGRESS) {
+ if (err && sk->sk_err == EBADMSG) {
*copied -= sk_msg_free(sk, msg);
tls_free_open_rec(sk);
+ err = -sk->sk_err;
goto out_err;
}
break;
--
1.8.3.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [net v3 0/2] net/tls: fix encryption error path
2020-05-20 8:41 [net v3 0/2] net/tls: fix encryption error path Vadim Fedorenko
2020-05-20 8:41 ` [net v3 1/2] net/tls: fix encryption error checking Vadim Fedorenko
2020-05-20 8:41 ` [net v3 2/2] net/tls: free record only on encryption error Vadim Fedorenko
@ 2020-05-20 20:46 ` Jakub Kicinski
2020-05-21 16:29 ` Pooja Trivedi
2020-05-22 0:20 ` David Miller
3 siblings, 1 reply; 6+ messages in thread
From: Jakub Kicinski @ 2020-05-20 20:46 UTC (permalink / raw)
To: Vadim Fedorenko, Pooja Trivedi
Cc: Boris Pismenny, Aviad Yehezkel, John Fastabend, Daniel Borkmann,
David S. Miller, netdev
On Wed, 20 May 2020 11:41:42 +0300 Vadim Fedorenko wrote:
> The problem with data stream corruption was found in KTLS
> transmit path with small socket send buffers and large
> amount of data. bpf_exec_tx_verdict() frees open record
> on any type of error including EAGAIN, ENOMEM and ENOSPC
> while callers are able to recover this transient errors.
> Also wrong error code was returned to user space in that
> case. This patchset fixes the problems.
Thanks:
Acked-by: Jakub Kicinski <kuba@kernel.org>
Pooja, I think Vadim's fix to check the socket error will make changes
to handling of -EAGAIN unnecessary, right? Still would be good to get
that selftest, triggering EAGAIN should be quite simple.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [net v3 0/2] net/tls: fix encryption error path
2020-05-20 20:46 ` [net v3 0/2] net/tls: fix encryption error path Jakub Kicinski
@ 2020-05-21 16:29 ` Pooja Trivedi
0 siblings, 0 replies; 6+ messages in thread
From: Pooja Trivedi @ 2020-05-21 16:29 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Vadim Fedorenko, Boris Pismenny, Aviad Yehezkel, John Fastabend,
Daniel Borkmann, David S. Miller, netdev
On Wed, May 20, 2020 at 4:46 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Wed, 20 May 2020 11:41:42 +0300 Vadim Fedorenko wrote:
> > The problem with data stream corruption was found in KTLS
> > transmit path with small socket send buffers and large
> > amount of data. bpf_exec_tx_verdict() frees open record
> > on any type of error including EAGAIN, ENOMEM and ENOSPC
> > while callers are able to recover this transient errors.
> > Also wrong error code was returned to user space in that
> > case. This patchset fixes the problems.
>
> Thanks:
>
> Acked-by: Jakub Kicinski <kuba@kernel.org>
>
> Pooja, I think Vadim's fix to check the socket error will make changes
> to handling of -EAGAIN unnecessary, right?
Correct, yes.
> Still would be good to get
> that selftest, triggering EAGAIN should be quite simple.
Agree.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [net v3 0/2] net/tls: fix encryption error path
2020-05-20 8:41 [net v3 0/2] net/tls: fix encryption error path Vadim Fedorenko
` (2 preceding siblings ...)
2020-05-20 20:46 ` [net v3 0/2] net/tls: fix encryption error path Jakub Kicinski
@ 2020-05-22 0:20 ` David Miller
3 siblings, 0 replies; 6+ messages in thread
From: David Miller @ 2020-05-22 0:20 UTC (permalink / raw)
To: vfedorenko; +Cc: kuba, borisp, aviadye, john.fastabend, daniel, netdev
From: Vadim Fedorenko <vfedorenko@novek.ru>
Date: Wed, 20 May 2020 11:41:42 +0300
> The problem with data stream corruption was found in KTLS
> transmit path with small socket send buffers and large
> amount of data. bpf_exec_tx_verdict() frees open record
> on any type of error including EAGAIN, ENOMEM and ENOSPC
> while callers are able to recover this transient errors.
> Also wrong error code was returned to user space in that
> case. This patchset fixes the problems.
Series applied, thanks.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-05-22 0:20 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-20 8:41 [net v3 0/2] net/tls: fix encryption error path Vadim Fedorenko
2020-05-20 8:41 ` [net v3 1/2] net/tls: fix encryption error checking Vadim Fedorenko
2020-05-20 8:41 ` [net v3 2/2] net/tls: free record only on encryption error Vadim Fedorenko
2020-05-20 20:46 ` [net v3 0/2] net/tls: fix encryption error path Jakub Kicinski
2020-05-21 16:29 ` Pooja Trivedi
2020-05-22 0:20 ` David Miller
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).