[MPTCP] Re: [RFC 1/2] mptcp: Export low-level routines for IPv6

[MPTCP] Re: [RFC 1/2] mptcp: Export low-level routines for IPv6

[Paolo Abeni]

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

Hi,

On Wed, 2019-10-16 at 17:02 -0700, Peter Krystad wrote:
> All routines exported already have their IPv4 counterparts exported.
> tcp_request_sock_ipv6_ops will be referenced during TCP subflow
> creation.
> 
> Signed-off-by: Peter Krystad <peter.krystad(a)linux.intel.com>

I guess this could be squashed into "tcp: Export TCP functions and ops
struct", right?

[...]

> diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
> index e3d9f4559c99..1f3a87f4867e 100644
> --- a/net/ipv6/tcp_ipv6.c
> +++ b/net/ipv6/tcp_ipv6.c
> @@ -75,7 +75,7 @@ static void	tcp_v6_reqsk_send_ack(const struct sock *sk, struct sk_buff *skb,
>  static int	tcp_v6_do_rcv(struct sock *sk, struct sk_buff *skb);
>  
>  static const struct inet_connection_sock_af_ops ipv6_mapped;
> -static const struct inet_connection_sock_af_ops ipv6_specific;
> +const struct inet_connection_sock_af_ops ipv6_specific;
>  #ifdef CONFIG_TCP_MD5SIG
>  static const struct tcp_sock_af_ops tcp_sock_ipv6_specific;
>  static const struct tcp_sock_af_ops tcp_sock_ipv6_mapped_specific;
> @@ -99,7 +99,7 @@ static struct ipv6_pinfo *tcp_inet6_sk(const struct sock *sk)
>  	return (struct ipv6_pinfo *)(((u8 *)sk) + offset);
>  }
>  
> -static void inet6_sk_rx_dst_set(struct sock *sk, const struct sk_buff *skb)
> +void inet6_sk_rx_dst_set(struct sock *sk, const struct sk_buff *skb)
>  {
>  	struct dst_entry *dst = skb_dst(skb);
>  
> @@ -111,6 +111,7 @@ static void inet6_sk_rx_dst_set(struct sock *sk, const struct sk_buff *skb)
>  		tcp_inet6_sk(sk)->rx_dst_cookie = rt6_get_cookie(rt);
>  	}
>  }
> +EXPORT_SYMBOL(inet6_sk_rx_dst_set);

I think this is not needed, as we don't build mptcp as a module.
Dropping the 'static' scope should suffice. This should apply also to
tcp_request_sock_ipv6_ops, tcp_v6_syn_recv_sock and ipv6_specific
below.

[...]

> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index 791f2c19cfb8..6f09fdfdd523 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -268,6 +268,9 @@ int mptcp_subflow_connect(struct sock *sk, struct sockaddr *local,
>  int mptcp_subflow_create_socket(struct sock *sk, struct socket **new_sock);
>  
>  extern const struct inet_connection_sock_af_ops ipv4_specific;
> +#if IS_ENABLED(CONFIG_IPV6)
> +extern const struct inet_connection_sock_af_ops ipv6_specific;
> +#endif
>  
>  void mptcp_proto_init(void);
> 
uhmm.. _if_ we want to squash these patches, this chunk should likely
go to a different patch than all the above - possibly "mptcp: Handle
MP_CAPABLE options for outgoing connections". Perhaps squashing is not
a good idea in this case.

Cheers,

Paolo

[MPTCP] Re: [RFC 1/2] mptcp: Export low-level routines for IPv6

[Florian Westphal]

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

Paolo Abeni <pabeni(a)redhat.com> wrote:
> > -static void inet6_sk_rx_dst_set(struct sock *sk, const struct sk_buff *skb)
> > +void inet6_sk_rx_dst_set(struct sock *sk, const struct sk_buff *skb)
> >  {
> >  	struct dst_entry *dst = skb_dst(skb);
> >  
> > @@ -111,6 +111,7 @@ static void inet6_sk_rx_dst_set(struct sock *sk, const struct sk_buff *skb)
> >  		tcp_inet6_sk(sk)->rx_dst_cookie = rt6_get_cookie(rt);
> >  	}
> >  }
> > +EXPORT_SYMBOL(inet6_sk_rx_dst_set);
> 
> I think this is not needed, as we don't build mptcp as a module.
> Dropping the 'static' scope should suffice. This should apply also to
> tcp_request_sock_ipv6_ops, tcp_v6_syn_recv_sock and ipv6_specific
> below.

Yes, but ipv6 can be built as a module, i.e. mptcp cannot call any ipv6
related symbol directly.

This will need rather ugly indirections, similar to
include/linux/netfilter_ipv6.h / "struct nf_ipv6_ops".

[MPTCP] Re: [RFC 1/2] mptcp: Export low-level routines for IPv6

[Paolo Abeni]

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

On Thu, 2019-10-17 at 11:40 +0200, Florian Westphal wrote:
> Paolo Abeni <pabeni(a)redhat.com> wrote:
> > > -static void inet6_sk_rx_dst_set(struct sock *sk, const struct sk_buff *skb)
> > > +void inet6_sk_rx_dst_set(struct sock *sk, const struct sk_buff *skb)
> > >  {
> > >  	struct dst_entry *dst = skb_dst(skb);
> > >  
> > > @@ -111,6 +111,7 @@ static void inet6_sk_rx_dst_set(struct sock *sk, const struct sk_buff *skb)
> > >  		tcp_inet6_sk(sk)->rx_dst_cookie = rt6_get_cookie(rt);
> > >  	}
> > >  }
> > > +EXPORT_SYMBOL(inet6_sk_rx_dst_set);
> > 
> > I think this is not needed, as we don't build mptcp as a module.
> > Dropping the 'static' scope should suffice. This should apply also to
> > tcp_request_sock_ipv6_ops, tcp_v6_syn_recv_sock and ipv6_specific
> > below.
> 
> Yes, but ipv6 can be built as a module, i.e. mptcp cannot call any ipv6
> related symbol directly.

whoops, I always forgot about that :(

On the plus side, we don't actually need to touch the scope of
tcp_v6_syn_recv_sock and inet6_sk_rx_dst_set, as we can access them via
'ipv6_specific', which we need to access no-matter-what.

On the flip side, hooking mptcpv6_init() in the next patch looks even
more messy :(

What if we make MPTCP IPv6 support depending on IPV6 = y ?

Cheers,

Paolo

[MPTCP] Re: [RFC 1/2] mptcp: Export low-level routines for IPv6

[Peter Krystad]

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


Hi Paolo -

On Thu, 2019-10-17 at 11:12 +0200, Paolo Abeni wrote:
> Hi,
> 
> On Wed, 2019-10-16 at 17:02 -0700, Peter Krystad wrote:
> > All routines exported already have their IPv4 counterparts exported.
> > tcp_request_sock_ipv6_ops will be referenced during TCP subflow
> > creation.
> > 
> > Signed-off-by: Peter Krystad <peter.krystad(a)linux.intel.com>
> 
> I guess this could be squashed into "tcp: Export TCP functions and ops
> struct", right?

Maybe, see my last comment.

> [...]
> 
> > diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
> > index e3d9f4559c99..1f3a87f4867e 100644
> > --- a/net/ipv6/tcp_ipv6.c
> > +++ b/net/ipv6/tcp_ipv6.c
> > @@ -75,7 +75,7 @@ static void	tcp_v6_reqsk_send_ack(const struct sock *sk, struct sk_buff *skb,
> >  static int	tcp_v6_do_rcv(struct sock *sk, struct sk_buff *skb);
> >  
> >  static const struct inet_connection_sock_af_ops ipv6_mapped;
> > -static const struct inet_connection_sock_af_ops ipv6_specific;
> > +const struct inet_connection_sock_af_ops ipv6_specific;
> >  #ifdef CONFIG_TCP_MD5SIG
> >  static const struct tcp_sock_af_ops tcp_sock_ipv6_specific;
> >  static const struct tcp_sock_af_ops tcp_sock_ipv6_mapped_specific;
> > @@ -99,7 +99,7 @@ static struct ipv6_pinfo *tcp_inet6_sk(const struct sock *sk)
> >  	return (struct ipv6_pinfo *)(((u8 *)sk) + offset);
> >  }
> >  
> > -static void inet6_sk_rx_dst_set(struct sock *sk, const struct sk_buff *skb)
> > +void inet6_sk_rx_dst_set(struct sock *sk, const struct sk_buff *skb)
> >  {
> >  	struct dst_entry *dst = skb_dst(skb);
> >  
> > @@ -111,6 +111,7 @@ static void inet6_sk_rx_dst_set(struct sock *sk, const struct sk_buff *skb)
> >  		tcp_inet6_sk(sk)->rx_dst_cookie = rt6_get_cookie(rt);
> >  	}
> >  }
> > +EXPORT_SYMBOL(inet6_sk_rx_dst_set);
> 
> I think this is not needed, as we don't build mptcp as a module.
> Dropping the 'static' scope should suffice. This should apply also to
> tcp_request_sock_ipv6_ops, tcp_v6_syn_recv_sock and ipv6_specific
> below.

Correct, and I'll do the same for the others.

> [...]
> 
> > diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> > index 791f2c19cfb8..6f09fdfdd523 100644
> > --- a/net/mptcp/protocol.h
> > +++ b/net/mptcp/protocol.h
> > @@ -268,6 +268,9 @@ int mptcp_subflow_connect(struct sock *sk, struct sockaddr *local,
> >  int mptcp_subflow_create_socket(struct sock *sk, struct socket **new_sock);
> >  
> >  extern const struct inet_connection_sock_af_ops ipv4_specific;
> > +#if IS_ENABLED(CONFIG_IPV6)
> > +extern const struct inet_connection_sock_af_ops ipv6_specific;
> > +#endif
> >  
> >  void mptcp_proto_init(void);
> > 
> uhmm.. _if_ we want to squash these patches, this chunk should likely
> go to a different patch than all the above - possibly "mptcp: Handle
> MP_CAPABLE options for outgoing connections". Perhaps squashing is not
> a good idea in this case.

Squashing depends on if we want the patch sequence to look like we did v6 at
the same time as v4, i.e. as each feature is introduced it has both v4 and v6.
I'm in favor of not squashing this patch.

Peter.

> Cheers,
> 
> Paolo
> 

[MPTCP] Re: [RFC 1/2] mptcp: Export low-level routines for IPv6

[Paolo Abeni]

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

On Mon, 2019-10-28 at 16:46 -0700, Peter Krystad wrote:
> On Thu, 2019-10-17 at 11:12 +0200, Paolo Abeni wrote:
> > On Wed, 2019-10-16 at 17:02 -0700, Peter Krystad wrote:
> > > diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> > > index 791f2c19cfb8..6f09fdfdd523 100644
> > > --- a/net/mptcp/protocol.h
> > > +++ b/net/mptcp/protocol.h
> > > @@ -268,6 +268,9 @@ int mptcp_subflow_connect(struct sock *sk, struct sockaddr *local,
> > >  int mptcp_subflow_create_socket(struct sock *sk, struct socket **new_sock);
> > >  
> > >  extern const struct inet_connection_sock_af_ops ipv4_specific;
> > > +#if IS_ENABLED(CONFIG_IPV6)
> > > +extern const struct inet_connection_sock_af_ops ipv6_specific;
> > > +#endif
> > >  
> > >  void mptcp_proto_init(void);
> > > 
> > uhmm.. _if_ we want to squash these patches, this chunk should likely
> > go to a different patch than all the above - possibly "mptcp: Handle
> > MP_CAPABLE options for outgoing connections". Perhaps squashing is not
> > a good idea in this case.
> 
> Squashing depends on if we want the patch sequence to look like we did v6 at
> the same time as v4, i.e. as each feature is introduced it has both v4 and v6.
> I'm in favor of not squashing this patch.

Ok, not a big deal. We just need to double check the total patch count
when we will be near to the submission shape - to make the series as
palatable as possible.

Cheers,

Paolo