* [PATCH] tcp_bpf: Fix one concurrency problem in the tcp_bpf_send_verdict function
@ 2021-09-29 8:45 Liu Jian
2021-09-30 22:25 ` John Fastabend
0 siblings, 1 reply; 3+ messages in thread
From: Liu Jian @ 2021-09-29 8:45 UTC (permalink / raw)
To: john.fastabend, daniel, jakub, lmb, edumazet, davem, yoshfuji,
dsahern, kuba, ast, andrii, kafai, songliubraving, yhs, kpsingh,
netdev, bpf
Cc: liujian56
In the following cases:
We need to redirect the first msg to sock1 and the second msg to sock2.
The sock lock needs to be released at __SK_REDIRECT and to get another
sock lock, this will cause the probability that psock->eval is not set to
__SK_NONE when the second msg comes.
If psock does not set apple bytes, fix this by do the cleanup before
releasing the sock lock. And keep the original logic in other cases.
Signed-off-by: Liu Jian <liujian56@huawei.com>
---
net/ipv4/tcp_bpf.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c
index d3e9386b493e..02442e43ac4d 100644
--- a/net/ipv4/tcp_bpf.c
+++ b/net/ipv4/tcp_bpf.c
@@ -232,6 +232,7 @@ static int tcp_bpf_send_verdict(struct sock *sk, struct sk_psock *psock,
bool cork = false, enospc = sk_msg_full(msg);
struct sock *sk_redir;
u32 tosend, delta = 0;
+ u32 eval = __SK_NONE;
int ret;
more_data:
@@ -274,6 +275,12 @@ static int tcp_bpf_send_verdict(struct sock *sk, struct sk_psock *psock,
break;
case __SK_REDIRECT:
sk_redir = psock->sk_redir;
+ if (!psock->apply_bytes) {
+ /* Clean up before releasing the sock lock. */
+ eval = psock->eval;
+ psock->eval = __SK_NONE;
+ psock->sk_redir = NULL;
+ }
sk_msg_apply_bytes(psock, tosend);
if (psock->cork) {
cork = true;
@@ -281,7 +288,12 @@ static int tcp_bpf_send_verdict(struct sock *sk, struct sk_psock *psock,
}
sk_msg_return(sk, msg, tosend);
release_sock(sk);
+
ret = tcp_bpf_sendmsg_redir(sk_redir, msg, tosend, flags);
+
+ if (eval == __SK_REDIRECT)
+ sock_put(sk_redir);
+
lock_sock(sk);
if (unlikely(ret < 0)) {
int free = sk_msg_free_nocharge(sk, msg);
--
2.17.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* RE: [PATCH] tcp_bpf: Fix one concurrency problem in the tcp_bpf_send_verdict function
2021-09-29 8:45 [PATCH] tcp_bpf: Fix one concurrency problem in the tcp_bpf_send_verdict function Liu Jian
@ 2021-09-30 22:25 ` John Fastabend
2021-10-04 4:21 ` liujian (CE)
0 siblings, 1 reply; 3+ messages in thread
From: John Fastabend @ 2021-09-30 22:25 UTC (permalink / raw)
To: Liu Jian, john.fastabend, daniel, jakub, lmb, edumazet, davem,
yoshfuji, dsahern, kuba, ast, andrii, kafai, songliubraving, yhs,
kpsingh, netdev, bpf
Cc: liujian56
Liu Jian wrote:
> In the following cases:
> We need to redirect the first msg to sock1 and the second msg to sock2.
> The sock lock needs to be released at __SK_REDIRECT and to get another
> sock lock, this will cause the probability that psock->eval is not set to
> __SK_NONE when the second msg comes.
>
> If psock does not set apple bytes, fix this by do the cleanup before
> releasing the sock lock. And keep the original logic in other cases.
It took me sometime to figure out the above description. Please include
a bit more details here this needs to be backported so we want to be
very clear what the error is and how to trigger it.
In this case we should list the flow to show how the interleaving of
msgs breaks.
"
With two Msgs, msgA and msgB and a user doing nonblocking sendmsg calls
(or multiple cores) on a single socket 'sk' we could get the following
flow.
msgA, sk msgB, sk
----------- ---------------
tcp_bpf_sendmsg()
lock(sk)
psock = sk->psock
tcp_bpf_sendmsg()
lock(sk) ... blocking
tcp_bpf_send_verdict
if (psock->eval == NONE)
psock->eval = sk_psock_msg_verdict
..
< handle SK_REDIRECT case >
release_sock(sk) < lock dropped so grab here >
ret = tcp_bpf_sendmsg_redir
psock = sk->psock
tcp_bpf_send_verdict
lock_sock(sk) ... blocking on B
if (psock->eval == NONE) <- boom.
psock->eval will have msgA state
The problem here is we dropped the lock on msgA and grabbed it with msgB.
Now we have old state in psock and importantly psock->eval has not
been cleared. So msgB will run whatever action was done on A and the
verdict program may never see it.
"
Showing the flow makes it painfully obvious why dropping that lock
with old state is broken.
>
> Signed-off-by: Liu Jian <liujian56@huawei.com>
We need a fixes tag as well so we can backport this.
> ---
> net/ipv4/tcp_bpf.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c
> index d3e9386b493e..02442e43ac4d 100644
> --- a/net/ipv4/tcp_bpf.c
> +++ b/net/ipv4/tcp_bpf.c
> @@ -232,6 +232,7 @@ static int tcp_bpf_send_verdict(struct sock *sk, struct sk_psock *psock,
> bool cork = false, enospc = sk_msg_full(msg);
> struct sock *sk_redir;
> u32 tosend, delta = 0;
> + u32 eval = __SK_NONE;
> int ret;
>
> more_data:
> @@ -274,6 +275,12 @@ static int tcp_bpf_send_verdict(struct sock *sk, struct sk_psock *psock,
> break;
> case __SK_REDIRECT:
> sk_redir = psock->sk_redir;
> + if (!psock->apply_bytes) {
> + /* Clean up before releasing the sock lock. */
> + eval = psock->eval;
> + psock->eval = __SK_NONE;
> + psock->sk_redir = NULL;
> + }
We need to move above chunk below sk_msg_apply_bytes() so we account for
the bytes and if we zero apply bytes with this send we clear the psock
state. Otherwise we could have the same issue with stale state on the
boundary where apply bytes is met.
> sk_msg_apply_bytes(psock, tosend);
<-- put above chunk here.
> if (psock->cork) {
> cork = true;
Interestingly, I caught the race with cork state, but missed it with
the eval case. Likely because our program redirected to a single sk.
> @@ -281,7 +288,12 @@ static int tcp_bpf_send_verdict(struct sock *sk, struct sk_psock *psock,
> }
> sk_msg_return(sk, msg, tosend);
> release_sock(sk);
> +
> ret = tcp_bpf_sendmsg_redir(sk_redir, msg, tosend, flags);
> +
> + if (eval == __SK_REDIRECT)
Is the 'if' needed? we are in this case because eval is SK_REDIRECT.
> + sock_put(sk_redir);
> +
> lock_sock(sk);
> if (unlikely(ret < 0)) {
> int free = sk_msg_free_nocharge(sk, msg);
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* RE: [PATCH] tcp_bpf: Fix one concurrency problem in the tcp_bpf_send_verdict function
2021-09-30 22:25 ` John Fastabend
@ 2021-10-04 4:21 ` liujian (CE)
0 siblings, 0 replies; 3+ messages in thread
From: liujian (CE) @ 2021-10-04 4:21 UTC (permalink / raw)
To: John Fastabend, daniel, jakub, lmb, edumazet, davem, yoshfuji,
dsahern, kuba, ast, andrii, kafai, songliubraving, yhs, kpsingh,
netdev, bpf
> -----Original Message-----
> From: John Fastabend [mailto:john.fastabend@gmail.com]
> Sent: Friday, October 1, 2021 6:25 AM
> To: liujian (CE) <liujian56@huawei.com>; john.fastabend@gmail.com;
> daniel@iogearbox.net; jakub@cloudflare.com; lmb@cloudflare.com;
> edumazet@google.com; davem@davemloft.net; yoshfuji@linux-ipv6.org;
> dsahern@kernel.org; kuba@kernel.org; ast@kernel.org; andrii@kernel.org;
> kafai@fb.com; songliubraving@fb.com; yhs@fb.com; kpsingh@kernel.org;
> netdev@vger.kernel.org; bpf@vger.kernel.org
> Cc: liujian (CE) <liujian56@huawei.com>
> Subject: RE: [PATCH] tcp_bpf: Fix one concurrency problem in the
> tcp_bpf_send_verdict function
>
> Liu Jian wrote:
> > In the following cases:
> > We need to redirect the first msg to sock1 and the second msg to sock2.
> > The sock lock needs to be released at __SK_REDIRECT and to get another
> > sock lock, this will cause the probability that psock->eval is not set
> > to __SK_NONE when the second msg comes.
> >
> > If psock does not set apple bytes, fix this by do the cleanup before
> > releasing the sock lock. And keep the original logic in other cases.
>
> It took me sometime to figure out the above description. Please include a bit
> more details here this needs to be backported so we want to be very clear
> what the error is and how to trigger it.
>
> In this case we should list the flow to show how the interleaving of msgs
> breaks.
>
> "
> With two Msgs, msgA and msgB and a user doing nonblocking sendmsg calls
> (or multiple cores) on a single socket 'sk' we could get the following flow.
>
> msgA, sk msgB, sk
> ----------- ---------------
> tcp_bpf_sendmsg()
> lock(sk)
> psock = sk->psock
> tcp_bpf_sendmsg()
> lock(sk) ... blocking tcp_bpf_send_verdict if (psock-
> >eval == NONE)
> psock->eval = sk_psock_msg_verdict
> ..
> < handle SK_REDIRECT case >
> release_sock(sk) < lock dropped so grab here >
> ret = tcp_bpf_sendmsg_redir
> psock = sk->psock
> tcp_bpf_send_verdict
> lock_sock(sk) ... blocking on B
> if (psock->eval == NONE) <- boom.
> psock->eval will have msgA state
>
> The problem here is we dropped the lock on msgA and grabbed it with msgB.
> Now we have old state in psock and importantly psock->eval has not been
> cleared. So msgB will run whatever action was done on A and the verdict
> program may never see it.
> "
>
> Showing the flow makes it painfully obvious why dropping that lock with old
> state is broken.
>
Thanks a lot for such a detailed example.
>
> >
> > Signed-off-by: Liu Jian <liujian56@huawei.com>
>
> We need a fixes tag as well so we can backport this.
I will add it.
>
> > ---
> > net/ipv4/tcp_bpf.c | 12 ++++++++++++
> > 1 file changed, 12 insertions(+)
> >
> > diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c index
> > d3e9386b493e..02442e43ac4d 100644
> > --- a/net/ipv4/tcp_bpf.c
> > +++ b/net/ipv4/tcp_bpf.c
> > @@ -232,6 +232,7 @@ static int tcp_bpf_send_verdict(struct sock *sk,
> struct sk_psock *psock,
> > bool cork = false, enospc = sk_msg_full(msg);
> > struct sock *sk_redir;
> > u32 tosend, delta = 0;
> > + u32 eval = __SK_NONE;
> > int ret;
> >
> > more_data:
> > @@ -274,6 +275,12 @@ static int tcp_bpf_send_verdict(struct sock *sk,
> struct sk_psock *psock,
> > break;
> > case __SK_REDIRECT:
> > sk_redir = psock->sk_redir;
> > + if (!psock->apply_bytes) {
> > + /* Clean up before releasing the sock lock. */
> > + eval = psock->eval;
> > + psock->eval = __SK_NONE;
> > + psock->sk_redir = NULL;
> > + }
>
> We need to move above chunk below sk_msg_apply_bytes() so we account
> for the bytes and if we zero apply bytes with this send we clear the psock
> state. Otherwise we could have the same issue with stale state on the
> boundary where apply bytes is met.
>
> > sk_msg_apply_bytes(psock, tosend);
>
> <-- put above chunk here.
yes, here looks better.
>
> > if (psock->cork) {
> > cork = true;
>
> Interestingly, I caught the race with cork state, but missed it with the eval
> case. Likely because our program redirected to a single sk.
>
Yes.
> > @@ -281,7 +288,12 @@ static int tcp_bpf_send_verdict(struct sock *sk,
> struct sk_psock *psock,
> > }
> > sk_msg_return(sk, msg, tosend);
> > release_sock(sk);
> > +
> > ret = tcp_bpf_sendmsg_redir(sk_redir, msg, tosend, flags);
> > +
> > + if (eval == __SK_REDIRECT)
>
> Is the 'if' needed? we are in this case because eval is SK_REDIRECT.
>
Need it, because If the "apply bytes" is not zero, i did not change the logic.
> > + sock_put(sk_redir);
> > +
> > lock_sock(sk);
> > if (unlikely(ret < 0)) {
> > int free = sk_msg_free_nocharge(sk, msg);
> > --
> > 2.17.1
> >
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-10-04 4:21 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-29 8:45 [PATCH] tcp_bpf: Fix one concurrency problem in the tcp_bpf_send_verdict function Liu Jian
2021-09-30 22:25 ` John Fastabend
2021-10-04 4:21 ` liujian (CE)
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).