This change updates mptcp_pm_nl_create_listen_socket() to create
listening sockets bound to IPv6 addresses (where IPv6 is supported).

Signed-off-by: Kishen Maloor <kishen.maloor@intel.com>
---
 net/mptcp/pm_netlink.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index 29f6d01ace2d..7adc8c73ec48 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -986,6 +986,7 @@ static int mptcp_pm_nl_create_listen_socket(struct sock *sk,
 					    struct mptcp_pm_addr_entry *entry,
 					    struct socket **lsk)
 {
+	int addrlen = sizeof(struct sockaddr_in);
 	struct sockaddr_storage addr;
 	struct mptcp_sock *msk;
 	struct socket *ssock;
@@ -1010,8 +1011,11 @@ static int mptcp_pm_nl_create_listen_socket(struct sock *sk,
 	}
 
 	mptcp_info2sockaddr(&entry->addr, &addr, entry->addr.family);
-	err = kernel_bind(ssock, (struct sockaddr *)&addr,
-			  sizeof(struct sockaddr_in));
+#if IS_ENABLED(CONFIG_MPTCP_IPV6)
+	if (entry->addr.family == AF_INET6)
+		addrlen = sizeof(struct sockaddr_in6);
+#endif
+	err = kernel_bind(ssock, (struct sockaddr *)&addr, addrlen);
 	if (err) {
 		pr_warn("kernel_bind error, err=%d", err);
 		goto out;
-- 
2.31.1

Hi Kishen,

On 16/12/2021 23:23, Kishen Maloor wrote:
> This change updates mptcp_pm_nl_create_listen_socket() to create
> listening sockets bound to IPv6 addresses (where IPv6 is supported).

Should we consider this as a bug?

I understand we change the behaviour but I guess we should have done
that from the beginning to support IPv6 and v6-mapped addresses, no?

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

On 12/17/21 8:29 AM, Matthieu Baerts wrote:
> Hi Kishen,
> 
> On 16/12/2021 23:23, Kishen Maloor wrote:
>> This change updates mptcp_pm_nl_create_listen_socket() to create
>> listening sockets bound to IPv6 addresses (where IPv6 is supported).
> 
> Should we consider this as a bug?

We could I suppose, at least for lack of completeness. But you're right that we've
now updated the behavior in this series in attempting to create listening sockets (lsks)
corresponding to every announcement, which necessitates this handling of
IPv6 addresses. 

But prior to this series: 
-lsk creation (through a subflow's port) did not happen in the kernel under the assumption
that MPTCP server applications would've established a listener,
-lsks were created only for port-based endpoints which (I believe) would not work with
IPv6 (lack of option space), and,
-the stack did not allow incoming MP_JOINs at machines running MPTCP client 
applications (with this series, subflows can be established from either end so there
needs to be an lsk).

So, may be we could also choose to not call this a bug :)

> 
> I understand we change the behaviour but I guess we should have done
> that from the beginning to support IPv6 and v6-mapped addresses, no?
> 
> Cheers,
> Matt
> 

On Mon, 2021-12-20 at 23:32 -0800, Kishen Maloor wrote:
> On 12/17/21 8:29 AM, Matthieu Baerts wrote:
> > Hi Kishen,
> > 
> > On 16/12/2021 23:23, Kishen Maloor wrote:
> > > This change updates mptcp_pm_nl_create_listen_socket() to create
> > > listening sockets bound to IPv6 addresses (where IPv6 is supported).
> > 
> > Should we consider this as a bug?
> 
> We could I suppose, at least for lack of completeness. But you're right that we've
> now updated the behavior in this series in attempting to create listening sockets (lsks)
> corresponding to every announcement, which necessitates this handling of
> IPv6 addresses. 
> 
> But prior to this series: 
> -lsk creation (through a subflow's port) did not happen in the kernel under the assumption
> that MPTCP server applications would've established a listener,
> -lsks were created only for port-based endpoints which (I believe) would not work with
> IPv6 (lack of option space), and,
> -the stack did not allow incoming MP_JOINs at machines running MPTCP client 
> applications (with this series, subflows can be established from either end so there
> needs to be an lsk).

Could you please elaborate more this last point? If the stack does not
allow the latter, it's definitely a bug. The port-based endpoint
implementation was aimed [also] at that goal.

Thanks!

Paolo

On 12/21/21 1:45 AM, Paolo Abeni wrote:
> On Mon, 2021-12-20 at 23:32 -0800, Kishen Maloor wrote:
>> On 12/17/21 8:29 AM, Matthieu Baerts wrote:
>>> Hi Kishen,
>>>
>>> On 16/12/2021 23:23, Kishen Maloor wrote:
>>>> This change updates mptcp_pm_nl_create_listen_socket() to create
>>>> listening sockets bound to IPv6 addresses (where IPv6 is supported).
>>>
>>> Should we consider this as a bug?
>>
>> We could I suppose, at least for lack of completeness. But you're right that we've
>> now updated the behavior in this series in attempting to create listening sockets (lsks)
>> corresponding to every announcement, which necessitates this handling of
>> IPv6 addresses. 
>>
>> But prior to this series: 
>> -lsk creation (through a subflow's port) did not happen in the kernel under the assumption
>> that MPTCP server applications would've established a listener,
>> -lsks were created only for port-based endpoints which (I believe) would not work with
>> IPv6 (lack of option space), and,
>> -the stack did not allow incoming MP_JOINs at machines running MPTCP client 
>> applications (with this series, subflows can be established from either end so there
>> needs to be an lsk).
> 
> Could you please elaborate more this last point? If the stack does not
> allow the latter, it's definitely a bug. The port-based endpoint
> implementation was aimed [also] at that goal.
> 

Prior to changes here: 

https://lore.kernel.org/mptcp/4cb68f04-5732-e1fe-4b3b-82a418d87f00@intel.com/T/#m637d52ce80f1ff21b20e9de9b877c016fdb4729d

I believe that MPJs received at endpoints running MPTCP client applications would fail to establish a subflow due to logic in mptcp_finish_join().

> Thanks!
> 
> Paolo
> 

Hi Kishen,

Thank you for your replies!

On 21/12/2021 08:32, Kishen Maloor wrote:
> On 12/17/21 8:29 AM, Matthieu Baerts wrote:
>> Hi Kishen,
>>
>> On 16/12/2021 23:23, Kishen Maloor wrote:
>>> This change updates mptcp_pm_nl_create_listen_socket() to create
>>> listening sockets bound to IPv6 addresses (where IPv6 is supported).
>>
>> Should we consider this as a bug?
> 
> We could I suppose, at least for lack of completeness. But you're right that we've
> now updated the behavior in this series in attempting to create listening sockets (lsks)
> corresponding to every announcement, which necessitates this handling of
> IPv6 addresses. 
> 
> But prior to this series: 
> -lsk creation (through a subflow's port) did not happen in the kernel under the assumption
> that MPTCP server applications would've established a listener,
> -lsks were created only for port-based endpoints

Yes, that's correct.

> which (I believe) would not work with
> IPv6 (lack of option space), and,

Yes, there is enough space. We even have packetdrill tests, no?
ADD_ADDR are sent in a dedicated ACK packet, without DSS. I think we
started to discuss about having a dedicated ACK packet to cover this
case and ADD_ADDRv6 + echo I think.

> -the stack did not allow incoming MP_JOINs at machines running MPTCP client 
> applications (with this series, subflows can be established from either end so there
> needs to be an lsk).

If I'm not mistaken, that was by design: to simplify things.

It is very rare and very specific when a server initiates connections. I
think in most cases, the client would like to be in charge of initiating
paths and would not like the server to do so.
But as it seems not to be too complex, it is good to have a way to do
that. But I think we would need an option to (dis)allow that.

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

On 12/29/21 5:52 AM, Matthieu Baerts wrote:
> Hi Kishen,
> 
> Thank you for your replies!
> 
> On 21/12/2021 08:32, Kishen Maloor wrote:
>> On 12/17/21 8:29 AM, Matthieu Baerts wrote:
>>> Hi Kishen,
>>>
>>> On 16/12/2021 23:23, Kishen Maloor wrote:
>>>> This change updates mptcp_pm_nl_create_listen_socket() to create
>>>> listening sockets bound to IPv6 addresses (where IPv6 is supported).
>>>
>>> Should we consider this as a bug?
>>
>> We could I suppose, at least for lack of completeness. But you're right that we've
>> now updated the behavior in this series in attempting to create listening sockets (lsks)
>> corresponding to every announcement, which necessitates this handling of
>> IPv6 addresses. 
>>
>> But prior to this series: 
>> -lsk creation (through a subflow's port) did not happen in the kernel under the assumption
>> that MPTCP server applications would've established a listener,
>> -lsks were created only for port-based endpoints
> 
> Yes, that's correct.
> 
>> which (I believe) would not work with
>> IPv6 (lack of option space), and,
> 
> Yes, there is enough space. We even have packetdrill tests, no?
> ADD_ADDR are sent in a dedicated ACK packet, without DSS. I think we
> started to discuss about having a dedicated ACK packet to cover this
> case and ADD_ADDRv6 + echo I think.

Based on what I briefly observed, it seemed like there wasn't sufficient option space to 
advertise a port in an ADD_ADDRv6 message (even in a dedicated ACK, I believe). So, I 
concluded that the port had to always be reused. Anyhow, if there is indeed sufficient
room for a port, then yes, we could consider this commit as a bug fix.

> 
>> -the stack did not allow incoming MP_JOINs at machines running MPTCP client 
>> applications (with this series, subflows can be established from either end so there
>> needs to be an lsk).
> 
> If I'm not mistaken, that was by design: to simplify things.
> 
> It is very rare and very specific when a server initiates connections. I
> think in most cases, the client would like to be in charge of initiating
> paths and would not like the server to do so.

I've been considering path management as a separate function (architecturally) from the role(s) of
the MPTCP application(s). So, in my mind it is not the client/server applications initiating new subflows, but is rather
the PM (in concert with the MPTCP stack).

So may be the requirements for and scope of path management policy needs to be discussed then (?) (if it hasn't 
already happened) which could be realized by userspace PM daemons.

> But as it seems not to be too complex, it is good to have a way to do
> that. But I think we would need an option to (dis)allow that.
> 
> Cheers,
> Matt