| Message ID | 20210811220652.567434-1-Rao.Shoaib@oracle.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | af_unix: fix holding spinlock in oob handling | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/subject_prefix | warning | Target tree name not specified in the subject |
| netdev/cc_maintainers | fail | 1 blamed authors not CCed: davem@davemloft.net; 5 maintainers not CCed: christian.brauner@ubuntu.com davem@davemloft.net cong.wang@bytedance.com ast@kernel.org mszeredi@redhat.com |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 4 this patch: 4 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | fail | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 56 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 4 this patch: 4 |
| netdev/header_inline | success | Link |
On Thu, Aug 12, 2021 at 12:07 AM Rao Shoaib <Rao.Shoaib@oracle.com> wrote: > > From: Rao Shoaib <rao.shoaib@oracle.com> > > syzkaller found that OOB code was holding spinlock > while calling a function in which it could sleep. > > Reported-by: syzbot+8760ca6c1ee783ac4abd@syzkaller.appspotmail.com > > Fixes: 314001f0bf92 ("af_unix: Add OOB support") > > Signed-off-by: Rao Shoaib <rao.shoaib@oracle.com> > --- Please do not add these empty lines. Fixes: ... Reported-by: ... Signed-off-by: ... Also you might take a look at queue_oob() 1) Setting skb->len tp 1 should not be needed, skb_put() already does that 2) After unix_state_lock(other); we probably need to check status of the other socket. 3) Some skb_free() calls should have been consume_skb() diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c index ec02e70a549b42f6c102253508c48426a13f7bc4..0c27e2976f9d234ca3bb131731375bc51a056846 100644 --- a/net/unix/af_unix.c +++ b/net/unix/af_unix.c @@ -1908,7 +1908,6 @@ static int queue_oob(struct socket *sock, struct msghdr *msg, struct sock *other return err; skb_put(skb, 1); - skb->len = 1; err = skb_copy_datagram_from_iter(skb, 0, &msg->msg_iter, 1); if (err) { @@ -1917,11 +1916,17 @@ static int queue_oob(struct socket *sock, struct msghdr *msg, struct sock *other } unix_state_lock(other); + if (sock_flag(other, SOCK_DEAD) || + (other->sk_shutdown & RCV_SHUTDOWN)) { + unix_state_unlock(other); + kfree_skb(skb); + return -EPIPE; + } maybe_add_creds(skb, sock, other); skb_get(skb); if (ousk->oob_skb) - kfree_skb(ousk->oob_skb); + consume_skb(ousk->oob_skb); ousk->oob_skb = skb;
Hi Eric, Thanks for your review I will take care of the comments. Shoaib On 8/12/21 12:53 AM, Eric Dumazet wrote: > On Thu, Aug 12, 2021 at 12:07 AM Rao Shoaib <Rao.Shoaib@oracle.com> wrote: >> From: Rao Shoaib <rao.shoaib@oracle.com> >> >> syzkaller found that OOB code was holding spinlock >> while calling a function in which it could sleep. >> >> Reported-by: syzbot+8760ca6c1ee783ac4abd@syzkaller.appspotmail.com >> >> Fixes: 314001f0bf92 ("af_unix: Add OOB support") >> >> Signed-off-by: Rao Shoaib <rao.shoaib@oracle.com> >> --- > Please do not add these empty lines. > > Fixes: ... > Reported-by: ... > Signed-off-by: ... > Also you might take a look at queue_oob() > > 1) Setting skb->len tp 1 should not be needed, skb_put() already does that > 2) After unix_state_lock(other); we probably need to check status of > the other socket. > 3) Some skb_free() calls should have been consume_skb() > > diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c > index ec02e70a549b42f6c102253508c48426a13f7bc4..0c27e2976f9d234ca3bb131731375bc51a056846 > 100644 > --- a/net/unix/af_unix.c > +++ b/net/unix/af_unix.c > @@ -1908,7 +1908,6 @@ static int queue_oob(struct socket *sock, struct > msghdr *msg, struct sock *other > return err; > > skb_put(skb, 1); > - skb->len = 1; > err = skb_copy_datagram_from_iter(skb, 0, &msg->msg_iter, 1); > > if (err) { > @@ -1917,11 +1916,17 @@ static int queue_oob(struct socket *sock, > struct msghdr *msg, struct sock *other > } > > unix_state_lock(other); > + if (sock_flag(other, SOCK_DEAD) || > + (other->sk_shutdown & RCV_SHUTDOWN)) { > + unix_state_unlock(other); > + kfree_skb(skb); > + return -EPIPE; > + } > maybe_add_creds(skb, sock, other); > skb_get(skb); > > if (ousk->oob_skb) > - kfree_skb(ousk->oob_skb); > + consume_skb(ousk->oob_skb); > > ousk->oob_skb = skb;
On 8/12/21 12:53 AM, Eric Dumazet wrote: > if (ousk->oob_skb) > - kfree_skb(ousk->oob_skb); > + consume_skb(ousk->oob_skb); Should I be using consume_skb(), as the skb is not being consumed, the ref count is decremented and if zero skb will be freed. Shoaib
On Thu, Aug 12, 2021 at 7:37 PM Shoaib Rao <rao.shoaib@oracle.com> wrote: > > > On 8/12/21 12:53 AM, Eric Dumazet wrote: > > if (ousk->oob_skb) > > - kfree_skb(ousk->oob_skb); > > + consume_skb(ousk->oob_skb); > > Should I be using consume_skb(), as the skb is not being consumed, the > ref count is decremented and if zero skb will be freed. > consume_skb() and kfree_skb() have the same ref count handling. The difference is that kfree_skb() is used by convention when a packet is dropped Admins can look closely at packet drops with drop_monitor, or : perf record -a -g -e skb:kfree_skb sleep 10 perf report In your case, the oob_skb is not really dropped. It is replaced by another one, it is part of the normal operation.
On 8/12/21 1:33 PM, Eric Dumazet wrote: > On Thu, Aug 12, 2021 at 7:37 PM Shoaib Rao <rao.shoaib@oracle.com> wrote: >> >> On 8/12/21 12:53 AM, Eric Dumazet wrote: >>> if (ousk->oob_skb) >>> - kfree_skb(ousk->oob_skb); >>> + consume_skb(ousk->oob_skb); >> Should I be using consume_skb(), as the skb is not being consumed, the >> ref count is decremented and if zero skb will be freed. >> > consume_skb() and kfree_skb() have the same ref count handling. > > The difference is that kfree_skb() is used by convention when a packet > is dropped > > Admins can look closely at packet drops with drop_monitor, or : > > perf record -a -g -e skb:kfree_skb sleep 10 > perf report > > In your case, the oob_skb is not really dropped. It is replaced by > another one, it is part of the normal operation. Thanks a lot for the explanation. This was very helpful. In my case the skb may be dropped (oob was not read but the read has passed beyond oob, or could become part of normal data). Anyways, I will change it to use consume_skb(). Regards, Shoaib
On Wed, 11 Aug 2021 15:06:52 -0700 Rao Shoaib wrote: > From: Rao Shoaib <rao.shoaib@oracle.com> > > syzkaller found that OOB code was holding spinlock > while calling a function in which it could sleep. > > Reported-by: syzbot+8760ca6c1ee783ac4abd@syzkaller.appspotmail.com > > Fixes: 314001f0bf92 ("af_unix: Add OOB support") > > Signed-off-by: Rao Shoaib <rao.shoaib@oracle.com> IIUC issues pointed out by Eric are separate so I removed the spacing between the tags and applied, thanks!
OH, I just sent you a combined patch. I will resend it with just the nits pointed out by Eric. Shoaib On 8/13/21 10:32 AM, Jakub Kicinski wrote: > On Wed, 11 Aug 2021 15:06:52 -0700 Rao Shoaib wrote: >> From: Rao Shoaib <rao.shoaib@oracle.com> >> >> syzkaller found that OOB code was holding spinlock >> while calling a function in which it could sleep. >> >> Reported-by: syzbot+8760ca6c1ee783ac4abd@syzkaller.appspotmail.com >> >> Fixes: 314001f0bf92 ("af_unix: Add OOB support") >> >> Signed-off-by: Rao Shoaib <rao.shoaib@oracle.com> > IIUC issues pointed out by Eric are separate so I removed the spacing > between the tags and applied, thanks!
diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c index 00d8b08cdbe1..a626e52c629a 100644 --- a/net/unix/af_unix.c +++ b/net/unix/af_unix.c @@ -2362,19 +2362,37 @@ static int unix_stream_recv_urg(struct unix_stream_read_state *state) struct sock *sk = sock->sk; struct unix_sock *u = unix_sk(sk); int chunk = 1; + struct sk_buff *oob_skb; - if (sock_flag(sk, SOCK_URGINLINE) || !u->oob_skb) + mutex_lock(&u->iolock); + unix_state_lock(sk); + + if (sock_flag(sk, SOCK_URGINLINE) || !u->oob_skb) { + unix_state_unlock(sk); + mutex_unlock(&u->iolock); return -EINVAL; + } - chunk = state->recv_actor(u->oob_skb, 0, chunk, state); - if (chunk < 0) - return -EFAULT; + oob_skb = u->oob_skb; if (!(state->flags & MSG_PEEK)) { - UNIXCB(u->oob_skb).consumed += 1; - kfree_skb(u->oob_skb); u->oob_skb = NULL; } + + unix_state_unlock(sk); + + chunk = state->recv_actor(oob_skb, 0, chunk, state); + + if (!(state->flags & MSG_PEEK)) { + UNIXCB(oob_skb).consumed += 1; + kfree_skb(oob_skb); + } + + mutex_unlock(&u->iolock); + + if (chunk < 0) + return -EFAULT; + state->msg->msg_flags |= MSG_OOB; return 1; } @@ -2434,13 +2452,7 @@ static int unix_stream_read_generic(struct unix_stream_read_state *state, if (unlikely(flags & MSG_OOB)) { err = -EOPNOTSUPP; #if IS_ENABLED(CONFIG_AF_UNIX_OOB) - mutex_lock(&u->iolock); - unix_state_lock(sk); - err = unix_stream_recv_urg(state); - - unix_state_unlock(sk); - mutex_unlock(&u->iolock); #endif goto out; }