Re: [RFC PATCH mptcp-next v8 4/7] mptfo variables for msk, options. Fix loop retrans

[Dmytro Shytyi <dmytro@shytyi.net>]

On 9/20/2022 4:56 PM, Paolo Abeni wrote:
> On Tue, 2022-09-20 at 14:52 +0200, Dmytro Shytyi wrote:
>> Introduce mptfo variables for msk and options.
>> Also fix the infinite retransmissions in the end of second session.
> I think the above sentence belongs more the changelog than to the
> commit message itself.

Fixed in v9.


>> The variable 2nd ack received in struct mptcp_options_received identifies the
>> received ack on the listener side during 3way handshake in mptfo context
>> and miningless alone if used alone. It is further used(checked)
>> in conjunction in the same "if" statement with variable is_mptfo
>> from struct mptcp_sock.
>>
>> Signed-off-by: Dmytro Shytyi <dmytro@shytyi.net>
>> ---
>>   net/mptcp/fastopen.c | 14 ++++++++++++++
>>   net/mptcp/options.c  |  4 ++++
>>   net/mptcp/protocol.h |  6 +++++-
>>   3 files changed, 23 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/mptcp/fastopen.c b/net/mptcp/fastopen.c
>> index 9ef49a2d2ea2..92885e459f93 100644
>> --- a/net/mptcp/fastopen.c
>> +++ b/net/mptcp/fastopen.c
>> @@ -30,3 +30,17 @@ int mptcp_setsockopt_sol_tcp_fastopen(struct mptcp_sock *msk, sockptr_t optval,
>>   
>>   	return ret;
>>   }
>> +
>> +void mptcp_gen_msk_ackseq_fastopen(struct mptcp_sock *msk, struct mptcp_subflow_context *subflow,
>> +				   struct mptcp_options_received mp_opt)
>> +{
>> +	u64 ack_seq;
>> +
>> +	msk->can_ack = true;
>> +	msk->remote_key = mp_opt.sndr_key;
> likely the above two statements need WRITE_ONCE() annotation.

I have added in v9 WRITE_ONCE() to the above two statements.


> Additionally it's better to add some safeguards here:
> - ssk should be in TCP_SYN_RECV state - to be verified, it's guess and
> I haven't check it yet.
> - msk should be in the same state of the ssk subflow.
>
>> +	mptcp_crypto_key_sha(msk->remote_key, NULL, &ack_seq);
>> +	ack_seq++;
>> +	WRITE_ONCE(msk->ack_seq, ack_seq);
>> +	pr_debug("ack_seq=%llu sndr_key=%llu", msk->ack_seq, mp_opt.sndr_key);
>> +	atomic64_set(&msk->rcv_wnd_sent, ack_seq);
>> +}
>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
>> index 30d289044e71..8852a13cfe62 100644
>> --- a/net/mptcp/options.c
>> +++ b/net/mptcp/options.c
>> @@ -91,6 +91,7 @@ static void mptcp_parse_option(const struct sk_buff *skb,
>>   			ptr += 8;
>>   		}
>>   		if (opsize >= TCPOLEN_MPTCP_MPC_ACK) {
>> +			mp_opt->hns_2nd_ack_rcvd = 1;
> Not needed, see below...


Removed in v9

>>   			mp_opt->rcvr_key = get_unaligned_be64(ptr);
>>   			ptr += 8;
>>   		}
>> @@ -1124,6 +1125,9 @@ bool mptcp_incoming_options(struct sock *sk, struct sk_buff *skb)
>>   		return sk->sk_state != TCP_CLOSE;
>>   
>>   	if (unlikely(mp_opt.suboptions != OPTION_MPTCP_DSS)) {
>> +		if (mp_opt.suboptions & OPTIONS_MPTCP_MPC && mp_opt.hns_2nd_ack_rcvd && msk->is_mptfo)
> You can simply check for:
>
> 		if (mp_opt.suboptions & OPTIONS_MPTCP_MPC_ACK) && msk->is_mptfo)


Removed in v9

>> +			mptcp_gen_msk_ackseq_fastopen(msk, subflow, mp_opt);
>> +
>>   		if ((mp_opt.suboptions & OPTION_MPTCP_FASTCLOSE) &&
>>   		    msk->local_key == mp_opt.rcvr_key) {
>>   			WRITE_ONCE(msk->rcv_fastclose, true);
>> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
>> index 57596cdfb1f9..3b9a349a7080 100644
>> --- a/net/mptcp/protocol.h
>> +++ b/net/mptcp/protocol.h
>> @@ -155,7 +155,8 @@ struct mptcp_options_received {
>>   		echo:1,
>>   		backup:1,
>>   		deny_join_id0:1,
>> -		__unused:2;
>> +		__unused:1;
>> +	u8	hns_2nd_ack_rcvd:1;
> ... again not needed, see above ;)
Removed in v9
>>   	u8	join_id;
>>   	u64	thmac;
>>   	u8	hmac[MPTCPOPT_HMAC_LEN];
>> @@ -282,6 +283,7 @@ struct mptcp_sock {
>>   	bool		use_64bit_ack; /* Set when we received a 64-bit DSN */
>>   	bool		csum_enabled;
>>   	bool		allow_infinite_fallback;
>> +	bool		is_mptfo;
>>   	u8		mpc_endpoint_id;
>>   	u8		recvmsg_inq:1,
>>   			cork:1,
>> @@ -842,6 +844,8 @@ int mptcp_stream_connect(struct socket *sock, struct sockaddr *uaddr, int addr_l
>>   // Fast Open Mechanism functions begin
>>   int mptcp_setsockopt_sol_tcp_fastopen(struct mptcp_sock *msk, sockptr_t optval,
>>   				      unsigned int optlen);
>> +void mptcp_gen_msk_ackseq_fastopen(struct mptcp_sock *msk, struct mptcp_subflow_context *subflow,
>> +				   struct mptcp_options_received mp_opt);
>>   // Fast Open Mechanism functions end
>>   
>>   static inline bool mptcp_pm_should_add_signal(struct mptcp_sock *msk)

Best,

Dmytro.