[MPTCP] Re: [PATCH v2 9/9] don't flag parent socket as RCV_SHUTDOWN if one one subflow has closed

[MPTCP] Re: [PATCH v2 9/9] don't flag parent socket as RCV_SHUTDOWN if one one subflow has closed

[Florian Westphal]

[-- Attachment #1: Type: text/plain, Size: 2060 bytes --]

Matthieu Baerts <matthieu.baerts(a)tessares.net> wrote:
> 00:07:58.688 [   14.025780] #PF: supervisor read access in kernel mode
> 00:07:58.689 [   14.026588] #PF: error_code(0x0000) - not-present page
> 00:07:58.690 [   14.027394] PGD 0 P4D 0
> 00:07:58.690 [   14.027801] Oops: 0000 [#1] SMP PTI
> 00:07:58.691 [   14.028355] CPU: 0 PID: 731 Comm: mptcp_connect Not tainted
> 5.4.0+ #5
> 00:07:58.692 [   14.029365] Hardware name: QEMU Standard PC (i440FX + PIIX,
> 1996), BIOS 1.10.2-1ubuntu1 04/01/2014
> 00:07:58.693 [   14.030752] RIP: 0010:mptcp_stream_accept+0x8d/0x160
> 00:07:58.694 [   14.031533] Code: 8b 9c 24 b0 05 00 00 49 81 c4 b0 05 00 00
> 4c 39 e3 74 71 49 8d 76 40 48 89 74 24 08 eb 08 48 8b 1b 4c 39 e3 74 5e 48
> 8b 6b 78 <48> 83 bd 78 02 00 00 00 75 ea 4c 8d bd 28 02 00 00 89 44 24 04 4c
> 00:07:58.697 [   14.034405] RSP: 0018:ffffad8d8044bdf0 EFLAGS: 00010283
> 00:07:58.698 [   14.035218] RAX: 0000000000000000 RBX: ffff96f35d0d9928 RCX:
> 0000000000000001

The faulty commit is

    make accept not allocate kernel socket struct


When doing the last version I broke tcp-ipv6 fallback.  This fixes
things:

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -1512,6 +1512,15 @@ static int mptcp_listen(struct socket *sock, int backlog)
 	return err;
 }
 
+static bool is_tcp_proto(const struct proto *p)
+{
+#ifdef CONFIG_MPTCP_IPV6
+	return p == &tcp_prot || p == &tcpv6_prot;
+#else
+	return p == &tcp_prot;
+#endif
+}
+
 static int mptcp_stream_accept(struct socket *sock, struct socket *newsock,
 			       int flags, bool kern)
 {
@@ -1526,7 +1535,7 @@ static int mptcp_stream_accept(struct socket *sock, struct socket *newsock,
 		return -EINVAL;
 
 	err = ssock->ops->accept(sock, newsock, flags, kern);
-	if (err == 0 && newsock->sk->sk_prot != &tcp_prot) {
+	if (err == 0 && !is_tcp_proto(newsock->sk->sk_prot)) {
 		struct mptcp_sock *msk = mptcp_sk(newsock->sk);
 		struct mptcp_subflow_context *subflow;
 

[MPTCP] Re: [PATCH v2 9/9] don't flag parent socket as RCV_SHUTDOWN if one one subflow has closed

[Matthieu Baerts]

[-- Attachment #1: Type: text/plain, Size: 2723 bytes --]

Hi Florian,

On 06/12/2019 17:37, Florian Westphal wrote:
> Matthieu Baerts <matthieu.baerts(a)tessares.net> wrote:
>> 00:07:58.688 [   14.025780] #PF: supervisor read access in kernel mode
>> 00:07:58.689 [   14.026588] #PF: error_code(0x0000) - not-present page
>> 00:07:58.690 [   14.027394] PGD 0 P4D 0
>> 00:07:58.690 [   14.027801] Oops: 0000 [#1] SMP PTI
>> 00:07:58.691 [   14.028355] CPU: 0 PID: 731 Comm: mptcp_connect Not tainted
>> 5.4.0+ #5
>> 00:07:58.692 [   14.029365] Hardware name: QEMU Standard PC (i440FX + PIIX,
>> 1996), BIOS 1.10.2-1ubuntu1 04/01/2014
>> 00:07:58.693 [   14.030752] RIP: 0010:mptcp_stream_accept+0x8d/0x160
>> 00:07:58.694 [   14.031533] Code: 8b 9c 24 b0 05 00 00 49 81 c4 b0 05 00 00
>> 4c 39 e3 74 71 49 8d 76 40 48 89 74 24 08 eb 08 48 8b 1b 4c 39 e3 74 5e 48
>> 8b 6b 78 <48> 83 bd 78 02 00 00 00 75 ea 4c 8d bd 28 02 00 00 89 44 24 04 4c
>> 00:07:58.697 [   14.034405] RSP: 0018:ffffad8d8044bdf0 EFLAGS: 00010283
>> 00:07:58.698 [   14.035218] RAX: 0000000000000000 RBX: ffff96f35d0d9928 RCX:
>> 0000000000000001
> 
> The faulty commit is
> 
>      make accept not allocate kernel socket struct
> 
> 
> When doing the last version I broke tcp-ipv6 fallback.  This fixes
> things:

Thank you very much for the quick reply and the fix!

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -1512,6 +1512,15 @@ static int mptcp_listen(struct socket *sock, int backlog)
>   	return err;
>   }
>   
> +static bool is_tcp_proto(const struct proto *p)
> +{
> +#ifdef CONFIG_MPTCP_IPV6
> +	return p == &tcp_prot || p == &tcpv6_prot;
> +#else
> +	return p == &tcp_prot;
> +#endif
> +}
> +
>   static int mptcp_stream_accept(struct socket *sock, struct socket *newsock,
>   			       int flags, bool kern)
>   {
> @@ -1526,7 +1535,7 @@ static int mptcp_stream_accept(struct socket *sock, struct socket *newsock,
>   		return -EINVAL;
>   
>   	err = ssock->ops->accept(sock, newsock, flags, kern);
> -	if (err == 0 && newsock->sk->sk_prot != &tcp_prot) {
> +	if (err == 0 && !is_tcp_proto(newsock->sk->sk_prot)) {
>   		struct mptcp_sock *msk = mptcp_sk(newsock->sk);
>   		struct mptcp_subflow_context *subflow;
>   
> 

- e927b98aa468: "squashed" in "make accept not allocate kernel socket 
struct (the commit is not properly formatted)
- ff06521bed5e..87b25c96264c

Tests are in progress, also checking each commit.
I will notify you when it's all done!

Cheers,
Matt
-- 
Matthieu Baerts | R&D Engineer
matthieu.baerts(a)tessares.net
Tessares SA | Hybrid Access Solutions
www.tessares.net
1 Avenue Jean Monnet, 1348 Louvain-la-Neuve, Belgium

[MPTCP] Re: [PATCH v2 9/9] don't flag parent socket as RCV_SHUTDOWN if one one subflow has closed

[Florian Westphal]

[-- Attachment #1: Type: text/plain, Size: 1077 bytes --]

Matthieu Baerts <matthieu.baerts(a)tessares.net> wrote:
> - 192c07c3fe9b: "squashed" patch 1/9 in "mptcp: Add handling of incoming
> MP_JOIN requests"
> - d6c1939a16c5..2cc726803b47: result
> - 0c24eaeadf60: "squashed" patch 2/9, part 1, in "mptcp: Add handling of
> incoming MP_JOIN requests"
> - f2a1eeb27eab: "squashed" patch 2/9, part 2, in "mptcp: Add handling of
> outgoing MP_JOIN requests"
> - 216b0b82f102: conflict in t/mptcp-introduce-MPTCP-retransmission-timer
> - e1012595cc3d: conflict in t/mptcp-add-and-use-mptcp-RTX-flag
> - 2cc726803b47..48f66afb6a81: result
> - the other patches have been added at the end. I had some conflicts with:
>  - "copy connection id from first subflow to mptcp socket"
>  - "make accept not allocate kernel socket struct"
>  - "subflow: place further subflows on new 'join_list'"
> 
> Tests on top of the commit introducing the kselftests are OK but not the one
> at the end of the series (export branch). Here is the trace (without
> CONFIG_KASAN nor PROVE_LOCKING)

I will have a look, thanks for reporting.

[MPTCP] Re: [PATCH v2 9/9] don't flag parent socket as RCV_SHUTDOWN if one one subflow has closed

[Matthieu Baerts]

[-- Attachment #1: Type: text/plain, Size: 9842 bytes --]

Hi Florian, Paolo, Mat,

On 03/12/2019 10:13, Matthieu Baerts wrote:
> Hi Paolo,
> 
> On 03/12/2019 10:05, Paolo Abeni wrote:
>> On Tue, 2019-12-03 at 08:44 +0100, Matthieu Baerts wrote:
>>> Hi Florian, Paolo, Mat,
>>>
>>> On 02/12/2019 22:21, Paolo Abeni wrote:
>>>> On Mon, 2019-12-02 at 20:10 +0100, Florian Westphal wrote:
>>>
>>> [...]
>>>
>>>> Thanks for addressing my comments.
>>>>
>>>> With 9/9 v2 the series is IMHO ready to me merged.
>>>
>>> Thank you for the nice patch series and the reviews!
>>> Related to what we discussed yesterday, is it OK if I merge this after:
>>>
>>>    1) DATA_FIN
>>>    2) MPTCPv1 → wait a bit for a review
>>>    3) SendMsg "refactor" → if reviewed
>>>
>>> Or can I do that after the patches for MPTCPv1 if the patches to fix
>>> sendmsg() have not been reviewed yet?
>>
>> I think/hope this and sendmsg patches should not conflict much - but
>> sendmsg patches do create quite of conflict with the following patches.
>>
>> I think that appling this series after v1 could be a good option, will
>> give extra time to review the sendmsg() patches, and I can rebase my
>> tree to help the merging.
> 
> Thank you for your reply!
> 
> Sounds good to me. If there is nothing to change in sendmsg() patches 
> after the review, I can also start applying them. If I am blocked, I can 
> ask for help.

Thank you again for the patches and the reviews!

- 192c07c3fe9b: "squashed" patch 1/9 in "mptcp: Add handling of incoming 
MP_JOIN requests"
- d6c1939a16c5..2cc726803b47: result
- 0c24eaeadf60: "squashed" patch 2/9, part 1, in "mptcp: Add handling of 
incoming MP_JOIN requests"
- f2a1eeb27eab: "squashed" patch 2/9, part 2, in "mptcp: Add handling of 
outgoing MP_JOIN requests"
- 216b0b82f102: conflict in t/mptcp-introduce-MPTCP-retransmission-timer
- e1012595cc3d: conflict in t/mptcp-add-and-use-mptcp-RTX-flag
- 2cc726803b47..48f66afb6a81: result
- the other patches have been added at the end. I had some conflicts with:
  - "copy connection id from first subflow to mptcp socket"
  - "make accept not allocate kernel socket struct"
  - "subflow: place further subflows on new 'join_list'"

Tests on top of the commit introducing the kselftests are OK but not the 
one at the end of the series (export branch). Here is the trace (without 
CONFIG_KASAN nor PROVE_LOCKING)



00:07:55.647 # INFO: set ns3-5dea74bc-S7alT1 dev ns3eth2: ethtool -K tso 
off gro off
00:07:55.658 # INFO: set ns4-5dea74bc-S7alT1 dev ns4eth3: ethtool -K 
gro off
00:07:55.685 # Created /tmp/tmp.FSDFbTHqEa (size 3070486) containing 
data sent by client
00:07:55.694 # Created /tmp/tmp.klmXQh5uiX (size 375225) containing data 
sent by server
00:07:55.742 # New MPTCP socket can be blocked via sysctl		[ OK ]
00:07:55.763 # setsockopt(..., TCP_ULP, "mptcp", ...) blocked	[ OK ]
00:07:55.770 # INFO: validating network environment with pings
00:07:56.500 [   11.837548] IPv6: ADDRCONF(NETDEV_CHANGE): ns2eth1: link 
becomes ready
00:07:58.413 # INFO: Using loss of 0.10% delay 87 ms reorder 96% 42% on 
ns3eth4
00:07:58.420 # ns1 MPTCP -> ns1 (10.0.1.1:10000      ) MPTCP	(duration 
  19ms) [ OK ]
00:07:58.471 # ns1 MPTCP -> ns1 (10.0.1.1:10001      ) TCP  	(duration 
  18ms) [ OK ]
00:07:58.518 # ns1 TCP   -> ns1 (10.0.1.1:10002      ) MPTCP	(duration 
  18ms) [ OK ]
00:07:58.566 # ns1 MPTCP -> ns1 (dead:beef:1::1:10003) MPTCP	(duration 
  17ms) [ OK ]
00:07:58.612 # ns1 MPTCP -> ns1 (dead:beef:1::1:10004) TCP  	(duration 
  18ms) [ OK ]
00:07:58.660 # ns1 TCP   -> ns1 (dead:beef:1::1:10005) MPTCP	[ 
14.024584] BUG: unable to handle page fault for address: 00000003435d11af
00:07:58.688 [   14.025780] #PF: supervisor read access in kernel mode
00:07:58.689 [   14.026588] #PF: error_code(0x0000) - not-present page
00:07:58.690 [   14.027394] PGD 0 P4D 0
00:07:58.690 [   14.027801] Oops: 0000 [#1] SMP PTI
00:07:58.691 [   14.028355] CPU: 0 PID: 731 Comm: mptcp_connect Not 
tainted 5.4.0+ #5
00:07:58.692 [   14.029365] Hardware name: QEMU Standard PC (i440FX + 
PIIX, 1996), BIOS 1.10.2-1ubuntu1 04/01/2014
00:07:58.693 [   14.030752] RIP: 0010:mptcp_stream_accept+0x8d/0x160
00:07:58.694 [   14.031533] Code: 8b 9c 24 b0 05 00 00 49 81 c4 b0 05 00 
00 4c 39 e3 74 71 49 8d 76 40 48 89 74 24 08 eb 08 48 8b 1b 4c 39 e3 74 
5e 48 8b 6b 78 <48> 83 bd 78 02 00 00 00 75 ea 4c 8d bd 28 02 00 00 89 
44 24 04 4c
00:07:58.697 [   14.034405] RSP: 0018:ffffad8d8044bdf0 EFLAGS: 00010283
00:07:58.698 [   14.035218] RAX: 0000000000000000 RBX: ffff96f35d0d9928 
RCX: 0000000000000001
00:07:58.699 [   14.036317] RDX: 0000000000000001 RSI: ffff96f35eaffb40 
RDI: ffffffffb6199b8a
00:07:58.700 [   14.037423] RBP: 00000003435d0f37 R08: ffff96f35f42e690 
R09: ffffad8d8044bdb0
00:07:58.701 [   14.038528] R10: ffff96f35e375340 R11: ffff96f35d0d0740 
R12: ffff96f35d0d9930
00:07:58.702 [   14.039622] R13: ffff96f35eafe300 R14: ffff96f35eaffb00 
R15: ffff96f35eaffb00
00:07:58.703 [   14.040721] FS:  00007fad7081d500(0000) 
GS:ffff96f35f400000(0000) knlGS:0000000000000000
00:07:58.704 [   14.041973] CS:  0010 DS: 0000 ES: 0000 CR0: 
0000000080050033
00:07:58.738 [   14.042879] CR2: 00000003435d11af CR3: 000000001e3f4006 
CR4: 00000000003606f0
00:07:58.738 [   14.043987] DR0: 0000000000000000 DR1: 0000000000000000 
DR2: 0000000000000000
00:07:58.738 [   14.045098] DR3: 0000000000000000 DR6: 00000000fffe0ff0 
DR7: 0000000000000400
00:07:58.738 [   14.046204] Call Trace:
00:07:58.738 [   14.046599]  ? selinux_socket_accept+0x41/0x80
00:07:58.738 [   14.047294]  __sys_accept4_file+0xf7/0x1c0
00:07:58.738 [   14.047937]  ? mptcp_bind+0x38/0xa0
00:07:58.738 [   14.048487]  ? __inet_hash+0x120/0x2c0
00:07:58.738 [   14.049084]  ? kvm_clock_get_cycles+0xd/0x10
00:07:58.738 [   14.049751]  ? ktime_get_ts64+0x47/0xe0
00:07:58.738 [   14.050351]  __sys_accept4+0x3b/0x70
00:07:58.738 [   14.050911]  __x64_sys_accept+0x13/0x20
00:07:58.738 [   14.051510]  do_syscall_64+0x43/0x160
00:07:58.738 [   14.052084]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
00:07:58.738 [   14.052864] RIP: 0033:0x7fad7032e7e4
00:07:58.738 [   14.053428] Code: 26 00 00 00 48 c7 c0 ff ff ff ff c3 66 
2e 0f 1f 84 00 00 00 00 00 48 8d 05 21 e1 2c 00 8b 00 85 c0 75 13 b8 2b 
00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 54 f3 c3 66 90 41 54 55 49 89 d4 
53 48 89 f5
00:07:58.738 [   14.056279] RSP: 002b:00007fffba32c8e8 EFLAGS: 00000246 
ORIG_RAX: 000000000000002b
00:07:58.738 [   14.057447] RAX: ffffffffffffffda RBX: 0000000000000005 
RCX: 00007fad7032e7e4
00:07:58.738 [   14.058544] RDX: 00007fffba32c900 RSI: 00007fffba32c910 
RDI: 0000000000000005
00:07:58.738 [   14.059640] RBP: 0000000000000005 R08: 0000000000000004 
R09: 0000000000000000
00:07:58.738 [   14.060742] R10: 00007fffba32cb24 R11: 0000000000000246 
R12: 00007fffba32c910
00:07:58.738 [   14.061863] R13: 000055e3b0610b4d R14: 00000000ffffffff 
R15: 0000000000000000
00:07:58.738 [   14.062974] Modules linked in:
00:07:58.738 [   14.063459] CR2: 00000003435d11af
00:07:58.738 [   14.063984] ---[ end trace 5eeeab694e375d46 ]---
00:07:58.738 [   14.064707] RIP: 0010:mptcp_stream_accept+0x8d/0x160
00:07:58.738 [   14.065487] Code: 8b 9c 24 b0 05 00 00 49 81 c4 b0 05 00 
00 4c 39 e3 74 71 49 8d 76 40 48 89 74 24 08 eb 08 48 8b 1b 4c 39 e3 74 
5e 48 8b 6b 78 <48> 83 bd 78 02 00 00 00 75 ea 4c 8d bd 28 02 00 00 89 
44 24 04 4c
00:07:58.738 [   14.068351] RSP: 0018:ffffad8d8044bdf0 EFLAGS: 00010283
00:07:58.738 [   14.069169] RAX: 0000000000000000 RBX: ffff96f35d0d9928 
RCX: 0000000000000001
00:07:58.738 [   14.070274] RDX: 0000000000000001 RSI: ffff96f35eaffb40 
RDI: ffffffffb6199b8a
00:07:58.738 [   14.071378] RBP: 00000003435d0f37 R08: ffff96f35f42e690 
R09: ffffad8d8044bdb0
00:07:58.738 [   14.072478] R10: ffff96f35e375340 R11: ffff96f35d0d0740 
R12: ffff96f35d0d9930
00:07:58.738 [   14.073582] R13: ffff96f35eafe300 R14: ffff96f35eaffb00 
R15: ffff96f35eaffb00
00:07:58.738 [   14.074685] FS:  00007fad7081d500(0000) 
GS:ffff96f35f400000(0000) knlGS:0000000000000000
00:07:58.738 [   14.075912] CS:  0010 DS: 0000 ES: 0000 CR0: 
0000000080050033
00:07:58.739 [   14.076805] CR2: 00000003435d11af CR3: 000000001e3f4006 
CR4: 00000000003606f0
00:07:58.740 [   14.077853] DR0: 0000000000000000 DR1: 0000000000000000 
DR2: 0000000000000000
00:07:58.741 [   14.078927] DR3: 0000000000000000 DR6: 00000000fffe0ff0 
DR7: 0000000000000400
00:08:28.978 copyfd_io_poll: poll timed out (events: POLLIN 1, POLLOUT 4)
00:08:28.979 # ./mptcp_connect.sh: line 392:   731 Killed 
   ip netns exec ${listener_ns} ./mptcp_connect -t $timeout -l -p $port 
-s ${srv_proto} $extra_args $local_addr < "$sin" > "$sout"
00:08:28.988 # (duration 30308ms) [ FAIL ] client exit code 2, server 137
00:08:28.989 # \nnetns ns1-5dea74bc-S7alT1 socket stat for 10005:
00:08:29.013 # State  Recv-Q   Send-Q         Local Address:Port 
  Peer Address:Port
00:08:29.013 # ESTAB  94728    0           [dead:beef:1::1]:10005 
[dead:beef:1::1]:40964    rto:0.2 ato:0.04 cwnd:10 reordering:0
00:08:29.015 # \nnetns ns1-5dea74bc-S7alT1 socket stat for 10005:
00:08:29.027 # State      Recv-Q  Send-Q        Local Address:Port 
   Peer Address:Port
00:08:29.027 # FIN-WAIT-1 0       1691129    [dead:beef:1::1]:40964 
[dead:beef:1::1]:10005   timer:(persist,21sec,0) rto:0.2 cwnd:10 
reordering:0
00:08:29.031 # FAIL: Could not even run loopback v6 test
00:08:29.062 not ok 1 selftests: net/mptcp: mptcp_connect.sh # exit=1


I guess it is not the top priority because part 1 and 2 are OK.

Cheers,
Matt
-- 
Matthieu Baerts | R&D Engineer
matthieu.baerts(a)tessares.net
Tessares SA | Hybrid Access Solutions
www.tessares.net
1 Avenue Jean Monnet, 1348 Louvain-la-Neuve, Belgium

[MPTCP] Re: [PATCH v2 9/9] don't flag parent socket as RCV_SHUTDOWN if one one subflow has closed

[Matthieu Baerts]

[-- Attachment #1: Type: text/plain, Size: 1462 bytes --]

Hi Paolo,

On 03/12/2019 10:05, Paolo Abeni wrote:
> On Tue, 2019-12-03 at 08:44 +0100, Matthieu Baerts wrote:
>> Hi Florian, Paolo, Mat,
>>
>> On 02/12/2019 22:21, Paolo Abeni wrote:
>>> On Mon, 2019-12-02 at 20:10 +0100, Florian Westphal wrote:
>>
>> [...]
>>
>>> Thanks for addressing my comments.
>>>
>>> With 9/9 v2 the series is IMHO ready to me merged.
>>
>> Thank you for the nice patch series and the reviews!
>> Related to what we discussed yesterday, is it OK if I merge this after:
>>
>>    1) DATA_FIN
>>    2) MPTCPv1 → wait a bit for a review
>>    3) SendMsg "refactor" → if reviewed
>>
>> Or can I do that after the patches for MPTCPv1 if the patches to fix
>> sendmsg() have not been reviewed yet?
> 
> I think/hope this and sendmsg patches should not conflict much - but
> sendmsg patches do create quite of conflict with the following patches.
> 
> I think that appling this series after v1 could be a good option, will
> give extra time to review the sendmsg() patches, and I can rebase my
> tree to help the merging.

Thank you for your reply!

Sounds good to me. If there is nothing to change in sendmsg() patches 
after the review, I can also start applying them. If I am blocked, I can 
ask for help.

Cheers,
Matt
-- 
Matthieu Baerts | R&D Engineer
matthieu.baerts(a)tessares.net
Tessares SA | Hybrid Access Solutions
www.tessares.net
1 Avenue Jean Monnet, 1348 Louvain-la-Neuve, Belgium

[MPTCP] Re: [PATCH v2 9/9] don't flag parent socket as RCV_SHUTDOWN if one one subflow has closed

[Paolo Abeni]

[-- Attachment #1: Type: text/plain, Size: 1013 bytes --]

On Tue, 2019-12-03 at 08:44 +0100, Matthieu Baerts wrote:
> Hi Florian, Paolo, Mat,
> 
> On 02/12/2019 22:21, Paolo Abeni wrote:
> > On Mon, 2019-12-02 at 20:10 +0100, Florian Westphal wrote:
> 
> [...]
> 
> > Thanks for addressing my comments.
> > 
> > With 9/9 v2 the series is IMHO ready to me merged.
> 
> Thank you for the nice patch series and the reviews!
> Related to what we discussed yesterday, is it OK if I merge this after:
> 
>   1) DATA_FIN
>   2) MPTCPv1 → wait a bit for a review
>   3) SendMsg "refactor" → if reviewed
> 
> Or can I do that after the patches for MPTCPv1 if the patches to fix 
> sendmsg() have not been reviewed yet?

I think/hope this and sendmsg patches should not conflict much - but
sendmsg patches do create quite of conflict with the following patches.

I think that appling this series after v1 could be a good option, will
give extra time to review the sendmsg() patches, and I can rebase my
tree to help the merging.

Thanks,

Paolo

[MPTCP] Re: [PATCH v2 9/9] don't flag parent socket as RCV_SHUTDOWN if one one subflow has closed

[Matthieu Baerts]

[-- Attachment #1: Type: text/plain, Size: 879 bytes --]

Hi Florian, Paolo, Mat,

On 02/12/2019 22:21, Paolo Abeni wrote:
> On Mon, 2019-12-02 at 20:10 +0100, Florian Westphal wrote:

[...]

> Thanks for addressing my comments.
> 
> With 9/9 v2 the series is IMHO ready to me merged.

Thank you for the nice patch series and the reviews!
Related to what we discussed yesterday, is it OK if I merge this after:

  1) DATA_FIN
  2) MPTCPv1 → wait a bit for a review
  3) SendMsg "refactor" → if reviewed

Or can I do that after the patches for MPTCPv1 if the patches to fix 
sendmsg() have not been reviewed yet?

Also just to be sure: do I squash the 2 first ones and then I add the 
rest at the end of the series?

Cheers,
Matt
-- 
Matthieu Baerts | R&D Engineer
matthieu.baerts(a)tessares.net
Tessares SA | Hybrid Access Solutions
www.tessares.net
1 Avenue Jean Monnet, 1348 Louvain-la-Neuve, Belgium

[MPTCP] Re: [PATCH v2 9/9] don't flag parent socket as RCV_SHUTDOWN if one one subflow has closed

[Paolo Abeni]

[-- Attachment #1: Type: text/plain, Size: 5643 bytes --]

On Mon, 2019-12-02 at 20:10 +0100, Florian Westphal wrote:
> instead schedule the mptcp worker and tell it to check eof state on all
> subflows.  If all have closed, then also flag the mptcp socket as closed
> and wake up userspace process blocking in poll().
> 
> v2: move eof checks to subflow_state_change() (Paolo)
> 
> Signed-off-by: Florian Westphal <fw(a)strlen.de>
> ---
>  net/mptcp/protocol.c | 41 +++++++++++++++++++++++++++++++++--------
>  net/mptcp/protocol.h |  2 ++
>  net/mptcp/subflow.c  | 22 ++++++++++++----------
>  3 files changed, 47 insertions(+), 18 deletions(-)
> 
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 36097008dd4a..feb4658b13a8 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -61,6 +61,15 @@ void mptcp_data_acked(struct sock *sk)
>  		sock_hold(sk);
>  }
>  
> +void mptcp_subflow_eof(struct sock *sk)
> +{
> +	struct mptcp_sock *msk = mptcp_sk(sk);
> +
> +	if (!test_and_set_bit(MPTCP_WORK_EOF, &msk->flags) &&
> +	    schedule_work(&msk->rtx_work))
> +		sock_hold(sk);
> +}
> +
>  static void mptcp_stop_timer(struct sock *sk)
>  {
>  	struct inet_connection_sock *icsk = inet_csk(sk);
> @@ -95,22 +104,14 @@ static struct sock *mptcp_subflow_recv_lookup(const struct mptcp_sock *msk)
>  {
>  	struct sock *sk = (struct sock *)msk;
>  	struct mptcp_subflow_context *subflow;
> -	int receivers = 0;
>  
>  	sock_owned_by_me(sk);
>  
>  	mptcp_for_each_subflow(msk, subflow) {
>  		if (subflow->data_avail)
>  			return mptcp_subflow_tcp_sock(subflow);
> -
> -		receivers += !subflow->rx_eof;
>  	}
>  
> -	/* hopefully temporary hack: propagate shutdown status from subflow
> -	 * to msk, when all subflows agree on it
> -	 */
> -	if (!receivers && !(sk->sk_shutdown & RCV_SHUTDOWN))
> -		sk->sk_shutdown |= RCV_SHUTDOWN;
>  	return NULL;
>  }
>  
> @@ -789,6 +790,27 @@ static void __mptcp_close_ssk(struct sock *sk, struct sock *ssk,
>  	}
>  }
>  
> +static void mptcp_check_for_eof(struct mptcp_sock *msk)
> +{
> +	struct mptcp_subflow_context *subflow;
> +	struct sock *sk = (struct sock *)msk;
> +	int receivers = 0;
> +
> +	mptcp_for_each_subflow(msk, subflow)
> +		receivers += !subflow->rx_eof;
> +
> +	if (!receivers && !(sk->sk_shutdown & RCV_SHUTDOWN)) {
> +		/* hopefully temporary hack: propagate shutdown status
> +		 * to msk, when all subflows agree on it
> +		 */
> +		sk->sk_shutdown |= RCV_SHUTDOWN;
> +
> +		smp_mb__before_atomic(); /* SHUTDOWN must be visible first */
> +		set_bit(MPTCP_DATA_READY, &msk->flags);
> +		sk->sk_data_ready(sk);
> +	}
> +}
> +
>  static void mptcp_worker(struct work_struct *work)
>  {
>  	int orig_len, orig_offset, ret, mss_now = 0, size_goal = 0;
> @@ -806,6 +828,9 @@ static void mptcp_worker(struct work_struct *work)
>  	lock_sock(sk);
>  	mptcp_clean_una(sk);
>  
> +	if (test_and_clear_bit(MPTCP_WORK_EOF, &msk->flags))
> +		mptcp_check_for_eof(msk);
> +
>  	if (!test_and_clear_bit(MPTCP_WORK_RTX, &msk->flags))
>  		goto unlock;
>  
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index 990da61fa51b..cf3b4ce483d9 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -78,6 +78,7 @@
>  #define MPTCP_DATA_READY	BIT(0)
>  #define MPTCP_WORK_RTX		BIT(1)
>  #define MPTCP_SEND_SPACE	BIT(2)
> +#define MPTCP_WORK_EOF		BIT(3)
>  
>  static inline __be32 mptcp_option(u8 subopt, u8 len, u8 nib, u8 field)
>  {
> @@ -291,6 +292,7 @@ void mptcp_get_options(const struct sk_buff *skb,
>  		       struct tcp_options_received *opt_rx);
>  
>  void mptcp_finish_connect(struct sock *sk);
> +void mptcp_subflow_eof(struct sock *sk);
>  bool mptcp_finish_join(struct sock *sk);
>  void mptcp_data_acked(struct sock *sk);
>  
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 8c13e9a145d5..fe6c61367bd7 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -639,11 +639,6 @@ bool mptcp_subflow_data_available(struct sock *sk)
>  
>  	if (!subflow_check_data_avail(sk)) {
>  		subflow->data_avail = 0;
> -		/* set EoF only there is no data available - we already spooled
> -		 * all the pending skbs
> -		 */
> -		if (sk->sk_shutdown & RCV_SHUTDOWN || sk->sk_state == TCP_CLOSE)
> -			subflow->rx_eof = 1;
>  		return false;
>  	}
>  
> @@ -666,8 +661,7 @@ static void subflow_data_ready(struct sock *sk)
>  		return;
>  	}
>  
> -	/* always propagate the EoF */
> -	if (mptcp_subflow_data_available(sk) || subflow->rx_eof) {
> +	if (mptcp_subflow_data_available(sk)) {
>  		smp_mb__before_atomic();
>  		set_bit(MPTCP_DATA_READY, &mptcp_sk(parent)->flags);
>  		smp_mb__after_atomic();
> @@ -807,15 +801,23 @@ static void __subflow_state_change(struct sock *sk)
>  	rcu_read_unlock();
>  }
>  
> +static bool subflow_is_done(const struct sock *sk)
> +{
> +	return sk->sk_shutdown & RCV_SHUTDOWN || sk->sk_state == TCP_CLOSE;
> +}
> +
>  static void subflow_state_change(struct sock *sk)
>  {
>  	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
> -	struct sock *parent = subflow->conn;
> +	struct sock *parent = READ_ONCE(subflow->conn);
>  
>  	__subflow_state_change(sk);
>  
> -	if (parent)
> -		__subflow_state_change(parent);
> +	if (parent && !(parent->sk_shutdown & RCV_SHUTDOWN) &&
> +	    !subflow->rx_eof && subflow_is_done(sk)) {
> +		subflow->rx_eof = 1;
> +		mptcp_subflow_eof(parent);
> +	}
>  }
>  
>  static int subflow_ulp_init(struct sock *sk)

Thanks for addressing my comments.

With 9/9 v2 the series is IMHO ready to me merged.

Thanks,

Paolo