Re: [MPTCP] [MPTCP][PATCH mptcp-next 3/6] mptcp: add the csum_reqd fields

[Mat Martineau <mathew.j.martineau@linux.intel.com>]

On Tue, 23 Mar 2021, Paolo Abeni wrote:

> On Mon, 2021-03-22 at 19:37 +0800, Geliang Tang wrote:
>> This patch added a new flag csum_reqd in struct mptcp_options_received, if
>> the flag MPTCP_CAP_CHECKSUM_REQD is set in the receiving MP_CAPABLE
>> suboption, set this csum_reqd flag.
>>
>> It also added another new csum_reqd member in struct mptcp_sock too, if the
>> csum_reqd flag in mptcp_options_received is set, set this member to true.
>>
>> Signed-off-by: Geliang Tang <geliangtang@gmail.com>
>> ---
>>  net/mptcp/options.c  | 8 +++++---
>>  net/mptcp/protocol.c | 1 +
>>  net/mptcp/protocol.h | 2 ++
>>  net/mptcp/subflow.c  | 4 ++++
>>  4 files changed, 12 insertions(+), 3 deletions(-)
>>
>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
>> index f45bebb923dc..bdced173edff 100644
>> --- a/net/mptcp/options.c
>> +++ b/net/mptcp/options.c
>> @@ -69,11 +69,9 @@ static void mptcp_parse_option(const struct sk_buff *skb,
>>  		 * "If a checksum is not present when its use has been
>>  		 * negotiated, the receiver MUST close the subflow with a RST as
>>  		 * it is considered broken."
>> -		 *
>> -		 * We don't implement DSS checksum - fall back to TCP.
>>  		 */
>>  		if (flags & MPTCP_CAP_CHECKSUM_REQD)
>> -			break;
>> +			mp_opt->csum_reqd = 1;
>>
>>  		mp_opt->mp_capable = 1;
>>  		if (opsize >= TCPOLEN_MPTCP_MPC_SYNACK) {
>> @@ -340,6 +338,7 @@ void mptcp_get_options(const struct sk_buff *skb,
>>  	mp_opt->dss = 0;
>>  	mp_opt->mp_prio = 0;
>>  	mp_opt->reset = 0;
>> +	mp_opt->csum_reqd = 0;
>>
>>  	length = (th->doff * 4) - sizeof(struct tcphdr);
>>  	ptr = (const unsigned char *)(th + 1);
>> @@ -1013,6 +1012,9 @@ void mptcp_incoming_options(struct sock *sk, struct sk_buff *skb)
>>  		mptcp_schedule_work((struct sock *)msk);
>>  	}
>>
>> +	if (mp_opt.csum_reqd)
>> +		WRITE_ONCE(msk->csum_reqd, true);
>> +
>>  	if (mp_opt.add_addr && add_addr_hmac_valid(msk, &mp_opt)) {
>>  		if (!mp_opt.echo) {
>>  			mptcp_pm_add_addr_received(msk, &mp_opt.addr);
>> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
>> index 9d7e7e13fba8..94c3568c7a60 100644
>> --- a/net/mptcp/protocol.c
>> +++ b/net/mptcp/protocol.c
>> @@ -2364,6 +2364,7 @@ static int __mptcp_init_sock(struct sock *sk)
>>  	msk->ack_hint = NULL;
>>  	msk->first = NULL;
>>  	inet_csk(sk)->icsk_sync_mss = mptcp_sync_mss;
>> +	WRITE_ONCE(msk->csum_reqd, mptcp_is_checksum_enabled(sock_net(sk)));
>>
>>  	mptcp_pm_data_init(msk);
>>
>> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
>> index 14f1e2fd3c08..4fc4871595ef 100644
>> --- a/net/mptcp/protocol.h
>> +++ b/net/mptcp/protocol.h
>> @@ -132,6 +132,7 @@ struct mptcp_options_received {
>>  		rm_addr : 1,
>>  		mp_prio : 1,
>>  		echo : 1,
>> +		csum_reqd : 1,
>>  		backup : 1;
>>  	u32	token;
>>  	u32	nonce;
>> @@ -233,6 +234,7 @@ struct mptcp_sock {
>>  	bool		snd_data_fin_enable;
>>  	bool		rcv_fastclose;
>>  	bool		use_64bit_ack; /* Set when we received a 64-bit DSN */
>> +	bool		csum_reqd;
>>  	spinlock_t	join_list_lock;
>>  	struct sock	*ack_hint;
>>  	struct work_struct work;
>> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
>> index e2e9cc34713d..9fec1a273100 100644
>> --- a/net/mptcp/subflow.c
>> +++ b/net/mptcp/subflow.c
>> @@ -403,6 +403,8 @@ static void subflow_finish_connect(struct sock *sk, const struct sk_buff *skb)
>>  			goto fallback;
>>  		}
>>
>> +		if (mp_opt.csum_reqd)
>> +			WRITE_ONCE(mptcp_sk(parent)->csum_reqd, true);
>>  		subflow->mp_capable = 1;
>>  		subflow->can_ack = 1;
>>  		subflow->remote_key = mp_opt.sndr_key;
>> @@ -643,6 +645,8 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
>>  		new_msk = mptcp_sk_clone(listener->conn, &mp_opt, req);
>>  		if (!new_msk)
>>  			fallback = true;
>> +		else if (mp_opt.csum_reqd)
>> +			WRITE_ONCE(mptcp_sk(new_msk)->csum_reqd, true);
>
> I think this chunk would be better located inside mptcp_sk_clone().
>
> @Mat, @Florian, slighly related: if we do the assignment there, the
> WRITE_ONCE will not be required, right? - since that functions runs
> under msk lock. We already have a few WRITE_ONCE there which are likely
> not really needed...

Yeah, I think it would fit well in mptcp_sk_clone(). Given that csum_reqd 
doesn't change after the connection starts I'm not sure 
WRITE_ONCE/READ_ONCE are really needed for the value, and I think the need 
for WRITE_ONCE is determined by the lockless readers rather than whether 
the writer is under msk lock (there's a recent LWN article I really should 
read to confirm my understanding).

--
Mat Martineau
Intel