[MPTCP] Re: [RFC 2/2] mptcp: Add IPv6 support

[Peter Krystad <peter.krystad at linux.intel.com>]

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


Hi Paolo -

Thanks for the review, sorry for the slow response.

On Thu, 2019-10-17 at 12:36 +0200, Paolo Abeni wrote:
> Hi,
> 
> On Wed, 2019-10-16 at 17:02 -0700, Peter Krystad wrote:
> > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> > index 4fdcbb0f4285..bc38527209c9 100644
> > @@ -1182,6 +1214,53 @@ static int mptcp_getname(struct socket *sock, struct sockaddr *uaddr,
> >  	return ret;
> >  }
> >  
> > +#if IS_ENABLED(CONFIG_IPV6)
> > +static int mptcp_v6_getname(struct socket *sock, struct sockaddr *uaddr,
> > +			    int peer)
> > +{
> > +	struct mptcp_sock *msk = mptcp_sk(sock->sk);
> > +	struct socket *ssock;
> > +	struct sock *ssk;
> > +	int ret;
> > +
> > +	if (sock->sk->sk_prot == &tcpv6_prot) {
> > +		/* we are being invoked from __sys_accept4, after
> > +		 * mptcp_accept() has just accepted a non-mp-capable
> > +		 * flow: sk is a tcp_sk, not an mptcp one.
> > +		 *
> > +		 * Hand the socket over to tcp so all further socket ops
> > +		 * bypass mptcp.
> > +		 */
> > +		sock->ops = &inet_stream_ops;
> 
> Should we have &inet6_stream_ops  here ^^^^^^ ???
> Possibly the comments should be adjusted as well.

Yes, nice catch. Comment, at least the __sys_accept4 part, is OK, I think the
4 refers to a version of accept, not the protocol.

> [...]
> 
> > @@ -1311,3 +1409,33 @@ void mptcp_proto_init(void)
> >  
> >  	inet_register_protosw(&mptcp_protosw);
> >  }
> > +
> > +#if IS_ENABLED(CONFIG_IPV6)
> > +static struct proto_ops mptcp_v6_stream_ops;
> > +
> > +static struct inet_protosw mptcp_v6_protosw = {
> > +	.type		= SOCK_STREAM,
> > +	.protocol	= IPPROTO_MPTCP,
> > +	.prot		= &mptcp_prot,
> > +	.ops		= &mptcp_v6_stream_ops,
> > +	.flags		= INET_PROTOSW_ICSK,
> > +};
> > +
> > +int mptcp_proto_v6_init(void)
> > +{
> > +	int err;
> > +
> > +	mptcp_v6_stream_ops = inet6_stream_ops;
> > +	mptcp_v6_stream_ops.bind = mptcp_v6_bind;
> > +	mptcp_v6_stream_ops.connect = mptcp_v6_stream_connect;
> > +	mptcp_v6_stream_ops.poll = mptcp_poll;
> > +	mptcp_v6_stream_ops.accept = mptcp_stream_accept;
> > +	mptcp_v6_stream_ops.getname = mptcp_v6_getname;
> > +	mptcp_v6_stream_ops.listen = mptcp_v6_listen;
> > +	mptcp_v6_stream_ops.shutdown = mptcp_shutdown;
> > +
> > +	err = inet6_register_protosw(&mptcp_v6_protosw);
> 
> For IPv4 we currently do panic, if we can't register the protosw
> struct, possibly we could explicitly handle the failure even there?

Sorry, are suggesting getting rid of the panic()'s in IPv4? Or adding them
here?
 
> Overall this looks nice and clean! What about moving the ipv6
> protocol.c support into a separate file, to reduce the preprocessor
> conditionals?

Thanks. Next step to is split the IPv6 into a seperate file, then I'll submit
as a PATCH.

Peter.

> [...]
> 
> > @@ -226,6 +290,26 @@ static int subflow_conn_request(struct sock *sk, struct sk_buff *skb)
> >  	return 0;
> >  }
> >  
> > +static int subflow_v6_conn_request(struct sock *sk, struct sk_buff *skb)
> > +{
> > +	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
> > +
> > +	pr_debug("subflow=%p", subflow);
> 
> Just to avoid duplicate debug messages for IPv4 packets, what about
> moving this one below the next if?
> 
> [...]
> 
> > @@ -309,7 +393,70 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
> >  	return NULL;
> >  }
> >  
> > -static struct inet_connection_sock_af_ops subflow_specific;
> > +#if IS_ENABLED(CONFIG_IPV6)
> > +static struct sock *subflow_v6_syn_recv_sock(const struct sock *sk,
> > +					     struct sk_buff *skb,
> > +					     struct request_sock *req,
> > +					     struct dst_entry *dst,
> > +					     struct request_sock *req_unhash,
> > +					     bool *own_req)
> > +{
> > +	struct mptcp_subflow_context *listener = mptcp_subflow_ctx(sk);
> > +	struct mptcp_subflow_request_sock *subflow_req;
> > +	struct tcp_options_received opt_rx;
> > +	struct sock *child;
> > +
> > +	pr_debug("listener=%p, req=%p, conn=%p", listener, req, listener->conn);
> > +
> > +	/* if the sk is MP_CAPABLE, we already received the client key */
> > +	subflow_req = mptcp_subflow_rsk(req);
> > +	if (!subflow_req->mp_capable && subflow_req->mp_join) {
> > +		opt_rx.mptcp.mp_join = 0;
> > +		mptcp_get_options(skb, &opt_rx);
> > +		if (!opt_rx.mptcp.mp_join ||
> > +		    !subflow_hmac_valid(req, &opt_rx))
> > +			return NULL;
> > +	}
> > +
> > +	child = tcp_v6_syn_recv_sock(sk, skb, req, dst, req_unhash, own_req);
> > +
> > +	if (child && *own_req) {
> > +		struct mptcp_subflow_context *ctx = mptcp_subflow_ctx(child);
> > +
> > +		if (!ctx)
> > +			goto close_child;
> > +
> > +		if (ctx->mp_capable) {
> > +			if (mptcp_token_new_accept(ctx->token))
> > +				goto close_child;
> > +		} else if (ctx->mp_join) {
> > +			struct mptcp_sock *owner;
> > +
> > +			owner = mptcp_token_get_sock(ctx->token);
> > +			if (!owner)
> > +				goto close_child;
> > +
> > +			ctx->conn = (struct sock *)owner;
> > +			mptcp_finish_join(child);
> > +		}
> > +	}
> > +
> > +	return child;
> > +
> > +close_child:
> > +	pr_debug("closing child socket");
> > +	inet_sk_set_state(child, TCP_CLOSE);
> > +	sock_set_flag(child, SOCK_DEAD);
> > +	inet_csk_destroy_sock(child);
> > +	return NULL;
> > +}
> 
> As this looks quite alike the ipv4 counter part, except for the
> tcp_v4_syn_recv_sock() call, replaced here by tcp_v6_syn_recv_sock(),
> what about using a function pointer and the same overall code? 
> 
> If the additional indirect call overhead is a concern, we can leverage
> the indirect call infrastructure ;) [shameless self-promotion;)]
> 
> How about moving even the subflow ipv6 code to a separate, ipv6
> specific file ? (possibly the same used for ipv6 protocol.c code)
> 
> Overall this looks much more clean and self-contained than expected!
> 
> Thank you,
> 
> Paolo
>