[MPTCP] Re: [MPTCP][PATCH v3 mptcp-next 1/2] mptcp: send out dedicated ADD_ADDR packet

[MPTCP] Re: [MPTCP][PATCH v3 mptcp-next 1/2] mptcp: send out dedicated ADD_ADDR packet

[Paolo Abeni]

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

On Mon, 2020-09-28 at 15:46 +0800, Geliang Tang wrote:
> @@ -584,7 +586,7 @@ static bool
> mptcp_established_options_add_addr(struct sock *sk,
>  	int len;
>  
>  	if (!mptcp_pm_should_add_signal(msk) ||
> -	    !(mptcp_pm_add_addr_signal(msk, remaining, &saddr, &echo)))
> +	    !(mptcp_pm_add_addr_signal(msk, sk, remaining, &saddr,
> &echo)))
>  		return false;
>  
>  	len = mptcp_add_addr_len(saddr.family);
> @@ -645,6 +647,17 @@ static bool
> mptcp_established_options_rm_addr(struct sock *sk,
>  	return true;
>  }
>  
> +static bool mptcp_check_add_addr_nospc(struct sock *sk, struct
> sk_buff *skb)
> +{
> +	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
> +	struct mptcp_sock *msk = mptcp_sk(subflow->conn);
> +
> +	if (skb && skb_is_tcp_pure_ack(skb) && READ_ONCE(msk-
> >pm.add_addr_nospc))
> +		return true;
> +
> +	return false;
> +}
> +
>  bool mptcp_established_options(struct sock *sk, struct sk_buff *skb,
>  			       unsigned int *size, unsigned int
> remaining,
>  			       struct mptcp_out_options *opts)
> @@ -657,6 +670,9 @@ bool mptcp_established_options(struct sock *sk,
> struct sk_buff *skb,
>  	if (unlikely(mptcp_check_fallback(sk)))
>  		return false;
>  
> +	if (unlikely(mptcp_check_add_addr_nospc(sk, skb)))
> +		goto add_addr;
> +

I can't follow easily the code. Here we check the 'add_addr_nospc'
field, but AFAICS, such field is initialized only later,
by mptcp_pm_add_addr_signal(), called
from mptcp_established_options_add_addr().

Should we instead set 'add_addr_nospc' in mptcp_pm_announce_addr() ?

Ninor note: the 'add_addr_nospc' name is a bit confusing to me, I would
use instead something alike 'add_addr_ipv6', or even better we could
change the 'add_addr_signal' type from bool to char, so that we could
encode the addr type there. Then here we could check for
'add_addr_signal == AF_INET6' - possibly no need for a new field.

>  	if (mptcp_established_options_mp(sk, skb, &opt_size, remaining,
> opts))
>  		ret = true;
>  	else if (mptcp_established_options_dss(sk, skb, &opt_size,
> remaining,
> @@ -671,6 +687,7 @@ bool mptcp_established_options(struct sock *sk,
> struct sk_buff *skb,
>  
>  	*size += opt_size;
>  	remaining -= opt_size;
> +add_addr:
>  	if (mptcp_established_options_add_addr(sk, &opt_size,
> remaining, opts)) {
>  		*size += opt_size;
>  		remaining -= opt_size;
> @@ -747,10 +764,10 @@ static bool check_fully_established(struct
> mptcp_sock *msk, struct sock *sk,
>  		goto fully_established;
>  	}
>  
> -	/* If the first established packet does not contain MP_CAPABLE
> + data
> +	/* If the first established packet does not contain MP_CAPABLE
> + data or ADD_ADDR
>  	 * then fallback to TCP
>  	 */
> -	if (!mp_opt->mp_capable) {
> +	if (!mp_opt->mp_capable && !mp_opt->add_addr) {
>  		subflow->mp_capable = 0;
>  		pr_fallback(msk);
>  		__mptcp_do_fallback(msk);

I think the RFC is a bit unclear with this point, because in
section 3.4.1. says: 

"""
This [ADD_ADDR] option can be used at any time during a connection
"""

but also says that here we expect a mp_capable + data packet. 

@Christoph, should we accept ADD_ADDR here or should we reject them?

> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index 7e81f53d1e5d..8015ee8ba621 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
> @@ -172,7 +172,8 @@ void mptcp_pm_rm_addr_received(struct mptcp_sock
> *msk, u8 rm_id)
>  
>  /* path manager helpers */
>  
> -bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int
> remaining,
> +bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct sock
> *ssk,
> +			      unsigned int remaining,
>  			      struct mptcp_addr_info *saddr, bool
> *echo)
>  {
>  	int ret = false;
> @@ -183,16 +184,26 @@ bool mptcp_pm_add_addr_signal(struct mptcp_sock
> *msk, unsigned int remaining,
>  	if (!mptcp_pm_should_add_signal(msk))
>  		goto out_unlock;
>  
> -	if (remaining < mptcp_add_addr_len(msk->pm.local.family))
> +	if (remaining < mptcp_add_addr_len(msk->pm.local.family)) {
> +		if (msk->pm.local.family == AF_INET6)
> +			WRITE_ONCE(msk->pm.add_addr_nospc, true);
>  		goto out_unlock;
> +	}
>  
>  	*saddr = msk->pm.local;
>  	*echo = READ_ONCE(msk->pm.add_addr_echo);
>  	WRITE_ONCE(msk->pm.add_addr_signal, false);
> +	WRITE_ONCE(msk->pm.add_addr_nospc, false);
>  	ret = true;
>  
>  out_unlock:
>  	spin_unlock_bh(&msk->pm.lock);
> +
> +	if (READ_ONCE(msk->pm.add_addr_nospc)) {
> +		pr_debug("send ack for add_addr6");
> +		tcp_send_ack(ssk);
> +	}

I think we should pick-up a subflow and call tcp_send_ack() when
invoking mptcp_pm_announce_addr(..., false), either directly
in mptcp_pm_announce_addr() or better in a wrapping helper.

/P

[MPTCP] Re: [MPTCP][PATCH v3 mptcp-next 1/2] mptcp: send out dedicated ADD_ADDR packet

[Christoph Paasch]

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

On 09/30/20 - 11:31, Matthieu Baerts wrote:
> Hi Christoph, Paolo, Geliang,
> 
> On 29/09/2020 20:07, Christoph Paasch wrote:
> > On 09/29/20 - 11:30, Paolo Abeni wrote:
> > > On Mon, 2020-09-28 at 15:46 +0800, Geliang Tang wrote:
> 
> (...)
> 
> > > > -	/* If the first established packet does not contain MP_CAPABLE
> > > > + data
> > > > +	/* If the first established packet does not contain MP_CAPABLE
> > > > + data or ADD_ADDR
> > > >   	 * then fallback to TCP
> > > >   	 */
> > > > -	if (!mp_opt->mp_capable) {
> > > > +	if (!mp_opt->mp_capable && !mp_opt->add_addr) {
> > > >   		subflow->mp_capable = 0;
> > > >   		pr_fallback(msk);
> > > >   		__mptcp_do_fallback(msk);
> > > 
> > > I think the RFC is a bit unclear with this point, because in
> > > section 3.4.1. says:
> > > 
> > > """
> > > This [ADD_ADDR] option can be used at any time during a connection
> > > """
> > > 
> > > but also says that here we expect a mp_capable + data packet.
> > > 
> > > @Christoph, should we accept ADD_ADDR here or should we reject them?
> > 
> > I think we should accept them. There are 2 things that can happen:
> > 
> > * MP_CAPABLE + data comes later on. In that case we can use the received
> >    addr.
> >    Note, for this to happen I am assuming that the ADD_ADDR comes on an
> >    out-of-order packet and that the MP_CAPABLE will still come on the packet
> >    with relative sequence-number 1.
> 
> If the ADD_ADDR comes on an out-of-order packet and because it is a ACK
> without data, is it not dropped earlier by the TCP stack?
> I think we have this behaviour with mptcp.org version.

No, as far as I can see in mptcp.org we do parse the ADD_ADDR even if we
haven't yet received an MP_CAPABLE with data.

cfr., mptcp_handle_options, called from tcp_validate_incoming, which is
called even on ACKs without data.


Christoph

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

[MPTCP] Re: [MPTCP][PATCH v3 mptcp-next 1/2] mptcp: send out dedicated ADD_ADDR packet

[Matthieu Baerts]

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

Hi Christoph, Paolo, Geliang,

On 29/09/2020 20:07, Christoph Paasch wrote:
> On 09/29/20 - 11:30, Paolo Abeni wrote:
>> On Mon, 2020-09-28 at 15:46 +0800, Geliang Tang wrote:

(...)

>>> -	/* If the first established packet does not contain MP_CAPABLE
>>> + data
>>> +	/* If the first established packet does not contain MP_CAPABLE
>>> + data or ADD_ADDR
>>>   	 * then fallback to TCP
>>>   	 */
>>> -	if (!mp_opt->mp_capable) {
>>> +	if (!mp_opt->mp_capable && !mp_opt->add_addr) {
>>>   		subflow->mp_capable = 0;
>>>   		pr_fallback(msk);
>>>   		__mptcp_do_fallback(msk);
>>
>> I think the RFC is a bit unclear with this point, because in
>> section 3.4.1. says:
>>
>> """
>> This [ADD_ADDR] option can be used at any time during a connection
>> """
>>
>> but also says that here we expect a mp_capable + data packet.
>>
>> @Christoph, should we accept ADD_ADDR here or should we reject them?
> 
> I think we should accept them. There are 2 things that can happen:
> 
> * MP_CAPABLE + data comes later on. In that case we can use the received
>    addr.
>    Note, for this to happen I am assuming that the ADD_ADDR comes on an
>    out-of-order packet and that the MP_CAPABLE will still come on the packet
>    with relative sequence-number 1.

If the ADD_ADDR comes on an out-of-order packet and because it is a ACK 
without data, is it not dropped earlier by the TCP stack?
I think we have this behaviour with mptcp.org version.

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

[MPTCP] Re: [MPTCP][PATCH v3 mptcp-next 1/2] mptcp: send out dedicated ADD_ADDR packet

[Christoph Paasch]

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

On 09/29/20 - 11:30, Paolo Abeni wrote:
> On Mon, 2020-09-28 at 15:46 +0800, Geliang Tang wrote:
> > @@ -584,7 +586,7 @@ static bool
> > mptcp_established_options_add_addr(struct sock *sk,
> >  	int len;
> >  
> >  	if (!mptcp_pm_should_add_signal(msk) ||
> > -	    !(mptcp_pm_add_addr_signal(msk, remaining, &saddr, &echo)))
> > +	    !(mptcp_pm_add_addr_signal(msk, sk, remaining, &saddr,
> > &echo)))
> >  		return false;
> >  
> >  	len = mptcp_add_addr_len(saddr.family);
> > @@ -645,6 +647,17 @@ static bool
> > mptcp_established_options_rm_addr(struct sock *sk,
> >  	return true;
> >  }
> >  
> > +static bool mptcp_check_add_addr_nospc(struct sock *sk, struct
> > sk_buff *skb)
> > +{
> > +	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
> > +	struct mptcp_sock *msk = mptcp_sk(subflow->conn);
> > +
> > +	if (skb && skb_is_tcp_pure_ack(skb) && READ_ONCE(msk-
> > >pm.add_addr_nospc))
> > +		return true;
> > +
> > +	return false;
> > +}
> > +
> >  bool mptcp_established_options(struct sock *sk, struct sk_buff *skb,
> >  			       unsigned int *size, unsigned int
> > remaining,
> >  			       struct mptcp_out_options *opts)
> > @@ -657,6 +670,9 @@ bool mptcp_established_options(struct sock *sk,
> > struct sk_buff *skb,
> >  	if (unlikely(mptcp_check_fallback(sk)))
> >  		return false;
> >  
> > +	if (unlikely(mptcp_check_add_addr_nospc(sk, skb)))
> > +		goto add_addr;
> > +
> 
> I can't follow easily the code. Here we check the 'add_addr_nospc'
> field, but AFAICS, such field is initialized only later,
> by mptcp_pm_add_addr_signal(), called
> from mptcp_established_options_add_addr().
> 
> Should we instead set 'add_addr_nospc' in mptcp_pm_announce_addr() ?
> 
> Ninor note: the 'add_addr_nospc' name is a bit confusing to me, I would
> use instead something alike 'add_addr_ipv6', or even better we could
> change the 'add_addr_signal' type from bool to char, so that we could
> encode the addr type there. Then here we could check for
> 'add_addr_signal == AF_INET6' - possibly no need for a new field.
> 
> >  	if (mptcp_established_options_mp(sk, skb, &opt_size, remaining,
> > opts))
> >  		ret = true;
> >  	else if (mptcp_established_options_dss(sk, skb, &opt_size,
> > remaining,
> > @@ -671,6 +687,7 @@ bool mptcp_established_options(struct sock *sk,
> > struct sk_buff *skb,
> >  
> >  	*size += opt_size;
> >  	remaining -= opt_size;
> > +add_addr:
> >  	if (mptcp_established_options_add_addr(sk, &opt_size,
> > remaining, opts)) {
> >  		*size += opt_size;
> >  		remaining -= opt_size;
> > @@ -747,10 +764,10 @@ static bool check_fully_established(struct
> > mptcp_sock *msk, struct sock *sk,
> >  		goto fully_established;
> >  	}
> >  
> > -	/* If the first established packet does not contain MP_CAPABLE
> > + data
> > +	/* If the first established packet does not contain MP_CAPABLE
> > + data or ADD_ADDR
> >  	 * then fallback to TCP
> >  	 */
> > -	if (!mp_opt->mp_capable) {
> > +	if (!mp_opt->mp_capable && !mp_opt->add_addr) {
> >  		subflow->mp_capable = 0;
> >  		pr_fallback(msk);
> >  		__mptcp_do_fallback(msk);
> 
> I think the RFC is a bit unclear with this point, because in
> section 3.4.1. says: 
> 
> """
> This [ADD_ADDR] option can be used at any time during a connection
> """
> 
> but also says that here we expect a mp_capable + data packet. 
> 
> @Christoph, should we accept ADD_ADDR here or should we reject them?

I think we should accept them. There are 2 things that can happen:

* MP_CAPABLE + data comes later on. In that case we can use the received
  addr.
  Note, for this to happen I am assuming that the ADD_ADDR comes on an
  out-of-order packet and that the MP_CAPABLE will still come on the packet
  with relative sequence-number 1.
* No MP_CAPABLE on first data-packet. In that case we have a seamless
  fallback to regular TCP.


Does that help?


Christoph

> 
> > diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> > index 7e81f53d1e5d..8015ee8ba621 100644
> > --- a/net/mptcp/pm.c
> > +++ b/net/mptcp/pm.c
> > @@ -172,7 +172,8 @@ void mptcp_pm_rm_addr_received(struct mptcp_sock
> > *msk, u8 rm_id)
> >  
> >  /* path manager helpers */
> >  
> > -bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int
> > remaining,
> > +bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct sock
> > *ssk,
> > +			      unsigned int remaining,
> >  			      struct mptcp_addr_info *saddr, bool
> > *echo)
> >  {
> >  	int ret = false;
> > @@ -183,16 +184,26 @@ bool mptcp_pm_add_addr_signal(struct mptcp_sock
> > *msk, unsigned int remaining,
> >  	if (!mptcp_pm_should_add_signal(msk))
> >  		goto out_unlock;
> >  
> > -	if (remaining < mptcp_add_addr_len(msk->pm.local.family))
> > +	if (remaining < mptcp_add_addr_len(msk->pm.local.family)) {
> > +		if (msk->pm.local.family == AF_INET6)
> > +			WRITE_ONCE(msk->pm.add_addr_nospc, true);
> >  		goto out_unlock;
> > +	}
> >  
> >  	*saddr = msk->pm.local;
> >  	*echo = READ_ONCE(msk->pm.add_addr_echo);
> >  	WRITE_ONCE(msk->pm.add_addr_signal, false);
> > +	WRITE_ONCE(msk->pm.add_addr_nospc, false);
> >  	ret = true;
> >  
> >  out_unlock:
> >  	spin_unlock_bh(&msk->pm.lock);
> > +
> > +	if (READ_ONCE(msk->pm.add_addr_nospc)) {
> > +		pr_debug("send ack for add_addr6");
> > +		tcp_send_ack(ssk);
> > +	}
> 
> I think we should pick-up a subflow and call tcp_send_ack() when
> invoking mptcp_pm_announce_addr(..., false), either directly
> in mptcp_pm_announce_addr() or better in a wrapping helper.
> 
> /P
>