| Message ID | 4efc3c8a58dc8e50c533b262860a63010b8babbd.1573488743.git.pabeni@redhat.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [1/4] mptcp: move mp_capable initialization at subflow_init_req() start | expand |
On Mon, 2019-11-11 at 17:53 +0100, Paolo Abeni wrote: > Likley we want to update the "mptcp: Create SUBFLOW socket for incoming connections" > commit message with something alike: > > Explicitly disable MPTCP if MD5SIG is enable, to prevent TCP option space > exaustion. > > Squash-to: mptcp: Create SUBFLOW socket for incoming connections > Signed-off-by: Paolo Abeni <pabeni@redhat.com> > --- > net/mptcp/protocol.c | 8 ++++++++ > net/mptcp/subflow.c | 8 ++++++++ > 2 files changed, 16 insertions(+) > > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c > index 81b7c21f4d3c..bb297e9dd6c5 100644 > --- a/net/mptcp/protocol.c > +++ b/net/mptcp/protocol.c > @@ -342,6 +342,14 @@ static int mptcp_stream_connect(struct socket *sock, struct sockaddr *uaddr, > if (IS_ERR(ssock)) > return PTR_ERR(ssock); > > +#ifdef CONFIG_TCP_MD5SIG > + /* no MPTCP is MD5SIG is enabled on this socket or we may run out of > + * TCP option space. > + */ typo in comment [two places]: "no MPTCP if MD5SIG" > + if (rcu_access_pointer(tcp_sk(ssock->sk)->md5sig_info)) > + mptcp_subflow_ctx(ssock->sk)->request_mptcp = 0; Would it be better to put this check in mptcp_socket_create_get() where request_mptcp is being initially? Then it only needs to be in one place and will cause bind() to fail appropriately as well. Peter. > +#endif > + > err = ssock->ops->connect(ssock, uaddr, addr_len, flags); > sock_put(ssock->sk); > return err; > diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c > index 9b56fe9a83f2..4ab751163965 100644 > --- a/net/mptcp/subflow.c > +++ b/net/mptcp/subflow.c > @@ -30,6 +30,14 @@ static void subflow_init_req(struct request_sock *req, > > subflow_req->mp_capable = 0; > > +#ifdef CONFIG_TCP_MD5SIG > + /* no MPTCP is MD5SIG is enabled on this socket or we may run out of > + * TCP option space. > + */ > + if (rcu_access_pointer(tcp_sk(sk_listener)->md5sig_info)) > + return; > +#endif > + > if (rx_opt.mptcp.mp_capable && listener->request_mptcp) { > subflow_req->mp_capable = 1; > if (rx_opt.mptcp.version >= listener->request_version)
On Mon, 2019-11-11 at 13:44 -0800, Peter Krystad wrote: > On Mon, 2019-11-11 at 17:53 +0100, Paolo Abeni wrote: > > Likley we want to update the "mptcp: Create SUBFLOW socket for incoming connections" > > commit message with something alike: > > > > Explicitly disable MPTCP if MD5SIG is enable, to prevent TCP option space > > exaustion. > > > > Squash-to: mptcp: Create SUBFLOW socket for incoming connections > > Signed-off-by: Paolo Abeni <pabeni@redhat.com> > > --- > > net/mptcp/protocol.c | 8 ++++++++ > > net/mptcp/subflow.c | 8 ++++++++ > > 2 files changed, 16 insertions(+) > > > > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c > > index 81b7c21f4d3c..bb297e9dd6c5 100644 > > --- a/net/mptcp/protocol.c > > +++ b/net/mptcp/protocol.c > > @@ -342,6 +342,14 @@ static int mptcp_stream_connect(struct socket *sock, struct sockaddr *uaddr, > > if (IS_ERR(ssock)) > > return PTR_ERR(ssock); > > > > +#ifdef CONFIG_TCP_MD5SIG > > + /* no MPTCP is MD5SIG is enabled on this socket or we may run out of > > + * TCP option space. > > + */ > > typo in comment [two places]: "no MPTCP if MD5SIG" thanx, will fix in v2. > > + if (rcu_access_pointer(tcp_sk(ssock->sk)->md5sig_info)) > > + mptcp_subflow_ctx(ssock->sk)->request_mptcp = 0; > > Would it be better to put this check in mptcp_socket_create_get() where > request_mptcp is being initially? Then it only needs to be in one place and > will cause bind() to fail appropriately as well. In my reply to Florian I reported an incorrect sequence leading to MPTCP + MD5SIG, the correct one should be: socket(MPTCP) bind() // creates fallback socket setsockopt(MD5SIG) // fallback found, calls setsockopt() on it connect() In the above sequence, when mptcp_socket_create_get() is called by bind(), rcu_access_pointer(tcp_sk(ssock->sk)->md5sig_info) is NULL. The md5sig info will be allocated by the next setsockopt(). Moving the check inside mptcp_socket_create_get() should not help. Additionally I don't see why bind() should fail here, can you please explain? Thank you! Paolo
On Tue, 2019-11-12 at 09:34 +0100, Paolo Abeni wrote: > On Mon, 2019-11-11 at 13:44 -0800, Peter Krystad wrote: > > On Mon, 2019-11-11 at 17:53 +0100, Paolo Abeni wrote: > > > Likley we want to update the "mptcp: Create SUBFLOW socket for incoming connections" > > > commit message with something alike: > > > > > > Explicitly disable MPTCP if MD5SIG is enable, to prevent TCP option space > > > exaustion. > > > > > > Squash-to: mptcp: Create SUBFLOW socket for incoming connections > > > Signed-off-by: Paolo Abeni <pabeni@redhat.com> > > > --- > > > net/mptcp/protocol.c | 8 ++++++++ > > > net/mptcp/subflow.c | 8 ++++++++ > > > 2 files changed, 16 insertions(+) > > > > > > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c > > > index 81b7c21f4d3c..bb297e9dd6c5 100644 > > > --- a/net/mptcp/protocol.c > > > +++ b/net/mptcp/protocol.c > > > @@ -342,6 +342,14 @@ static int mptcp_stream_connect(struct socket *sock, struct sockaddr *uaddr, > > > if (IS_ERR(ssock)) > > > return PTR_ERR(ssock); > > > > > > +#ifdef CONFIG_TCP_MD5SIG > > > + /* no MPTCP is MD5SIG is enabled on this socket or we may run out of > > > + * TCP option space. > > > + */ > > > > typo in comment [two places]: "no MPTCP if MD5SIG" > > thanx, will fix in v2. > > > > + if (rcu_access_pointer(tcp_sk(ssock->sk)->md5sig_info)) > > > + mptcp_subflow_ctx(ssock->sk)->request_mptcp = 0; > > > > Would it be better to put this check in mptcp_socket_create_get() where > > request_mptcp is being initially? Then it only needs to be in one place and > > will cause bind() to fail appropriately as well. > > In my reply to Florian I reported an incorrect sequence leading to > MPTCP + MD5SIG, the correct one should be: > > socket(MPTCP) > bind() // creates fallback socket > setsockopt(MD5SIG) // fallback found, calls setsockopt() on it > connect() > > In the above sequence, when mptcp_socket_create_get() is called by > bind(), rcu_access_pointer(tcp_sk(ssock->sk)->md5sig_info) is NULL. The > md5sig info will be allocated by the next setsockopt(). > > Moving the check inside mptcp_socket_create_get() should not help. > Additionally I don't see why bind() should fail here, can you please > explain? I see the sequence now. My poor words for "bind fail", I meant setting request_mptcp=0 in bind(), which, from the above sequence, isn't right. Carry on! Peter. > Thank you! > > Paolo >
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c index 81b7c21f4d3c..bb297e9dd6c5 100644 --- a/net/mptcp/protocol.c +++ b/net/mptcp/protocol.c @@ -342,6 +342,14 @@ static int mptcp_stream_connect(struct socket *sock, struct sockaddr *uaddr, if (IS_ERR(ssock)) return PTR_ERR(ssock); +#ifdef CONFIG_TCP_MD5SIG + /* no MPTCP is MD5SIG is enabled on this socket or we may run out of + * TCP option space. + */ + if (rcu_access_pointer(tcp_sk(ssock->sk)->md5sig_info)) + mptcp_subflow_ctx(ssock->sk)->request_mptcp = 0; +#endif + err = ssock->ops->connect(ssock, uaddr, addr_len, flags); sock_put(ssock->sk); return err; diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c index 9b56fe9a83f2..4ab751163965 100644 --- a/net/mptcp/subflow.c +++ b/net/mptcp/subflow.c @@ -30,6 +30,14 @@ static void subflow_init_req(struct request_sock *req, subflow_req->mp_capable = 0; +#ifdef CONFIG_TCP_MD5SIG + /* no MPTCP is MD5SIG is enabled on this socket or we may run out of + * TCP option space. + */ + if (rcu_access_pointer(tcp_sk(sk_listener)->md5sig_info)) + return; +#endif + if (rx_opt.mptcp.mp_capable && listener->request_mptcp) { subflow_req->mp_capable = 1; if (rx_opt.mptcp.version >= listener->request_version)
Likley we want to update the "mptcp: Create SUBFLOW socket for incoming connections" commit message with something alike: Explicitly disable MPTCP if MD5SIG is enable, to prevent TCP option space exaustion. Squash-to: mptcp: Create SUBFLOW socket for incoming connections Signed-off-by: Paolo Abeni <pabeni@redhat.com> --- net/mptcp/protocol.c | 8 ++++++++ net/mptcp/subflow.c | 8 ++++++++ 2 files changed, 16 insertions(+)