Avoid tcp_done() on already closed sockets

[matttbe]

Calling tcp_done() on already closed sockets is illegal.

To catch the issue early, it would be good if the CI could look at that:

diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 33f559f491c8..d21ec9bdd453 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -4639,6 +4639,8 @@ void tcp_done(struct sock *sk)
         */
        req = rcu_dereference_protected(tcp_sk(sk)->fastopen_rsk, 1);

+       WARN_ON_ONCE(sk->sk_state == TCP_CLOSE);
+
        if (sk->sk_state == TCP_SYN_SENT || sk->sk_state == TCP_SYN_RECV)
                TCP_INC_STATS(sock_net(sk), TCP_MIB_ATTEMPTFAILS);

But before, we need to fix code already doing that.

EDIT: we can also add a WARN in tcp_set_state() instead:

diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 33f559f491c8..13ae0f598bd2 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -2740,6 +2740,8 @@ void tcp_set_state(struct sock *sk, int state)
                break;
 
        case TCP_CLOSE:
+               WARN_ON_ONCE(oldstate == TCP_CLOSE);
+
                if (oldstate == TCP_CLOSE_WAIT || oldstate == TCP_ESTABLISHED)
                        TCP_INC_STATS(sock_net(sk), TCP_MIB_ESTABRESETS);
 

[matttbe]

First WARN:

TAP version 13
1..1
++ ./mptcp_join.sh 1 2 3
[    1.397879] IPv6: ADDRCONF(NETDEV_CHANGE): eth0: link becomes ready
# Created /tmp/tmp.H6IM7tU3EY (size 1 KB) containing data sent by client
# Created /tmp/tmp.Bn7dnDuyFb (size 1 KB) containing data sent by server
[    1.713704] ip (164) used greatest stack depth: 12616 bytes left
[    1.775312] IPv6: ADDRCONF(NETDEV_CHANGE): ns1eth1: link becomes ready
[    1.878457] IPv6: ADDRCONF(NETDEV_CHANGE): ns1eth2: link becomes ready
[    1.972804] IPv6: ADDRCONF(NETDEV_CHANGE): ns1eth3: link becomes ready
[    2.087505] IPv6: ADDRCONF(NETDEV_CHANGE): ns1eth4: link becomes ready
# 001 no JOIN                              syn[ ok ] - synack[ ok ] - ack[ ok ]
[    2.912059] ip (258) used greatest stack depth: 12592 bytes left
[    2.975145] IPv6: ADDRCONF(NETDEV_CHANGE): ns1eth1: link becomes ready
[    3.072293] IPv6: ADDRCONF(NETDEV_CHANGE): ns1eth2: link becomes ready
[    3.166476] IPv6: ADDRCONF(NETDEV_CHANGE): ns1eth3: link becomes ready
[    3.265253] IPv6: ADDRCONF(NETDEV_CHANGE): ns1eth4: link becomes ready
# 002 single subflow, limited by client    syn[ ok ] - synack[ ok ] - ack[ ok ]
[    4.102468] IPv6: ADDRCONF(NETDEV_CHANGE): ns1eth1: link becomes ready
[    4.213885] IPv6: ADDRCONF(NETDEV_CHANGE): ns1eth2: link becomes ready
[    4.325856] IPv6: ADDRCONF(NETDEV_CHANGE): ns1eth3: link becomes ready
[    4.441443] IPv6: ADDRCONF(NETDEV_CHANGE): ns1eth4: link becomes ready
[    4.601526] ------------[ cut here ]------------
[    4.601919] WARNING: CPU: 0 PID: 180 at net/ipv4/tcp.c:4642 tcp_done (net/ipv4/tcp.c:4642 (discriminator 1))
[    4.602473] Modules linked in:
[    4.602683] CPU: 0 PID: 180 Comm: kworker/0:2 Not tainted 6.2.0-rc8-48357-gdfa25741f79e-dirty #122
[    4.603251] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.15.0-1 04/01/2014
[    4.603772] Workqueue: events mptcp_worker
[    4.604059] RIP: 0010:tcp_done (net/ipv4/tcp.c:4642 (discriminator 1))
[ 4.604343] Code: bd 80 00 00 00 b8 ff ff ff ff f0 0f c1 85 80 00 00 00 83 f8 01 0f 8f 58 ff ff ff be 04 00 00 00 e8 1a a2 c4 ff e9 49 ff ff ff <0f> 0b e9 10 ff ff ff 66 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 00 0f
All code
========
   0:   bd 80 00 00 00          mov    $0x80,%ebp
   5:   b8 ff ff ff ff          mov    $0xffffffff,%eax
   a:   f0 0f c1 85 80 00 00    lock xadd %eax,0x80(%rbp)
  11:   00
  12:   83 f8 01                cmp    $0x1,%eax
  15:   0f 8f 58 ff ff ff       jg     0xffffffffffffff73
  1b:   be 04 00 00 00          mov    $0x4,%esi
  20:   e8 1a a2 c4 ff          call   0xffffffffffc4a23f
  25:   e9 49 ff ff ff          jmp    0xffffffffffffff73
  2a:*  0f 0b                   ud2             <-- trapping instruction
  2c:   e9 10 ff ff ff          jmp    0xffffffffffffff41
  31:   66 66 2e 0f 1f 84 00    data16 cs nopw 0x0(%rax,%rax,1)
  38:   00 00 00 00
  3c:   0f 1f 00                nopl   (%rax)
  3f:   0f                      .byte 0xf

Code starting with the faulting instruction
===========================================
   0:   0f 0b                   ud2
   2:   e9 10 ff ff ff          jmp    0xffffffffffffff17
   7:   66 66 2e 0f 1f 84 00    data16 cs nopw 0x0(%rax,%rax,1)
   e:   00 00 00 00
  12:   0f 1f 00                nopl   (%rax)
  15:   0f                      .byte 0xf
[    4.605505] RSP: 0018:ffffad38004339f8 EFLAGS: 00010246
[    4.605858] RAX: 0000000000000007 RBX: ffffa25c82f78700 RCX: 0000000000000000
[    4.606340] RDX: 0000000000000001 RSI: ffffffff9ad8a08a RDI: ffffa25c832388c0
[    4.606800] RBP: ffffa25c832388c0 R08: ffffa25c8132ed88 R09: 0000000000000101
[    4.607276] R10: ffffffff9b0060e0 R11: 0000000000000000 R12: 0000000000000000
[    4.607736] R13: ffffa25c83230080 R14: ffffa25c83230000 R15: ffffa25c832388c0
[    4.608205] FS:  0000000000000000(0000) GS:ffffa25cfdc00000(0000) knlGS:0000000000000000
[    4.608719] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[    4.609104] CR2: 00007ffd59ce9488 CR3: 0000000003194000 CR4: 0000000000350ef0
[    4.609567] Call Trace:
[    4.609754]  <TASK>
[    4.609927] mptcp_subflow_reset (arch/x86/include/asm/bitops.h:138 include/asm-generic/bitops/instrumented-atomic.h:72 net/mptcp/subflow.c:412)
[    4.610241] mptcp_incoming_options (net/mptcp/options.c:1005 net/mptcp/options.c:1127)
[    4.610557] ? __tcp_transmit_skb (net/ipv4/tcp_output.c:1399)
[    4.610872] ? subflow_finish_connect (net/mptcp/subflow.c:473)
[    4.611183] ? sock_alloc_send_pskb (net/core/sock.c:2745)
[    4.611475] tcp_reset (net/ipv4/tcp_input.c:4320)
[    4.611704] tcp_validate_incoming (net/ipv4/tcp_input.c:5818)
[    4.612005] tcp_rcv_established (net/ipv4/tcp_input.c:6002)
[    4.612278] tcp_v4_do_rcv (net/ipv4/tcp_ipv4.c:1767)
[    4.612509] __release_sock (include/linux/sched.h:2071 net/core/sock.c:2923)
[    4.612735] release_sock (net/core/sock.c:3493)
[    4.612965] inet_stream_connect (net/ipv4/af_inet.c:733)
[    4.613222] __mptcp_subflow_connect (net/mptcp/subflow.c:1623)
[    4.613511] mptcp_pm_create_subflow_or_signal_addr (net/mptcp/pm_netlink.c:606 (discriminator 3))
[    4.613896] ? _raw_spin_unlock (arch/x86/include/asm/preempt.h:104 include/linux/spinlock_api_smp.h:143 kernel/locking/spinlock.c:186)
[    4.614166] ? __schedule (kernel/sched/core.c:6614)
[    4.614413] ? preempt_count_add (include/linux/ftrace.h:980 kernel/sched/core.c:5737 kernel/sched/core.c:5734 kernel/sched/core.c:5762)
[    4.614684] ? __lock_sock (net/core/sock.c:2903)
[    4.614932] mptcp_pm_nl_work (net/mptcp/pm_netlink.c:880)
[    4.615189] mptcp_worker (arch/x86/include/asm/bitops.h:162 include/asm-generic/bitops/instrumented-atomic.h:86 net/mptcp/protocol.c:2699)
[    4.615422] process_one_work (kernel/workqueue.c:2294)
[    4.615679] worker_thread (include/linux/list.h:292 kernel/workqueue.c:2437)
[    4.615927] ? process_one_work (kernel/workqueue.c:2379)
[    4.616197] kthread (kernel/kthread.c:376)
[    4.616408] ? kthread_complete_and_exit (kernel/kthread.c:331)
[    4.616714] ret_from_fork (arch/x86/entry/entry_64.S:314)
[    4.616964]  </TASK>
[    4.617122] ---[ end trace 0000000000000000 ]---
# 003 single subflow, limited by server    syn[ ok ] - synack[ ok ] - ack[ ok ]
++ rc=0

This one is a bit obvious: the state is changed to CLOSE then tcp_done() is called:

mptcp_net-next/net/mptcp/subflow.c

Lines 409 to 411 in 0ca349b

	tcp_set_state(ssk, TCP_CLOSE);
	tcp_send_active_reset(ssk, GFP_ATOMIC);
	tcp_done(ssk);

I guess we can remove tcp_set_state(ssk, TCP_CLOSE);, no?

[matttbe]

FYI, after having applied this patch, I could not see any others when running the selftests + packetdrill:

diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 28c64811a8af..a36d1b1088d7 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -406,7 +406,6 @@ void mptcp_subflow_reset(struct sock *ssk)
        /* must hold: tcp_done() could drop last reference on parent */
        sock_hold(sk);
 
-       tcp_set_state(ssk, TCP_CLOSE);
        tcp_send_active_reset(ssk, GFP_ATOMIC);
        tcp_done(ssk);
        if (!test_and_set_bit(MPTCP_WORK_CLOSE_SUBFLOW, &mptcp_sk(sk)->flags) &&