* [PATCH] af_unix: fix holding spinlock in oob handling
@ 2021-08-11 22:06 Rao Shoaib
2021-08-12 7:53 ` Eric Dumazet
2021-08-13 17:32 ` Jakub Kicinski
0 siblings, 2 replies; 9+ messages in thread
From: Rao Shoaib @ 2021-08-11 22:06 UTC (permalink / raw)
To: netdev, kuba, viro, rao.shoaib, edumazet
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>
---
net/unix/af_unix.c | 36 ++++++++++++++++++++++++------------
1 file changed, 24 insertions(+), 12 deletions(-)
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;
}
--
2.27.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] af_unix: fix holding spinlock in oob handling
2021-08-11 22:06 [PATCH] af_unix: fix holding spinlock in oob handling Rao Shoaib
@ 2021-08-12 7:53 ` Eric Dumazet
2021-08-12 16:23 ` Shoaib Rao
2021-08-12 17:37 ` Shoaib Rao
2021-08-13 17:32 ` Jakub Kicinski
1 sibling, 2 replies; 9+ messages in thread
From: Eric Dumazet @ 2021-08-12 7:53 UTC (permalink / raw)
To: Rao Shoaib; +Cc: netdev, Jakub Kicinski, Al Viro
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;
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] af_unix: fix holding spinlock in oob handling
2021-08-12 7:53 ` Eric Dumazet
@ 2021-08-12 16:23 ` Shoaib Rao
2021-08-12 17:37 ` Shoaib Rao
1 sibling, 0 replies; 9+ messages in thread
From: Shoaib Rao @ 2021-08-12 16:23 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev, Jakub Kicinski, Al Viro
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;
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] af_unix: fix holding spinlock in oob handling
2021-08-12 7:53 ` Eric Dumazet
2021-08-12 16:23 ` Shoaib Rao
@ 2021-08-12 17:37 ` Shoaib Rao
2021-08-12 20:33 ` Eric Dumazet
1 sibling, 1 reply; 9+ messages in thread
From: Shoaib Rao @ 2021-08-12 17:37 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev, Jakub Kicinski, Al Viro
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] af_unix: fix holding spinlock in oob handling
2021-08-12 17:37 ` Shoaib Rao
@ 2021-08-12 20:33 ` Eric Dumazet
2021-08-12 20:38 ` Shoaib Rao
0 siblings, 1 reply; 9+ messages in thread
From: Eric Dumazet @ 2021-08-12 20:33 UTC (permalink / raw)
To: Shoaib Rao; +Cc: netdev, Jakub Kicinski, Al Viro
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] af_unix: fix holding spinlock in oob handling
2021-08-12 20:33 ` Eric Dumazet
@ 2021-08-12 20:38 ` Shoaib Rao
0 siblings, 0 replies; 9+ messages in thread
From: Shoaib Rao @ 2021-08-12 20:38 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev, Jakub Kicinski, Al Viro
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] af_unix: fix holding spinlock in oob handling
2021-08-11 22:06 [PATCH] af_unix: fix holding spinlock in oob handling Rao Shoaib
2021-08-12 7:53 ` Eric Dumazet
@ 2021-08-13 17:32 ` Jakub Kicinski
2021-08-13 17:59 ` Shoaib Rao
1 sibling, 1 reply; 9+ messages in thread
From: Jakub Kicinski @ 2021-08-13 17:32 UTC (permalink / raw)
To: Rao Shoaib; +Cc: netdev, viro, edumazet
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!
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] af_unix: fix holding spinlock in oob handling
2021-08-13 17:32 ` Jakub Kicinski
@ 2021-08-13 17:59 ` Shoaib Rao
0 siblings, 0 replies; 9+ messages in thread
From: Shoaib Rao @ 2021-08-13 17:59 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: netdev, viro, edumazet
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!
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] af_unix: fix holding spinlock in oob handling
@ 2021-08-13 17:46 Rao Shoaib
0 siblings, 0 replies; 9+ messages in thread
From: Rao Shoaib @ 2021-08-13 17:46 UTC (permalink / raw)
To: netdev, kuba, rao.shoaib, viro, edumazet
From: Rao Shoaib <rao.shoaib@oracle.com>
syzkaller found that OOB code was holding spinlock
while calling a function in which it could sleep.
Also addressed comments from edumazet@google.com.
Reported-by: syzbot+8760ca6c1ee783ac4abd@syzkaller.appspotmail.com
Fixes: 314001f0bf92 ("af_unix: Add OOB support")
Signed-off-by: Rao Shoaib <rao.shoaib@oracle.com>
---
net/unix/af_unix.c | 47 ++++++++++++++++++++++++++++++++--------------
1 file changed, 33 insertions(+), 14 deletions(-)
diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index 00d8b08cdbe1..0f59fed993d8 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -1891,7 +1891,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) {
@@ -1900,11 +1899,19 @@ 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;
@@ -2362,19 +2369,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;
+
+ mutex_lock(&u->iolock);
+ unix_state_lock(sk);
- if (sock_flag(sk, SOCK_URGINLINE) || !u->oob_skb)
+ 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 +2459,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;
}
--
2.27.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-08-13 17:59 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-11 22:06 [PATCH] af_unix: fix holding spinlock in oob handling Rao Shoaib
2021-08-12 7:53 ` Eric Dumazet
2021-08-12 16:23 ` Shoaib Rao
2021-08-12 17:37 ` Shoaib Rao
2021-08-12 20:33 ` Eric Dumazet
2021-08-12 20:38 ` Shoaib Rao
2021-08-13 17:32 ` Jakub Kicinski
2021-08-13 17:59 ` Shoaib Rao
2021-08-13 17:46 Rao Shoaib
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.