TFO: IPv6: the generated cookie is not the expected one

[matttbe]

Packetdrill is reporting that the cookie generated by the kernel is not the expected one when testing in IPv6.

After a bit of debugging, I noticed req->rsk_ops->family is set to AF_INET (not 6) because the subflows extends the request_sock_ops of TCP v4 (tcp_request_sock_ops).

Validation in progress but here is a ticket to start discussions (and not to forget to talk about that at the next weekly meeting ;) )

[matttbe]

The Packetdrill test I was using:

// Receive data with TFO + cookie
--tolerance_usecs=100000
`../common/defaults.sh`

// A first connection to store the cookie
 0.0    socket(..., SOCK_STREAM, IPPROTO_MPTCP) = 3
+0.0    setsockopt(3, SOL_SOCKET, SO_REUSEADDR, [1], 4) = 0
+0.0    getsockopt(3, SOL_TCP, TCP_FASTOPEN, [0], [4]) = 0
+0.0    setsockopt(3, SOL_TCP, TCP_FASTOPEN, [2], 4) = 0
+0.0    getsockopt(3, SOL_TCP, TCP_FASTOPEN, [2], [4]) = 0

+0.0    bind(3, ..., ...) = 0
+0.0    listen(3, 1) = 0

+0.0      <  S   0:0(0)                win 65535  <mss 1460, sackOK, TS val 100 ecr 0,   nop, wscale 8, FO,            nop, nop, mpcapable v1 flags[flag_h] nokey>
+0.01     >  S.  0:0(0)      ack 1                <mss 1460, nop, nop, sackOK,           nop, wscale 8, FO TFO_COOKIE, nop, nop, mpcapable v1 flags[flag_h] key[skey]>
+0.01     <   .  1:1(0)      ack 1     win 450    <                                                                              mpcapable v1 flags[flag_h] key[ckey=2, skey]>

+0.2    accept(3, ..., ...) = 4
+0.2    close(4) = 0
+0.0      >   .  1:1(0)      ack 1                <dss dack4=1 dsn8=1 ssn=0 dll=1 nocs fin, nop, nop>
+0.1      <   .  1:1(0)      ack 1     win 450    <dss dack4=2 dsn4=1 ssn=0 dll=1 nocs fin, nop, nop>
+0.0      >   .  1:1(0)      ack 1                <dss dack4=2 nocs>
+0.0      >  F.  1:1(0)      ack 1                <dss dack4=2 nocs>
+0.0      <   .  1:1(0)      ack 2     win 450    <dss dack4=2 nocs>

// reply with a small delay to let the kernel switching to a time-wait socket.
+0.4      <  F.  1:1(0)      ack 2     win 450    <dss dack4=2 nocs>
+0.0      >   .  2:2(0)      ack 2
+0.0      <  R.  2:2(0)      ack 2     win 450


// Not supported by Packetdrill yet: new connection
// Another Fastopen request, now SYN will have data
+0.01     <  S   0:500(500)            win 65535  <mss 1460, sackOK, TS val 100 ecr 0,   nop, wscale 8, FO TFO_COOKIE, nop, nop, mpcapable v1 flags[flag_h] nokey>
// +0.01  >  S.  0:0(0)      ack 501              <mss 1460, sackOK, TS val 100 ecr 100, nop, wscale 8,                          mpcapable v1 flags[flag_h] key[skey]>

And the command:

packetdrill -v --ip_version=ipv6 --mtu=1520 --local_ip=fd3d:fa7b:d17d::0 --gateway_ip=fd3d:fa7b:d17d:8888::0 --remote_ip=2001:DB8::1/32 -D CMSG_LEVEL_IP=SOL_IPV6 -D CMSG_TYPE_RECVERR=IP6_RECVERR -D TFO_COOKIE=6aa6ae70c288023b fastopen/server-TCP_FASTOPEN.pkt

[matttbe]

WIP kernel code: https://github.com/matttbe/mptcp_net-next/commits/mptcp_subflow_v6_request_sock_ops

[matttbe]

Remaining question:

• do we need the same slab for v4 and v6 or better with two different ones
• (I guess it is not linked with ↑ but we should still allow a mix of subflows in v4 and v6 for the same MPTCP connection)

[pabeni]

Remaining question:
* do we need the same slab for v4 and v6 or better with two different ones

note that kmem_cache_create can re-use existing slabs when the arguments matches - in practice tcp_prot and tcpv6_prot should have/use the same slab.

so both copying the slab field or calling twice kmem_cache_create should yeld to the same result

[matttbe]

Hi @pabeni ,

Thank you for your feedback!

If I'm not mistaken, for the moment, we have slabs for mptcp_prot (proto, requests, TW), mptcp_v6_prot (proto, requests, TW) and mptcp_subflow_request_sock_ops (requests only).

tcp_prot and tcpv6_prot have different names (TCP vs TCPv6) and the name is used to create the different slabs (proto, requests, TW). So similar to MPTCP proto, there are different slabs per family.

Should we then do the same for the subflow requests then? I'm not sure to understand all the consequences (memory / perf) but it might be good to keep them isolated. If yes, do you think it would be OK to change this behaviour in -net?

[pabeni]

If I'm not mistaken, for the moment, we have slabs for mptcp_prot (proto, requests, TW), mptcp_v6_prot (proto, requests, TW) and mptcp_subflow_request_sock_ops (requests only).

tcp_prot and tcpv6_prot have different names (TCP vs TCPv6) and the name is used to create the different slabs (proto, requests, TW). So similar to MPTCP proto, there are different slabs per family.

Note that kmem_cache_create can still reuse an existing slab even if asked to created a new one with a different name, if overall argments allow for that, see:

kmem_cache_create -> kmem_cache_create_usercopy -> __kmem_cache_create -> __kmem_cache_alias

the relevant slab name for request socket should be: "request_sock_TCP" "request_sock_TCPv6" see req_prot_init()

Note that you can inspect the existing slabs in /proc/slabinfo - if you are using the slub allocator

Should we then do the same for the subflow requests then? I'm not sure to understand all the consequences (memory / perf) but it might be good to keep them isolated. If yes, do you think it would be OK to change this behaviour in -net?

No, we don't need different slabs for v4/v6 request socket, if the struct size are the same. The slab thing has build-in per-cpu optimization, and that is good enough from resource usage/performance pov

[matttbe]

If I'm not mistaken, for the moment, we have slabs for mptcp_prot (proto, requests, TW), mptcp_v6_prot (proto, requests, TW) and mptcp_subflow_request_sock_ops (requests only).
tcp_prot and tcpv6_prot have different names (TCP vs TCPv6) and the name is used to create the different slabs (proto, requests, TW). So similar to MPTCP proto, there are different slabs per family.

Note that kmem_cache_create can still reuse an existing slab even if asked to created a new one with a different name, if overall argments allow for that, see:

kmem_cache_create -> kmem_cache_create_usercopy -> __kmem_cache_create -> __kmem_cache_alias

the relevant slab name for request socket should be: "request_sock_TCP" "request_sock_TCPv6" see req_prot_init()

Note that you can inspect the existing slabs in /proc/slabinfo - if you are using the slub allocator

Thank you for the explanations, it is clearer now, I missed the part about the "merging" bit :-)

Should we then do the same for the subflow requests then? I'm not sure to understand all the consequences (memory / perf) but it might be good to keep them isolated. If yes, do you think it would be OK to change this behaviour in -net?

No, we don't need different slabs for v4/v6 request socket, if the struct size are the same. The slab thing has build-in per-cpu optimization, and that is good enough from resource usage/performance pov

OK thank you for this feedback!

If merges are done (and no new allocations), should I then keep the code "simple" by keeping subflow_ops_init() untouched and calling it twice: once for v4 and once for v6 (with the same name then)?

[matttbe]

This has been fixed thanks to:

• 5819cf2: mptcp: remove MPTCP 'ifdef' in TCP SYN cookies
• 46b00ed: mptcp: dedicated request sock for subflow in v6
• 2df1717: mptcp: use proper req destructor for IPv6
• Results: f48d240..81092f1 (export-net)
• Results: 9324c81..ec528d5 (export)

(I forgot to add the Closes tag in patch 2/3)