[MPTCP] Re: [PATCH] Squash-to: "mptcp: cope with later TCP fallback"

[MPTCP] Re: [PATCH] Squash-to: "mptcp: cope with later TCP fallback"

[Peter Krystad]

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

On Tue, 2020-01-21 at 08:49 -0800, Mat Martineau wrote:
> On Tue, 21 Jan 2020, Paolo Abeni wrote:
> 
> > kbuildbot reported a build breakage with IPV6 disabled.
> > 
> > Signed-off-by: Paolo Abeni <pabeni(a)redha.com>
> > ---
> > net/mptcp/protocol.c | 12 ++++++++++--
> > 1 file changed, 10 insertions(+), 2 deletions(-)
> > 
> > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> > index 6da609791c7a..4e289ff748ad 100644
> > --- a/net/mptcp/protocol.c
> > +++ b/net/mptcp/protocol.c
> > @@ -26,6 +26,15 @@
> > 
> > static void __mptcp_close(struct sock *sk, long timeout);
> > 
> > +static const struct proto_ops * tcp_proto_ops(struct sock *sk)
> > +{
> > +#if IS_ENABLED(IPV6)
> 
> Does this need to be IS_ENABLED(CONFIG_IPV6) ?

Or IS_ENABLED(CONFIG_MPTCP_IPV6)?

Peter.

> > +	if (sk->sk_family == AF_INET6)
> > +		return &inet6_stream_ops;
> > +#endif
> > +	return &inet_stream_ops;
> > +}
> > +
> > /* MP_CAPABLE handshake failed, convert msk to plain tcp, replacing
> >  * socket->sk and stream ops and destroying msk
> >  * return the msk socket, as we can't access msk anymore after this function
> > @@ -60,8 +69,7 @@ static struct socket *__mptcp_fallback_to_tcp(struct mptcp_sock *msk,
> > 		subflow->conn = NULL;
> > 	}
> > 	release_sock(ssk);
> > -	sock->ops = sk->sk_family == AF_INET6 ? &inet6_stream_ops :
> > -						&inet_stream_ops;
> > +	sock->ops = tcp_proto_ops(ssk);
> > 
> > 	/* destroy the left-over msk sock */
> > 	__mptcp_close(sk, 0);
> > -- 
> > 2.21.0
> 
> --
> Mat Martineau
> Intel
> _______________________________________________
> mptcp mailing list -- mptcp(a)lists.01.org
> To unsubscribe send an email to mptcp-leave(a)lists.01.org

[MPTCP] Re: [PATCH] Squash-to: "mptcp: cope with later TCP fallback"

[Matthieu Baerts]

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



On 21/01/2020 23:13, Matthieu Baerts wrote:
> Hi Paolo,
> 
> On 21/01/2020 21:40, Paolo Abeni wrote:
>> On Tue, 2020-01-21 at 18:20 +0100, Matthieu Baerts wrote:
>>> Hi Paolo, Mat,
>>>
>>> On 21/01/2020 17:32, Paolo Abeni wrote:
>>>> kbuildbot reported a build breakage with IPV6 disabled.
>>>
>>> Do you know why my CI didn't get the issue?
>>>
>>> It runs:
>>>
>>>     make defconfig
>>>     echo | scripts/config -d (...) ## to disable DRM, SOUND, USB, etc.
>>>     echo | scripts/config -e MPTCP -e MPTCP_IPV6
>>>     echo | scripts/config -d IPV6 -d MPTCP_IPV6
>>>     make
>>>
>>> MPTCP is enabled and IPV6 is not.
>>>
>>> Which combination did I miss?
>>
>> weird. I can reproduce the build failure with
>>
>> CONFIG_MPTCP=y
>> # CONFIG_IPV6 is not set
>> # CONFIG_MPTCP_IPV6 is not set
>>
>>
>> net/mptcp/protocol.o: In function `__mptcp_fallback_to_tcp':
>> /home/pabeni/linux-net/net/mptcp/protocol.c:63: undefined reference to
>> `inet6_stream_ops'
>>
>> the inet6_stream_ops struct is unconditionally referenced by the mptcp
>> code.
> 
> I don't understand why but my CI didn't catch errors when compiling 
> without IPv6 :-/

Mat found the issue:

In short if you have this script:


===
#!/bin/bash
set -e

ff() {
	echo before
	false
	echo after
}

ff
===


only "before" will be printed. But with this one:


===
#!/bin/bash
set -e

ff() {
	echo before
	false
	echo after
}

ff || echo "error"
===

both "before" and "after" are printed.


Mat also found the explanation in the manual:

    Exit immediately unless the command is "part of any command executed 
in a && or || list except the command following the final && or ||"


In other words, better to always use 'if ! X; then Y; fi' then 'X || Y'.

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

[MPTCP] Re: [PATCH] Squash-to: "mptcp: cope with later TCP fallback"

[Matthieu Baerts]

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

Hi Paolo,

On 21/01/2020 21:40, Paolo Abeni wrote:
> On Tue, 2020-01-21 at 18:20 +0100, Matthieu Baerts wrote:
>> Hi Paolo, Mat,
>>
>> On 21/01/2020 17:32, Paolo Abeni wrote:
>>> kbuildbot reported a build breakage with IPV6 disabled.
>>
>> Do you know why my CI didn't get the issue?
>>
>> It runs:
>>
>>     make defconfig
>>     echo | scripts/config -d (...) ## to disable DRM, SOUND, USB, etc.
>>     echo | scripts/config -e MPTCP -e MPTCP_IPV6
>>     echo | scripts/config -d IPV6 -d MPTCP_IPV6
>>     make
>>
>> MPTCP is enabled and IPV6 is not.
>>
>> Which combination did I miss?
> 
> weird. I can reproduce the build failure with
> 
> CONFIG_MPTCP=y
> # CONFIG_IPV6 is not set
> # CONFIG_MPTCP_IPV6 is not set
> 
> 
> net/mptcp/protocol.o: In function `__mptcp_fallback_to_tcp':
> /home/pabeni/linux-net/net/mptcp/protocol.c:63: undefined reference to
> `inet6_stream_ops'
> 
> the inet6_stream_ops struct is unconditionally referenced by the mptcp
> code.

I don't understand why but my CI didn't catch errors when compiling 
without IPv6 :-/

Here is the script I use:

 
https://github.com/multipath-tcp/mptcp_net-next/blob/d77caf0c96ed3825c6c750445a7a5e772da27477/ci/update-tg-tree.sh#L278


And here is what I have in the logs:



+ tg checkout next
+ break
+ is_tg_top t/upstream
+ '[' t/upstream = t/upstream ']'
+ check_compilation_no_ipv6
+ generate_config_mptcp
+ generate_config_no_mptcp
+ make defconfig
*** Default configuration is based on 'x86_64_defconfig'
#
# configuration written to .config
#
+ echo
+ scripts/config --disable DRM --disable PCCARD --disable ATA --disable 
MD --disable PPS --disable SOUND --disable USB --disable IOMMU_SUPPORT 
--disable INPUT_LEDS --disable AGP --disable VGA_ARB --disable EFI 
--disable WLAN --disable WIRELESS --disable LOGO --disable NFS_FS 
--disable XFRM_USER --disable INET6_AH --disable INET6_ESP --disable 
NETDEVICES
+ echo
+ scripts/config -e MPTCP -e MPTCP_IPV6
+ echo
+ scripts/config -d IPV6 -d MPTCP_IPV6
+ compile_kernel 'without IPv6 and with CONFIG_MPTCP'
++ nproc
++ nproc
+ KCFLAGS=-Werror
+ make -j8 -l8
scripts/kconfig/conf  --syncconfig Kconfig
   DESCEND  objtool
   CALL    scripts/atomic/check-atomics.sh
   CALL    scripts/checksyscalls.sh
   CHK     include/generated/compile.h

[...]

   CC      net/mptcp/protocol.o
   CC      net/mptcp/subflow.o
   CC      kernel/sched/pelt.o
   CC      kernel/sched/stats.o
   CC      kernel/auditfilter.o
   CC      net/core/dev_ioctl.o
   CC      kernel/sched/cpuacct.o
   CC      net/ipv4/tcp_ulp.o
net/mptcp/protocol.c: In function 'mptcp_copy_inaddrs':
net/mptcp/protocol.c:1133:21: error: unused variable 'msk6' 
[-Werror=unused-variable]
   struct ipv6_pinfo *msk6 = inet6_sk(msk);
                      ^~~~
net/mptcp/protocol.c:1132:27: error: unused variable 'ssk6' 
[-Werror=unused-variable]
   const struct ipv6_pinfo *ssk6 = inet6_sk(ssk);
                            ^~~~
cc1: all warnings being treated as errors
scripts/Makefile.build:265: recipe for target 'net/mptcp/protocol.o' failed
make[2]: *** [net/mptcp/protocol.o] Error 1
make[2]: *** Waiting for unfinished jobs....
   CC      net/ipv4/tcp_offload.o
   CC      net/ipv4/datagram.o
scripts/Makefile.build:503: recipe for target 'net/mptcp' failed
make[1]: *** [net/mptcp] Error 2
make[1]: *** Waiting for unfinished jobs....
   CC      net/ipv4/raw.o

[...]

   AR      net/ipv4/built-in.a
make: *** [net] Error 2
Makefile:1693: recipe for target 'net' failed
+ err 'Unable to compile without IPv6 and with CONFIG_MPTCP'
+ echo 'ERROR: Unable to compile without IPv6 and with CONFIG_MPTCP'
ERROR: Unable to compile without IPv6 and with CONFIG_MPTCP
+ return 1
+ return 1
+ check_compilation_i386

[...]


I have no idea why "check_compilation_i386" is executed while "set -e" 
is launched at the beginning of the script (even set by the caller: 
"bash -ehxB patches/update-tg-tree.sh") and it should prevent that.

If you have any idea why, I would be very interested by the explanation :-)

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

[MPTCP] Re: [PATCH] Squash-to: "mptcp: cope with later TCP fallback"

[Paolo Abeni]

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

On Tue, 2020-01-21 at 18:20 +0100, Matthieu Baerts wrote:
> Hi Paolo, Mat,
> 
> On 21/01/2020 17:32, Paolo Abeni wrote:
> > kbuildbot reported a build breakage with IPV6 disabled.
> 
> Do you know why my CI didn't get the issue?
> 
> It runs:
> 
>    make defconfig
>    echo | scripts/config -d (...) ## to disable DRM, SOUND, USB, etc.
>    echo | scripts/config -e MPTCP -e MPTCP_IPV6
>    echo | scripts/config -d IPV6 -d MPTCP_IPV6
>    make
> 
> MPTCP is enabled and IPV6 is not.
> 
> Which combination did I miss?

weird. I can reproduce the build failure with

CONFIG_MPTCP=y
# CONFIG_IPV6 is not set
# CONFIG_MPTCP_IPV6 is not set


net/mptcp/protocol.o: In function `__mptcp_fallback_to_tcp':
/home/pabeni/linux-net/net/mptcp/protocol.c:63: undefined reference to
`inet6_stream_ops'

the inet6_stream_ops struct is unconditionally referenced by the mptcp
code.

/P

[MPTCP] Re: [PATCH] Squash-to: "mptcp: cope with later TCP fallback"

[Paolo Abeni]

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

On Tue, 2020-01-21 at 08:51 -0800, Mat Martineau wrote:
> On Tue, 21 Jan 2020, Mat Martineau wrote:
> 
> > On Tue, 21 Jan 2020, Paolo Abeni wrote:
> > 
> > > kbuildbot reported a build breakage with IPV6 disabled.
> > > 
> > > Signed-off-by: Paolo Abeni <pabeni(a)redha.com>
> 
> One more thing - typo in signoff      ^^^^^

Thanks, will fix.

> > > ---
> > > net/mptcp/protocol.c | 12 ++++++++++--
> > > 1 file changed, 10 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> > > index 6da609791c7a..4e289ff748ad 100644
> > > --- a/net/mptcp/protocol.c
> > > +++ b/net/mptcp/protocol.c
> > > @@ -26,6 +26,15 @@
> > > 
> > > static void __mptcp_close(struct sock *sk, long timeout);
> > > 
> > > +static const struct proto_ops * tcp_proto_ops(struct sock *sk)
> > > +{
> > > +#if IS_ENABLED(IPV6)
> > 
> > Does this need to be IS_ENABLED(CONFIG_IPV6) ?

yep, CONFIG_IPV6 is the thing.

v2 is coming.

Cheers,

Paolo

[MPTCP] Re: [PATCH] Squash-to: "mptcp: cope with later TCP fallback"

[Matthieu Baerts]

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

Hi Paolo, Mat,

On 21/01/2020 17:32, Paolo Abeni wrote:
> kbuildbot reported a build breakage with IPV6 disabled.

Do you know why my CI didn't get the issue?

It runs:

   make defconfig
   echo | scripts/config -d (...) ## to disable DRM, SOUND, USB, etc.
   echo | scripts/config -e MPTCP -e MPTCP_IPV6
   echo | scripts/config -d IPV6 -d MPTCP_IPV6
   make

MPTCP is enabled and IPV6 is not.

Which combination did I miss?

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

[MPTCP] Re: [PATCH] Squash-to: "mptcp: cope with later TCP fallback"

[Matthieu Baerts]

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

Hi Paolo,

On 21/01/2020 17:51, Mat Martineau wrote:
> On Tue, 21 Jan 2020, Mat Martineau wrote:
> 
>>
>> On Tue, 21 Jan 2020, Paolo Abeni wrote:
>>
>>> kbuildbot reported a build breakage with IPV6 disabled.
>>>
>>> Signed-off-by: Paolo Abeni <pabeni(a)redha.com>
> 
> One more thing - typo in signoff      ^^^^^

If "redhat.com" is too long, I guess you can also write "ibm.com"!

...

:-D

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

[MPTCP] Re: [PATCH] Squash-to: "mptcp: cope with later TCP fallback"

[Mat Martineau]

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

On Tue, 21 Jan 2020, Mat Martineau wrote:

>
> On Tue, 21 Jan 2020, Paolo Abeni wrote:
>
>> kbuildbot reported a build breakage with IPV6 disabled.
>> 
>> Signed-off-by: Paolo Abeni <pabeni(a)redha.com>

One more thing - typo in signoff      ^^^^^

>> ---
>> net/mptcp/protocol.c | 12 ++++++++++--
>> 1 file changed, 10 insertions(+), 2 deletions(-)
>> 
>> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
>> index 6da609791c7a..4e289ff748ad 100644
>> --- a/net/mptcp/protocol.c
>> +++ b/net/mptcp/protocol.c
>> @@ -26,6 +26,15 @@
>> 
>> static void __mptcp_close(struct sock *sk, long timeout);
>> 
>> +static const struct proto_ops * tcp_proto_ops(struct sock *sk)
>> +{
>> +#if IS_ENABLED(IPV6)
>
> Does this need to be IS_ENABLED(CONFIG_IPV6) ?
>
>> +	if (sk->sk_family == AF_INET6)
>> +		return &inet6_stream_ops;
>> +#endif
>> +	return &inet_stream_ops;
>> +}
>> +
>> /* MP_CAPABLE handshake failed, convert msk to plain tcp, replacing
>>  * socket->sk and stream ops and destroying msk
>>  * return the msk socket, as we can't access msk anymore after this 
>> function
>> @@ -60,8 +69,7 @@ static struct socket *__mptcp_fallback_to_tcp(struct 
>> mptcp_sock *msk,
>> 		subflow->conn = NULL;
>> 	}
>> 	release_sock(ssk);
>> -	sock->ops = sk->sk_family == AF_INET6 ? &inet6_stream_ops :
>> -						&inet_stream_ops;
>> +	sock->ops = tcp_proto_ops(ssk);
>>
>> 	/* destroy the left-over msk sock */
>> 	__mptcp_close(sk, 0);
>> -- 
>> 2.21.0
>
> --
> Mat Martineau
> Intel
>

--
Mat Martineau
Intel

[MPTCP] Re: [PATCH] Squash-to: "mptcp: cope with later TCP fallback"

[Mat Martineau]

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


On Tue, 21 Jan 2020, Paolo Abeni wrote:

> kbuildbot reported a build breakage with IPV6 disabled.
>
> Signed-off-by: Paolo Abeni <pabeni(a)redha.com>
> ---
> net/mptcp/protocol.c | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 6da609791c7a..4e289ff748ad 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -26,6 +26,15 @@
>
> static void __mptcp_close(struct sock *sk, long timeout);
>
> +static const struct proto_ops * tcp_proto_ops(struct sock *sk)
> +{
> +#if IS_ENABLED(IPV6)

Does this need to be IS_ENABLED(CONFIG_IPV6) ?

> +	if (sk->sk_family == AF_INET6)
> +		return &inet6_stream_ops;
> +#endif
> +	return &inet_stream_ops;
> +}
> +
> /* MP_CAPABLE handshake failed, convert msk to plain tcp, replacing
>  * socket->sk and stream ops and destroying msk
>  * return the msk socket, as we can't access msk anymore after this function
> @@ -60,8 +69,7 @@ static struct socket *__mptcp_fallback_to_tcp(struct mptcp_sock *msk,
> 		subflow->conn = NULL;
> 	}
> 	release_sock(ssk);
> -	sock->ops = sk->sk_family == AF_INET6 ? &inet6_stream_ops :
> -						&inet_stream_ops;
> +	sock->ops = tcp_proto_ops(ssk);
>
> 	/* destroy the left-over msk sock */
> 	__mptcp_close(sk, 0);
> -- 
> 2.21.0

--
Mat Martineau
Intel

[MPTCP] Re: [PATCH] Squash-to: "mptcp: cope with later TCP fallback"

[Matthieu Baerts]

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

Hi Paolo,

On 16/12/2019 13:22, Paolo Abeni wrote:
> Clean subflow->conn, under the ssk socket lock, and make
> __mptcp_tcp_fallback() less unreadable
> ---
> This should address Mat's concern on the fallback code path

Thank you for looking at that!

The code looks good to me. Should we maybe fix typos in comments? :)

> ---
>   net/mptcp/protocol.c | 14 ++++++++++++--
>   1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 2b20f4d3e179..9e288559e228 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -49,7 +49,17 @@ static struct socket *__mptcp_fallback_to_tcp(struct mptcp_sock *msk,
>   	sock->sk = NULL;
>   
>   	/* socket is now TCP */
> +	lock_sock(ssk);
>   	sock_graft(ssk, sock);
> +	if (subflow->conn) {
> +		/* Clearing the 'conn' field will make the ULP-ovverriden

detail: s/ovverriden/overridden/

> +		 * oops behaving alike plain TCP ones.

detail: s/oops/ops/

and: s/alike/like/

> +		 * Note: we can't release the ULP data on a live socket.
> +		 */
> +		sock_put(subflow->conn);
> +		subflow->conn = NULL;
> +	}
> +	release_sock(ssk);
>   	sock->ops = sk->sk_family == AF_INET6 ? &inet6_stream_ops :
>   						&inet_stream_ops;
>   
> @@ -76,8 +86,8 @@ static bool __mptcp_needs_tcp_fallback(const struct mptcp_sock *msk)
>   	return msk->first && !tcp_sk(msk->first)->is_mptcp;
>   }
>   
> -/* if the mp_capable handshake is failed, return a tcp socket
> - * return it.
> +/* if the mp_capable handshake is failed, fallback msk to plain tcp,

detail: s/is/has/

> + * release the socket lock and returns a reference to the now TCP socket

detail: s/returns/return/ (or add 'it' after the coma and 's' at the end 
of 'fallback' and 'release')

and maybe: s/now/current/

>    * Otherwise returns NULL

(same here: s/returns/return/ )

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