* [Patch bpf-next] unix_bpf: fix a potential deadlock in unix_dgram_bpf_recvmsg()
@ 2021-07-23 18:36 Cong Wang
2021-07-27 16:12 ` John Fastabend
2021-07-28 10:12 ` Jakub Sitnicki
0 siblings, 2 replies; 6+ messages in thread
From: Cong Wang @ 2021-07-23 18:36 UTC (permalink / raw)
To: netdev
Cc: Cong Wang, Eric Dumazet, John Fastabend, Daniel Borkmann,
Jakub Sitnicki, Lorenz Bauer
From: Cong Wang <cong.wang@bytedance.com>
As Eric noticed, __unix_dgram_recvmsg() may acquire u->iolock
too, so we have to release it before calling this function.
Fixes: 9825d866ce0d ("af_unix: Implement unix_dgram_bpf_recvmsg()")
Reported-by: Eric Dumazet <eric.dumazet@gmail.com>
Cc: John Fastabend <john.fastabend@gmail.com>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Jakub Sitnicki <jakub@cloudflare.com>
Cc: Lorenz Bauer <lmb@cloudflare.com>
Signed-off-by: Cong Wang <cong.wang@bytedance.com>
---
net/unix/unix_bpf.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/net/unix/unix_bpf.c b/net/unix/unix_bpf.c
index db0cda29fb2f..b07cb30e87b1 100644
--- a/net/unix/unix_bpf.c
+++ b/net/unix/unix_bpf.c
@@ -53,8 +53,9 @@ static int unix_dgram_bpf_recvmsg(struct sock *sk, struct msghdr *msg,
mutex_lock(&u->iolock);
if (!skb_queue_empty(&sk->sk_receive_queue) &&
sk_psock_queue_empty(psock)) {
- ret = __unix_dgram_recvmsg(sk, msg, len, flags);
- goto out;
+ mutex_unlock(&u->iolock);
+ sk_psock_put(sk, psock);
+ return __unix_dgram_recvmsg(sk, msg, len, flags);
}
msg_bytes_ready:
@@ -68,13 +69,13 @@ static int unix_dgram_bpf_recvmsg(struct sock *sk, struct msghdr *msg,
if (data) {
if (!sk_psock_queue_empty(psock))
goto msg_bytes_ready;
- ret = __unix_dgram_recvmsg(sk, msg, len, flags);
- goto out;
+ mutex_unlock(&u->iolock);
+ sk_psock_put(sk, psock);
+ return __unix_dgram_recvmsg(sk, msg, len, flags);
}
copied = -EAGAIN;
}
ret = copied;
-out:
mutex_unlock(&u->iolock);
sk_psock_put(sk, psock);
return ret;
--
2.27.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* RE: [Patch bpf-next] unix_bpf: fix a potential deadlock in unix_dgram_bpf_recvmsg()
2021-07-23 18:36 [Patch bpf-next] unix_bpf: fix a potential deadlock in unix_dgram_bpf_recvmsg() Cong Wang
@ 2021-07-27 16:12 ` John Fastabend
2021-07-28 3:06 ` Cong Wang
2021-07-28 10:12 ` Jakub Sitnicki
1 sibling, 1 reply; 6+ messages in thread
From: John Fastabend @ 2021-07-27 16:12 UTC (permalink / raw)
To: Cong Wang, netdev
Cc: Cong Wang, Eric Dumazet, John Fastabend, Daniel Borkmann,
Jakub Sitnicki, Lorenz Bauer
Cong Wang wrote:
> From: Cong Wang <cong.wang@bytedance.com>
>
> As Eric noticed, __unix_dgram_recvmsg() may acquire u->iolock
> too, so we have to release it before calling this function.
>
> Fixes: 9825d866ce0d ("af_unix: Implement unix_dgram_bpf_recvmsg()")
> Reported-by: Eric Dumazet <eric.dumazet@gmail.com>
> Cc: John Fastabend <john.fastabend@gmail.com>
> Cc: Daniel Borkmann <daniel@iogearbox.net>
> Cc: Jakub Sitnicki <jakub@cloudflare.com>
> Cc: Lorenz Bauer <lmb@cloudflare.com>
> Signed-off-by: Cong Wang <cong.wang@bytedance.com>
> ---
> net/unix/unix_bpf.c | 11 ++++++-----
> 1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/net/unix/unix_bpf.c b/net/unix/unix_bpf.c
> index db0cda29fb2f..b07cb30e87b1 100644
> --- a/net/unix/unix_bpf.c
> +++ b/net/unix/unix_bpf.c
> @@ -53,8 +53,9 @@ static int unix_dgram_bpf_recvmsg(struct sock *sk, struct msghdr *msg,
> mutex_lock(&u->iolock);
> if (!skb_queue_empty(&sk->sk_receive_queue) &&
> sk_psock_queue_empty(psock)) {
> - ret = __unix_dgram_recvmsg(sk, msg, len, flags);
> - goto out;
> + mutex_unlock(&u->iolock);
> + sk_psock_put(sk, psock);
> + return __unix_dgram_recvmsg(sk, msg, len, flags);
> }
Is there a reason to grab the mutex_lock(u->iolock) above the
skb_queue_emptyaand sk_psock_queue_empty checks?
Could it be move here just above the msg_bytes_ready label?
>
> msg_bytes_ready:
> @@ -68,13 +69,13 @@ static int unix_dgram_bpf_recvmsg(struct sock *sk, struct msghdr *msg,
> if (data) {
> if (!sk_psock_queue_empty(psock))
> goto msg_bytes_ready;
> - ret = __unix_dgram_recvmsg(sk, msg, len, flags);
> - goto out;
> + mutex_unlock(&u->iolock);
> + sk_psock_put(sk, psock);
> + return __unix_dgram_recvmsg(sk, msg, len, flags);
> }
> copied = -EAGAIN;
> }
> ret = copied;
> -out:
> mutex_unlock(&u->iolock);
> sk_psock_put(sk, psock);
> return ret;
> --
> 2.27.0
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Patch bpf-next] unix_bpf: fix a potential deadlock in unix_dgram_bpf_recvmsg()
2021-07-27 16:12 ` John Fastabend
@ 2021-07-28 3:06 ` Cong Wang
0 siblings, 0 replies; 6+ messages in thread
From: Cong Wang @ 2021-07-28 3:06 UTC (permalink / raw)
To: John Fastabend
Cc: Linux Kernel Network Developers, Cong Wang, Eric Dumazet,
Daniel Borkmann, Jakub Sitnicki, Lorenz Bauer
On Tue, Jul 27, 2021 at 9:12 AM John Fastabend <john.fastabend@gmail.com> wrote:
>
> Is there a reason to grab the mutex_lock(u->iolock) above the
> skb_queue_emptyaand sk_psock_queue_empty checks?
>
> Could it be move here just above the msg_bytes_ready label?
The check of the receive queue is more accurate with lock.
Thanks.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Patch bpf-next] unix_bpf: fix a potential deadlock in unix_dgram_bpf_recvmsg()
2021-07-23 18:36 [Patch bpf-next] unix_bpf: fix a potential deadlock in unix_dgram_bpf_recvmsg() Cong Wang
2021-07-27 16:12 ` John Fastabend
@ 2021-07-28 10:12 ` Jakub Sitnicki
2021-07-28 18:52 ` John Fastabend
1 sibling, 1 reply; 6+ messages in thread
From: Jakub Sitnicki @ 2021-07-28 10:12 UTC (permalink / raw)
To: Cong Wang
Cc: netdev, Cong Wang, Eric Dumazet, John Fastabend, Daniel Borkmann,
Lorenz Bauer
On Fri, Jul 23, 2021 at 08:36 PM CEST, Cong Wang wrote:
> From: Cong Wang <cong.wang@bytedance.com>
>
> As Eric noticed, __unix_dgram_recvmsg() may acquire u->iolock
> too, so we have to release it before calling this function.
>
> Fixes: 9825d866ce0d ("af_unix: Implement unix_dgram_bpf_recvmsg()")
> Reported-by: Eric Dumazet <eric.dumazet@gmail.com>
> Cc: John Fastabend <john.fastabend@gmail.com>
> Cc: Daniel Borkmann <daniel@iogearbox.net>
> Cc: Jakub Sitnicki <jakub@cloudflare.com>
> Cc: Lorenz Bauer <lmb@cloudflare.com>
> Signed-off-by: Cong Wang <cong.wang@bytedance.com>
> ---
> net/unix/unix_bpf.c | 11 ++++++-----
> 1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/net/unix/unix_bpf.c b/net/unix/unix_bpf.c
> index db0cda29fb2f..b07cb30e87b1 100644
> --- a/net/unix/unix_bpf.c
> +++ b/net/unix/unix_bpf.c
> @@ -53,8 +53,9 @@ static int unix_dgram_bpf_recvmsg(struct sock *sk, struct msghdr *msg,
> mutex_lock(&u->iolock);
> if (!skb_queue_empty(&sk->sk_receive_queue) &&
> sk_psock_queue_empty(psock)) {
> - ret = __unix_dgram_recvmsg(sk, msg, len, flags);
> - goto out;
> + mutex_unlock(&u->iolock);
> + sk_psock_put(sk, psock);
> + return __unix_dgram_recvmsg(sk, msg, len, flags);
> }
>
> msg_bytes_ready:
> @@ -68,13 +69,13 @@ static int unix_dgram_bpf_recvmsg(struct sock *sk, struct msghdr *msg,
> if (data) {
> if (!sk_psock_queue_empty(psock))
> goto msg_bytes_ready;
> - ret = __unix_dgram_recvmsg(sk, msg, len, flags);
> - goto out;
> + mutex_unlock(&u->iolock);
> + sk_psock_put(sk, psock);
> + return __unix_dgram_recvmsg(sk, msg, len, flags);
> }
> copied = -EAGAIN;
> }
> ret = copied;
> -out:
> mutex_unlock(&u->iolock);
> sk_psock_put(sk, psock);
> return ret;
Nit: Can be just `return copied`. `ret` became useless.
Acked-by: Jakub Sitnicki <jakub@cloudflare.com>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Patch bpf-next] unix_bpf: fix a potential deadlock in unix_dgram_bpf_recvmsg()
2021-07-28 10:12 ` Jakub Sitnicki
@ 2021-07-28 18:52 ` John Fastabend
2021-07-30 19:45 ` Andrii Nakryiko
0 siblings, 1 reply; 6+ messages in thread
From: John Fastabend @ 2021-07-28 18:52 UTC (permalink / raw)
To: Jakub Sitnicki, Cong Wang
Cc: netdev, Cong Wang, Eric Dumazet, John Fastabend, Daniel Borkmann,
Lorenz Bauer
Jakub Sitnicki wrote:
> On Fri, Jul 23, 2021 at 08:36 PM CEST, Cong Wang wrote:
> > From: Cong Wang <cong.wang@bytedance.com>
> >
> > As Eric noticed, __unix_dgram_recvmsg() may acquire u->iolock
> > too, so we have to release it before calling this function.
> >
> > Fixes: 9825d866ce0d ("af_unix: Implement unix_dgram_bpf_recvmsg()")
> > Reported-by: Eric Dumazet <eric.dumazet@gmail.com>
> > Cc: John Fastabend <john.fastabend@gmail.com>
> > Cc: Daniel Borkmann <daniel@iogearbox.net>
> > Cc: Jakub Sitnicki <jakub@cloudflare.com>
> > Cc: Lorenz Bauer <lmb@cloudflare.com>
> > Signed-off-by: Cong Wang <cong.wang@bytedance.com>
> > ---
> > net/unix/unix_bpf.c | 11 ++++++-----
> > 1 file changed, 6 insertions(+), 5 deletions(-)
> >
> > diff --git a/net/unix/unix_bpf.c b/net/unix/unix_bpf.c
> > index db0cda29fb2f..b07cb30e87b1 100644
> > --- a/net/unix/unix_bpf.c
> > +++ b/net/unix/unix_bpf.c
> > @@ -53,8 +53,9 @@ static int unix_dgram_bpf_recvmsg(struct sock *sk, struct msghdr *msg,
> > mutex_lock(&u->iolock);
> > if (!skb_queue_empty(&sk->sk_receive_queue) &&
> > sk_psock_queue_empty(psock)) {
> > - ret = __unix_dgram_recvmsg(sk, msg, len, flags);
> > - goto out;
> > + mutex_unlock(&u->iolock);
> > + sk_psock_put(sk, psock);
> > + return __unix_dgram_recvmsg(sk, msg, len, flags);
> > }
> >
> > msg_bytes_ready:
> > @@ -68,13 +69,13 @@ static int unix_dgram_bpf_recvmsg(struct sock *sk, struct msghdr *msg,
> > if (data) {
> > if (!sk_psock_queue_empty(psock))
> > goto msg_bytes_ready;
> > - ret = __unix_dgram_recvmsg(sk, msg, len, flags);
> > - goto out;
> > + mutex_unlock(&u->iolock);
> > + sk_psock_put(sk, psock);
> > + return __unix_dgram_recvmsg(sk, msg, len, flags);
> > }
> > copied = -EAGAIN;
> > }
> > ret = copied;
> > -out:
> > mutex_unlock(&u->iolock);
> > sk_psock_put(sk, psock);
> > return ret;
>
> Nit: Can be just `return copied`. `ret` became useless.
>
> Acked-by: Jakub Sitnicki <jakub@cloudflare.com>
Worth doing the small cleanup pointed out by Jakub but feel free to add
my ack.
Acked-by: John Fastabend <john.fastabend@gmail.com>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Patch bpf-next] unix_bpf: fix a potential deadlock in unix_dgram_bpf_recvmsg()
2021-07-28 18:52 ` John Fastabend
@ 2021-07-30 19:45 ` Andrii Nakryiko
0 siblings, 0 replies; 6+ messages in thread
From: Andrii Nakryiko @ 2021-07-30 19:45 UTC (permalink / raw)
To: John Fastabend
Cc: Jakub Sitnicki, Cong Wang, Networking, Cong Wang, Eric Dumazet,
Daniel Borkmann, Lorenz Bauer
On Wed, Jul 28, 2021 at 11:53 AM John Fastabend
<john.fastabend@gmail.com> wrote:
>
> Jakub Sitnicki wrote:
> > On Fri, Jul 23, 2021 at 08:36 PM CEST, Cong Wang wrote:
> > > From: Cong Wang <cong.wang@bytedance.com>
> > >
> > > As Eric noticed, __unix_dgram_recvmsg() may acquire u->iolock
> > > too, so we have to release it before calling this function.
> > >
> > > Fixes: 9825d866ce0d ("af_unix: Implement unix_dgram_bpf_recvmsg()")
> > > Reported-by: Eric Dumazet <eric.dumazet@gmail.com>
> > > Cc: John Fastabend <john.fastabend@gmail.com>
> > > Cc: Daniel Borkmann <daniel@iogearbox.net>
> > > Cc: Jakub Sitnicki <jakub@cloudflare.com>
> > > Cc: Lorenz Bauer <lmb@cloudflare.com>
> > > Signed-off-by: Cong Wang <cong.wang@bytedance.com>
> > > ---
> > > net/unix/unix_bpf.c | 11 ++++++-----
> > > 1 file changed, 6 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/net/unix/unix_bpf.c b/net/unix/unix_bpf.c
> > > index db0cda29fb2f..b07cb30e87b1 100644
> > > --- a/net/unix/unix_bpf.c
> > > +++ b/net/unix/unix_bpf.c
> > > @@ -53,8 +53,9 @@ static int unix_dgram_bpf_recvmsg(struct sock *sk, struct msghdr *msg,
> > > mutex_lock(&u->iolock);
> > > if (!skb_queue_empty(&sk->sk_receive_queue) &&
> > > sk_psock_queue_empty(psock)) {
> > > - ret = __unix_dgram_recvmsg(sk, msg, len, flags);
> > > - goto out;
> > > + mutex_unlock(&u->iolock);
> > > + sk_psock_put(sk, psock);
> > > + return __unix_dgram_recvmsg(sk, msg, len, flags);
> > > }
> > >
> > > msg_bytes_ready:
> > > @@ -68,13 +69,13 @@ static int unix_dgram_bpf_recvmsg(struct sock *sk, struct msghdr *msg,
> > > if (data) {
> > > if (!sk_psock_queue_empty(psock))
> > > goto msg_bytes_ready;
> > > - ret = __unix_dgram_recvmsg(sk, msg, len, flags);
> > > - goto out;
> > > + mutex_unlock(&u->iolock);
> > > + sk_psock_put(sk, psock);
> > > + return __unix_dgram_recvmsg(sk, msg, len, flags);
> > > }
> > > copied = -EAGAIN;
> > > }
> > > ret = copied;
> > > -out:
> > > mutex_unlock(&u->iolock);
> > > sk_psock_put(sk, psock);
> > > return ret;
> >
> > Nit: Can be just `return copied`. `ret` became useless.
> >
> > Acked-by: Jakub Sitnicki <jakub@cloudflare.com>
>
> Worth doing the small cleanup pointed out by Jakub but feel free to add
> my ack.
>
I cleaned it up while applying. Applied to bpf-next, thanks.
> Acked-by: John Fastabend <john.fastabend@gmail.com>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-07-30 19:45 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-23 18:36 [Patch bpf-next] unix_bpf: fix a potential deadlock in unix_dgram_bpf_recvmsg() Cong Wang
2021-07-27 16:12 ` John Fastabend
2021-07-28 3:06 ` Cong Wang
2021-07-28 10:12 ` Jakub Sitnicki
2021-07-28 18:52 ` John Fastabend
2021-07-30 19:45 ` Andrii Nakryiko
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.