Re: Patch "mptcp: strict local address ID selection" has been added to the 5.17-stable tree

Re: Patch "mptcp: strict local address ID selection" has been added to the 5.17-stable tree

[Matthieu Baerts]

Hi Mat, Paolo,

(- stable list and netdev maintainers, + mptcp list)

On 23/05/2022 05:06, Sasha Levin wrote:
> This is a note to let you know that I've just added the patch titled
> 
>     mptcp: strict local address ID selection
> 
> to the 5.17-stable tree which can be found at:
>     http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary
> 
> The filename of the patch is:
>      mptcp-strict-local-address-id-selection.patch
> and it can be found in the queue-5.17 subdirectory.
> 
> If you, or anyone else, feels it should not be added to the stable tree,
> please let <stable@vger.kernel.org> know about it.


Do we want the patch below to be backported to stable?

It think it is slightly changing the behaviour and we might not want it
there, no?

Cheers,
Matt


> commit dd874ab95a2df6fce00831bf16a06ed827639f6c
> Author: Paolo Abeni <pabeni@redhat.com>
> Date:   Mon Mar 7 12:44:37 2022 -0800
> 
>     mptcp: strict local address ID selection
>     
>     [ Upstream commit 4cf86ae84c718333928fd2d43168a1e359a28329 ]
>     
>     The address ID selection for MPJ subflows created in response
>     to incoming ADD_ADDR option is currently unreliable: it happens
>     at MPJ socket creation time, when the local address could be
>     unknown.
>     
>     Additionally, if the no local endpoint is available for the local
>     address, a new dummy endpoint is created, confusing the user-land.
>     
>     This change refactor the code to move the address ID selection inside
>     the rebuild_header() helper, when the local address eventually
>     selected by the route lookup is finally known. If the address used
>     is not mapped by any endpoint - and thus can't be advertised/removed
>     pick the id 0 instead of allocate a new endpoint.
>     
>     Signed-off-by: Paolo Abeni <pabeni@redhat.com>
>     Signed-off-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
>     Signed-off-by: Jakub Kicinski <kuba@kernel.org>
>     Signed-off-by: Sasha Levin <sashal@kernel.org>
> 
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index 4b5d795383cd..ec73bd4be0a8 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -83,16 +83,6 @@ static bool addresses_equal(const struct mptcp_addr_info *a,
>  	return a->port == b->port;
>  }
>  
> -static bool address_zero(const struct mptcp_addr_info *addr)
> -{
> -	struct mptcp_addr_info zero;
> -
> -	memset(&zero, 0, sizeof(zero));
> -	zero.family = addr->family;
> -
> -	return addresses_equal(addr, &zero, true);
> -}
> -
>  static void local_address(const struct sock_common *skc,
>  			  struct mptcp_addr_info *addr)
>  {
> @@ -1011,9 +1001,6 @@ int mptcp_pm_nl_get_local_id(struct mptcp_sock *msk, struct sock_common *skc)
>  	if (addresses_equal(&msk_local, &skc_local, false))
>  		return 0;
>  
> -	if (address_zero(&skc_local))
> -		return 0;
> -
>  	pernet = net_generic(sock_net((struct sock *)msk), pm_nl_pernet_id);
>  
>  	rcu_read_lock();
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 014c9d88f947..cb90941840b1 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -117,6 +117,9 @@ static int __mptcp_socket_create(struct mptcp_sock *msk)
>  	list_add(&subflow->node, &msk->conn_list);
>  	sock_hold(ssock->sk);
>  	subflow->request_mptcp = 1;
> +
> +	/* This is the first subflow, always with id 0 */
> +	subflow->local_id_valid = 1;
>  	mptcp_sock_graft(msk->first, sk->sk_socket);
>  
>  	return 0;
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index aec767ee047a..e4413b3e50c2 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -442,7 +442,8 @@ struct mptcp_subflow_context {
>  		rx_eof : 1,
>  		can_ack : 1,        /* only after processing the remote a key */
>  		disposable : 1,	    /* ctx can be free at ulp release time */
> -		stale : 1;	    /* unable to snd/rcv data, do not use for xmit */
> +		stale : 1,	    /* unable to snd/rcv data, do not use for xmit */
> +		local_id_valid : 1; /* local_id is correctly initialized */
>  	enum mptcp_data_avail data_avail;
>  	u32	remote_nonce;
>  	u64	thmac;
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 651f01d13191..e27574e9f969 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -483,6 +483,51 @@ static void subflow_finish_connect(struct sock *sk, const struct sk_buff *skb)
>  	mptcp_subflow_reset(sk);
>  }
>  
> +static void subflow_set_local_id(struct mptcp_subflow_context *subflow, int local_id)
> +{
> +	subflow->local_id = local_id;
> +	subflow->local_id_valid = 1;
> +}
> +
> +static int subflow_chk_local_id(struct sock *sk)
> +{
> +	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
> +	struct mptcp_sock *msk = mptcp_sk(subflow->conn);
> +	int err;
> +
> +	if (likely(subflow->local_id_valid))
> +		return 0;
> +
> +	err = mptcp_pm_get_local_id(msk, (struct sock_common *)sk);
> +	if (err < 0)
> +		return err;
> +
> +	subflow_set_local_id(subflow, err);
> +	return 0;
> +}
> +
> +static int subflow_rebuild_header(struct sock *sk)
> +{
> +	int err = subflow_chk_local_id(sk);
> +
> +	if (unlikely(err < 0))
> +		return err;
> +
> +	return inet_sk_rebuild_header(sk);
> +}
> +
> +#if IS_ENABLED(CONFIG_MPTCP_IPV6)
> +static int subflow_v6_rebuild_header(struct sock *sk)
> +{
> +	int err = subflow_chk_local_id(sk);
> +
> +	if (unlikely(err < 0))
> +		return err;
> +
> +	return inet6_sk_rebuild_header(sk);
> +}
> +#endif
> +
>  struct request_sock_ops mptcp_subflow_request_sock_ops;
>  EXPORT_SYMBOL_GPL(mptcp_subflow_request_sock_ops);
>  static struct tcp_request_sock_ops subflow_request_sock_ipv4_ops;
> @@ -1401,13 +1446,8 @@ int __mptcp_subflow_connect(struct sock *sk, const struct mptcp_addr_info *loc,
>  		get_random_bytes(&subflow->local_nonce, sizeof(u32));
>  	} while (!subflow->local_nonce);
>  
> -	if (!local_id) {
> -		err = mptcp_pm_get_local_id(msk, (struct sock_common *)ssk);
> -		if (err < 0)
> -			goto failed;
> -
> -		local_id = err;
> -	}
> +	if (local_id)
> +		subflow_set_local_id(subflow, local_id);
>  
>  	mptcp_pm_get_flags_and_ifindex_by_id(sock_net(sk), local_id,
>  					     &flags, &ifindex);
> @@ -1432,7 +1472,6 @@ int __mptcp_subflow_connect(struct sock *sk, const struct mptcp_addr_info *loc,
>  	pr_debug("msk=%p remote_token=%u local_id=%d remote_id=%d", msk,
>  		 remote_token, local_id, remote_id);
>  	subflow->remote_token = remote_token;
> -	subflow->local_id = local_id;
>  	subflow->remote_id = remote_id;
>  	subflow->request_join = 1;
>  	subflow->request_bkup = !!(flags & MPTCP_PM_ADDR_FLAG_BACKUP);
> @@ -1737,15 +1776,22 @@ static void subflow_ulp_clone(const struct request_sock *req,
>  		new_ctx->token = subflow_req->token;
>  		new_ctx->ssn_offset = subflow_req->ssn_offset;
>  		new_ctx->idsn = subflow_req->idsn;
> +
> +		/* this is the first subflow, id is always 0 */
> +		new_ctx->local_id_valid = 1;
>  	} else if (subflow_req->mp_join) {
>  		new_ctx->ssn_offset = subflow_req->ssn_offset;
>  		new_ctx->mp_join = 1;
>  		new_ctx->fully_established = 1;
>  		new_ctx->backup = subflow_req->backup;
> -		new_ctx->local_id = subflow_req->local_id;
>  		new_ctx->remote_id = subflow_req->remote_id;
>  		new_ctx->token = subflow_req->token;
>  		new_ctx->thmac = subflow_req->thmac;
> +
> +		/* the subflow req id is valid, fetched via subflow_check_req()
> +		 * and subflow_token_join_request()
> +		 */
> +		subflow_set_local_id(new_ctx, subflow_req->local_id);
>  	}
>  }
>  
> @@ -1798,6 +1844,7 @@ void __init mptcp_subflow_init(void)
>  	subflow_specific.conn_request = subflow_v4_conn_request;
>  	subflow_specific.syn_recv_sock = subflow_syn_recv_sock;
>  	subflow_specific.sk_rx_dst_set = subflow_finish_connect;
> +	subflow_specific.rebuild_header = subflow_rebuild_header;
>  
>  	tcp_prot_override = tcp_prot;
>  	tcp_prot_override.release_cb = tcp_release_cb_override;
> @@ -1810,6 +1857,7 @@ void __init mptcp_subflow_init(void)
>  	subflow_v6_specific.conn_request = subflow_v6_conn_request;
>  	subflow_v6_specific.syn_recv_sock = subflow_syn_recv_sock;
>  	subflow_v6_specific.sk_rx_dst_set = subflow_finish_connect;
> +	subflow_v6_specific.rebuild_header = subflow_v6_rebuild_header;
>  
>  	subflow_v6m_specific = subflow_v6_specific;
>  	subflow_v6m_specific.queue_xmit = ipv4_specific.queue_xmit;
> @@ -1817,6 +1865,7 @@ void __init mptcp_subflow_init(void)
>  	subflow_v6m_specific.net_header_len = ipv4_specific.net_header_len;
>  	subflow_v6m_specific.mtu_reduced = ipv4_specific.mtu_reduced;
>  	subflow_v6m_specific.net_frag_header_len = 0;
> +	subflow_v6m_specific.rebuild_header = subflow_rebuild_header;
>  
>  	tcpv6_prot_override = tcpv6_prot;
>  	tcpv6_prot_override.release_cb = tcp_release_cb_override;

-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

Re: Patch "mptcp: strict local address ID selection" has been added to the 5.17-stable tree

[Matthieu Baerts]

On 23/05/2022 17:33, Matthieu Baerts wrote:
> Hi Mat, Paolo,
> 
> (- stable list and netdev maintainers, + mptcp list)
> 
> On 23/05/2022 05:06, Sasha Levin wrote:
>> This is a note to let you know that I've just added the patch titled
>>
>>     mptcp: strict local address ID selection
>>
>> to the 5.17-stable tree which can be found at:
>>     http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary
>>
>> The filename of the patch is:
>>      mptcp-strict-local-address-id-selection.patch
>> and it can be found in the queue-5.17 subdirectory.
>>
>> If you, or anyone else, feels it should not be added to the stable tree,
>> please let <stable@vger.kernel.org> know about it.
> 
> 
> Do we want the patch below to be backported to stable?
> 
> It think it is slightly changing the behaviour and we might not want it
> there, no?

(...)

>> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
>> index aec767ee047a..e4413b3e50c2 100644
>> --- a/net/mptcp/protocol.h
>> +++ b/net/mptcp/protocol.h
>> @@ -442,7 +442,8 @@ struct mptcp_subflow_context {
>>  		rx_eof : 1,
>>  		can_ack : 1,        /* only after processing the remote a key */
>>  		disposable : 1,	    /* ctx can be free at ulp release time */
>> -		stale : 1;	    /* unable to snd/rcv data, do not use for xmit */
>> +		stale : 1,	    /* unable to snd/rcv data, do not use for xmit */
>> +		local_id_valid : 1; /* local_id is correctly initialized */

I guess the reason this patch has been added to the stable queue is
because this patch adds 'local_id_valid' element here in
'mptcp_subflow_context' structure.

If you look at "mptcp: Do TCP fallback on early DSS checksum failure"
patch, Mat added 'valid_csum_seen' just after 'local_id_valid'.

If we don't want  "mptcp: strict local address ID selection" to be
backported, we can probably easily resolve the conflict and send a new
version adding 'valid_csum_seen' just after 'stale'.

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

Re: Patch "mptcp: strict local address ID selection" has been added to the 5.17-stable tree

[Paolo Abeni]

On Mon, 2022-05-23 at 17:33 +0200, Matthieu Baerts wrote:
> Hi Mat, Paolo,
> 
> (- stable list and netdev maintainers, + mptcp list)
> 
> On 23/05/2022 05:06, Sasha Levin wrote:
> > This is a note to let you know that I've just added the patch titled
> > 
> >     mptcp: strict local address ID selection
> > 
> > to the 5.17-stable tree which can be found at:
> >     http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary
> > 
> > The filename of the patch is:
> >      mptcp-strict-local-address-id-selection.patch
> > and it can be found in the queue-5.17 subdirectory.
> > 
> > If you, or anyone else, feels it should not be added to the stable tree,
> > please let <stable@vger.kernel.org> know about it.
> 
> 
> Do we want the patch below to be backported to stable?
> 
> It think it is slightly changing the behaviour and we might not want it
> there, no?

I don't have hard-preferences, but I *think* we are better-off with
this patch and the csum-one in without any additional effort, than to
craft csum specifically for stable (and delay it). 

Cheers,

Paolo

Re: Patch "mptcp: strict local address ID selection" has been added to the 5.17-stable tree

[Mat Martineau]

On Mon, 23 May 2022, Matthieu Baerts wrote:

> On 23/05/2022 17:33, Matthieu Baerts wrote:
>> Hi Mat, Paolo,
>>
>> (- stable list and netdev maintainers, + mptcp list)
>>
>> On 23/05/2022 05:06, Sasha Levin wrote:
>>> This is a note to let you know that I've just added the patch titled
>>>
>>>     mptcp: strict local address ID selection
>>>
>>> to the 5.17-stable tree which can be found at:
>>>     http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary
>>>
>>> The filename of the patch is:
>>>      mptcp-strict-local-address-id-selection.patch
>>> and it can be found in the queue-5.17 subdirectory.
>>>
>>> If you, or anyone else, feels it should not be added to the stable tree,
>>> please let <stable@vger.kernel.org> know about it.
>>
>>
>> Do we want the patch below to be backported to stable?
>>
>> It think it is slightly changing the behaviour and we might not want it
>> there, no?
>
> (...)
>
>>> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
>>> index aec767ee047a..e4413b3e50c2 100644
>>> --- a/net/mptcp/protocol.h
>>> +++ b/net/mptcp/protocol.h
>>> @@ -442,7 +442,8 @@ struct mptcp_subflow_context {
>>>  		rx_eof : 1,
>>>  		can_ack : 1,        /* only after processing the remote a key */
>>>  		disposable : 1,	    /* ctx can be free at ulp release time */
>>> -		stale : 1;	    /* unable to snd/rcv data, do not use for xmit */
>>> +		stale : 1,	    /* unable to snd/rcv data, do not use for xmit */
>>> +		local_id_valid : 1; /* local_id is correctly initialized */
>
> I guess the reason this patch has been added to the stable queue is
> because this patch adds 'local_id_valid' element here in
> 'mptcp_subflow_context' structure.
>
> If you look at "mptcp: Do TCP fallback on early DSS checksum failure"
> patch, Mat added 'valid_csum_seen' just after 'local_id_valid'.
>
> If we don't want  "mptcp: strict local address ID selection" to be
> backported, we can probably easily resolve the conflict and send a new
> version adding 'valid_csum_seen' just after 'stale'.
>

Like Paolo, I don't have a strong preference - but since the dependency is 
only because of diff context (not because of any actual functionality) the 
choice seems less clear!

I'm not aware of any negative impacts from "mptcp: strict local address ID 
selection", so I lean in Paolo's direction to let -stable cherry-pick the 
"extra" commit to make the stable process easier.

I'll try running the self tests on 5.15 and 5.17 -stable with these 
commits and see if there is any problem there.

--
Mat Martineau
Intel