| Message ID | 20210723183630.5088-1-xiyou.wangcong@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [bpf-next] unix_bpf: fix a potential deadlock in unix_dgram_bpf_recvmsg() | expand |
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 >
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.
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>
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>
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>
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;