[MPTCP] Re: [MPTCP][PATCH v5 mptcp-next 02/10] mptcp: add the pernet listening socket operations

* [MPTCP] Re: [MPTCP][PATCH v5 mptcp-next 02/10] mptcp: add the pernet listening socket operations
@ 2020-11-23  9:04 Paolo Abeni
  0 siblings, 0 replies; 3+ messages in thread
From: Paolo Abeni @ 2020-11-23  9:04 UTC (permalink / raw)
  To: mptcp

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

On Mon, 2020-11-23 at 15:57 +0800, Geliang Tang wrote:
> Hi Paolo,
> 
> Thanks for your suggestions.
> 
> Paolo Abeni <pabeni(a)redhat.com> 于2020年11月20日周五 下午6:24写道：
> > On Thu, 2020-11-19 at 18:19 +0800, Geliang Tang wrote:
> > > This patch added three new pernet listening socket operations:
> > > 
> > > mptcp_pm_alloc_listen_socket, mptcp_pm_free_listen_socket and
> > > mptcp_pm_get_listen_socket to alloc, free and get the pernet
> > > listening socket.
> > > 
> > > Signed-off-by: Geliang Tang <geliangtang(a)gmail.com>
> > > ---
> > >  net/mptcp/pm_netlink.c | 54 ++++++++++++++++++++++++++++++++++++++++++
> > >  net/mptcp/protocol.c   |  5 ++++
> > >  net/mptcp/protocol.h   |  5 ++++
> > >  net/mptcp/subflow.c    |  2 +-
> > >  4 files changed, 65 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> > > index 2560c502356b..19d557df4684 100644
> > > --- a/net/mptcp/pm_netlink.c
> > > +++ b/net/mptcp/pm_netlink.c
> > > @@ -45,6 +45,8 @@ struct pm_nl_pernet {
> > >       unsigned int            add_addr_accept_max;
> > >       unsigned int            local_addr_max;
> > >       unsigned int            subflows_max;
> > > +     struct socket           *listen_socket;
> > > +     bool                    sock_created;
> > >       unsigned int            next_id;
> > >  };
> > > 
> > > @@ -306,6 +308,56 @@ void mptcp_pm_free_anno_list(struct mptcp_sock *msk)
> > >       }
> > >  }
> > > 
> > > +int mptcp_pm_alloc_listen_socket(struct mptcp_sock *msk)
> > > +{
> > > +     struct sock *sk = (struct sock *)msk;
> > > +     struct pm_nl_pernet *pernet;
> > > +     bool need_create = false;
> > > +
> > > +     pernet = net_generic(sock_net(sk), pm_nl_pernet_id);
> > > +
> > > +     spin_lock_bh(&pernet->lock);
> > > +     if (!pernet->sock_created) {
> > > +             pernet->sock_created = true;
> > > +             need_create = true;
> > > +     }
> > > +     spin_unlock_bh(&pernet->lock);
> > > +
> > > +     if (!need_create)
> > > +             return 0;
> > > +
> > > +     return mptcp_create_listen_socket(sk, &pernet->listen_socket);
> > > +}
> > > +
> > > +void mptcp_pm_free_listen_socket(struct mptcp_sock *msk)
> > > +{
> > > +     struct sock *sk = (struct sock *)msk;
> > > +     struct pm_nl_pernet *pernet;
> > > +
> > > +     pernet = net_generic(sock_net(sk), pm_nl_pernet_id);
> > > +
> > > +     if (!pernet || !pernet->listen_socket || !pernet->listen_socket->sk)
> > > +             return;
> > > +
> > > +     subflow_drop_ctx(pernet->listen_socket->sk);
> > > +     sock_release(pernet->listen_socket);
> > > +     pernet->listen_socket = NULL;
> > > +     pernet->sock_created = false;
> > > +}
> > > +
> > > +struct socket *mptcp_pm_get_listen_socket(struct mptcp_sock *msk)
> > > +{
> > > +     struct pm_nl_pernet *pernet;
> > > +     struct sock *sk = (struct sock *)msk;
> > > +
> > > +     pernet = net_generic(sock_net(sk), pm_nl_pernet_id);
> > > +
> > > +     if (!pernet)
> > > +             return NULL;
> > > +
> > > +     return pernet->listen_socket;
> > > +}
> > > +
> > >  static void mptcp_pm_create_subflow_or_signal_addr(struct mptcp_sock *msk)
> > >  {
> > >       struct mptcp_addr_info remote = { 0 };
> > > @@ -885,6 +937,8 @@ static void __reset_counters(struct pm_nl_pernet *pernet)
> > >       pernet->add_addr_accept_max = 0;
> > >       pernet->local_addr_max = 0;
> > >       pernet->addrs = 0;
> > > +     pernet->listen_socket = NULL;
> > > +     pernet->sock_created = false;
> > >  }
> > > 
> > >  static int mptcp_nl_cmd_flush_addrs(struct sk_buff *skb, struct genl_info *info)
> > > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> > > index 7a0364860feb..1a9e895e953e 100644
> > > --- a/net/mptcp/protocol.c
> > > +++ b/net/mptcp/protocol.c
> > > @@ -2006,6 +2006,10 @@ static int mptcp_init_sock(struct sock *sk)
> > >       if (ret)
> > >               return ret;
> > > 
> > > +     ret = mptcp_pm_alloc_listen_socket(mptcp_sk(sk));
> > > +     if (ret)
> > > +             return ret;
> > > +
> > >       sk_sockets_allocated_inc(sk);
> > >       sk->sk_rcvbuf = sock_net(sk)->ipv4.sysctl_tcp_rmem[1];
> > >       sk->sk_sndbuf = sock_net(sk)->ipv4.sysctl_tcp_wmem[1];
> 
> This v5 still has bugs in it, it can't work when we use announced multiple
> addresses with port, the following new test cases will fail:
[...]

Yep, with a single socket we can't accomodate multiple ports, we would
need one socket per port (specifcally one socket per port-based
endpoint), see below.

> I think the listening socket creation and bind should be done at
> > endpoint creation time, that is, while handling the
> > MPTCP_PM_CMD_ADD_ADDR netlink command.
> 
> Do you mean we can do it in mptcp_nl_cmd_add_addr? I had tested moving the
> creation and bind to mptcp_nl_cmd_add_addr, but it not work. This moment is
> much earlier for the socket creation. Since we want to create the subflow
> within a msk, but at this moment, the msk is not created yet.

The idea is that the listening socket is created _before_ we actually
create the msk or subflow.

Thanks to the destination port number and token checks, already in
place, that should be fine from security PoV: we will accept connection
only after a msk is created and fully established.

> I think maybe we can create and bind the listening socket in
> mptcp_pm_create_subflow_or_signal_addr. Do you think it's OK?

Doing the creation earlier, inside the scope of the user-space process
doing the netlink configuration:
- reduces the overhead for subflow creation. 
- allows closing the listening socket ASA the port endpoint is deleted.
- allows reporting to the user-space any port usage conflict and/or
other socket creation errors.

It looks the better option to me.

> > The socket pointer itself could be stored inside the 'struct
> > mptcp_pm_addr_entr'.
> 
> We have defined another struct mptcp_pm_add_entry, it will be added
> in
> anno_list. I think we can stored the socket pointer inside it, instead of
> mptcp_pm_addr_entry. But in that case, the listening socket is not per
> netns, it is per announced address. There are several listening sockets
> per netns. Is this acceptable?

I mentioned the 'mptcp_pm_addr_entry' struct because it's per netns.
Why can't we use it? Hopefully should be just a matter of moving the
socket creation and initialization inside mptcp_nl_cmd_add_addr().

Thanks,

Paolo

^ permalink raw reply	[flat|nested] 3+ messages in thread