All of lore.kernel.org
 help / color / mirror / Atom feed
* [MPTCP] Re: [PATCH v4] mptcp: fix option length of mp_capable syn/ack
@ 2019-12-06 16:13 Matthieu Baerts
  0 siblings, 0 replies; 2+ messages in thread
From: Matthieu Baerts @ 2019-12-06 16:13 UTC (permalink / raw)
  To: mptcp

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

Hi Davide, Peter,

On 05/12/2019 19:14, Peter Krystad wrote:
> 
> Looks good.

Thank you for the new version and the review!

- 40c5aac59cf7: "squashed" part 1 in "mptcp: Handle MPTCP TCP options"
- 29710f885028: "Signed-off-by" + "Co-developed-by"
- 36e93ae12137: "squashed" part 2 in "mptcp: Create SUBFLOW socket for 
incoming connections"
- cab89c119b1c: conflict in 
t/mptcp-parse-and-emit-MP_CAPABLE-option-according-to-v1-spec
- in this commit, for mptcp_synack_options(), only this change was left 
over:

===

diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index 701b6243c377..da94c263197a 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -476,7 +476,7 @@ bool mptcp_synack_options(const struct request_sock 
*req, unsigned int *size,
                 opts->suboptions = OPTION_MPTCP_MPC_SYNACK;
                 opts->sndr_key = subflow_req->local_key;
                 *size = TCPOLEN_MPTCP_MPC_SYNACK;
-               pr_debug("subflow_req=%p, local_key=%llu",
+               pr_debug("req=%p, local_key=%llu",
                          subflow_req, subflow_req->local_key);
                 return true;
         }

===

I then undo this change. That's also the only change we can see as a 
result (as expected): 7e9aa2a662bf..7c57362bfddb

Tests and re-creation of the 'export' branch is in progress.

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

^ permalink raw reply related	[flat|nested] 2+ messages in thread
* [MPTCP] Re: [PATCH v4] mptcp: fix option length of mp_capable syn/ack
@ 2019-12-05 18:14 Peter Krystad
  0 siblings, 0 replies; 2+ messages in thread
From: Peter Krystad @ 2019-12-05 18:14 UTC (permalink / raw)
  To: mptcp

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


Looks good.

Peter.

On Thu, 2019-12-05 at 18:07 +0100, Davide Caratti wrote:
> squashto: mptcp: Handle MPTCP TCP options
> 
> in current MPTCP, the server sends the client's key back in the syn/ack
> packet. Because of that, both tcpdump and packetdrill refuse to parse
> the mp_capable option in the syn/ack. Both RFC6824 and RFC6824bis don't
> require this in section 3.1, only the server's key should be sent in
> the syn/ack.
> 
> - change TCPOLEN_MPTCP_MPC_SYNACK from 20 (4+8+8) to 12 (4+8)
> - don't write the client's key in mp_capable syn/ack
> - adjust tests in mptcp_parse_option() to cope with the changed
>   value of TCPOLEN_MPTCP_MPC_SYNACK
> 
> 3-way handshakes obtained with
> 
>  # ./mptcp_connect.sh -4 -c -e tx
>  [...]
>  # tcpdump -c3 -tnnr ns1-5de04ec0-TGs4tf-ns1-5de04ec0-TGs4tf-MPTCP-MPTCP-10.0.1.1.pcap tcp
> 
> unpatched kernel:
> 
>  IP 10.0.1.1.49752 > 10.0.1.1.10000: Flags [S], seq 2594718977, win 65535, options [mss 65495,sackOK,TS val 3944347269 ecr 0,nop,wscale 8,mptcp capable {0xf931b304deb39a42}], length 0
>  IP 10.0.1.1.10000 > 10.0.1.1.49752: Flags [S.], seq 3996234497, ack 2594718978, win 65535, options [mss 65495,sackOK,TS val 3944347270 ecr 3944347269,nop,wscale 8,mptcp capable[bad opt]>
> ^^^ bad option
>  IP 10.0.1.1.49752 > 10.0.1.1.10000: Flags [.], ack 1, win 256, options [nop,nop,TS val 3944347270 ecr 3944347270,mptcp capable {0xf931b304deb39a42,0x96614f185e633ce}], length 0
> 
> patched kernel:
> 
>  IP 10.0.1.1.50850 > 10.0.1.1.10000: Flags [S], seq 626898452, win 65495, options [mss 65495,sackOK,TS val 2266878231 ecr 0,nop,wscale 7,mptcp capable {0x7fe37bfd872b9f9f}], length 0
>  IP 10.0.1.1.10000 > 10.0.1.1.50850: Flags [S.], seq 3966041155, ack 626898453, win 65483, options [mss 65495,sackOK,TS val 2266878232 ecr 2266878231,nop,wscale 7,mptcp capable {0x8a167e3af39b5fc1}], length 0
> ^^^ no more bad option
>  IP 10.0.1.1.50850 > 10.0.1.1.10000: Flags [.], ack 1, win 512, options [nop,nop,TS val 2266878232 ecr 2266878232,mptcp capable {0x7fe37bfd872b9f9f,0x8a167e3af39b5fc1}], length 0
> 
> v2: more conservative behavior when parsing received options
> v3: adjust tests in mptcp_parse_option() to catch all possible
>     values of MP_CAPABLE option length specified by v0 protocol,
>     thanks Matthieu Baerts
> v4: avoid copying the receiver key in mptcp_synack_options(),
>     thanks Peter Krystad
> 
> squashto: "mptcp: Handle MPTCP TCP options"
> Fixes: 8fa520f034ed ("mptcp: Handle MPTCP TCP options")
> Signed-off-by: Davide Caratti <dcaratti(a)redhat.com>
> ---
>  net/mptcp/options.c  | 13 +++++--------
>  net/mptcp/protocol.h |  2 +-
>  2 files changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index 98eb0281d196..b150e881ef7b 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -25,7 +25,7 @@ void mptcp_parse_option(const unsigned char *ptr, int opsize,
>  	 */
>  	case MPTCPOPT_MP_CAPABLE:
>  		if (opsize != TCPOLEN_MPTCP_MPC_SYN &&
> -		    opsize != TCPOLEN_MPTCP_MPC_SYNACK)
> +		    opsize != TCPOLEN_MPTCP_MPC_ACK)
>  			break;
>  
>  		mp_opt->version = *ptr++ & MPTCP_VERSION_MASK;
> @@ -57,7 +57,7 @@ void mptcp_parse_option(const unsigned char *ptr, int opsize,
>  		mp_opt->sndr_key = get_unaligned_be64(ptr);
>  		ptr += 8;
>  
> -		if (opsize == TCPOLEN_MPTCP_MPC_SYNACK) {
> +		if (opsize == TCPOLEN_MPTCP_MPC_ACK) {
>  			mp_opt->rcvr_key = get_unaligned_be64(ptr);
>  			ptr += 8;
>  			pr_debug("MP_CAPABLE flags=%x, sndr=%llu, rcvr=%llu",
> @@ -516,11 +516,9 @@ bool mptcp_synack_options(const struct request_sock *req, unsigned int *size,
>  	if (subflow_req->mp_capable) {
>  		opts->suboptions = OPTION_MPTCP_MPC_SYNACK;
>  		opts->sndr_key = subflow_req->local_key;
> -		opts->rcvr_key = subflow_req->remote_key;
>  		*size = TCPOLEN_MPTCP_MPC_SYNACK;
> -		pr_debug("req=%p, local_key=%llu, remote_key=%llu",
> -			 subflow_req, subflow_req->local_key,
> -			 subflow_req->remote_key);
> +		pr_debug("req=%p, local_key=%llu",
> +			 subflow_req, subflow_req->local_key);
>  		return true;
>  	} else if (subflow_req->mp_join) {
>  		opts->suboptions = OPTION_MPTCP_MPJ_SYNACK;
> @@ -650,8 +648,7 @@ void mptcp_write_options(__be32 *ptr, struct mptcp_out_options *opts)
>  				      MPTCP_CAP_HMAC_SHA1);
>  		put_unaligned_be64(opts->sndr_key, ptr);
>  		ptr += 2;
> -		if ((OPTION_MPTCP_MPC_SYNACK |
> -		     OPTION_MPTCP_MPC_ACK) & opts->suboptions) {
> +		if (OPTION_MPTCP_MPC_ACK & opts->suboptions) {
>  			put_unaligned_be64(opts->rcvr_key, ptr);
>  			ptr += 2;
>  		}
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index acad61c70de9..10347c9b4dff 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -34,7 +34,7 @@
>  
>  /* MPTCP suboption lengths */
>  #define TCPOLEN_MPTCP_MPC_SYN		12
> -#define TCPOLEN_MPTCP_MPC_SYNACK	20
> +#define TCPOLEN_MPTCP_MPC_SYNACK	12
>  #define TCPOLEN_MPTCP_MPC_ACK		20
>  #define TCPOLEN_MPTCP_MPJ_SYN		12
>  #define TCPOLEN_MPTCP_MPJ_SYNACK	16

^ permalink raw reply	[flat|nested] 2+ messages in thread
end of thread, other threads:[~2019-12-06 16:13 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-06 16:13 [MPTCP] Re: [PATCH v4] mptcp: fix option length of mp_capable syn/ack Matthieu Baerts
  -- strict thread matches above, loose matches on Subject: below --
2019-12-05 18:14 Peter Krystad

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.