* [PATCH net v2] unix: Fix race in SOCK_SEQPACKET's unix_dgram_sendmsg()
@ 2022-11-26 22:46 Kirill Tkhai
2022-11-26 23:35 ` Kuniyuki Iwashima
2022-12-01 9:30 ` Paolo Abeni
0 siblings, 2 replies; 8+ messages in thread
From: Kirill Tkhai @ 2022-11-26 22:46 UTC (permalink / raw)
To: davem, edumazet, kuba, pabeni, netdev, tkhai, Kuniyuki Iwashima
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
to fix race with unix_dgram_connect():
unix_dgram_connect(other) unix_dgram_sendmsg(sk)
unix_peer(sk) = NULL
unix_state_unlock(sk)
unix_state_double_lock(sk, other)
sk->sk_state = TCP_ESTABLISHED
unix_peer(sk) = other
unix_state_double_unlock(sk, other)
sk->sk_state = TCP_CLOSED
This patch fixes both of these races.
Fixes: 83301b5367a9 ("af_unix: Set TCP_ESTABLISHED for datagram sockets too")
Suggested-by: Kuniyuki Iwashima <kuniyu@amazon.com>
Signed-off-by: Kirill Tkhai <tkhai@ya.ru>
---
v2: Disconnect from peer right there.
net/unix/af_unix.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
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;
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH net v2] unix: Fix race in SOCK_SEQPACKET's unix_dgram_sendmsg()
2022-11-26 22:46 [PATCH net v2] unix: Fix race in SOCK_SEQPACKET's unix_dgram_sendmsg() Kirill Tkhai
@ 2022-11-26 23:35 ` Kuniyuki Iwashima
2022-12-01 9:30 ` Paolo Abeni
1 sibling, 0 replies; 8+ messages in thread
From: Kuniyuki Iwashima @ 2022-11-26 23:35 UTC (permalink / raw)
To: tkhai; +Cc: davem, edumazet, kuba, kuniyu, netdev, pabeni
From: Kirill Tkhai <tkhai@ya.ru>
Date: Sun, 27 Nov 2022 01:46:51 +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.
>
> 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
> to fix race with unix_dgram_connect():
>
> unix_dgram_connect(other) unix_dgram_sendmsg(sk)
> unix_peer(sk) = NULL
> unix_state_unlock(sk)
> unix_state_double_lock(sk, other)
> sk->sk_state = TCP_ESTABLISHED
> unix_peer(sk) = other
> unix_state_double_unlock(sk, other)
> sk->sk_state = TCP_CLOSED
>
> This patch fixes both of these races.
>
> Fixes: 83301b5367a9 ("af_unix: Set TCP_ESTABLISHED for datagram sockets too")
> Suggested-by: Kuniyuki Iwashima <kuniyu@amazon.com>
> Signed-off-by: Kirill Tkhai <tkhai@ya.ru>
Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
Thank you, Kirill.
> ---
> v2: Disconnect from peer right there.
>
> net/unix/af_unix.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> 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;
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH net v2] unix: Fix race in SOCK_SEQPACKET's unix_dgram_sendmsg()
2022-11-26 22:46 [PATCH net v2] unix: Fix race in SOCK_SEQPACKET's unix_dgram_sendmsg() Kirill Tkhai
2022-11-26 23:35 ` Kuniyuki Iwashima
@ 2022-12-01 9:30 ` Paolo Abeni
2022-12-02 22:43 ` Kirill Tkhai
1 sibling, 1 reply; 8+ messages in thread
From: Paolo Abeni @ 2022-12-01 9:30 UTC (permalink / raw)
To: Kirill Tkhai, davem, edumazet, kuba, netdev, Kuniyuki Iwashima
On Sun, 2022-11-27 at 01:46 +0300, Kirill Tkhai wrote:
> 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
> to fix race with unix_dgram_connect():
>
> unix_dgram_connect(other) unix_dgram_sendmsg(sk)
> unix_peer(sk) = NULL
> unix_state_unlock(sk)
> unix_state_double_lock(sk, other)
> sk->sk_state = TCP_ESTABLISHED
> unix_peer(sk) = other
> unix_state_double_unlock(sk, other)
> sk->sk_state = TCP_CLOSED
>
> This patch fixes both of these races.
>
> Fixes: 83301b5367a9 ("af_unix: Set TCP_ESTABLISHED for datagram sockets too")
I don't think this commmit introduces the issues, both behavior
described above appear to be present even before?
Thank!
Paolo
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH net v2] unix: Fix race in SOCK_SEQPACKET's unix_dgram_sendmsg()
2022-12-01 9:30 ` Paolo Abeni
@ 2022-12-02 22:43 ` Kirill Tkhai
2022-12-02 23:18 ` Iwashima, Kuniyuki
2022-12-03 10:37 ` Kirill Tkhai
0 siblings, 2 replies; 8+ messages in thread
From: Kirill Tkhai @ 2022-12-02 22:43 UTC (permalink / raw)
To: Paolo Abeni, davem, edumazet, kuba, netdev, Kuniyuki Iwashima
On 01.12.2022 12:30, Paolo Abeni wrote:
> On Sun, 2022-11-27 at 01:46 +0300, Kirill Tkhai wrote:
>> 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
>> to fix race with unix_dgram_connect():
>>
>> unix_dgram_connect(other) unix_dgram_sendmsg(sk)
>> unix_peer(sk) = NULL
>> unix_state_unlock(sk)
>> unix_state_double_lock(sk, other)
>> sk->sk_state = TCP_ESTABLISHED
>> unix_peer(sk) = other
>> unix_state_double_unlock(sk, other)
>> sk->sk_state = TCP_CLOSED
>>
>> This patch fixes both of these races.
>>
>> Fixes: 83301b5367a9 ("af_unix: Set TCP_ESTABLISHED for datagram sockets too")
>
> I don't think this commmit introduces the issues, both behavior
> described above appear to be present even before?
1)Hm, I pointed to the commit suggested by Kuniyuki without checking it.
Possible, the real problem commit is dc56ad7028c5 "af_unix: fix potential NULL deref in unix_dgram_connect()",
since it added TCP_CLOSED assignment to unix_dgram_sendmsg().
2)What do you think about initial version of fix?
https://patchwork.kernel.org/project/netdevbpf/patch/38a920a7-cfba-7929-886d-c3c6effc0c43@ya.ru/
Despite there are some arguments, I'm not still sure that v2 is better.
Thanks,
Kirill
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH net v2] unix: Fix race in SOCK_SEQPACKET's unix_dgram_sendmsg()
2022-12-02 22:43 ` Kirill Tkhai
@ 2022-12-02 23:18 ` Iwashima, Kuniyuki
2022-12-05 9:22 ` Paolo Abeni
2022-12-03 10:37 ` Kirill Tkhai
1 sibling, 1 reply; 8+ messages in thread
From: Iwashima, Kuniyuki @ 2022-12-02 23:18 UTC (permalink / raw)
To: Kirill Tkhai; +Cc: Paolo Abeni, davem, edumazet, kuba, netdev
> On Dec 3, 2022, at 7:44, Kirill Tkhai <tkhai@ya.ru> wrote:
>> On 01.12.2022 12:30, Paolo Abeni wrote:
>>> On Sun, 2022-11-27 at 01:46 +0300, Kirill Tkhai wrote:
>>> 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
>>> to fix race with unix_dgram_connect():
>>>
>>> unix_dgram_connect(other) unix_dgram_sendmsg(sk)
>>> unix_peer(sk) = NULL
>>> unix_state_unlock(sk)
>>> unix_state_double_lock(sk, other)
>>> sk->sk_state = TCP_ESTABLISHED
>>> unix_peer(sk) = other
>>> unix_state_double_unlock(sk, other)
>>> sk->sk_state = TCP_CLOSED
>>>
>>> This patch fixes both of these races.
>>>
>>> Fixes: 83301b5367a9 ("af_unix: Set TCP_ESTABLISHED for datagram sockets too")
>>
>> I don't think this commmit introduces the issues, both behavior
>> described above appear to be present even before?
>
> 1)Hm, I pointed to the commit suggested by Kuniyuki without checking it.
>
> Possible, the real problem commit is dc56ad7028c5 "af_unix: fix potential NULL deref in unix_dgram_connect()",
> since it added TCP_CLOSED assignment to unix_dgram_sendmsg().
The commit just moved the assignment.
Note unix_dgram_disconnected() is called for SOCK_SEQPACKET
after releasing the lock, and 83301b5367a9 introduced the
TCP_CLOSE assignment.
> 2)What do you think about initial version of fix?
>
> https://patchwork.kernel.org/project/netdevbpf/patch/38a920a7-cfba-7929-886d-c3c6effc0c43@ya.ru/
>
> Despite there are some arguments, I'm not still sure that v2 is better.
>
> Thanks,
> Kirill
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH net v2] unix: Fix race in SOCK_SEQPACKET's unix_dgram_sendmsg()
2022-12-02 23:18 ` Iwashima, Kuniyuki
@ 2022-12-05 9:22 ` Paolo Abeni
2022-12-05 17:07 ` Kirill Tkhai
0 siblings, 1 reply; 8+ messages in thread
From: Paolo Abeni @ 2022-12-05 9:22 UTC (permalink / raw)
To: Iwashima, Kuniyuki, Kirill Tkhai; +Cc: davem, edumazet, kuba, netdev
On Fri, 2022-12-02 at 23:18 +0000, Iwashima, Kuniyuki wrote:
>
> > On Dec 3, 2022, at 7:44, Kirill Tkhai <tkhai@ya.ru> wrote:
> > > On 01.12.2022 12:30, Paolo Abeni wrote:
> > > > On Sun, 2022-11-27 at 01:46 +0300, Kirill Tkhai wrote:
> > > > 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
> > > > to fix race with unix_dgram_connect():
> > > >
> > > > unix_dgram_connect(other) unix_dgram_sendmsg(sk)
> > > > unix_peer(sk) = NULL
> > > > unix_state_unlock(sk)
> > > > unix_state_double_lock(sk, other)
> > > > sk->sk_state = TCP_ESTABLISHED
> > > > unix_peer(sk) = other
> > > > unix_state_double_unlock(sk, other)
> > > > sk->sk_state = TCP_CLOSED
> > > >
> > > > This patch fixes both of these races.
> > > >
> > > > Fixes: 83301b5367a9 ("af_unix: Set TCP_ESTABLISHED for datagram sockets too")
> > >
> > > I don't think this commmit introduces the issues, both behavior
> > > described above appear to be present even before?
> >
> > 1)Hm, I pointed to the commit suggested by Kuniyuki without checking it.
> >
> > Possible, the real problem commit is dc56ad7028c5 "af_unix: fix potential NULL deref in unix_dgram_connect()",
> > since it added TCP_CLOSED assignment to unix_dgram_sendmsg().
>
> The commit just moved the assignment.
>
> Note unix_dgram_disconnected() is called for SOCK_SEQPACKET
> after releasing the lock, and 83301b5367a9 introduced the
> TCP_CLOSE assignment.
I'm sorry for the back and forth, I think I initally misread the code.
I agree 83301b5367a9 is good fixes tag.
> > 2)What do you think about initial version of fix?
> >
> > https://patchwork.kernel.org/project/netdevbpf/patch/38a920a7-cfba-7929-886d-c3c6effc0c43@ya.ru/
> >
> > Despite there are some arguments, I'm not still sure that v2 is better.
v1 introduces quite a few behavior changes (different error code,
different cleanup schema) that could be IMHO more risky for a stable
patch. I suggest to pick the minimal change that addresses the issue
(v2 in this case).
Thanks,
Paolo
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH net v2] unix: Fix race in SOCK_SEQPACKET's unix_dgram_sendmsg()
2022-12-05 9:22 ` Paolo Abeni
@ 2022-12-05 17:07 ` Kirill Tkhai
0 siblings, 0 replies; 8+ messages in thread
From: Kirill Tkhai @ 2022-12-05 17:07 UTC (permalink / raw)
To: Paolo Abeni, Iwashima, Kuniyuki; +Cc: davem, edumazet, kuba, netdev
On 05.12.2022 12:22, Paolo Abeni wrote:
> On Fri, 2022-12-02 at 23:18 +0000, Iwashima, Kuniyuki wrote:
>>
>>> On Dec 3, 2022, at 7:44, Kirill Tkhai <tkhai@ya.ru> wrote:
>>>> On 01.12.2022 12:30, Paolo Abeni wrote:
>>>>> On Sun, 2022-11-27 at 01:46 +0300, Kirill Tkhai wrote:
>>>>> 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
>>>>> to fix race with unix_dgram_connect():
>>>>>
>>>>> unix_dgram_connect(other) unix_dgram_sendmsg(sk)
>>>>> unix_peer(sk) = NULL
>>>>> unix_state_unlock(sk)
>>>>> unix_state_double_lock(sk, other)
>>>>> sk->sk_state = TCP_ESTABLISHED
>>>>> unix_peer(sk) = other
>>>>> unix_state_double_unlock(sk, other)
>>>>> sk->sk_state = TCP_CLOSED
>>>>>
>>>>> This patch fixes both of these races.
>>>>>
>>>>> Fixes: 83301b5367a9 ("af_unix: Set TCP_ESTABLISHED for datagram sockets too")
>>>>
>>>> I don't think this commmit introduces the issues, both behavior
>>>> described above appear to be present even before?
>>>
>>> 1)Hm, I pointed to the commit suggested by Kuniyuki without checking it.
>>>
>>> Possible, the real problem commit is dc56ad7028c5 "af_unix: fix potential NULL deref in unix_dgram_connect()",
>>> since it added TCP_CLOSED assignment to unix_dgram_sendmsg().
>>
>> The commit just moved the assignment.
>>
>> Note unix_dgram_disconnected() is called for SOCK_SEQPACKET
>> after releasing the lock, and 83301b5367a9 introduced the
>> TCP_CLOSE assignment.
>
> I'm sorry for the back and forth, I think I initally misread the code.
> I agree 83301b5367a9 is good fixes tag.
>
>>> 2)What do you think about initial version of fix?
>>>
>>> https://patchwork.kernel.org/project/netdevbpf/patch/38a920a7-cfba-7929-886d-c3c6effc0c43@ya.ru/
>>>
>>> Despite there are some arguments, I'm not still sure that v2 is better.
>
> v1 introduces quite a few behavior changes (different error code,
> different cleanup schema) that could be IMHO more risky for a stable
> patch. I suggest to pick the minimal change that addresses the issue
> (v2 in this case).
Hm, not exactly. EPIPE is regular return value, which is normally returned from
unix_dgram_sendmsg()->sock_alloc_send_pskb (see SEND_SHUTDOWN check).
ECONNREFUSED is a race case return value, it does not returned normally.
What different cleanup scheme do you mean? IMO, there is the same behavior
as we get, when race is failed, and sock_alloc_send_pskb() returns EPIPE as in regular case.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net v2] unix: Fix race in SOCK_SEQPACKET's unix_dgram_sendmsg()
2022-12-02 22:43 ` Kirill Tkhai
2022-12-02 23:18 ` Iwashima, Kuniyuki
@ 2022-12-03 10:37 ` Kirill Tkhai
1 sibling, 0 replies; 8+ messages in thread
From: Kirill Tkhai @ 2022-12-03 10:37 UTC (permalink / raw)
To: Paolo Abeni, davem, edumazet, kuba, netdev, Kuniyuki Iwashima
On 03.12.2022 01:43, Kirill Tkhai wrote:
> On 01.12.2022 12:30, Paolo Abeni wrote:
>> On Sun, 2022-11-27 at 01:46 +0300, Kirill Tkhai wrote:
>>> 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
>>> to fix race with unix_dgram_connect():
>>>
>>> unix_dgram_connect(other) unix_dgram_sendmsg(sk)
>>> unix_peer(sk) = NULL
>>> unix_state_unlock(sk)
>>> unix_state_double_lock(sk, other)
>>> sk->sk_state = TCP_ESTABLISHED
>>> unix_peer(sk) = other
>>> unix_state_double_unlock(sk, other)
>>> sk->sk_state = TCP_CLOSED
>>>
>>> This patch fixes both of these races.
>>>
>>> Fixes: 83301b5367a9 ("af_unix: Set TCP_ESTABLISHED for datagram sockets too")
>>
>> I don't think this commmit introduces the issues, both behavior
>> described above appear to be present even before?
>
> 1)Hm, I pointed to the commit suggested by Kuniyuki without checking it.
>
> Possible, the real problem commit is dc56ad7028c5 "af_unix: fix potential NULL deref in unix_dgram_connect()",
> since it added TCP_CLOSED assignment to unix_dgram_sendmsg().
>
> 2)What do you think about initial version of fix?
>
> https://patchwork.kernel.org/project/netdevbpf/patch/38a920a7-cfba-7929-886d-c3c6effc0c43@ya.ru/
>
> Despite there are some arguments, I'm not still sure that v2 is better.
Rethinking again, I think v1 is better, and we don't have to introduce optimizations,
which works only in very rare race cases. So, I'm going to return to V1 version,
which is better.
Kirill
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-12-05 17:07 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-26 22:46 [PATCH net v2] unix: Fix race in SOCK_SEQPACKET's unix_dgram_sendmsg() Kirill Tkhai
2022-11-26 23:35 ` Kuniyuki Iwashima
2022-12-01 9:30 ` Paolo Abeni
2022-12-02 22:43 ` Kirill Tkhai
2022-12-02 23:18 ` Iwashima, Kuniyuki
2022-12-05 9:22 ` Paolo Abeni
2022-12-05 17:07 ` Kirill Tkhai
2022-12-03 10:37 ` Kirill Tkhai
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.