* [PATCH net] unix: Fix race in SOCK_SEQPACKET's unix_dgram_sendmsg()
@ 2022-11-19 23:16 Kirill Tkhai
2022-11-20 9:09 ` Kuniyuki Iwashima
0 siblings, 1 reply; 6+ messages in thread
From: Kirill Tkhai @ 2022-11-19 23:16 UTC (permalink / raw)
To: davem, edumazet, kuba, pabeni, netdev, tkhai
There is a race resulting in alive SOCK_SEQPACKET socket
may change its state from TCP_ESTABLISHED to TCP_CLOSE:
unix_release_sock(peer) unix_dgram_sendmsg(sk)
sock_orphan(peer)
sock_set_flag(peer, SOCK_DEAD)
sock_alloc_send_pskb()
if !(sk->sk_shutdown & SEND_SHUTDOWN)
OK
if sock_flag(peer, SOCK_DEAD)
sk->sk_state = TCP_CLOSE
sk->sk_shutdown = SHUTDOWN_MASK
After that socket sk remains almost normal: it is able to connect, listen, accept
and recvmsg, while it can't sendmsg.
Since this is the only possibility for alive SOCK_SEQPACKET to change
the state in such way, we should better fix this strange and potentially
danger corner case.
Also, move TCP_CLOSE assignment for SOCK_DGRAM sockets under state lock.
Signed-off-by: Kirill Tkhai <tkhai@ya.ru>
---
net/unix/af_unix.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index b3545fc68097..6fd745cfc492 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -1999,13 +1999,20 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
unix_state_lock(sk);
err = 0;
- if (unix_peer(sk) == other) {
+ if (sk->sk_type == SOCK_SEQPACKET) {
+ /* We are here only when racing with unix_release_sock()
+ * is clearing @other. Never change state to TCP_CLOSE
+ * unlike SOCK_DGRAM wants.
+ */
+ unix_state_unlock(sk);
+ err = -EPIPE;
+ } else if (unix_peer(sk) == other) {
unix_peer(sk) = NULL;
unix_dgram_peer_wake_disconnect_wakeup(sk, other);
+ sk->sk_state = TCP_CLOSE;
unix_state_unlock(sk);
- sk->sk_state = TCP_CLOSE;
unix_dgram_disconnected(sk, other);
sock_put(other);
err = -ECONNREFUSED;
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH net] unix: Fix race in SOCK_SEQPACKET's unix_dgram_sendmsg()
2022-11-19 23:16 [PATCH net] unix: Fix race in SOCK_SEQPACKET's unix_dgram_sendmsg() Kirill Tkhai
@ 2022-11-20 9:09 ` Kuniyuki Iwashima
2022-11-20 9:46 ` Kirill Tkhai
0 siblings, 1 reply; 6+ messages in thread
From: Kuniyuki Iwashima @ 2022-11-20 9:09 UTC (permalink / raw)
To: tkhai; +Cc: davem, edumazet, kuba, netdev, pabeni, kuniyu
From: Kirill Tkhai <tkhai@ya.ru>
Date: Sun, 20 Nov 2022 02:16:47 +0300
> There is a race resulting in alive SOCK_SEQPACKET socket
> may change its state from TCP_ESTABLISHED to TCP_CLOSE:
>
> unix_release_sock(peer) unix_dgram_sendmsg(sk)
> sock_orphan(peer)
> sock_set_flag(peer, SOCK_DEAD)
> sock_alloc_send_pskb()
> if !(sk->sk_shutdown & SEND_SHUTDOWN)
> OK
> if sock_flag(peer, SOCK_DEAD)
> sk->sk_state = TCP_CLOSE
> sk->sk_shutdown = SHUTDOWN_MASK
>
>
> After that socket sk remains almost normal: it is able to connect, listen, accept
> and recvmsg, while it can't sendmsg.
nit: Then, also recvmsg() fails with -ENOTCONN. And after connect(), even
both of recvmsg() and sendmsg() does not fail.
static int unix_seqpacket_recvmsg(struct socket *sock, struct msghdr *msg,
size_t size, int flags)
{
struct sock *sk = sock->sk;
if (sk->sk_state != TCP_ESTABLISHED)
return -ENOTCONN;
return unix_dgram_recvmsg(sock, msg, size, flags);
}
>
> Since this is the only possibility for alive SOCK_SEQPACKET to change
> the state in such way, we should better fix this strange and potentially
> danger corner case.
>
> Also, move TCP_CLOSE assignment for SOCK_DGRAM sockets under state lock.
>
> Signed-off-by: Kirill Tkhai <tkhai@ya.ru>
Fixes tag is needed:
Fixes: 83301b5367a9 ("af_unix: Set TCP_ESTABLISHED for datagram sockets too")
Before this commit, there was no state change and SEQPACKET sk also went
through the same path. The bug was introduced because the commit did not
consider SEAPACKET.
So, I think the fix should be like below, then we can free the peer faster.
Note unix_dgram_peer_wake_disconnect_wakeup() is dgram-specific too.
---8<---
diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index b3545fc68097..be40023a61fb 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -2001,11 +2001,14 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
err = 0;
if (unix_peer(sk) == other) {
unix_peer(sk) = NULL;
- unix_dgram_peer_wake_disconnect_wakeup(sk, other);
+
+ if (sk->sk_type == SOCK_DGRAM) {
+ unix_dgram_peer_wake_disconnect_wakeup(sk, other);
+ sk->sk_state = TCP_CLOSE;
+ }
unix_state_unlock(sk);
- sk->sk_state = TCP_CLOSE;
unix_dgram_disconnected(sk, other);
sock_put(other);
err = -ECONNREFUSED;
---8<---
Also, it's better to mention that moving TCP_CLOSE under the lock resolves
another rare race with unix_dgram_connect() for DGRAM sk:
unix_state_unlock(sk);
<--------------------------> connect() could set TCP_ESTABLISHED here.
sk->sk_state = TCP_CLOSE;
Thank you!
> ---
> net/unix/af_unix.c | 11 +++++++++--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
> index b3545fc68097..6fd745cfc492 100644
> --- a/net/unix/af_unix.c
> +++ b/net/unix/af_unix.c
> @@ -1999,13 +1999,20 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
> unix_state_lock(sk);
>
> err = 0;
> - if (unix_peer(sk) == other) {
> + if (sk->sk_type == SOCK_SEQPACKET) {
> + /* We are here only when racing with unix_release_sock()
> + * is clearing @other. Never change state to TCP_CLOSE
> + * unlike SOCK_DGRAM wants.
> + */
> + unix_state_unlock(sk);
> + err = -EPIPE;
> + } else if (unix_peer(sk) == other) {
> unix_peer(sk) = NULL;
> unix_dgram_peer_wake_disconnect_wakeup(sk, other);
>
> + sk->sk_state = TCP_CLOSE;
> unix_state_unlock(sk);
>
> - sk->sk_state = TCP_CLOSE;
> unix_dgram_disconnected(sk, other);
> sock_put(other);
> err = -ECONNREFUSED;
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH net] unix: Fix race in SOCK_SEQPACKET's unix_dgram_sendmsg()
2022-11-20 9:09 ` Kuniyuki Iwashima
@ 2022-11-20 9:46 ` Kirill Tkhai
2022-11-20 11:43 ` Kirill Tkhai
0 siblings, 1 reply; 6+ messages in thread
From: Kirill Tkhai @ 2022-11-20 9:46 UTC (permalink / raw)
To: Kuniyuki Iwashima; +Cc: davem, edumazet, kuba, netdev, pabeni
On 20.11.2022 12:09, Kuniyuki Iwashima wrote:
> From: Kirill Tkhai <tkhai@ya.ru>
> Date: Sun, 20 Nov 2022 02:16:47 +0300
>> There is a race resulting in alive SOCK_SEQPACKET socket
>> may change its state from TCP_ESTABLISHED to TCP_CLOSE:
>>
>> unix_release_sock(peer) unix_dgram_sendmsg(sk)
>> sock_orphan(peer)
>> sock_set_flag(peer, SOCK_DEAD)
>> sock_alloc_send_pskb()
>> if !(sk->sk_shutdown & SEND_SHUTDOWN)
>> OK
>> if sock_flag(peer, SOCK_DEAD)
>> sk->sk_state = TCP_CLOSE
>> sk->sk_shutdown = SHUTDOWN_MASK
>>
>>
>> After that socket sk remains almost normal: it is able to connect, listen, accept
>> and recvmsg, while it can't sendmsg.
>
> nit: Then, also recvmsg() fails with -ENOTCONN. And after connect(), even
> both of recvmsg() and sendmsg() does not fail.
Possible, I wrote not clear. I mean sendmsg fails after connect, while recvmsg does not fail :)
Here is sendmsg abort path:
unix_dgram_sendmsg()
sock_alloc_send_pskb()
if (sk->sk_shutdown & SEND_SHUTDOWN)
FAIL
> static int unix_seqpacket_recvmsg(struct socket *sock, struct msghdr *msg,
> size_t size, int flags)
> {
> struct sock *sk = sock->sk;
>
> if (sk->sk_state != TCP_ESTABLISHED)
> return -ENOTCONN;
>
> return unix_dgram_recvmsg(sock, msg, size, flags);
> }
>
>
>>
>> Since this is the only possibility for alive SOCK_SEQPACKET to change
>> the state in such way, we should better fix this strange and potentially
>> danger corner case.
>>
>> Also, move TCP_CLOSE assignment for SOCK_DGRAM sockets under state lock.
>>
>> Signed-off-by: Kirill Tkhai <tkhai@ya.ru>
>
> Fixes tag is needed:
>
> Fixes: 83301b5367a9 ("af_unix: Set TCP_ESTABLISHED for datagram sockets too")
>> Before this commit, there was no state change and SEQPACKET sk also went
> through the same path. The bug was introduced because the commit did not
> consider SEAPACKET.
>
> So, I think the fix should be like below, then we can free the peer faster.
> Note unix_dgram_peer_wake_disconnect_wakeup() is dgram-specific too.
>
> ---8<---
> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
> index b3545fc68097..be40023a61fb 100644
> --- a/net/unix/af_unix.c
> +++ b/net/unix/af_unix.c
> @@ -2001,11 +2001,14 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
> err = 0;
> if (unix_peer(sk) == other) {
> unix_peer(sk) = NULL;
> - unix_dgram_peer_wake_disconnect_wakeup(sk, other);
> +
> + if (sk->sk_type == SOCK_DGRAM) {
> + unix_dgram_peer_wake_disconnect_wakeup(sk, other);
> + sk->sk_state = TCP_CLOSE;
> + }
>
> unix_state_unlock(sk);
>
> - sk->sk_state = TCP_CLOSE;
> unix_dgram_disconnected(sk, other);
> sock_put(other);
> err = -ECONNREFUSED;
> ---8<---
>
> Also, it's better to mention that moving TCP_CLOSE under the lock resolves
> another rare race with unix_dgram_connect() for DGRAM sk:
>
> unix_state_unlock(sk);
> <--------------------------> connect() could set TCP_ESTABLISHED here.
> sk->sk_state = TCP_CLOSE;
Sounds good, I'll test this variant. Thanks!
Kirill
>> ---
>> net/unix/af_unix.c | 11 +++++++++--
>> 1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
>> index b3545fc68097..6fd745cfc492 100644
>> --- a/net/unix/af_unix.c
>> +++ b/net/unix/af_unix.c
>> @@ -1999,13 +1999,20 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
>> unix_state_lock(sk);
>>
>> err = 0;
>> - if (unix_peer(sk) == other) {
>> + if (sk->sk_type == SOCK_SEQPACKET) {
>> + /* We are here only when racing with unix_release_sock()
>> + * is clearing @other. Never change state to TCP_CLOSE
>> + * unlike SOCK_DGRAM wants.
>> + */
>> + unix_state_unlock(sk);
>> + err = -EPIPE;
>> + } else if (unix_peer(sk) == other) {
>> unix_peer(sk) = NULL;
>> unix_dgram_peer_wake_disconnect_wakeup(sk, other);
>>
>> + sk->sk_state = TCP_CLOSE;
>> unix_state_unlock(sk);
>>
>> - sk->sk_state = TCP_CLOSE;
>> unix_dgram_disconnected(sk, other);
>> sock_put(other);
>> err = -ECONNREFUSED;
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] unix: Fix race in SOCK_SEQPACKET's unix_dgram_sendmsg()
2022-11-20 9:46 ` Kirill Tkhai
@ 2022-11-20 11:43 ` Kirill Tkhai
2022-11-21 17:16 ` Kuniyuki Iwashima
0 siblings, 1 reply; 6+ messages in thread
From: Kirill Tkhai @ 2022-11-20 11:43 UTC (permalink / raw)
To: Kuniyuki Iwashima; +Cc: davem, edumazet, kuba, netdev, pabeni
On 20.11.2022 12:46, Kirill Tkhai wrote:
> On 20.11.2022 12:09, Kuniyuki Iwashima wrote:
>> From: Kirill Tkhai <tkhai@ya.ru>
>> Date: Sun, 20 Nov 2022 02:16:47 +0300
>>> There is a race resulting in alive SOCK_SEQPACKET socket
>>> may change its state from TCP_ESTABLISHED to TCP_CLOSE:
>>>
>>> unix_release_sock(peer) unix_dgram_sendmsg(sk)
>>> sock_orphan(peer)
>>> sock_set_flag(peer, SOCK_DEAD)
>>> sock_alloc_send_pskb()
>>> if !(sk->sk_shutdown & SEND_SHUTDOWN)
>>> OK
>>> if sock_flag(peer, SOCK_DEAD)
>>> sk->sk_state = TCP_CLOSE
>>> sk->sk_shutdown = SHUTDOWN_MASK
>>>
>>>
>>> After that socket sk remains almost normal: it is able to connect, listen, accept
>>> and recvmsg, while it can't sendmsg.
>>
>> nit: Then, also recvmsg() fails with -ENOTCONN. And after connect(), even
>> both of recvmsg() and sendmsg() does not fail.
>
> Possible, I wrote not clear. I mean sendmsg fails after connect, while recvmsg does not fail :)
> Here is sendmsg abort path:
>
> unix_dgram_sendmsg()
> sock_alloc_send_pskb()
> if (sk->sk_shutdown & SEND_SHUTDOWN)
> FAIL
>
>> static int unix_seqpacket_recvmsg(struct socket *sock, struct msghdr *msg,
>> size_t size, int flags)
>> {
>> struct sock *sk = sock->sk;
>>
>> if (sk->sk_state != TCP_ESTABLISHED)
>> return -ENOTCONN;
>>
>> return unix_dgram_recvmsg(sock, msg, size, flags);
>> }
>>
>>
>>>
>>> Since this is the only possibility for alive SOCK_SEQPACKET to change
>>> the state in such way, we should better fix this strange and potentially
>>> danger corner case.
>>>
>>> Also, move TCP_CLOSE assignment for SOCK_DGRAM sockets under state lock.
>>>
>>> Signed-off-by: Kirill Tkhai <tkhai@ya.ru>
>>
>> Fixes tag is needed:
>>
>> Fixes: 83301b5367a9 ("af_unix: Set TCP_ESTABLISHED for datagram sockets too")
>>> Before this commit, there was no state change and SEQPACKET sk also went
>> through the same path. The bug was introduced because the commit did not
>> consider SEAPACKET.
>>
>> So, I think the fix should be like below, then we can free the peer faster.
>> Note unix_dgram_peer_wake_disconnect_wakeup() is dgram-specific too.
>>
>> ---8<---
>> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
>> index b3545fc68097..be40023a61fb 100644
>> --- a/net/unix/af_unix.c
>> +++ b/net/unix/af_unix.c
>> @@ -2001,11 +2001,14 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
>> err = 0;
>> if (unix_peer(sk) == other) {
>> unix_peer(sk) = NULL;
>> - unix_dgram_peer_wake_disconnect_wakeup(sk, other);
>> +
>> + if (sk->sk_type == SOCK_DGRAM) {
>> + unix_dgram_peer_wake_disconnect_wakeup(sk, other);
>> + sk->sk_state = TCP_CLOSE;
>> + }
>>
>> unix_state_unlock(sk);
>>
>> - sk->sk_state = TCP_CLOSE;
>> unix_dgram_disconnected(sk, other);
>> sock_put(other);
>> err = -ECONNREFUSED;
>> ---8<---
>>
>> Also, it's better to mention that moving TCP_CLOSE under the lock resolves
>> another rare race with unix_dgram_connect() for DGRAM sk:
>>
>> unix_state_unlock(sk);
>> <--------------------------> connect() could set TCP_ESTABLISHED here.
>> sk->sk_state = TCP_CLOSE;
>
> Sounds good, I'll test this variant. Thanks!
Thinking again, I'd argue about disconnecting from peer (unix_peer(sk) = NULL) here.
Normally, unix_dgram_sendmsg() never came here like I wrote:
unix_dgram_sendmsg()
sock_alloc_send_pskb()
if (sk->sk_shutdown & SEND_SHUTDOWN)
FAIL with EPIPE
So, this optimization will work very rare, it fact there will not real performance profit.
But this creates a cornet case (safe but corner), which seems not good. Corner cases are not
good in general.
I'd leave an original variant. What do you think about this?
Kirill
>>> ---
>>> net/unix/af_unix.c | 11 +++++++++--
>>> 1 file changed, 9 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
>>> index b3545fc68097..6fd745cfc492 100644
>>> --- a/net/unix/af_unix.c
>>> +++ b/net/unix/af_unix.c
>>> @@ -1999,13 +1999,20 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
>>> unix_state_lock(sk);
>>>
>>> err = 0;
>>> - if (unix_peer(sk) == other) {
>>> + if (sk->sk_type == SOCK_SEQPACKET) {
>>> + /* We are here only when racing with unix_release_sock()
>>> + * is clearing @other. Never change state to TCP_CLOSE
>>> + * unlike SOCK_DGRAM wants.
>>> + */
>>> + unix_state_unlock(sk);
>>> + err = -EPIPE;
>>> + } else if (unix_peer(sk) == other) {
>>> unix_peer(sk) = NULL;
>>> unix_dgram_peer_wake_disconnect_wakeup(sk, other);
>>>
>>> + sk->sk_state = TCP_CLOSE;
>>> unix_state_unlock(sk);
>>>
>>> - sk->sk_state = TCP_CLOSE;
>>> unix_dgram_disconnected(sk, other);
>>> sock_put(other);
>>> err = -ECONNREFUSED;
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] unix: Fix race in SOCK_SEQPACKET's unix_dgram_sendmsg()
2022-11-20 11:43 ` Kirill Tkhai
@ 2022-11-21 17:16 ` Kuniyuki Iwashima
2022-12-03 10:34 ` Kirill Tkhai
0 siblings, 1 reply; 6+ messages in thread
From: Kuniyuki Iwashima @ 2022-11-21 17:16 UTC (permalink / raw)
To: tkhai; +Cc: davem, edumazet, kuba, kuniyu, netdev, pabeni
From: Kirill Tkhai <tkhai@ya.ru>
Date: Sun, 20 Nov 2022 14:43:06 +0300
> On 20.11.2022 12:46, Kirill Tkhai wrote:
> > On 20.11.2022 12:09, Kuniyuki Iwashima wrote:
> >> From: Kirill Tkhai <tkhai@ya.ru>
> >> Date: Sun, 20 Nov 2022 02:16:47 +0300
> >>> There is a race resulting in alive SOCK_SEQPACKET socket
> >>> may change its state from TCP_ESTABLISHED to TCP_CLOSE:
> >>>
> >>> unix_release_sock(peer) unix_dgram_sendmsg(sk)
> >>> sock_orphan(peer)
> >>> sock_set_flag(peer, SOCK_DEAD)
> >>> sock_alloc_send_pskb()
> >>> if !(sk->sk_shutdown & SEND_SHUTDOWN)
> >>> OK
> >>> if sock_flag(peer, SOCK_DEAD)
> >>> sk->sk_state = TCP_CLOSE
> >>> sk->sk_shutdown = SHUTDOWN_MASK
> >>>
> >>>
> >>> After that socket sk remains almost normal: it is able to connect, listen, accept
> >>> and recvmsg, while it can't sendmsg.
> >>
> >> nit: Then, also recvmsg() fails with -ENOTCONN. And after connect(), even
> >> both of recvmsg() and sendmsg() does not fail.
> >
> > Possible, I wrote not clear. I mean sendmsg fails after connect, while recvmsg does not fail :)
> > Here is sendmsg abort path:
> >
> > unix_dgram_sendmsg()
> > sock_alloc_send_pskb()
> > if (sk->sk_shutdown & SEND_SHUTDOWN)
> > FAIL
> >
> >> static int unix_seqpacket_recvmsg(struct socket *sock, struct msghdr *msg,
> >> size_t size, int flags)
> >> {
> >> struct sock *sk = sock->sk;
> >>
> >> if (sk->sk_state != TCP_ESTABLISHED)
> >> return -ENOTCONN;
> >>
> >> return unix_dgram_recvmsg(sock, msg, size, flags);
> >> }
> >>
> >>
> >>>
> >>> Since this is the only possibility for alive SOCK_SEQPACKET to change
> >>> the state in such way, we should better fix this strange and potentially
> >>> danger corner case.
> >>>
> >>> Also, move TCP_CLOSE assignment for SOCK_DGRAM sockets under state lock.
> >>>
> >>> Signed-off-by: Kirill Tkhai <tkhai@ya.ru>
> >>
> >> Fixes tag is needed:
> >>
> >> Fixes: 83301b5367a9 ("af_unix: Set TCP_ESTABLISHED for datagram sockets too")
> >>> Before this commit, there was no state change and SEQPACKET sk also went
> >> through the same path. The bug was introduced because the commit did not
> >> consider SEAPACKET.
> >>
> >> So, I think the fix should be like below, then we can free the peer faster.
> >> Note unix_dgram_peer_wake_disconnect_wakeup() is dgram-specific too.
> >>
> >> ---8<---
> >> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
> >> index b3545fc68097..be40023a61fb 100644
> >> --- a/net/unix/af_unix.c
> >> +++ b/net/unix/af_unix.c
> >> @@ -2001,11 +2001,14 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
> >> err = 0;
> >> if (unix_peer(sk) == other) {
> >> unix_peer(sk) = NULL;
> >> - unix_dgram_peer_wake_disconnect_wakeup(sk, other);
> >> +
> >> + if (sk->sk_type == SOCK_DGRAM) {
> >> + unix_dgram_peer_wake_disconnect_wakeup(sk, other);
> >> + sk->sk_state = TCP_CLOSE;
> >> + }
> >>
> >> unix_state_unlock(sk);
> >>
> >> - sk->sk_state = TCP_CLOSE;
> >> unix_dgram_disconnected(sk, other);
> >> sock_put(other);
> >> err = -ECONNREFUSED;
> >> ---8<---
> >>
> >> Also, it's better to mention that moving TCP_CLOSE under the lock resolves
> >> another rare race with unix_dgram_connect() for DGRAM sk:
> >>
> >> unix_state_unlock(sk);
> >> <--------------------------> connect() could set TCP_ESTABLISHED here.
> >> sk->sk_state = TCP_CLOSE;
> >
> > Sounds good, I'll test this variant. Thanks!
>
> Thinking again, I'd argue about disconnecting from peer (unix_peer(sk) = NULL) here.
> Normally, unix_dgram_sendmsg() never came here like I wrote:
>
> unix_dgram_sendmsg()
> sock_alloc_send_pskb()
> if (sk->sk_shutdown & SEND_SHUTDOWN)
> FAIL with EPIPE
>
> So, this optimization will work very rare, it fact there will not real performance profit.
> But this creates a cornet case (safe but corner), which seems not good. Corner cases are not
> good in general.
>
> I'd leave an original variant. What do you think about this?
I think there is no good reason to delay freeing unused memory, not only
sock_put(other), unix_dgram_disconnected() frees sk->sk_receive_queue.
And if peer is cleared, we need not call sock_alloc_send_pskb() and
return earlier with -ENOTCONN in the following sendmsg()s. It's easy
to read because we need not dive into another layer implemented in
sock_alloc_send_pskb().
Also, at least, the code has been safe for decades, and if we don't
clear peer and sk_receive_queue, we have to check other places if
there are sanity checks for such cases. IMHO, we should minimize the
risk if the patch is for -stable.
Thank you.
>
> Kirill
>
> >>> ---
> >>> net/unix/af_unix.c | 11 +++++++++--
> >>> 1 file changed, 9 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
> >>> index b3545fc68097..6fd745cfc492 100644
> >>> --- a/net/unix/af_unix.c
> >>> +++ b/net/unix/af_unix.c
> >>> @@ -1999,13 +1999,20 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
> >>> unix_state_lock(sk);
> >>>
> >>> err = 0;
> >>> - if (unix_peer(sk) == other) {
> >>> + if (sk->sk_type == SOCK_SEQPACKET) {
> >>> + /* We are here only when racing with unix_release_sock()
> >>> + * is clearing @other. Never change state to TCP_CLOSE
> >>> + * unlike SOCK_DGRAM wants.
> >>> + */
> >>> + unix_state_unlock(sk);
> >>> + err = -EPIPE;
> >>> + } else if (unix_peer(sk) == other) {
> >>> unix_peer(sk) = NULL;
> >>> unix_dgram_peer_wake_disconnect_wakeup(sk, other);
> >>>
> >>> + sk->sk_state = TCP_CLOSE;
> >>> unix_state_unlock(sk);
> >>>
> >>> - sk->sk_state = TCP_CLOSE;
> >>> unix_dgram_disconnected(sk, other);
> >>> sock_put(other);
> >>> err = -ECONNREFUSED;
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] unix: Fix race in SOCK_SEQPACKET's unix_dgram_sendmsg()
2022-11-21 17:16 ` Kuniyuki Iwashima
@ 2022-12-03 10:34 ` Kirill Tkhai
0 siblings, 0 replies; 6+ messages in thread
From: Kirill Tkhai @ 2022-12-03 10:34 UTC (permalink / raw)
To: Kuniyuki Iwashima; +Cc: davem, edumazet, kuba, netdev, pabeni
On 21.11.2022 20:16, Kuniyuki Iwashima wrote:
> From: Kirill Tkhai <tkhai@ya.ru>
> Date: Sun, 20 Nov 2022 14:43:06 +0300
>> On 20.11.2022 12:46, Kirill Tkhai wrote:
>>> On 20.11.2022 12:09, Kuniyuki Iwashima wrote:
>>>> From: Kirill Tkhai <tkhai@ya.ru>
>>>> Date: Sun, 20 Nov 2022 02:16:47 +0300
>>>>> There is a race resulting in alive SOCK_SEQPACKET socket
>>>>> may change its state from TCP_ESTABLISHED to TCP_CLOSE:
>>>>>
>>>>> unix_release_sock(peer) unix_dgram_sendmsg(sk)
>>>>> sock_orphan(peer)
>>>>> sock_set_flag(peer, SOCK_DEAD)
>>>>> sock_alloc_send_pskb()
>>>>> if !(sk->sk_shutdown & SEND_SHUTDOWN)
>>>>> OK
>>>>> if sock_flag(peer, SOCK_DEAD)
>>>>> sk->sk_state = TCP_CLOSE
>>>>> sk->sk_shutdown = SHUTDOWN_MASK
>>>>>
>>>>>
>>>>> After that socket sk remains almost normal: it is able to connect, listen, accept
>>>>> and recvmsg, while it can't sendmsg.
>>>>
>>>> nit: Then, also recvmsg() fails with -ENOTCONN. And after connect(), even
>>>> both of recvmsg() and sendmsg() does not fail.
>>>
>>> Possible, I wrote not clear. I mean sendmsg fails after connect, while recvmsg does not fail :)
>>> Here is sendmsg abort path:
>>>
>>> unix_dgram_sendmsg()
>>> sock_alloc_send_pskb()
>>> if (sk->sk_shutdown & SEND_SHUTDOWN)
>>> FAIL
>>>
>>>> static int unix_seqpacket_recvmsg(struct socket *sock, struct msghdr *msg,
>>>> size_t size, int flags)
>>>> {
>>>> struct sock *sk = sock->sk;
>>>>
>>>> if (sk->sk_state != TCP_ESTABLISHED)
>>>> return -ENOTCONN;
>>>>
>>>> return unix_dgram_recvmsg(sock, msg, size, flags);
>>>> }
>>>>
>>>>
>>>>>
>>>>> Since this is the only possibility for alive SOCK_SEQPACKET to change
>>>>> the state in such way, we should better fix this strange and potentially
>>>>> danger corner case.
>>>>>
>>>>> Also, move TCP_CLOSE assignment for SOCK_DGRAM sockets under state lock.
>>>>>
>>>>> Signed-off-by: Kirill Tkhai <tkhai@ya.ru>
>>>>
>>>> Fixes tag is needed:
>>>>
>>>> Fixes: 83301b5367a9 ("af_unix: Set TCP_ESTABLISHED for datagram sockets too")
>>>>> Before this commit, there was no state change and SEQPACKET sk also went
>>>> through the same path. The bug was introduced because the commit did not
>>>> consider SEAPACKET.
>>>>
>>>> So, I think the fix should be like below, then we can free the peer faster.
>>>> Note unix_dgram_peer_wake_disconnect_wakeup() is dgram-specific too.
>>>>
>>>> ---8<---
>>>> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
>>>> index b3545fc68097..be40023a61fb 100644
>>>> --- a/net/unix/af_unix.c
>>>> +++ b/net/unix/af_unix.c
>>>> @@ -2001,11 +2001,14 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
>>>> err = 0;
>>>> if (unix_peer(sk) == other) {
>>>> unix_peer(sk) = NULL;
>>>> - unix_dgram_peer_wake_disconnect_wakeup(sk, other);
>>>> +
>>>> + if (sk->sk_type == SOCK_DGRAM) {
>>>> + unix_dgram_peer_wake_disconnect_wakeup(sk, other);
>>>> + sk->sk_state = TCP_CLOSE;
>>>> + }
>>>>
>>>> unix_state_unlock(sk);
>>>>
>>>> - sk->sk_state = TCP_CLOSE;
>>>> unix_dgram_disconnected(sk, other);
>>>> sock_put(other);
>>>> err = -ECONNREFUSED;
>>>> ---8<---
>>>>
>>>> Also, it's better to mention that moving TCP_CLOSE under the lock resolves
>>>> another rare race with unix_dgram_connect() for DGRAM sk:
>>>>
>>>> unix_state_unlock(sk);
>>>> <--------------------------> connect() could set TCP_ESTABLISHED here.
>>>> sk->sk_state = TCP_CLOSE;
>>>
>>> Sounds good, I'll test this variant. Thanks!
>>
>> Thinking again, I'd argue about disconnecting from peer (unix_peer(sk) = NULL) here.
>> Normally, unix_dgram_sendmsg() never came here like I wrote:
>>
>> unix_dgram_sendmsg()
>> sock_alloc_send_pskb()
>> if (sk->sk_shutdown & SEND_SHUTDOWN)
>> FAIL with EPIPE
>>
>> So, this optimization will work very rare, it fact there will not real performance profit.
>> But this creates a cornet case (safe but corner), which seems not good. Corner cases are not
>> good in general.
>>
>> I'd leave an original variant. What do you think about this?
>
> I think there is no good reason to delay freeing unused memory, not only
> sock_put(other), unix_dgram_disconnected() frees sk->sk_receive_queue.
Hm, it's not about freeing memory. This optimization will work almost never, because of the probability
to get into this is very small. This just adds additional race condition, which is bad from any side.
So, finally I don't like this, sorry.
Thanks for your opinion.
Kirill
> And if peer is cleared, we need not call sock_alloc_send_pskb() and
> return earlier with -ENOTCONN in the following sendmsg()s. It's easy
> to read because we need not dive into another layer implemented in
> sock_alloc_send_pskb().
>
> Also, at least, the code has been safe for decades, and if we don't
> clear peer and sk_receive_queue, we have to check other places if
> there are sanity checks for such cases. IMHO, we should minimize the
> risk if the patch is for -stable.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-12-03 10:40 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-19 23:16 [PATCH net] unix: Fix race in SOCK_SEQPACKET's unix_dgram_sendmsg() Kirill Tkhai
2022-11-20 9:09 ` Kuniyuki Iwashima
2022-11-20 9:46 ` Kirill Tkhai
2022-11-20 11:43 ` Kirill Tkhai
2022-11-21 17:16 ` Kuniyuki Iwashima
2022-12-03 10:34 ` Kirill Tkhai
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).