11:22 a.m.
On Mon, 2019-10-28 at 16:37 -0700, Peter Krystad wrote:
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.
Oh, nice! I really missed the real meaning of the trailing '4' in
__sys_accept4(), thanks for pointing that out.
Switching to inet6_stream_ops should be ok anyway, right?
> [...]
>
> > @@ -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?
In my previous email I suggested getting rid of the panic in IPv4 -
just to harmoize the ipv4 and ipv6 behavior - but it's not a big deal:
for simplicity we can stick to the current implementation.
> 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.
Excellent! looking forward to it!
Paolo