11:48 p.m.
This set aims to improve handling of multiple subflows.
There are a few remaining problems, see below, but I wanted to send what
I have now rather than keep this back.
The first two patches are new, following few patches are a resend of the
earlier RFC series, I think they're good for acceptance now.
Patch 6 had a few changes since the RFC based on Paolos feedback, please
see patch description for details.
The last two patches are there to allow cancellation/teardown of
not-yet completed join requests. The connecting subflow flows
are placed on a 'join_list' so mptcp_shutdown can close these sockets
as well.
Last patch moves the 'eof hack' to a work queue so we don't set EOF on the
mptcp socket until all subflows have closed. Maybe this isn't needed, I
would be fine with dropping it in favor of mptcp-level DATA_FIN and
reset handling.
The rest of this work is to have msk->conn_list contain tcp_sock structs
without an outer 'struct socket'.
The problem is that for incoming join requests (which are not subject
to accept/request socket queue), there is no 'struct socket', so the kernel
will crash. Even when fixing those up, a NULL sk->sk_socket is problematic
as tcp stack will assume that such tcp sk is already detached from userspace,
i.e. NOSPACE wakeups do not work for them.
The solution here (suggested by Paolo) is to have sk->sk_socket refer
to the mptcp parent socket structure. This assignment is done by
a private function similar to sock_graft().
mptcp.org kernel will be able to establish multiple connections per
mptcp socket but does not consider those extra joins as established, as the
"4th ack" is missing.
A simple "tcp_send_ack" is enough, but there are two problems with this:
1. It can be lost
2. Given we are not expecting to receive any data on such flows this
doesn't seem to be a problem.
During testing I saw that sometimes mptcp.org kernel will emit further
join requests to mptcp-next but those trigger no response (and those
packets do not even show up in perfs network drop monitor, sigh).
I also saw a UAF at mptcp_close where a sock_hold() in tcp_close()
caused a 0->1 recount transition, but i've been unable to reproduce it so far
(might have been caused by a bug that has been fixed in the mean time).
The following changes since commit dd25ff04840135581b833762571aac08a2a94c6c:
sendmsg: truncate source buffer if mptcp sndbuf size was set from userspace (2019-11-30
02:47:59 +0000)
are available in the Git repository at:
git://git.breakpoint.cc/fw/mptcp-next.git mptcp_multijoin_11
for you to fetch changes up to 010c2e19c2d643a87c991bf1e7d4bfc140dfdba5:
don't flag parent socket as RCV_SHUTDOWN if one one subflow has closed (2019-11-30
23:21:25 +0100)
----------------------------------------------------------------
Florian Westphal (9):
token: handle join-before-accept-completion case
mptcp: fix race from mptcp_close/mptcp join
copy connection id from first subflow to mptcp socket
store tcp_sk, not socket
add mptcp_subflow_shutdown
make accept not allocate kernel socket struct
pass subflow socket to mptcp_finish_connect
subflow: place further subflows on new 'join_list'
don't flag parent socket as RCV_SHUTDOWN if one one subflow has closed
net/mptcp/pm.c | 18 +--
net/mptcp/protocol.c | 313 +++++++++++++++++++++++++++++++++++++--------------
net/mptcp/protocol.h | 16 +--
net/mptcp/subflow.c | 56 +++++----
net/mptcp/token.c | 9 +-
5 files changed, 292 insertions(+), 120 deletions(-)
11:48 p.m.
New subject: [PATCH 1/9] token: handle join-before-accept-completion case
squashto: mptcp: Add handling of incoming MP_JOIN requests
If we get a join request while the socket hasn't been accepted yet,
the tree still contains the dummy reserved value. Return NULL in
this case, the token is still invalid.
Only alternative would be to serialize such request into a work queue
to replay it at a future data when the address might have become
available.
Signed-off-by: Florian Westphal <fw(a)strlen.de>
---
net/mptcp/token.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/net/mptcp/token.c b/net/mptcp/token.c
index 56f1bd2641f6..15cd71a33205 100644
--- a/net/mptcp/token.c
+++ b/net/mptcp/token.c
@@ -182,8 +182,13 @@ struct mptcp_sock *mptcp_token_get_sock(u32 token)
spin_lock_bh(&token_tree_lock);
conn = radix_tree_lookup(&token_tree, token);
- if (conn)
- sock_hold(conn);
+ if (conn) {
+ /* token still reserved? */
+ if (conn == (struct sock *)&token_used)
+ conn = NULL;
+ else
+ sock_hold(conn);
+ }
spin_unlock_bh(&token_tree_lock);
return mptcp_sk(conn);
--
2.23.0
1:04 p.m.
New subject: [PATCH 1/9] token: handle join-before-accept-completion case
On Sat, 2019-11-30 at 23:48 +0100, Florian Westphal wrote:
squashto: mptcp: Add handling of incoming MP_JOIN requests
If we get a join request while the socket hasn't been accepted yet,
the tree still contains the dummy reserved value. Return NULL in
this case, the token is still invalid.
Only alternative would be to serialize such request into a work queue
to replay it at a future data when the address might have become
available.
Signed-off-by: Florian Westphal <fw(a)strlen.de>
---
net/mptcp/token.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/net/mptcp/token.c b/net/mptcp/token.c
index 56f1bd2641f6..15cd71a33205 100644
--- a/net/mptcp/token.c
+++ b/net/mptcp/token.c
@@ -182,8 +182,13 @@ struct mptcp_sock *mptcp_token_get_sock(u32 token)
spin_lock_bh(&token_tree_lock);
conn = radix_tree_lookup(&token_tree, token);
- if (conn)
- sock_hold(conn);
+ if (conn) {
+ /* token still reserved? */
+ if (conn == (struct sock *)&token_used)
+ conn = NULL;
+ else
+ sock_hold(conn);
+ }
spin_unlock_bh(&token_tree_lock);
return mptcp_sk(conn);
LGTM.
Note, with MPTCP v1, we will probably have to add additional checks
here (e.g. con->fourth_ack must be true). Perhaps we can add that
already?
/P
2:31 p.m.
New subject: [PATCH 1/9] token: handle join-before-accept-completion case
Paolo Abeni <pabeni(a)redhat.com> wrote:
Note, with MPTCP v1, we will probably have to add additional checks
here (e.g. con->fourth_ack must be true). Perhaps we can add that
already?
I'm not sure this is the right location. Should that check not
be done where we add the ssk to the conn_list?
4 p.m.
New subject: [PATCH 1/9] token: handle join-before-accept-completion case
On Mon, 2019-12-02 at 14:31 +0100, Florian Westphal wrote:
Paolo Abeni <pabeni(a)redhat.com> wrote:
> Note, with MPTCP v1, we will probably have to add additional checks
> here (e.g. con->fourth_ack must be true). Perhaps we can add that
> already?
I'm not sure this is the right location. Should that check not
be done where we add the ssk to the conn_list?
My [mis?]understanding is that MP_JOIN should not be attemped before
the msk is fully established (fourth_ack), so I thought it would be
nice to catch that erroneous condition ASAP.
Cheers,
Paolo
5:57 p.m.
New subject: [PATCH 1/9] token: handle join-before-accept-completion case
Paolo Abeni <pabeni(a)redhat.com> wrote:
On Mon, 2019-12-02 at 14:31 +0100, Florian Westphal wrote:
> Paolo Abeni <pabeni(a)redhat.com> wrote:
> > Note, with MPTCP v1, we will probably have to add additional checks
> > here (e.g. con->fourth_ack must be true). Perhaps we can add that
> > already?
>
> I'm not sure this is the right location. Should that check not
> be done where we add the ssk to the conn_list?
My [mis?]understanding is that MP_JOIN should not be attemped before
the msk is fully established (fourth_ack), so I thought it would be
nice to catch that erroneous condition ASAP.
I see. What would that buy us though wrt. error handling?
11:48 p.m.
New subject: [PATCH 2/9] mptcp: fix race from mptcp_close/mptcp join
squashto: mptcp: Add handling of incoming MP_JOIN requests
When we get join request, we can run into following problem:
cpu0 cpu1
mptcp_close() mptcp_finish_join()
lock_mptcp_sk()
close all ssk
unlock_mptcp_sk()
lock_mptcp_sk()
... add to empty list
... graft ssk to invalid parent
Fix this by indicating via mptcp_finish_join if the join was ok or not.
We can do so by testing the mptcp sk_state from mptcp_finish_join after
we've obtained the msk socket lock.
If the state is established, mptcp_close wasn't called yet (or, its
waiting on the msk lock -- the latter makes no difference to the
former).
If the state is different, then don't don't graft the ssk, don't
add it the the connection list, and let the caller close the ssk.
Also, do NOT advance the ssk to established state. The tcp stack
already does this a bit later on, after the full tcp_sk state has been
set up. Doing this here can trigger a divide-by-0 bug when the ssk
receives data right away, because tp->->rcvq_space.space has not been
initialized yet.
Signed-off-by: Florian Westphal <fw(a)strlen.de>
---
net/mptcp/protocol.c | 13 +++++++++++--
net/mptcp/protocol.h | 2 +-
net/mptcp/subflow.c | 8 +++++---
3 files changed, 17 insertions(+), 6 deletions(-)
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 72e6950fbbc2..529a08095512 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -1174,19 +1174,28 @@ void mptcp_finish_connect(struct sock *sk, int mp_capable)
inet_sk_state_store(sk, TCP_ESTABLISHED);
}
-void mptcp_finish_join(struct sock *sk)
+bool mptcp_finish_join(struct sock *sk)
{
struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
struct mptcp_sock *msk = mptcp_sk(subflow->conn);
+ struct sock *parent = (void *)msk;
+ bool ret = false;
pr_debug("msk=%p, subflow=%p", msk, subflow);
local_bh_disable();
bh_lock_sock_nested(subflow->conn);
+
+ /* mptcp socket already closing? */
+ if (parent->sk_state != TCP_ESTABLISHED)
+ goto out;
+
+ ret = true;
list_add_tail(&subflow->node, &msk->conn_list);
+out:
bh_unlock_sock(subflow->conn);
local_bh_enable();
- inet_sk_state_store(sk, TCP_ESTABLISHED);
+ return ret;
}
bool mptcp_sk_is_subflow(const struct sock *sk)
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index acad61c70de9..ac18dd38618e 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -291,7 +291,7 @@ void mptcp_get_options(const struct sk_buff *skb,
struct tcp_options_received *opt_rx);
void mptcp_finish_connect(struct sock *sk, int mp_capable);
-void mptcp_finish_join(struct sock *sk);
+bool mptcp_finish_join(struct sock *sk);
void mptcp_data_acked(struct sock *sk);
int mptcp_token_new_request(struct request_sock *req);
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 995af1acc0b1..555d54d065b1 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -234,8 +234,9 @@ static void subflow_finish_connect(struct sock *sk, const struct
sk_buff *skb)
if (skb)
subflow->ssn_offset = TCP_SKB_CB(skb)->seq;
- mptcp_finish_join(sk);
- subflow->conn_finished = 1;
+
+ if (mptcp_finish_join(sk))
+ subflow->conn_finished = 1;
}
}
@@ -355,7 +356,8 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
goto close_child;
ctx->conn = (struct sock *)owner;
- mptcp_finish_join(child);
+ if (!mptcp_finish_join(child))
+ goto close_child;
}
}
--
2.23.0
1:10 p.m.
New subject: [PATCH 2/9] mptcp: fix race from mptcp_close/mptcp join
On Sat, 2019-11-30 at 23:48 +0100, Florian Westphal wrote:
squashto: mptcp: Add handling of incoming MP_JOIN requests
When we get join request, we can run into following problem:
cpu0 cpu1
mptcp_close() mptcp_finish_join()
lock_mptcp_sk()
close all ssk
unlock_mptcp_sk()
lock_mptcp_sk()
... add to empty list
... graft ssk to invalid parent
Fix this by indicating via mptcp_finish_join if the join was ok or not.
We can do so by testing the mptcp sk_state from mptcp_finish_join after
we've obtained the msk socket lock.
If the state is established, mptcp_close wasn't called yet (or, its
waiting on the msk lock -- the latter makes no difference to the
former).
If the state is different, then don't don't graft the ssk, don't
add it the the connection list, and let the caller close the ssk.
Also, do NOT advance the ssk to established state. The tcp stack
already does this a bit later on, after the full tcp_sk state has been
set up. Doing this here can trigger a divide-by-0 bug when the ssk
receives data right away, because tp->->rcvq_space.space has not been
initialized yet.
Signed-off-by: Florian Westphal <fw(a)strlen.de>
---
net/mptcp/protocol.c | 13 +++++++++++--
net/mptcp/protocol.h | 2 +-
net/mptcp/subflow.c | 8 +++++---
3 files changed, 17 insertions(+), 6 deletions(-)
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 72e6950fbbc2..529a08095512 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -1174,19 +1174,28 @@ void mptcp_finish_connect(struct sock *sk, int mp_capable)
inet_sk_state_store(sk, TCP_ESTABLISHED);
}
-void mptcp_finish_join(struct sock *sk)
+bool mptcp_finish_join(struct sock *sk)
{
struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
struct mptcp_sock *msk = mptcp_sk(subflow->conn);
+ struct sock *parent = (void *)msk;
+ bool ret = false;
pr_debug("msk=%p, subflow=%p", msk, subflow);
local_bh_disable();
bh_lock_sock_nested(subflow->conn);
+
+ /* mptcp socket already closing? */
+ if (parent->sk_state != TCP_ESTABLISHED)
+ goto out;
+
+ ret = true;
list_add_tail(&subflow->node, &msk->conn_list);
This relies on mptcp_close() setting msk status before acquiring the
msk socket lock, right? If so perhaps we can add a comment there to
document the locking schema?
The patch LGTM!
/P
2:29 p.m.
New subject: [PATCH 2/9] mptcp: fix race from mptcp_close/mptcp join
Paolo Abeni <pabeni(a)redhat.com> wrote:
> + /* mptcp socket already closing? */
> + if (parent->sk_state != TCP_ESTABLISHED)
> + goto out;
> +
> + ret = true;
> list_add_tail(&subflow->node, &msk->conn_list);
This relies on mptcp_close() setting msk status before acquiring the
msk socket lock, right? If so perhaps we can add a comment there to
document the locking schema?
No, if mptcp_close would set its state after acquiring the lock this
will still work, either the above check triggers (mptcp_close already
finished) or mptcp_close is still blocked on the lock.
The second case is no different from "mptcp_close was not called".
3:38 p.m.
New subject: [PATCH 2/9] mptcp: fix race from mptcp_close/mptcp join
On Mon, 2019-12-02 at 14:29 +0100, Florian Westphal wrote:
Paolo Abeni <pabeni(a)redhat.com> wrote:
> > + /* mptcp socket already closing? */
> > + if (parent->sk_state != TCP_ESTABLISHED)
> > + goto out;
> > +
> > + ret = true;
> > list_add_tail(&subflow->node, &msk->conn_list);
>
> This relies on mptcp_close() setting msk status before acquiring the
> msk socket lock, right? If so perhaps we can add a comment there to
> document the locking schema?
No, if mptcp_close would set its state after acquiring the lock this
will still work, either the above check triggers (mptcp_close already
finished) or mptcp_close is still blocked on the lock.
The second case is no different from "mptcp_close was not called".
Ah, I see mptcp_close still have to acquire the ssk socket lock!
Yep, no additional changes required.
Thanks,
Paolo
11:48 p.m.
New subject: [PATCH 3/9] copy connection id from first subflow to mptcp socket
This allows to call inet(6)_getname on the parent socket
and it will return the right result.
At this time it only works because the first incoming subflow has a
'struct socket' allocated.
This will change in followup patch, passive subflows will not have
a socket struct anymore.
Signed-off-by: Florian Westphal <fw(a)strlen.de>
---
net/mptcp/pm.c | 18 ++++----
net/mptcp/protocol.c | 98 ++++++++++++++++++++++++--------------------
net/mptcp/protocol.h | 1 -
3 files changed, 63 insertions(+), 54 deletions(-)
diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index 7e493d81afef..e474bd9dc05d 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -81,6 +81,7 @@ int mptcp_pm_create_subflow(u32 token, u8 remote_id, struct in_addr
*addr)
struct mptcp_sock *msk = mptcp_token_get_sock(token);
struct sockaddr_in remote;
struct sockaddr_in local;
+ struct sock *sk;
int err;
if (!msk)
@@ -88,6 +89,7 @@ int mptcp_pm_create_subflow(u32 token, u8 remote_id, struct in_addr
*addr)
pr_debug("msk=%p", msk);
+ sk = (struct sock *)msk;
if (!msk->pm.remote_valid || remote_id != msk->pm.remote_id) {
err = -EBADR;
goto create_put;
@@ -101,15 +103,14 @@ int mptcp_pm_create_subflow(u32 token, u8 remote_id, struct in_addr
*addr)
local.sin_addr.s_addr = htonl(INADDR_ANY);
remote.sin_family = msk->pm.remote_family;
- remote.sin_port = htons(msk->dport);
+ remote.sin_port = inet_sk(sk)->inet_dport;
remote.sin_addr = msk->pm.remote_addr;
- err = mptcp_subflow_connect((struct sock *)msk,
- (struct sockaddr *)&local,
+ err = mptcp_subflow_connect(sk, (struct sockaddr *)&local,
(struct sockaddr *)&remote, remote_id);
create_put:
- sock_put((struct sock *)msk);
+ sock_put(sk);
return err;
}
@@ -119,12 +120,14 @@ int mptcp_pm_create_subflow6(u32 token, u8 remote_id, struct
in6_addr *addr)
struct mptcp_sock *msk = mptcp_token_get_sock(token);
struct sockaddr_in6 remote;
struct sockaddr_in6 local;
+ struct sock *sk;
int err;
if (!msk)
return -EINVAL;
pr_debug("msk=%p", msk);
+ sk = (struct sock *)msk;
if (!msk->pm.remote_valid || remote_id != msk->pm.remote_id) {
err = -EBADR;
@@ -139,15 +142,14 @@ int mptcp_pm_create_subflow6(u32 token, u8 remote_id, struct
in6_addr *addr)
local.sin6_addr = in6addr_any;
remote.sin6_family = msk->pm.remote_family;
- remote.sin6_port = htons(msk->dport);
+ remote.sin6_port = inet_sk(sk)->inet_dport;
remote.sin6_addr = msk->pm.remote_addr6;
- err = mptcp_subflow_connect((struct sock *)msk,
- (struct sockaddr *)&local,
+ err = mptcp_subflow_connect(sk, (struct sockaddr *)&local,
(struct sockaddr *)&remote, remote_id);
create_put:
- sock_put((struct sock *)msk);
+ sock_put(sk);
return err;
}
#endif
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 529a08095512..7e16e5878fa5 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -91,19 +91,6 @@ static struct socket *mptcp_fallback_get_ref(const struct mptcp_sock
*msk)
return ssock;
}
-static struct sock *mptcp_subflow_get(const struct mptcp_sock *msk)
-{
- struct mptcp_subflow_context *subflow;
-
- sock_owned_by_me((const struct sock *)msk);
-
- mptcp_for_each_subflow(msk, subflow) {
- return mptcp_subflow_tcp_socket(subflow)->sk;
- }
-
- return NULL;
-}
-
static struct sock *mptcp_subflow_recv_lookup(const struct mptcp_sock *msk)
{
struct sock *sk = (struct sock *)msk;
@@ -934,6 +921,29 @@ static int mptcp_disconnect(struct sock *sk, int flags)
return tcp_disconnect(sk, flags);
}
+static void mptcp_copy_inaddrs(struct sock *msk, const struct sock *ssk)
+{
+ const struct ipv6_pinfo *ssk6 = inet6_sk(ssk);
+ struct ipv6_pinfo *msk6 = inet6_sk(msk);
+
+ inet_sk(msk)->inet_num = inet_sk(ssk)->inet_num;
+ inet_sk(msk)->inet_dport = inet_sk(ssk)->inet_dport;
+ inet_sk(msk)->inet_sport = inet_sk(ssk)->inet_sport;
+ inet_sk(msk)->inet_daddr = inet_sk(ssk)->inet_daddr;
+ inet_sk(msk)->inet_saddr = inet_sk(ssk)->inet_saddr;
+ inet_sk(msk)->inet_rcv_saddr = inet_sk(ssk)->inet_rcv_saddr;
+
+#if IS_ENABLED(CONFIG_IPV6)
+ msk->sk_v6_daddr = ssk->sk_v6_daddr;
+ msk->sk_v6_rcv_saddr = ssk->sk_v6_rcv_saddr;
+
+ if (msk6 && ssk6) {
+ msk6->saddr = ssk6->saddr;
+ msk6->flow_label = ssk6->flow_label;
+ }
+#endif
+}
+
static struct sock *mptcp_accept(struct sock *sk, int flags, int *err,
bool kern)
{
@@ -954,6 +964,7 @@ static struct sock *mptcp_accept(struct sock *sk, int flags, int
*err,
pr_debug("msk=%p, new subflow=%p, ", msk, subflow);
if (subflow->mp_capable) {
+ struct sock *ssk = new_sock->sk;
struct sock *new_mptcp_sock;
u64 ack_seq;
@@ -988,6 +999,7 @@ static struct sock *mptcp_accept(struct sock *sk, int flags, int
*err,
ack_seq++;
msk->ack_seq = ack_seq;
newsk = new_mptcp_sock;
+ mptcp_copy_inaddrs(newsk, ssk);
list_add(&subflow->node, &msk->conn_list);
bh_unlock_sock(new_mptcp_sock);
@@ -999,7 +1011,7 @@ static struct sock *mptcp_accept(struct sock *sk, int flags, int
*err,
/* the subflow can already receive packet, avoid racing with
* the receive path and process the pending ones
*/
- lock_sock(new_sock->sk);
+ lock_sock(ssk);
subflow->map_seq = ack_seq;
subflow->map_subflow_seq = 1;
subflow->rel_write_seq = 1;
@@ -1007,7 +1019,7 @@ static struct sock *mptcp_accept(struct sock *sk, int flags, int
*err,
subflow->conn = new_mptcp_sock;
if (unlikely(!skb_queue_empty(&new_sock->sk->sk_receive_queue)))
mptcp_subflow_data_available(new_sock->sk);
- release_sock(new_sock->sk);
+ release_sock(ssk);
} else {
newsk = new_sock->sk;
tcp_sk(newsk)->is_mptcp = 0;
@@ -1122,6 +1134,7 @@ void mptcp_finish_connect(struct sock *sk, int mp_capable)
subflow = mptcp_subflow_ctx(msk->subflow->sk);
if (mp_capable) {
+ struct sock *ssk = msk->subflow->sk;
u64 ack_seq;
/* sk (new subflow socket) is already locked, but we need
@@ -1151,7 +1164,7 @@ void mptcp_finish_connect(struct sock *sk, int mp_capable)
msk->local_key = subflow->local_key;
msk->token = subflow->token;
pr_debug("msk=%p, token=%u", msk, msk->token);
- msk->dport = ntohs(inet_sk(msk->subflow->sk)->inet_dport);
+ mptcp_copy_inaddrs(sk, ssk);
mptcp_pm_new_connection(msk, 0);
@@ -1309,43 +1322,44 @@ static int mptcp_stream_connect(struct socket *sock, struct
sockaddr *uaddr,
}
static int mptcp_getname(struct socket *sock, struct sockaddr *uaddr,
- int peer)
+ int peer, int af)
{
struct mptcp_sock *msk = mptcp_sk(sock->sk);
struct socket *ssock;
- struct sock *ssk;
int ret;
- lock_sock(sock->sk);
- ssock = __mptcp_fallback_get_ref(msk);
- if (ssock) {
+ if (msk->subflow) {
+ lock_sock(sock->sk);
+ ssock = __mptcp_fallback_get_ref(msk);
release_sock(sock->sk);
- pr_debug("subflow=%p", ssock->sk);
- ret = ssock->ops->getname(ssock, uaddr, peer);
- sock_put(ssock->sk);
- return ret;
+ if (ssock) {
+ pr_debug("subflow=%p", ssock->sk);
+ ret = ssock->ops->getname(ssock, uaddr, peer);
+ sock_put(ssock->sk);
+ return ret;
+ }
}
- /* @@ the meaning of getname() for the remote peer when the socket
- * is connected and there are multiple subflows is not defined.
- * For now just use the first subflow on the list.
- */
- ssk = mptcp_subflow_get(msk);
- if (!ssk) {
- release_sock(sock->sk);
- return -ENOTCONN;
+ switch (af) {
+ case AF_INET:
+ ret = inet_getname(sock, uaddr, peer);
+ break;
+#if IS_ENABLED(CONFIG_MPTCP_IPV6)
+ case AF_INET6:
+ ret = inet6_getname(sock, uaddr, peer);
+ break;
+#endif
+ default:
+ ret = -ENOPROTOOPT;
+ WARN_ON_ONCE(1);
}
- ret = ssk->sk_socket->ops->getname(ssk->sk_socket, uaddr, peer);
- release_sock(sock->sk);
return ret;
}
static int mptcp_v4_getname(struct socket *sock, struct sockaddr *uaddr,
int peer)
{
- int ret;
-
if (sock->sk->sk_prot == &tcp_prot) {
/* we are being invoked from __sys_accept4, after
* mptcp_accept() has just accepted a non-mp-capable
@@ -1358,17 +1372,13 @@ static int mptcp_v4_getname(struct socket *sock, struct sockaddr
*uaddr,
return sock->ops->getname(sock, uaddr, peer);
}
- ret = mptcp_getname(sock, uaddr, peer);
-
- return ret;
+ return mptcp_getname(sock, uaddr, peer, AF_INET);
}
#if IS_ENABLED(CONFIG_MPTCP_IPV6)
static int mptcp_v6_getname(struct socket *sock, struct sockaddr *uaddr,
int peer)
{
- int ret;
-
if (sock->sk->sk_prot == &tcpv6_prot) {
/* we are being invoked from __sys_accept4 after
* mptcp_accept() has accepted a non-mp-capable
@@ -1381,9 +1391,7 @@ static int mptcp_v6_getname(struct socket *sock, struct sockaddr
*uaddr,
return sock->ops->getname(sock, uaddr, peer);
}
- ret = mptcp_getname(sock, uaddr, peer);
-
- return ret;
+ return mptcp_getname(sock, uaddr, peer, AF_INET6);
}
#endif
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index ac18dd38618e..61c77616482c 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -134,7 +134,6 @@ struct mptcp_sock {
unsigned long timer_ival;
u32 token;
unsigned long flags;
- u16 dport;
struct work_struct rtx_work;
struct list_head conn_list;
struct list_head rtx_queue;
--
2.23.0
11:48 p.m.
New subject: [PATCH 4/9] store tcp_sk, not socket
preparation patch to avoid outer 'struct socket'.
Signed-off-by: Florian Westphal <fw(a)strlen.de>
---
net/mptcp/protocol.c | 23 ++++++++++++-----------
net/mptcp/protocol.h | 8 ++++----
net/mptcp/subflow.c | 8 +++-----
3 files changed, 19 insertions(+), 20 deletions(-)
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 7e16e5878fa5..a616e18066ae 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -101,7 +101,7 @@ static struct sock *mptcp_subflow_recv_lookup(const struct mptcp_sock
*msk)
mptcp_for_each_subflow(msk, subflow) {
if (subflow->data_avail)
- return mptcp_subflow_tcp_socket(subflow)->sk;
+ return mptcp_subflow_tcp_sock(subflow);
receivers += !subflow->rx_eof;
}
@@ -382,7 +382,7 @@ static struct sock *mptcp_subflow_get_send(struct mptcp_sock *msk)
sock_owned_by_me((const struct sock *)msk);
mptcp_for_each_subflow(msk, subflow) {
- struct sock *ssk = mptcp_subflow_tcp_socket(subflow)->sk;
+ struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
if (!sk_stream_memory_free(ssk)) {
struct socket *sock = ssk->sk_socket;
@@ -745,7 +745,7 @@ static struct sock *mptcp_subflow_get_retrans(const struct mptcp_sock
*msk)
sock_owned_by_me((const struct sock *)msk);
mptcp_for_each_subflow(msk, subflow) {
- struct sock *ssk = mptcp_subflow_tcp_socket(subflow)->sk;
+ struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
/* still data outstanding at TCP level? Don't retransmit. */
if (!tcp_write_queue_empty(ssk))
@@ -900,8 +900,11 @@ static void mptcp_close(struct sock *sk, long timeout)
}
list_for_each_entry_safe(subflow, tmp, &msk->conn_list, node) {
+ struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
+
pr_debug("conn_list->subflow=%p", subflow);
- sock_release(mptcp_subflow_tcp_socket(subflow));
+ list_del(&subflow->node);
+ sock_release(ssk->sk_socket);
}
__mptcp_clear_xmit(sk);
@@ -1015,10 +1018,10 @@ static struct sock *mptcp_accept(struct sock *sk, int flags, int
*err,
subflow->map_seq = ack_seq;
subflow->map_subflow_seq = 1;
subflow->rel_write_seq = 1;
- subflow->tcp_sock = new_sock;
+ subflow->tcp_sock = ssk;
subflow->conn = new_mptcp_sock;
- if (unlikely(!skb_queue_empty(&new_sock->sk->sk_receive_queue)))
- mptcp_subflow_data_available(new_sock->sk);
+ if (unlikely(!skb_queue_empty(&ssk->sk_receive_queue)))
+ mptcp_subflow_data_available(ssk);
release_sock(ssk);
} else {
newsk = new_sock->sk;
@@ -1484,11 +1487,9 @@ static int mptcp_shutdown(struct socket *sock, int how)
}
mptcp_for_each_subflow(msk, subflow) {
- struct socket *tcp_socket;
+ struct sock *tcp_sk = mptcp_subflow_tcp_sock(subflow);
- tcp_socket = mptcp_subflow_tcp_socket(subflow);
- pr_debug("conn_list->subflow=%p", subflow);
- ret = kernel_sock_shutdown(tcp_socket, how);
+ ret = kernel_sock_shutdown(tcp_sk->sk_socket, how);
}
release_sock(sock->sk);
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 61c77616482c..7f559436fc9b 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -226,7 +226,7 @@ struct mptcp_subflow_context {
u8 local_id;
u8 remote_id;
- struct socket *tcp_sock; /* underlying tcp_sock */
+ struct sock *tcp_sock; /* tcp sk backpointer */
struct sock *conn; /* parent mptcp_sock */
const struct inet_connection_sock_af_ops *icsk_af_ops;
void (*tcp_sk_data_ready)(struct sock *sk);
@@ -242,8 +242,8 @@ mptcp_subflow_ctx(const struct sock *sk)
return (__force struct mptcp_subflow_context *)icsk->icsk_ulp_data;
}
-static inline struct socket *
-mptcp_subflow_tcp_socket(const struct mptcp_subflow_context *subflow)
+static inline struct sock *
+mptcp_subflow_tcp_sock(const struct mptcp_subflow_context *subflow)
{
return subflow->tcp_sock;
}
@@ -251,7 +251,7 @@ mptcp_subflow_tcp_socket(const struct mptcp_subflow_context *subflow)
static inline u64
mptcp_subflow_get_map_offset(const struct mptcp_subflow_context *subflow)
{
- return tcp_sk(mptcp_subflow_tcp_socket(subflow)->sk)->copied_seq -
+ return tcp_sk(mptcp_subflow_tcp_sock(subflow))->copied_seq -
subflow->ssn_offset -
subflow->map_subflow_seq;
}
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 555d54d065b1..edec2ecbd93f 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -773,7 +773,6 @@ int mptcp_subflow_create_socket(struct sock *sk, struct socket
**new_sock)
}
static struct mptcp_subflow_context *subflow_create_ctx(struct sock *sk,
- struct socket *sock,
gfp_t priority)
{
struct inet_connection_sock *icsk = inet_csk(sk);
@@ -786,8 +785,7 @@ static struct mptcp_subflow_context *subflow_create_ctx(struct sock
*sk,
pr_debug("subflow=%p", ctx);
- /* might be NULL */
- ctx->tcp_sock = sock;
+ ctx->tcp_sock = sk;
return ctx;
}
@@ -829,7 +827,7 @@ static int subflow_ulp_init(struct sock *sk)
goto out;
}
- ctx = subflow_create_ctx(sk, sk->sk_socket, GFP_KERNEL);
+ ctx = subflow_create_ctx(sk, GFP_KERNEL);
if (!ctx) {
err = -ENOMEM;
goto out;
@@ -874,7 +872,7 @@ static void subflow_ulp_clone(const struct request_sock *req,
struct mptcp_subflow_context *new_ctx;
/* newsk->sk_socket is NULL at this point */
- new_ctx = subflow_create_ctx(newsk, NULL, priority);
+ new_ctx = subflow_create_ctx(newsk, priority);
if (!new_ctx)
return;
--
2.23.0
11:48 p.m.
New subject: [PATCH 5/9] add mptcp_subflow_shutdown
soon tcp_sk->sk_socket may be pointing at the parent mptcp socket.
Add a private shutdown function and use it instead.
Signed-off-by: Florian Westphal <fw(a)strlen.de>
---
net/mptcp/protocol.c | 42 +++++++++++++++++++++++++++++++++++++++++-
1 file changed, 41 insertions(+), 1 deletion(-)
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index a616e18066ae..360b04edb895 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -884,6 +884,29 @@ static void mptcp_cancel_work(struct sock *sk)
sock_put(sk);
}
+static void mptcp_subflow_shutdown(struct sock *ssk, int how)
+{
+ lock_sock(ssk);
+
+ switch (ssk->sk_state) {
+ case TCP_LISTEN:
+ if (!(how & RCV_SHUTDOWN))
+ break;
+ /* fall through */
+ case TCP_SYN_SENT:
+ tcp_disconnect(ssk, O_NONBLOCK);
+ break;
+ default:
+ ssk->sk_shutdown |= how;
+ tcp_shutdown(ssk, how);
+ break;
+ }
+
+ /* Wake up anyone sleeping in poll. */
+ ssk->sk_state_change(ssk);
+ release_sock(ssk);
+}
+
static void mptcp_close(struct sock *sk, long timeout)
{
struct mptcp_subflow_context *subflow, *tmp;
@@ -1486,11 +1509,28 @@ static int mptcp_shutdown(struct socket *sock, int how)
return ret;
}
+ how++;
+
+ if ((how & ~SHUTDOWN_MASK) || !how) {
+ ret = -EINVAL;
+ goto out_unlock;
+ }
+
+ if (sock->state == SS_CONNECTING) {
+ if ((1 << sock->sk->sk_state) &
+ (TCPF_SYN_SENT | TCPF_SYN_RECV | TCPF_CLOSE))
+ sock->state = SS_DISCONNECTING;
+ else
+ sock->state = SS_CONNECTED;
+ }
+
mptcp_for_each_subflow(msk, subflow) {
struct sock *tcp_sk = mptcp_subflow_tcp_sock(subflow);
- ret = kernel_sock_shutdown(tcp_sk->sk_socket, how);
+ mptcp_subflow_shutdown(tcp_sk, how);
}
+
+out_unlock:
release_sock(sock->sk);
return ret;
--
2.23.0
11:48 p.m.
New subject: [PATCH 6/9] make accept not allocate kernel socket struct
Once lower-level accept returns, walk the subflow list (there should
be one subflow) and make sk->sk_socket point to the new mptcp socket.
This is needed to make NOSPACE flag work: tcp stack can't signal
availability of new data unless ->sk_socket points to a "struct socket".
If sk_socket is NULL, then from TCP stack p.o.v. the socket has been
detached from the userspace program already, so nothing to wake up in
first place.
mptcp_close needs to handle both cases: connect()ing sockets have a
'struct socket' allocated, while incoming ones do not.
v2: clarify sk_prot == &mptcp_prot || is_mptcp_v6(newsock) construct,
prefer a '!= tcp_prot' here.
Add and use helper for subflow closing and add comment as to
why incoming/outgoing ssk get different treatment.
Signed-off-by: Florian Westphal <fw(a)strlen.de>
---
net/mptcp/protocol.c | 83 +++++++++++++++++++++++++++++++++++---------
1 file changed, 67 insertions(+), 16 deletions(-)
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 360b04edb895..bca290c05839 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -763,6 +763,32 @@ static struct sock *mptcp_subflow_get_retrans(const struct mptcp_sock
*msk)
return backup;
}
+/* subflow sockets can be either outgoing (connect) or incoming
+ * (accept).
+ *
+ * Outgoing subflows use in-kernel sockets.
+ * Incoming subflows do not have their own 'struct socket' allocated,
+ * so we need to use tcp_close() after detaching them from the mptcp
+ * parent socket.
+ */
+static void __mptcp_close_ssk(struct sock *sk, struct sock *ssk,
+ struct mptcp_subflow_context *subflow,
+ long timeout)
+{
+ struct socket *sock = READ_ONCE(ssk->sk_socket);
+
+ list_del(&subflow->node);
+
+ if (sock && sock != sk->sk_socket) {
+ /* outgoing subflow */
+ sock_release(sock);
+ } else {
+ /* incoming subflow */
+ sock_orphan(ssk);
+ tcp_close(ssk, timeout);
+ }
+}
+
static void mptcp_worker(struct work_struct *work)
{
int orig_len, orig_offset, ret, mss_now = 0, size_goal = 0;
@@ -925,9 +951,7 @@ static void mptcp_close(struct sock *sk, long timeout)
list_for_each_entry_safe(subflow, tmp, &msk->conn_list, node) {
struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
- pr_debug("conn_list->subflow=%p", subflow);
- list_del(&subflow->node);
- sock_release(ssk->sk_socket);
+ __mptcp_close_ssk(sk, ssk, subflow, timeout);
}
__mptcp_clear_xmit(sk);
@@ -975,23 +999,22 @@ static struct sock *mptcp_accept(struct sock *sk, int flags, int
*err,
{
struct mptcp_sock *msk = mptcp_sk(sk);
struct mptcp_subflow_context *subflow;
- struct socket *new_sock;
struct socket *listener;
struct sock *newsk;
listener = msk->subflow;
pr_debug("msk=%p, listener=%p", msk, mptcp_subflow_ctx(listener->sk));
- *err = kernel_accept(listener, &new_sock, flags);
- if (*err < 0)
+ newsk = inet_csk_accept(listener->sk, flags, err, kern);
+ if (!newsk)
return NULL;
- subflow = mptcp_subflow_ctx(new_sock->sk);
+ subflow = mptcp_subflow_ctx(newsk);
pr_debug("msk=%p, new subflow=%p, ", msk, subflow);
if (subflow->mp_capable) {
- struct sock *ssk = new_sock->sk;
struct sock *new_mptcp_sock;
+ struct sock *ssk = newsk;
u64 ack_seq;
lock_sock(sk);
@@ -1002,8 +1025,8 @@ static struct sock *mptcp_accept(struct sock *sk, int flags, int
*err,
*err = -ENOBUFS;
local_bh_enable();
release_sock(sk);
- kernel_sock_shutdown(new_sock, SHUT_RDWR);
- sock_release(new_sock);
+ mptcp_subflow_shutdown(newsk, SHUT_RDWR + 1);
+ tcp_close(newsk, 0);
return NULL;
}
@@ -1014,7 +1037,7 @@ static struct sock *mptcp_accept(struct sock *sk, int flags, int
*err,
msk->local_key = subflow->local_key;
msk->token = subflow->token;
- mptcp_token_update_accept(new_sock->sk, new_mptcp_sock);
+ mptcp_token_update_accept(newsk, new_mptcp_sock);
msk->subflow = NULL;
mptcp_pm_new_connection(msk, 1);
@@ -1047,10 +1070,7 @@ static struct sock *mptcp_accept(struct sock *sk, int flags, int
*err,
mptcp_subflow_data_available(ssk);
release_sock(ssk);
} else {
- newsk = new_sock->sk;
tcp_sk(newsk)->is_mptcp = 0;
- new_sock->sk = NULL;
- sock_release(new_sock);
MPTCP_INC_STATS(sock_net(sk),
MPTCP_MIB_MPCAPABLEPASSIVEFALLBACK);
@@ -1213,11 +1233,21 @@ void mptcp_finish_connect(struct sock *sk, int mp_capable)
inet_sk_state_store(sk, TCP_ESTABLISHED);
}
+static void mptcp_sock_graft(struct sock *sk, struct socket *parent)
+{
+ write_lock_bh(&sk->sk_callback_lock);
+ rcu_assign_pointer(sk->sk_wq, &parent->wq);
+ sk_set_socket(sk, parent);
+ sk->sk_uid = SOCK_INODE(parent)->i_uid;
+ write_unlock_bh(&sk->sk_callback_lock);
+}
+
bool mptcp_finish_join(struct sock *sk)
{
struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
struct mptcp_sock *msk = mptcp_sk(subflow->conn);
struct sock *parent = (void *)msk;
+ struct socket *parent_sock;
bool ret = false;
pr_debug("msk=%p, subflow=%p", msk, subflow);
@@ -1229,8 +1259,14 @@ bool mptcp_finish_join(struct sock *sk)
if (parent->sk_state != TCP_ESTABLISHED)
goto out;
- ret = true;
- list_add_tail(&subflow->node, &msk->conn_list);
+ parent_sock = READ_ONCE(parent->sk_socket);
+ if (parent_sock) {
+ if (!sk->sk_socket)
+ mptcp_sock_graft(sk, parent_sock);
+
+ ret = true;
+ list_add_tail(&subflow->node, &msk->conn_list);
+ }
out:
bh_unlock_sock(subflow->conn);
local_bh_enable();
@@ -1452,6 +1488,21 @@ static int mptcp_stream_accept(struct socket *sock, struct socket
*newsock,
return -EINVAL;
err = ssock->ops->accept(sock, newsock, flags, kern);
+ if (err == 0 && newsock->sk->sk_prot != &tcp_prot) {
+ struct mptcp_sock *msk = mptcp_sk(newsock->sk);
+ struct mptcp_subflow_context *subflow;
+
+ /* set ssk->sk_socket of accept()ed flows to mptcp socket.
+ * This is needed so NOSPACE flag can be set from tcp stack.
+ */
+ list_for_each_entry(subflow, &msk->conn_list, node) {
+ struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
+
+ if (!ssk->sk_socket)
+ mptcp_sock_graft(ssk, newsock);
+ }
+ }
+
sock_put(ssock->sk);
return err;
}
--
2.23.0
11:48 p.m.
New subject: [PATCH 7/9] pass subflow socket to mptcp_finish_connect
Unlike _join(), we pass the mptcp socket and then use msk->subflow
to get back the subflow socket.
Just pass the subflow directly.
Right now we take the mptcp socket from the subflow context
(subflow->conn), pass that to mptcp_finish_connect and then use
the msk->subflow pointer to get to the subflow socket, i.e.
we do:
subflow_ctx(ssk)->mptcp_socket->subflow
just to get what the caller already had: ssk.
Signed-off-by: Florian Westphal <fw(a)strlen.de>
---
net/mptcp/protocol.c | 11 ++++++-----
net/mptcp/protocol.h | 2 +-
net/mptcp/subflow.c | 2 +-
3 files changed, 8 insertions(+), 7 deletions(-)
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index bca290c05839..0c61487a587d 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -1172,15 +1172,16 @@ static int mptcp_get_port(struct sock *sk, unsigned short snum)
return inet_csk_get_port(msk->subflow->sk, snum);
}
-void mptcp_finish_connect(struct sock *sk, int mp_capable)
+void mptcp_finish_connect(struct sock *ssk)
{
struct mptcp_subflow_context *subflow;
- struct mptcp_sock *msk = mptcp_sk(sk);
+ struct sock *sk;
- subflow = mptcp_subflow_ctx(msk->subflow->sk);
+ subflow = mptcp_subflow_ctx(ssk);
+ sk = subflow->conn;
- if (mp_capable) {
- struct sock *ssk = msk->subflow->sk;
+ if (subflow->mp_capable) {
+ struct mptcp_sock *msk = mptcp_sk(sk);
u64 ack_seq;
/* sk (new subflow socket) is already locked, but we need
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 7f559436fc9b..90af5a84f16c 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -289,7 +289,7 @@ int mptcp_read_actor(read_descriptor_t *desc, struct sk_buff *skb,
void mptcp_get_options(const struct sk_buff *skb,
struct tcp_options_received *opt_rx);
-void mptcp_finish_connect(struct sock *sk, int mp_capable);
+void mptcp_finish_connect(struct sock *sk);
bool mptcp_finish_join(struct sock *sk);
void mptcp_data_acked(struct sock *sk);
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index edec2ecbd93f..a89513210ea1 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -209,7 +209,7 @@ static void subflow_finish_connect(struct sock *sk, const struct
sk_buff *skb)
if (subflow->mp_capable && !subflow->conn_finished) {
pr_debug("subflow=%p, remote_key=%llu", mptcp_subflow_ctx(sk),
subflow->remote_key);
- mptcp_finish_connect(subflow->conn, subflow->mp_capable);
+ mptcp_finish_connect(sk);
subflow->conn_finished = 1;
if (skb) {
--
2.23.0
11:48 p.m.
New subject: [PATCH 8/9] subflow: place further subflows on new 'join_list'
When the pm worker queue establishes a new subflow in addition to an
existing one, we never place this socket on any list, ie. if such a request
would time out/never complete we can't clean it up.
Place them on a new join_list. When the connection completes, the
subflow is moved to the conn_list.
If it never completes, the socket gets closed at mptcp_shutdown time.
Not done yet:
1. extend mptcp worker to collect timed-out entries
2. prevent arbitrary growth of pending ssks.
Signed-off-by: Florian Westphal <fw(a)strlen.de>
---
net/mptcp/protocol.c | 14 +++++++++++++-
net/mptcp/protocol.h | 1 +
net/mptcp/subflow.c | 16 +++++++++++-----
3 files changed, 25 insertions(+), 6 deletions(-)
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 0c61487a587d..36097008dd4a 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -859,6 +859,7 @@ static int __mptcp_init_sock(struct sock *sk)
struct mptcp_sock *msk = mptcp_sk(sk);
INIT_LIST_HEAD(&msk->conn_list);
+ INIT_LIST_HEAD(&msk->join_list);
INIT_LIST_HEAD(&msk->rtx_queue);
INIT_WORK(&msk->rtx_work, mptcp_worker);
@@ -948,6 +949,12 @@ static void mptcp_close(struct sock *sk, long timeout)
msk->subflow = NULL;
}
+ list_for_each_entry_safe(subflow, tmp, &msk->join_list, node) {
+ struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
+
+ __mptcp_close_ssk(sk, ssk, subflow, timeout);
+ }
+
list_for_each_entry_safe(subflow, tmp, &msk->conn_list, node) {
struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
@@ -1223,7 +1230,7 @@ void mptcp_finish_connect(struct sock *ssk)
subflow->map_seq = ack_seq;
subflow->map_subflow_seq = 1;
subflow->rel_write_seq = 1;
- list_add(&subflow->node, &msk->conn_list);
+ list_move(&subflow->node, &msk->conn_list);
msk->subflow = NULL;
bh_unlock_sock(sk);
local_bh_enable();
@@ -1260,6 +1267,11 @@ bool mptcp_finish_join(struct sock *sk)
if (parent->sk_state != TCP_ESTABLISHED)
goto out;
+ /* unlink from join_list; caller must free ssk if
+ * we return false below.
+ */
+ list_del_init(&subflow->node);
+
parent_sock = READ_ONCE(parent->sk_socket);
if (parent_sock) {
if (!sk->sk_socket)
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 90af5a84f16c..990da61fa51b 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -137,6 +137,7 @@ struct mptcp_sock {
struct work_struct rtx_work;
struct list_head conn_list;
struct list_head rtx_queue;
+ struct list_head join_list;
struct socket *subflow; /* outgoing connect/listener/!mp_capable */
struct mptcp_pm_data pm;
u8 addr_signal;
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index a89513210ea1..8c13e9a145d5 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -700,6 +700,11 @@ int mptcp_subflow_connect(struct sock *sk, struct sockaddr *local,
int err;
lock_sock(sk);
+ if (sk->sk_state != TCP_ESTABLISHED) {
+ release_sock(sk);
+ return -ENOTCONN;
+ }
+
err = mptcp_subflow_create_socket(sk, &sf);
if (err) {
release_sock(sk);
@@ -711,9 +716,6 @@ int mptcp_subflow_connect(struct sock *sk, struct sockaddr *local,
subflow->local_key = msk->local_key;
subflow->token = msk->token;
- sock_hold(sf->sk);
- release_sock(sk);
-
addrlen = sizeof(struct sockaddr_in);
#if IS_ENABLED(CONFIG_MPTCP_IPV6)
if (local->sa_family == AF_INET6)
@@ -730,15 +732,18 @@ int mptcp_subflow_connect(struct sock *sk, struct sockaddr *local,
subflow->request_join = 1;
subflow->request_bkup = 1;
+ list_add(&subflow->node, &msk->join_list);
+
err = kernel_connect(sf, remote, addrlen, O_NONBLOCK);
if (err && err != -EINPROGRESS)
goto failed;
- sock_put(sf->sk);
+ release_sock(sk);
return err;
failed:
- sock_put(sf->sk);
+ list_del_init(&subflow->node);
+ release_sock(sk);
sock_release(sf);
return err;
}
@@ -782,6 +787,7 @@ static struct mptcp_subflow_context *subflow_create_ctx(struct sock
*sk,
if (!ctx)
return NULL;
rcu_assign_pointer(icsk->icsk_ulp_data, ctx);
+ INIT_LIST_HEAD(&ctx->node);
pr_debug("subflow=%p", ctx);
--
2.23.0
1:35 p.m.
New subject: [PATCH 8/9] subflow: place further subflows on new 'join_list'
On Sat, 2019-11-30 at 23:48 +0100, Florian Westphal wrote:
When the pm worker queue establishes a new subflow in addition to an
existing one, we never place this socket on any list, ie. if such a request
would time out/never complete we can't clean it up.
Place them on a new join_list. When the connection completes, the
subflow is moved to the conn_list.
If it never completes, the socket gets closed at mptcp_shutdown time.
Not done yet:
1. extend mptcp worker to collect timed-out entries
2. prevent arbitrary growth of pending ssks.
Uhm... am I correct, the above will always happen if the client send
[multiple] MP_JOIN at synack reception time? what can we do to prevent
that? scheduling a cleanup workqueue on error?
Signed-off-by: Florian Westphal <fw(a)strlen.de>
---
net/mptcp/protocol.c | 14 +++++++++++++-
net/mptcp/protocol.h | 1 +
net/mptcp/subflow.c | 16 +++++++++++-----
3 files changed, 25 insertions(+), 6 deletions(-)
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 0c61487a587d..36097008dd4a 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -859,6 +859,7 @@ static int __mptcp_init_sock(struct sock *sk)
struct mptcp_sock *msk = mptcp_sk(sk);
INIT_LIST_HEAD(&msk->conn_list);
+ INIT_LIST_HEAD(&msk->join_list);
INIT_LIST_HEAD(&msk->rtx_queue);
INIT_WORK(&msk->rtx_work, mptcp_worker);
@@ -948,6 +949,12 @@ static void mptcp_close(struct sock *sk, long timeout)
msk->subflow = NULL;
}
+ list_for_each_entry_safe(subflow, tmp, &msk->join_list, node) {
+ struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
+
+ __mptcp_close_ssk(sk, ssk, subflow, timeout);
+ }
+
list_for_each_entry_safe(subflow, tmp, &msk->conn_list, node) {
struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
@@ -1223,7 +1230,7 @@ void mptcp_finish_connect(struct sock *ssk)
subflow->map_seq = ack_seq;
subflow->map_subflow_seq = 1;
subflow->rel_write_seq = 1;
- list_add(&subflow->node, &msk->conn_list);
+ list_move(&subflow->node, &msk->conn_list);
msk->subflow = NULL;
bh_unlock_sock(sk);
local_bh_enable();
@@ -1260,6 +1267,11 @@ bool mptcp_finish_join(struct sock *sk)
if (parent->sk_state != TCP_ESTABLISHED)
goto out;
+ /* unlink from join_list; caller must free ssk if
+ * we return false below.
+ */
+ list_del_init(&subflow->node);
+
parent_sock = READ_ONCE(parent->sk_socket);
if (parent_sock) {
if (!sk->sk_socket)
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 90af5a84f16c..990da61fa51b 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -137,6 +137,7 @@ struct mptcp_sock {
struct work_struct rtx_work;
struct list_head conn_list;
struct list_head rtx_queue;
+ struct list_head join_list;
struct socket *subflow; /* outgoing connect/listener/!mp_capable */
struct mptcp_pm_data pm;
u8 addr_signal;
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index a89513210ea1..8c13e9a145d5 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -700,6 +700,11 @@ int mptcp_subflow_connect(struct sock *sk, struct sockaddr *local,
int err;
lock_sock(sk);
+ if (sk->sk_state != TCP_ESTABLISHED) {
+ release_sock(sk);
+ return -ENOTCONN;
+ }
+
err = mptcp_subflow_create_socket(sk, &sf);
if (err) {
release_sock(sk);
@@ -711,9 +716,6 @@ int mptcp_subflow_connect(struct sock *sk, struct sockaddr *local,
subflow->local_key = msk->local_key;
subflow->token = msk->token;
- sock_hold(sf->sk);
- release_sock(sk);
-
addrlen = sizeof(struct sockaddr_in);
#if IS_ENABLED(CONFIG_MPTCP_IPV6)
if (local->sa_family == AF_INET6)
@@ -730,15 +732,18 @@ int mptcp_subflow_connect(struct sock *sk, struct sockaddr *local,
subflow->request_join = 1;
subflow->request_bkup = 1;
+ list_add(&subflow->node, &msk->join_list);
+
err = kernel_connect(sf, remote, addrlen, O_NONBLOCK);
if (err && err != -EINPROGRESS)
goto failed;
- sock_put(sf->sk);
+ release_sock(sk);
return err;
failed:
- sock_put(sf->sk);
+ list_del_init(&subflow->node);
Can we release the subflow socket immediatelly here?
Thanks!
Paolo
5:56 p.m.
New subject: [PATCH 8/9] subflow: place further subflows on new 'join_list'
Paolo Abeni <pabeni(a)redhat.com> wrote:
> Not done yet:
> 1. extend mptcp worker to collect timed-out entries
> 2. prevent arbitrary growth of pending ssks.
Uhm... am I correct, the above will always happen if the client send
[multiple] MP_JOIN at synack reception time? what can we do to prevent
that? scheduling a cleanup workqueue on error?
We can't prevent it, client could flood us with hundreds of joins.
I think its best if we just add a upper ceiling on both
join and conn lists.
As for cleanup, I think we could have the rtx worker scan the
list for timed-out/"dead" joins.
11:49 p.m.
New subject: [PATCH 9/9] don't flag parent socket as RCV_SHUTDOWN if one one subflow has closed
instead schedule the mptcp worker and tell it to check eof state on all
subflows. If all have closed, then also flag the mptcp socket as closed
and wake up userspace process blocking in poll().
Signed-off-by: Florian Westphal <fw(a)strlen.de>
---
net/mptcp/protocol.c | 41 +++++++++++++++++++++++++++++++++--------
net/mptcp/protocol.h | 2 ++
net/mptcp/subflow.c | 22 ++++++++++++++++------
3 files changed, 51 insertions(+), 14 deletions(-)
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 36097008dd4a..c1734005e89e 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -61,6 +61,15 @@ void mptcp_data_acked(struct sock *sk)
sock_hold(sk);
}
+void mptcp_subflow_eof(struct sock *sk)
+{
+ struct mptcp_sock *msk = mptcp_sk(sk);
+
+ if (!test_and_set_bit(MPTCP_WORK_EOF, &msk->flags) &&
+ schedule_work(&msk->rtx_work))
+ sock_hold(sk);
+}
+
static void mptcp_stop_timer(struct sock *sk)
{
struct inet_connection_sock *icsk = inet_csk(sk);
@@ -95,22 +104,14 @@ static struct sock *mptcp_subflow_recv_lookup(const struct mptcp_sock
*msk)
{
struct sock *sk = (struct sock *)msk;
struct mptcp_subflow_context *subflow;
- int receivers = 0;
sock_owned_by_me(sk);
mptcp_for_each_subflow(msk, subflow) {
if (subflow->data_avail)
return mptcp_subflow_tcp_sock(subflow);
-
- receivers += !subflow->rx_eof;
}
- /* hopefully temporary hack: propagate shutdown status from subflow
- * to msk, when all subflows agree on it
- */
- if (!receivers && !(sk->sk_shutdown & RCV_SHUTDOWN))
- sk->sk_shutdown |= RCV_SHUTDOWN;
return NULL;
}
@@ -789,6 +790,27 @@ static void __mptcp_close_ssk(struct sock *sk, struct sock *ssk,
}
}
+static void mptcp_check_for_eof(struct mptcp_sock *msk)
+{
+ struct mptcp_subflow_context *subflow;
+ struct sock *sk = (struct sock *)msk;
+ int receivers = 0;
+
+ mptcp_for_each_subflow(msk, subflow)
+ receivers += !subflow->rx_eof;
+
+ if (!receivers && !(sk->sk_shutdown & RCV_SHUTDOWN)) {
+ /* hopefully temporary hack: propagate shutdown status
+ * to msk, when all subflows agree on it
+ */
+ sk->sk_shutdown |= RCV_SHUTDOWN;
+
+ smp_mb__before_atomic(); /* SHUTDOWN must be visible first */
+ set_bit(MPTCP_DATA_READY, &msk->flags);
+ sk->sk_data_ready(sk);
+ }
+}
+
static void mptcp_worker(struct work_struct *work)
{
int orig_len, orig_offset, ret, mss_now = 0, size_goal = 0;
@@ -806,6 +828,9 @@ static void mptcp_worker(struct work_struct *work)
lock_sock(sk);
mptcp_clean_una(sk);
+ if (test_and_clear_bit(MPTCP_WORK_EOF, &msk->flags))
+ mptcp_check_for_eof(msk);
+
if (!test_and_clear_bit(MPTCP_WORK_RTX, &msk->flags))
goto unlock;
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 990da61fa51b..cf3b4ce483d9 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -78,6 +78,7 @@
#define MPTCP_DATA_READY BIT(0)
#define MPTCP_WORK_RTX BIT(1)
#define MPTCP_SEND_SPACE BIT(2)
+#define MPTCP_WORK_EOF BIT(3)
static inline __be32 mptcp_option(u8 subopt, u8 len, u8 nib, u8 field)
{
@@ -291,6 +292,7 @@ void mptcp_get_options(const struct sk_buff *skb,
struct tcp_options_received *opt_rx);
void mptcp_finish_connect(struct sock *sk);
+void mptcp_subflow_eof(struct sock *sk);
bool mptcp_finish_join(struct sock *sk);
void mptcp_data_acked(struct sock *sk);
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 8c13e9a145d5..e63a95217b1f 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -621,6 +621,11 @@ static bool subflow_check_data_avail(struct sock *ssk)
return false;
}
+static bool subflow_is_done(const struct sock *sk)
+{
+ return sk->sk_shutdown & RCV_SHUTDOWN || sk->sk_state == TCP_CLOSE;
+}
+
bool mptcp_subflow_data_available(struct sock *sk)
{
struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
@@ -642,7 +647,7 @@ bool mptcp_subflow_data_available(struct sock *sk)
/* set EoF only there is no data available - we already spooled
* all the pending skbs
*/
- if (sk->sk_shutdown & RCV_SHUTDOWN || sk->sk_state == TCP_CLOSE)
+ if (subflow_is_done(sk))
subflow->rx_eof = 1;
return false;
}
@@ -666,13 +671,15 @@ static void subflow_data_ready(struct sock *sk)
return;
}
- /* always propagate the EoF */
- if (mptcp_subflow_data_available(sk) || subflow->rx_eof) {
+ if (mptcp_subflow_data_available(sk)) {
smp_mb__before_atomic();
set_bit(MPTCP_DATA_READY, &mptcp_sk(parent)->flags);
smp_mb__after_atomic();
parent->sk_data_ready(parent);
+ } else if (subflow->rx_eof) {
+ /* tell parent to (re-)check eof on all subflows */
+ mptcp_subflow_eof(parent);
}
}
@@ -810,12 +817,15 @@ static void __subflow_state_change(struct sock *sk)
static void subflow_state_change(struct sock *sk)
{
struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
- struct sock *parent = subflow->conn;
+ struct sock *parent = READ_ONCE(subflow->conn);
__subflow_state_change(sk);
- if (parent)
- __subflow_state_change(parent);
+ if (parent && !(parent->sk_shutdown & RCV_SHUTDOWN) &&
+ !subflow->rx_eof && subflow_is_done(sk)) {
+ subflow->rx_eof = 1;
+ mptcp_subflow_eof(parent);
+ }
}
static int subflow_ulp_init(struct sock *sk)
--
2.23.0
1:43 p.m.
New subject: [PATCH 9/9] don't flag parent socket as RCV_SHUTDOWN if one one subflow has closed
On Sat, 2019-11-30 at 23:49 +0100, Florian Westphal wrote:
@@ -642,7 +647,7 @@ bool mptcp_subflow_data_available(struct sock
*sk)
/* set EoF only there is no data available - we already spooled
* all the pending skbs
*/
- if (sk->sk_shutdown & RCV_SHUTDOWN || sk->sk_state == TCP_CLOSE)
+ if (subflow_is_done(sk))
subflow->rx_eof = 1;
return false;
}
@@ -666,13 +671,15 @@ static void subflow_data_ready(struct sock *sk)
return;
}
- /* always propagate the EoF */
- if (mptcp_subflow_data_available(sk) || subflow->rx_eof) {
+ if (mptcp_subflow_data_available(sk)) {
smp_mb__before_atomic();
set_bit(MPTCP_DATA_READY, &mptcp_sk(parent)->flags);
smp_mb__after_atomic();
parent->sk_data_ready(parent);
+ } else if (subflow->rx_eof) {
+ /* tell parent to (re-)check eof on all subflows */
+ mptcp_subflow_eof(parent);
}
}
With subflow_state_change() in place, do we still need the above
chunks?!? e.g. why is not subflow_state_change() enough to catch the
subflow eof condition?
Thanks!
Paolo
7 p.m.
New subject: [PATCH 9/9] don't flag parent socket as RCV_SHUTDOWN if one one subflow has closed
Paolo Abeni <pabeni(a)redhat.com> wrote:
On Sat, 2019-11-30 at 23:49 +0100, Florian Westphal wrote:
> @@ -642,7 +647,7 @@ bool mptcp_subflow_data_available(struct sock *sk)
> /* set EoF only there is no data available - we already spooled
> * all the pending skbs
> */
> - if (sk->sk_shutdown & RCV_SHUTDOWN || sk->sk_state == TCP_CLOSE)
> + if (subflow_is_done(sk))
> subflow->rx_eof = 1;
> return false;
> }
> @@ -666,13 +671,15 @@ static void subflow_data_ready(struct sock *sk)
> return;
> }
>
> - /* always propagate the EoF */
> - if (mptcp_subflow_data_available(sk) || subflow->rx_eof) {
> + if (mptcp_subflow_data_available(sk)) {
> smp_mb__before_atomic();
> set_bit(MPTCP_DATA_READY, &mptcp_sk(parent)->flags);
> smp_mb__after_atomic();
>
> parent->sk_data_ready(parent);
> + } else if (subflow->rx_eof) {
> + /* tell parent to (re-)check eof on all subflows */
> + mptcp_subflow_eof(parent);
> }
> }
>
With subflow_state_change() in place, do we still need the above
chunks?!? e.g. why is not subflow_state_change() enough to catch the
subflow eof condition?
Right, it might be possible to drop that indeed.
7:10 p.m.
On Sat, 2019-11-30 at 23:48 +0100, Florian Westphal wrote:
This set aims to improve handling of multiple subflows.
There are a few remaining problems, see below, but I wanted to send what
I have now rather than keep this back.
The first two patches are new, following few patches are a resend of the
earlier RFC series, I think they're good for acceptance now.
Patch 6 had a few changes since the RFC based on Paolos feedback, please
see patch description for details.
The last two patches are there to allow cancellation/teardown of
not-yet completed join requests. The connecting subflow flows
are placed on a 'join_list' so mptcp_shutdown can close these sockets
as well.
Last patch moves the 'eof hack' to a work queue so we don't set EOF on the
mptcp socket until all subflows have closed. Maybe this isn't needed, I
would be fine with dropping it in favor of mptcp-level DATA_FIN and
reset handling.
The rest of this work is to have msk->conn_list contain tcp_sock structs
without an outer 'struct socket'.
The problem is that for incoming join requests (which are not subject
to accept/request socket queue), there is no 'struct socket', so the kernel
will crash. Even when fixing those up, a NULL sk->sk_socket is problematic
as tcp stack will assume that such tcp sk is already detached from userspace,
i.e. NOSPACE wakeups do not work for them.
The solution here (suggested by Paolo) is to have sk->sk_socket refer
to the mptcp parent socket structure. This assignment is done by
a private function similar to sock_graft().
mptcp.org kernel will be able to establish multiple connections per
mptcp socket but does not consider those extra joins as established, as the
"4th ack" is missing.
A simple "tcp_send_ack" is enough, but there are two problems with this:
1. It can be lost
2. Given we are not expecting to receive any data on such flows this
doesn't seem to be a problem.
During testing I saw that sometimes mptcp.org kernel will emit further
join requests to mptcp-next but those trigger no response (and those
packets do not even show up in perfs network drop monitor, sigh).
I also saw a UAF at mptcp_close where a sock_hold() in tcp_close()
caused a 0->1 recount transition, but i've been unable to reproduce it so far
(might have been caused by a bug that has been fixed in the mean time).
The following changes since commit dd25ff04840135581b833762571aac08a2a94c6c:
sendmsg: truncate source buffer if mptcp sndbuf size was set from userspace (2019-11-30
02:47:59 +0000)
are available in the Git repository at:
git://git.breakpoint.cc/fw/mptcp-next.git mptcp_multijoin_11
for you to fetch changes up to 010c2e19c2d643a87c991bf1e7d4bfc140dfdba5:
don't flag parent socket as RCV_SHUTDOWN if one one subflow has closed (2019-11-30
23:21:25 +0100)
----------------------------------------------------------------
Florian Westphal (9):
token: handle join-before-accept-completion case
mptcp: fix race from mptcp_close/mptcp join
copy connection id from first subflow to mptcp socket
store tcp_sk, not socket
add mptcp_subflow_shutdown
make accept not allocate kernel socket struct
pass subflow socket to mptcp_finish_connect
subflow: place further subflows on new 'join_list'
don't flag parent socket as RCV_SHUTDOWN if one one subflow has closed
net/mptcp/pm.c | 18 +--
net/mptcp/protocol.c | 313 +++++++++++++++++++++++++++++++++++++--------------
net/mptcp/protocol.h | 16 +--
net/mptcp/subflow.c | 56 +++++----
net/mptcp/token.c | 9 +-
5 files changed, 292 insertions(+), 120 deletions(-)
As per mtg discussion, the series LGTM. There is a small change
possible on patch 9/9, but I think it should not block the series. We
can either merge up to 8/9 and have the last one later, or even merge
as-is and refine the code touched by the last patch later.
Cheers,
Paolo
2:23 a.m.
On Mon, 2 Dec 2019, Paolo Abeni wrote:
On Sat, 2019-11-30 at 23:48 +0100, Florian Westphal wrote:
> This set aims to improve handling of multiple subflows.
>
> There are a few remaining problems, see below, but I wanted to send what
> I have now rather than keep this back.
>
> The first two patches are new, following few patches are a resend of the
> earlier RFC series, I think they're good for acceptance now.
> Patch 6 had a few changes since the RFC based on Paolos feedback, please
> see patch description for details.
>
> The last two patches are there to allow cancellation/teardown of
> not-yet completed join requests. The connecting subflow flows
> are placed on a 'join_list' so mptcp_shutdown can close these sockets
> as well.
>
> Last patch moves the 'eof hack' to a work queue so we don't set EOF on
the
> mptcp socket until all subflows have closed. Maybe this isn't needed, I
> would be fine with dropping it in favor of mptcp-level DATA_FIN and
> reset handling.
I'll keep this in mind with the DATA_FIN code. For now, I think it makes
sense move the eof code.
>
> The rest of this work is to have msk->conn_list contain tcp_sock structs
> without an outer 'struct socket'.
>
> The problem is that for incoming join requests (which are not subject
> to accept/request socket queue), there is no 'struct socket', so the kernel
> will crash. Even when fixing those up, a NULL sk->sk_socket is problematic
> as tcp stack will assume that such tcp sk is already detached from userspace,
> i.e. NOSPACE wakeups do not work for them.
>
> The solution here (suggested by Paolo) is to have sk->sk_socket refer
> to the mptcp parent socket structure. This assignment is done by
> a private function similar to sock_graft().
>
> mptcp.org kernel will be able to establish multiple connections per
> mptcp socket but does not consider those extra joins as established, as the
> "4th ack" is missing.
>
> A simple "tcp_send_ack" is enough, but there are two problems with this:
> 1. It can be lost
> 2. Given we are not expecting to receive any data on such flows this
> doesn't seem to be a problem.
>
> During testing I saw that sometimes mptcp.org kernel will emit further
> join requests to mptcp-next but those trigger no response (and those
> packets do not even show up in perfs network drop monitor, sigh).
>
> I also saw a UAF at mptcp_close where a sock_hold() in tcp_close()
> caused a 0->1 recount transition, but i've been unable to reproduce it so
far
> (might have been caused by a bug that has been fixed in the mean time).
>
> The following changes since commit dd25ff04840135581b833762571aac08a2a94c6c:
>
> sendmsg: truncate source buffer if mptcp sndbuf size was set from userspace
(2019-11-30 02:47:59 +0000)
>
> are available in the Git repository at:
>
> git://git.breakpoint.cc/fw/mptcp-next.git mptcp_multijoin_11
>
> for you to fetch changes up to 010c2e19c2d643a87c991bf1e7d4bfc140dfdba5:
>
> don't flag parent socket as RCV_SHUTDOWN if one one subflow has closed
(2019-11-30 23:21:25 +0100)
>
> ----------------------------------------------------------------
> Florian Westphal (9):
> token: handle join-before-accept-completion case
> mptcp: fix race from mptcp_close/mptcp join
> copy connection id from first subflow to mptcp socket
> store tcp_sk, not socket
> add mptcp_subflow_shutdown
> make accept not allocate kernel socket struct
> pass subflow socket to mptcp_finish_connect
> subflow: place further subflows on new 'join_list'
> don't flag parent socket as RCV_SHUTDOWN if one one subflow has closed
>
> net/mptcp/pm.c | 18 +--
> net/mptcp/protocol.c | 313 +++++++++++++++++++++++++++++++++++++--------------
> net/mptcp/protocol.h | 16 +--
> net/mptcp/subflow.c | 56 +++++----
> net/mptcp/token.c | 9 +-
> 5 files changed, 292 insertions(+), 120 deletions(-)
As per mtg discussion, the series LGTM. There is a small change
possible on patch 9/9, but I think it should not block the series. We
can either merge up to 8/9 and have the last one later, or even merge
as-is and refine the code touched by the last patch later.
Cheers,
Paolo
I also reviewed the series today, and don't have further changes to
suggest. Patches look ready to apply, thanks Florian.
--
Mat Martineau
Intel
8:10 p.m.
New subject: [PATCH v2 9/9] don't flag parent socket as RCV_SHUTDOWN if one one subflow has closed
instead schedule the mptcp worker and tell it to check eof state on all
subflows. If all have closed, then also flag the mptcp socket as closed
and wake up userspace process blocking in poll().
v2: move eof checks to subflow_state_change() (Paolo)
Signed-off-by: Florian Westphal <fw(a)strlen.de>
---
net/mptcp/protocol.c | 41 +++++++++++++++++++++++++++++++++--------
net/mptcp/protocol.h | 2 ++
net/mptcp/subflow.c | 22 ++++++++++++----------
3 files changed, 47 insertions(+), 18 deletions(-)
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 36097008dd4a..feb4658b13a8 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -61,6 +61,15 @@ void mptcp_data_acked(struct sock *sk)
sock_hold(sk);
}
+void mptcp_subflow_eof(struct sock *sk)
+{
+ struct mptcp_sock *msk = mptcp_sk(sk);
+
+ if (!test_and_set_bit(MPTCP_WORK_EOF, &msk->flags) &&
+ schedule_work(&msk->rtx_work))
+ sock_hold(sk);
+}
+
static void mptcp_stop_timer(struct sock *sk)
{
struct inet_connection_sock *icsk = inet_csk(sk);
@@ -95,22 +104,14 @@ static struct sock *mptcp_subflow_recv_lookup(const struct mptcp_sock
*msk)
{
struct sock *sk = (struct sock *)msk;
struct mptcp_subflow_context *subflow;
- int receivers = 0;
sock_owned_by_me(sk);
mptcp_for_each_subflow(msk, subflow) {
if (subflow->data_avail)
return mptcp_subflow_tcp_sock(subflow);
-
- receivers += !subflow->rx_eof;
}
- /* hopefully temporary hack: propagate shutdown status from subflow
- * to msk, when all subflows agree on it
- */
- if (!receivers && !(sk->sk_shutdown & RCV_SHUTDOWN))
- sk->sk_shutdown |= RCV_SHUTDOWN;
return NULL;
}
@@ -789,6 +790,27 @@ static void __mptcp_close_ssk(struct sock *sk, struct sock *ssk,
}
}
+static void mptcp_check_for_eof(struct mptcp_sock *msk)
+{
+ struct mptcp_subflow_context *subflow;
+ struct sock *sk = (struct sock *)msk;
+ int receivers = 0;
+
+ mptcp_for_each_subflow(msk, subflow)
+ receivers += !subflow->rx_eof;
+
+ if (!receivers && !(sk->sk_shutdown & RCV_SHUTDOWN)) {
+ /* hopefully temporary hack: propagate shutdown status
+ * to msk, when all subflows agree on it
+ */
+ sk->sk_shutdown |= RCV_SHUTDOWN;
+
+ smp_mb__before_atomic(); /* SHUTDOWN must be visible first */
+ set_bit(MPTCP_DATA_READY, &msk->flags);
+ sk->sk_data_ready(sk);
+ }
+}
+
static void mptcp_worker(struct work_struct *work)
{
int orig_len, orig_offset, ret, mss_now = 0, size_goal = 0;
@@ -806,6 +828,9 @@ static void mptcp_worker(struct work_struct *work)
lock_sock(sk);
mptcp_clean_una(sk);
+ if (test_and_clear_bit(MPTCP_WORK_EOF, &msk->flags))
+ mptcp_check_for_eof(msk);
+
if (!test_and_clear_bit(MPTCP_WORK_RTX, &msk->flags))
goto unlock;
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 990da61fa51b..cf3b4ce483d9 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -78,6 +78,7 @@
#define MPTCP_DATA_READY BIT(0)
#define MPTCP_WORK_RTX BIT(1)
#define MPTCP_SEND_SPACE BIT(2)
+#define MPTCP_WORK_EOF BIT(3)
static inline __be32 mptcp_option(u8 subopt, u8 len, u8 nib, u8 field)
{
@@ -291,6 +292,7 @@ void mptcp_get_options(const struct sk_buff *skb,
struct tcp_options_received *opt_rx);
void mptcp_finish_connect(struct sock *sk);
+void mptcp_subflow_eof(struct sock *sk);
bool mptcp_finish_join(struct sock *sk);
void mptcp_data_acked(struct sock *sk);
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 8c13e9a145d5..fe6c61367bd7 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -639,11 +639,6 @@ bool mptcp_subflow_data_available(struct sock *sk)
if (!subflow_check_data_avail(sk)) {
subflow->data_avail = 0;
- /* set EoF only there is no data available - we already spooled
- * all the pending skbs
- */
- if (sk->sk_shutdown & RCV_SHUTDOWN || sk->sk_state == TCP_CLOSE)
- subflow->rx_eof = 1;
return false;
}
@@ -666,8 +661,7 @@ static void subflow_data_ready(struct sock *sk)
return;
}
- /* always propagate the EoF */
- if (mptcp_subflow_data_available(sk) || subflow->rx_eof) {
+ if (mptcp_subflow_data_available(sk)) {
smp_mb__before_atomic();
set_bit(MPTCP_DATA_READY, &mptcp_sk(parent)->flags);
smp_mb__after_atomic();
@@ -807,15 +801,23 @@ static void __subflow_state_change(struct sock *sk)
rcu_read_unlock();
}
+static bool subflow_is_done(const struct sock *sk)
+{
+ return sk->sk_shutdown & RCV_SHUTDOWN || sk->sk_state == TCP_CLOSE;
+}
+
static void subflow_state_change(struct sock *sk)
{
struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
- struct sock *parent = subflow->conn;
+ struct sock *parent = READ_ONCE(subflow->conn);
__subflow_state_change(sk);
- if (parent)
- __subflow_state_change(parent);
+ if (parent && !(parent->sk_shutdown & RCV_SHUTDOWN) &&
+ !subflow->rx_eof && subflow_is_done(sk)) {
+ subflow->rx_eof = 1;
+ mptcp_subflow_eof(parent);
+ }
}
static int subflow_ulp_init(struct sock *sk)
--
2.23.0
10:21 p.m.
New subject: [PATCH v2 9/9] don't flag parent socket as RCV_SHUTDOWN if one one subflow has closed
On Mon, 2019-12-02 at 20:10 +0100, Florian Westphal wrote:
instead schedule the mptcp worker and tell it to check eof state on
all
subflows. If all have closed, then also flag the mptcp socket as closed
and wake up userspace process blocking in poll().
v2: move eof checks to subflow_state_change() (Paolo)
Signed-off-by: Florian Westphal <fw(a)strlen.de>
---
net/mptcp/protocol.c | 41 +++++++++++++++++++++++++++++++++--------
net/mptcp/protocol.h | 2 ++
net/mptcp/subflow.c | 22 ++++++++++++----------
3 files changed, 47 insertions(+), 18 deletions(-)
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 36097008dd4a..feb4658b13a8 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -61,6 +61,15 @@ void mptcp_data_acked(struct sock *sk)
sock_hold(sk);
}
+void mptcp_subflow_eof(struct sock *sk)
+{
+ struct mptcp_sock *msk = mptcp_sk(sk);
+
+ if (!test_and_set_bit(MPTCP_WORK_EOF, &msk->flags) &&
+ schedule_work(&msk->rtx_work))
+ sock_hold(sk);
+}
+
static void mptcp_stop_timer(struct sock *sk)
{
struct inet_connection_sock *icsk = inet_csk(sk);
@@ -95,22 +104,14 @@ static struct sock *mptcp_subflow_recv_lookup(const struct
mptcp_sock *msk)
{
struct sock *sk = (struct sock *)msk;
struct mptcp_subflow_context *subflow;
- int receivers = 0;
sock_owned_by_me(sk);
mptcp_for_each_subflow(msk, subflow) {
if (subflow->data_avail)
return mptcp_subflow_tcp_sock(subflow);
-
- receivers += !subflow->rx_eof;
}
- /* hopefully temporary hack: propagate shutdown status from subflow
- * to msk, when all subflows agree on it
- */
- if (!receivers && !(sk->sk_shutdown & RCV_SHUTDOWN))
- sk->sk_shutdown |= RCV_SHUTDOWN;
return NULL;
}
@@ -789,6 +790,27 @@ static void __mptcp_close_ssk(struct sock *sk, struct sock *ssk,
}
}
+static void mptcp_check_for_eof(struct mptcp_sock *msk)
+{
+ struct mptcp_subflow_context *subflow;
+ struct sock *sk = (struct sock *)msk;
+ int receivers = 0;
+
+ mptcp_for_each_subflow(msk, subflow)
+ receivers += !subflow->rx_eof;
+
+ if (!receivers && !(sk->sk_shutdown & RCV_SHUTDOWN)) {
+ /* hopefully temporary hack: propagate shutdown status
+ * to msk, when all subflows agree on it
+ */
+ sk->sk_shutdown |= RCV_SHUTDOWN;
+
+ smp_mb__before_atomic(); /* SHUTDOWN must be visible first */
+ set_bit(MPTCP_DATA_READY, &msk->flags);
+ sk->sk_data_ready(sk);
+ }
+}
+
static void mptcp_worker(struct work_struct *work)
{
int orig_len, orig_offset, ret, mss_now = 0, size_goal = 0;
@@ -806,6 +828,9 @@ static void mptcp_worker(struct work_struct *work)
lock_sock(sk);
mptcp_clean_una(sk);
+ if (test_and_clear_bit(MPTCP_WORK_EOF, &msk->flags))
+ mptcp_check_for_eof(msk);
+
if (!test_and_clear_bit(MPTCP_WORK_RTX, &msk->flags))
goto unlock;
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 990da61fa51b..cf3b4ce483d9 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -78,6 +78,7 @@
#define MPTCP_DATA_READY BIT(0)
#define MPTCP_WORK_RTX BIT(1)
#define MPTCP_SEND_SPACE BIT(2)
+#define MPTCP_WORK_EOF BIT(3)
static inline __be32 mptcp_option(u8 subopt, u8 len, u8 nib, u8 field)
{
@@ -291,6 +292,7 @@ void mptcp_get_options(const struct sk_buff *skb,
struct tcp_options_received *opt_rx);
void mptcp_finish_connect(struct sock *sk);
+void mptcp_subflow_eof(struct sock *sk);
bool mptcp_finish_join(struct sock *sk);
void mptcp_data_acked(struct sock *sk);
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 8c13e9a145d5..fe6c61367bd7 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -639,11 +639,6 @@ bool mptcp_subflow_data_available(struct sock *sk)
if (!subflow_check_data_avail(sk)) {
subflow->data_avail = 0;
- /* set EoF only there is no data available - we already spooled
- * all the pending skbs
- */
- if (sk->sk_shutdown & RCV_SHUTDOWN || sk->sk_state == TCP_CLOSE)
- subflow->rx_eof = 1;
return false;
}
@@ -666,8 +661,7 @@ static void subflow_data_ready(struct sock *sk)
return;
}
- /* always propagate the EoF */
- if (mptcp_subflow_data_available(sk) || subflow->rx_eof) {
+ if (mptcp_subflow_data_available(sk)) {
smp_mb__before_atomic();
set_bit(MPTCP_DATA_READY, &mptcp_sk(parent)->flags);
smp_mb__after_atomic();
@@ -807,15 +801,23 @@ static void __subflow_state_change(struct sock *sk)
rcu_read_unlock();
}
+static bool subflow_is_done(const struct sock *sk)
+{
+ return sk->sk_shutdown & RCV_SHUTDOWN || sk->sk_state == TCP_CLOSE;
+}
+
static void subflow_state_change(struct sock *sk)
{
struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
- struct sock *parent = subflow->conn;
+ struct sock *parent = READ_ONCE(subflow->conn);
__subflow_state_change(sk);
- if (parent)
- __subflow_state_change(parent);
+ if (parent && !(parent->sk_shutdown & RCV_SHUTDOWN) &&
+ !subflow->rx_eof && subflow_is_done(sk)) {
+ subflow->rx_eof = 1;
+ mptcp_subflow_eof(parent);
+ }
}
static int subflow_ulp_init(struct sock *sk)
Thanks for addressing my comments.
With 9/9 v2 the series is IMHO ready to me merged.
Thanks,
Paolo
8:44 a.m.
New subject: [PATCH v2 9/9] don't flag parent socket as RCV_SHUTDOWN if one one subflow has closed
Hi Florian, Paolo, Mat,
On 02/12/2019 22:21, Paolo Abeni wrote:
On Mon, 2019-12-02 at 20:10 +0100, Florian Westphal wrote:
[...]
Thanks for addressing my comments.
With 9/9 v2 the series is IMHO ready to me merged.
Thank you for the nice patch series and the reviews!
Related to what we discussed yesterday, is it OK if I merge this after:
1) DATA_FIN
2) MPTCPv1 → wait a bit for a review
3) SendMsg "refactor" → if reviewed
Or can I do that after the patches for MPTCPv1 if the patches to fix
sendmsg() have not been reviewed yet?
Also just to be sure: do I squash the 2 first ones and then I add the
rest at the end of the series?
Cheers,
Matt
--
Matthieu Baerts | R&D Engineer
matthieu.baerts(a)tessares.net
Tessares SA | Hybrid Access Solutions
www.tessares.net
1 Avenue Jean Monnet, 1348 Louvain-la-Neuve, Belgium
10:05 a.m.
New subject: [PATCH v2 9/9] don't flag parent socket as RCV_SHUTDOWN if one one subflow has closed
On Tue, 2019-12-03 at 08:44 +0100, Matthieu Baerts wrote:
Hi Florian, Paolo, Mat,
On 02/12/2019 22:21, Paolo Abeni wrote:
> On Mon, 2019-12-02 at 20:10 +0100, Florian Westphal wrote:
[...]
> Thanks for addressing my comments.
>
> With 9/9 v2 the series is IMHO ready to me merged.
Thank you for the nice patch series and the reviews!
Related to what we discussed yesterday, is it OK if I merge this after:
1) DATA_FIN
2) MPTCPv1 → wait a bit for a review
3) SendMsg "refactor" → if reviewed
Or can I do that after the patches for MPTCPv1 if the patches to fix
sendmsg() have not been reviewed yet?
I think/hope this and sendmsg patches should not conflict much - but
sendmsg patches do create quite of conflict with the following patches.
I think that appling this series after v1 could be a good option, will
give extra time to review the sendmsg() patches, and I can rebase my
tree to help the merging.
Thanks,
Paolo
10:13 a.m.
New subject: [PATCH v2 9/9] don't flag parent socket as RCV_SHUTDOWN if one one subflow has closed
Hi Paolo,
On 03/12/2019 10:05, Paolo Abeni wrote:
On Tue, 2019-12-03 at 08:44 +0100, Matthieu Baerts wrote:
> Hi Florian, Paolo, Mat,
>
> On 02/12/2019 22:21, Paolo Abeni wrote:
>> On Mon, 2019-12-02 at 20:10 +0100, Florian Westphal wrote:
>
> [...]
>
>> Thanks for addressing my comments.
>>
>> With 9/9 v2 the series is IMHO ready to me merged.
>
> Thank you for the nice patch series and the reviews!
> Related to what we discussed yesterday, is it OK if I merge this after:
>
> 1) DATA_FIN
> 2) MPTCPv1 → wait a bit for a review
> 3) SendMsg "refactor" → if reviewed
>
> Or can I do that after the patches for MPTCPv1 if the patches to fix
> sendmsg() have not been reviewed yet?
I think/hope this and sendmsg patches should not conflict much - but
sendmsg patches do create quite of conflict with the following patches.
I think that appling this series after v1 could be a good option, will
give extra time to review the sendmsg() patches, and I can rebase my
tree to help the merging.
Thank you for your reply!
Sounds good to me. If there is nothing to change in sendmsg() patches
after the review, I can also start applying them. If I am blocked, I can
ask for help.
Cheers,
Matt
--
Matthieu Baerts | R&D Engineer
matthieu.baerts(a)tessares.net
Tessares SA | Hybrid Access Solutions
www.tessares.net
1 Avenue Jean Monnet, 1348 Louvain-la-Neuve, Belgium
4:52 p.m.
New subject: [PATCH v2 9/9] don't flag parent socket as RCV_SHUTDOWN if one one subflow has closed
Hi Florian, Paolo, Mat,
On 03/12/2019 10:13, Matthieu Baerts wrote:
Hi Paolo,
On 03/12/2019 10:05, Paolo Abeni wrote:
> On Tue, 2019-12-03 at 08:44 +0100, Matthieu Baerts wrote:
>> Hi Florian, Paolo, Mat,
>>
>> On 02/12/2019 22:21, Paolo Abeni wrote:
>>> On Mon, 2019-12-02 at 20:10 +0100, Florian Westphal wrote:
>>
>> [...]
>>
>>> Thanks for addressing my comments.
>>>
>>> With 9/9 v2 the series is IMHO ready to me merged.
>>
>> Thank you for the nice patch series and the reviews!
>> Related to what we discussed yesterday, is it OK if I merge this after:
>>
>> 1) DATA_FIN
>> 2) MPTCPv1 → wait a bit for a review
>> 3) SendMsg "refactor" → if reviewed
>>
>> Or can I do that after the patches for MPTCPv1 if the patches to fix
>> sendmsg() have not been reviewed yet?
>
> I think/hope this and sendmsg patches should not conflict much - but
> sendmsg patches do create quite of conflict with the following patches.
>
> I think that appling this series after v1 could be a good option, will
> give extra time to review the sendmsg() patches, and I can rebase my
> tree to help the merging.
Thank you for your reply!
Sounds good to me. If there is nothing to change in sendmsg() patches
after the review, I can also start applying them. If I am blocked, I can
ask for help.
Thank you again for the patches and the reviews!
- 192c07c3fe9b: "squashed" patch 1/9 in "mptcp: Add handling of incoming
MP_JOIN requests"
- d6c1939a16c5..2cc726803b47: result
- 0c24eaeadf60: "squashed" patch 2/9, part 1, in "mptcp: Add handling of
incoming MP_JOIN requests"
- f2a1eeb27eab: "squashed" patch 2/9, part 2, in "mptcp: Add handling of
outgoing MP_JOIN requests"
- 216b0b82f102: conflict in t/mptcp-introduce-MPTCP-retransmission-timer
- e1012595cc3d: conflict in t/mptcp-add-and-use-mptcp-RTX-flag
- 2cc726803b47..48f66afb6a81: result
- the other patches have been added at the end. I had some conflicts with:
- "copy connection id from first subflow to mptcp socket"
- "make accept not allocate kernel socket struct"
- "subflow: place further subflows on new 'join_list'"
Tests on top of the commit introducing the kselftests are OK but not the
one at the end of the series (export branch). Here is the trace (without
CONFIG_KASAN nor PROVE_LOCKING)
00:07:55.647 # INFO: set ns3-5dea74bc-S7alT1 dev ns3eth2: ethtool -K tso
off gro off
00:07:55.658 # INFO: set ns4-5dea74bc-S7alT1 dev ns4eth3: ethtool -K
gro off
00:07:55.685 # Created /tmp/tmp.FSDFbTHqEa (size 3070486) containing
data sent by client
00:07:55.694 # Created /tmp/tmp.klmXQh5uiX (size 375225) containing data
sent by server
00:07:55.742 # New MPTCP socket can be blocked via sysctl [ OK ]
00:07:55.763 # setsockopt(..., TCP_ULP, "mptcp", ...) blocked [ OK ]
00:07:55.770 # INFO: validating network environment with pings
00:07:56.500 [ 11.837548] IPv6: ADDRCONF(NETDEV_CHANGE): ns2eth1: link
becomes ready
00:07:58.413 # INFO: Using loss of 0.10% delay 87 ms reorder 96% 42% on
ns3eth4
00:07:58.420 # ns1 MPTCP -> ns1 (10.0.1.1:10000 ) MPTCP (duration
19ms) [ OK ]
00:07:58.471 # ns1 MPTCP -> ns1 (10.0.1.1:10001 ) TCP (duration
18ms) [ OK ]
00:07:58.518 # ns1 TCP -> ns1 (10.0.1.1:10002 ) MPTCP (duration
18ms) [ OK ]
00:07:58.566 # ns1 MPTCP -> ns1 (dead:beef:1::1:10003) MPTCP (duration
17ms) [ OK ]
00:07:58.612 # ns1 MPTCP -> ns1 (dead:beef:1::1:10004) TCP (duration
18ms) [ OK ]
00:07:58.660 # ns1 TCP -> ns1 (dead:beef:1::1:10005) MPTCP [
14.024584] BUG: unable to handle page fault for address: 00000003435d11af
00:07:58.688 [ 14.025780] #PF: supervisor read access in kernel mode
00:07:58.689 [ 14.026588] #PF: error_code(0x0000) - not-present page
00:07:58.690 [ 14.027394] PGD 0 P4D 0
00:07:58.690 [ 14.027801] Oops: 0000 [#1] SMP PTI
00:07:58.691 [ 14.028355] CPU: 0 PID: 731 Comm: mptcp_connect Not
tainted 5.4.0+ #5
00:07:58.692 [ 14.029365] Hardware name: QEMU Standard PC (i440FX +
PIIX, 1996), BIOS 1.10.2-1ubuntu1 04/01/2014
00:07:58.693 [ 14.030752] RIP: 0010:mptcp_stream_accept+0x8d/0x160
00:07:58.694 [ 14.031533] Code: 8b 9c 24 b0 05 00 00 49 81 c4 b0 05 00
00 4c 39 e3 74 71 49 8d 76 40 48 89 74 24 08 eb 08 48 8b 1b 4c 39 e3 74
5e 48 8b 6b 78 <48> 83 bd 78 02 00 00 00 75 ea 4c 8d bd 28 02 00 00 89
44 24 04 4c
00:07:58.697 [ 14.034405] RSP: 0018:ffffad8d8044bdf0 EFLAGS: 00010283
00:07:58.698 [ 14.035218] RAX: 0000000000000000 RBX: ffff96f35d0d9928
RCX: 0000000000000001
00:07:58.699 [ 14.036317] RDX: 0000000000000001 RSI: ffff96f35eaffb40
RDI: ffffffffb6199b8a
00:07:58.700 [ 14.037423] RBP: 00000003435d0f37 R08: ffff96f35f42e690
R09: ffffad8d8044bdb0
00:07:58.701 [ 14.038528] R10: ffff96f35e375340 R11: ffff96f35d0d0740
R12: ffff96f35d0d9930
00:07:58.702 [ 14.039622] R13: ffff96f35eafe300 R14: ffff96f35eaffb00
R15: ffff96f35eaffb00
00:07:58.703 [ 14.040721] FS: 00007fad7081d500(0000)
GS:ffff96f35f400000(0000) knlGS:0000000000000000
00:07:58.704 [ 14.041973] CS: 0010 DS: 0000 ES: 0000 CR0:
0000000080050033
00:07:58.738 [ 14.042879] CR2: 00000003435d11af CR3: 000000001e3f4006
CR4: 00000000003606f0
00:07:58.738 [ 14.043987] DR0: 0000000000000000 DR1: 0000000000000000
DR2: 0000000000000000
00:07:58.738 [ 14.045098] DR3: 0000000000000000 DR6: 00000000fffe0ff0
DR7: 0000000000000400
00:07:58.738 [ 14.046204] Call Trace:
00:07:58.738 [ 14.046599] ? selinux_socket_accept+0x41/0x80
00:07:58.738 [ 14.047294] __sys_accept4_file+0xf7/0x1c0
00:07:58.738 [ 14.047937] ? mptcp_bind+0x38/0xa0
00:07:58.738 [ 14.048487] ? __inet_hash+0x120/0x2c0
00:07:58.738 [ 14.049084] ? kvm_clock_get_cycles+0xd/0x10
00:07:58.738 [ 14.049751] ? ktime_get_ts64+0x47/0xe0
00:07:58.738 [ 14.050351] __sys_accept4+0x3b/0x70
00:07:58.738 [ 14.050911] __x64_sys_accept+0x13/0x20
00:07:58.738 [ 14.051510] do_syscall_64+0x43/0x160
00:07:58.738 [ 14.052084] entry_SYSCALL_64_after_hwframe+0x44/0xa9
00:07:58.738 [ 14.052864] RIP: 0033:0x7fad7032e7e4
00:07:58.738 [ 14.053428] Code: 26 00 00 00 48 c7 c0 ff ff ff ff c3 66
2e 0f 1f 84 00 00 00 00 00 48 8d 05 21 e1 2c 00 8b 00 85 c0 75 13 b8 2b
00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 54 f3 c3 66 90 41 54 55 49 89 d4
53 48 89 f5
00:07:58.738 [ 14.056279] RSP: 002b:00007fffba32c8e8 EFLAGS: 00000246
ORIG_RAX: 000000000000002b
00:07:58.738 [ 14.057447] RAX: ffffffffffffffda RBX: 0000000000000005
RCX: 00007fad7032e7e4
00:07:58.738 [ 14.058544] RDX: 00007fffba32c900 RSI: 00007fffba32c910
RDI: 0000000000000005
00:07:58.738 [ 14.059640] RBP: 0000000000000005 R08: 0000000000000004
R09: 0000000000000000
00:07:58.738 [ 14.060742] R10: 00007fffba32cb24 R11: 0000000000000246
R12: 00007fffba32c910
00:07:58.738 [ 14.061863] R13: 000055e3b0610b4d R14: 00000000ffffffff
R15: 0000000000000000
00:07:58.738 [ 14.062974] Modules linked in:
00:07:58.738 [ 14.063459] CR2: 00000003435d11af
00:07:58.738 [ 14.063984] ---[ end trace 5eeeab694e375d46 ]---
00:07:58.738 [ 14.064707] RIP: 0010:mptcp_stream_accept+0x8d/0x160
00:07:58.738 [ 14.065487] Code: 8b 9c 24 b0 05 00 00 49 81 c4 b0 05 00
00 4c 39 e3 74 71 49 8d 76 40 48 89 74 24 08 eb 08 48 8b 1b 4c 39 e3 74
5e 48 8b 6b 78 <48> 83 bd 78 02 00 00 00 75 ea 4c 8d bd 28 02 00 00 89
44 24 04 4c
00:07:58.738 [ 14.068351] RSP: 0018:ffffad8d8044bdf0 EFLAGS: 00010283
00:07:58.738 [ 14.069169] RAX: 0000000000000000 RBX: ffff96f35d0d9928
RCX: 0000000000000001
00:07:58.738 [ 14.070274] RDX: 0000000000000001 RSI: ffff96f35eaffb40
RDI: ffffffffb6199b8a
00:07:58.738 [ 14.071378] RBP: 00000003435d0f37 R08: ffff96f35f42e690
R09: ffffad8d8044bdb0
00:07:58.738 [ 14.072478] R10: ffff96f35e375340 R11: ffff96f35d0d0740
R12: ffff96f35d0d9930
00:07:58.738 [ 14.073582] R13: ffff96f35eafe300 R14: ffff96f35eaffb00
R15: ffff96f35eaffb00
00:07:58.738 [ 14.074685] FS: 00007fad7081d500(0000)
GS:ffff96f35f400000(0000) knlGS:0000000000000000
00:07:58.738 [ 14.075912] CS: 0010 DS: 0000 ES: 0000 CR0:
0000000080050033
00:07:58.739 [ 14.076805] CR2: 00000003435d11af CR3: 000000001e3f4006
CR4: 00000000003606f0
00:07:58.740 [ 14.077853] DR0: 0000000000000000 DR1: 0000000000000000
DR2: 0000000000000000
00:07:58.741 [ 14.078927] DR3: 0000000000000000 DR6: 00000000fffe0ff0
DR7: 0000000000000400
00:08:28.978 copyfd_io_poll: poll timed out (events: POLLIN 1, POLLOUT 4)
00:08:28.979 # ./mptcp_connect.sh: line 392: 731 Killed
ip netns exec ${listener_ns} ./mptcp_connect -t $timeout -l -p $port
-s ${srv_proto} $extra_args $local_addr < "$sin" > "$sout"
00:08:28.988 # (duration 30308ms) [ FAIL ] client exit code 2, server 137
00:08:28.989 # \nnetns ns1-5dea74bc-S7alT1 socket stat for 10005:
00:08:29.013 # State Recv-Q Send-Q Local Address:Port
Peer Address:Port
00:08:29.013 # ESTAB 94728 0 [dead:beef:1::1]:10005
[dead:beef:1::1]:40964 rto:0.2 ato:0.04 cwnd:10 reordering:0
00:08:29.015 # \nnetns ns1-5dea74bc-S7alT1 socket stat for 10005:
00:08:29.027 # State Recv-Q Send-Q Local Address:Port
Peer Address:Port
00:08:29.027 # FIN-WAIT-1 0 1691129 [dead:beef:1::1]:40964
[dead:beef:1::1]:10005 timer:(persist,21sec,0) rto:0.2 cwnd:10
reordering:0
00:08:29.031 # FAIL: Could not even run loopback v6 test
00:08:29.062 not ok 1 selftests: net/mptcp: mptcp_connect.sh # exit=1
I guess it is not the top priority because part 1 and 2 are OK.
Cheers,
Matt
--
Matthieu Baerts | R&D Engineer
matthieu.baerts(a)tessares.net
Tessares SA | Hybrid Access Solutions
www.tessares.net
1 Avenue Jean Monnet, 1348 Louvain-la-Neuve, Belgium
4:57 p.m.
New subject: [PATCH v2 9/9] don't flag parent socket as RCV_SHUTDOWN if one one subflow has closed
Matthieu Baerts <matthieu.baerts(a)tessares.net> wrote:
- 192c07c3fe9b: "squashed" patch 1/9 in "mptcp: Add
handling of incoming
MP_JOIN requests"
- d6c1939a16c5..2cc726803b47: result
- 0c24eaeadf60: "squashed" patch 2/9, part 1, in "mptcp: Add handling of
incoming MP_JOIN requests"
- f2a1eeb27eab: "squashed" patch 2/9, part 2, in "mptcp: Add handling of
outgoing MP_JOIN requests"
- 216b0b82f102: conflict in t/mptcp-introduce-MPTCP-retransmission-timer
- e1012595cc3d: conflict in t/mptcp-add-and-use-mptcp-RTX-flag
- 2cc726803b47..48f66afb6a81: result
- the other patches have been added at the end. I had some conflicts with:
- "copy connection id from first subflow to mptcp socket"
- "make accept not allocate kernel socket struct"
- "subflow: place further subflows on new 'join_list'"
Tests on top of the commit introducing the kselftests are OK but not the one
at the end of the series (export branch). Here is the trace (without
CONFIG_KASAN nor PROVE_LOCKING)
I will have a look, thanks for reporting.
5:37 p.m.
New subject: [PATCH v2 9/9] don't flag parent socket as RCV_SHUTDOWN if one one subflow has closed
Matthieu Baerts <matthieu.baerts(a)tessares.net> wrote:
00:07:58.688 [ 14.025780] #PF: supervisor read access in kernel
mode
00:07:58.689 [ 14.026588] #PF: error_code(0x0000) - not-present page
00:07:58.690 [ 14.027394] PGD 0 P4D 0
00:07:58.690 [ 14.027801] Oops: 0000 [#1] SMP PTI
00:07:58.691 [ 14.028355] CPU: 0 PID: 731 Comm: mptcp_connect Not tainted
5.4.0+ #5
00:07:58.692 [ 14.029365] Hardware name: QEMU Standard PC (i440FX + PIIX,
1996), BIOS 1.10.2-1ubuntu1 04/01/2014
00:07:58.693 [ 14.030752] RIP: 0010:mptcp_stream_accept+0x8d/0x160
00:07:58.694 [ 14.031533] Code: 8b 9c 24 b0 05 00 00 49 81 c4 b0 05 00 00
4c 39 e3 74 71 49 8d 76 40 48 89 74 24 08 eb 08 48 8b 1b 4c 39 e3 74 5e 48
8b 6b 78 <48> 83 bd 78 02 00 00 00 75 ea 4c 8d bd 28 02 00 00 89 44 24 04 4c
00:07:58.697 [ 14.034405] RSP: 0018:ffffad8d8044bdf0 EFLAGS: 00010283
00:07:58.698 [ 14.035218] RAX: 0000000000000000 RBX: ffff96f35d0d9928 RCX:
0000000000000001
The faulty commit is
make accept not allocate kernel socket struct
When doing the last version I broke tcp-ipv6 fallback. This fixes
things:
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -1512,6 +1512,15 @@ static int mptcp_listen(struct socket *sock, int backlog)
return err;
}
+static bool is_tcp_proto(const struct proto *p)
+{
+#ifdef CONFIG_MPTCP_IPV6
+ return p == &tcp_prot || p == &tcpv6_prot;
+#else
+ return p == &tcp_prot;
+#endif
+}
+
static int mptcp_stream_accept(struct socket *sock, struct socket *newsock,
int flags, bool kern)
{
@@ -1526,7 +1535,7 @@ static int mptcp_stream_accept(struct socket *sock, struct socket
*newsock,
return -EINVAL;
err = ssock->ops->accept(sock, newsock, flags, kern);
- if (err == 0 && newsock->sk->sk_prot != &tcp_prot) {
+ if (err == 0 && !is_tcp_proto(newsock->sk->sk_prot)) {
struct mptcp_sock *msk = mptcp_sk(newsock->sk);
struct mptcp_subflow_context *subflow;
6:04 p.m.
New subject: [PATCH v2 9/9] don't flag parent socket as RCV_SHUTDOWN if one one subflow has closed
Hi Florian,
On 06/12/2019 17:37, Florian Westphal wrote:
Matthieu Baerts <matthieu.baerts(a)tessares.net> wrote:
> 00:07:58.688 [ 14.025780] #PF: supervisor read access in kernel mode
> 00:07:58.689 [ 14.026588] #PF: error_code(0x0000) - not-present page
> 00:07:58.690 [ 14.027394] PGD 0 P4D 0
> 00:07:58.690 [ 14.027801] Oops: 0000 [#1] SMP PTI
> 00:07:58.691 [ 14.028355] CPU: 0 PID: 731 Comm: mptcp_connect Not tainted
> 5.4.0+ #5
> 00:07:58.692 [ 14.029365] Hardware name: QEMU Standard PC (i440FX + PIIX,
> 1996), BIOS 1.10.2-1ubuntu1 04/01/2014
> 00:07:58.693 [ 14.030752] RIP: 0010:mptcp_stream_accept+0x8d/0x160
> 00:07:58.694 [ 14.031533] Code: 8b 9c 24 b0 05 00 00 49 81 c4 b0 05 00 00
> 4c 39 e3 74 71 49 8d 76 40 48 89 74 24 08 eb 08 48 8b 1b 4c 39 e3 74 5e 48
> 8b 6b 78 <48> 83 bd 78 02 00 00 00 75 ea 4c 8d bd 28 02 00 00 89 44 24 04 4c
> 00:07:58.697 [ 14.034405] RSP: 0018:ffffad8d8044bdf0 EFLAGS: 00010283
> 00:07:58.698 [ 14.035218] RAX: 0000000000000000 RBX: ffff96f35d0d9928 RCX:
> 0000000000000001
The faulty commit is
make accept not allocate kernel socket struct
When doing the last version I broke tcp-ipv6 fallback. This fixes
things:
Thank you very much for the quick reply and the fix!
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -1512,6 +1512,15 @@ static int mptcp_listen(struct socket *sock, int backlog)
return err;
}
+static bool is_tcp_proto(const struct proto *p)
+{
+#ifdef CONFIG_MPTCP_IPV6
+ return p == &tcp_prot || p == &tcpv6_prot;
+#else
+ return p == &tcp_prot;
+#endif
+}
+
static int mptcp_stream_accept(struct socket *sock, struct socket *newsock,
int flags, bool kern)
{
@@ -1526,7 +1535,7 @@ static int mptcp_stream_accept(struct socket *sock, struct socket
*newsock,
return -EINVAL;
err = ssock->ops->accept(sock, newsock, flags, kern);
- if (err == 0 && newsock->sk->sk_prot != &tcp_prot) {
+ if (err == 0 && !is_tcp_proto(newsock->sk->sk_prot)) {
struct mptcp_sock *msk = mptcp_sk(newsock->sk);
struct mptcp_subflow_context *subflow;
- e927b98aa468: "squashed" in "make accept not allocate kernel socket
struct (the commit is not properly formatted)
- ff06521bed5e..87b25c96264c
Tests are in progress, also checking each commit.
I will notify you when it's all done!
Cheers,
Matt
--
Matthieu Baerts | R&D Engineer
matthieu.baerts(a)tessares.net
Tessares SA | Hybrid Access Solutions
www.tessares.net
1 Avenue Jean Monnet, 1348 Louvain-la-Neuve, Belgium
723
days inactive
729
days old
31 comments
4 participants
participants (4)
-
Florian Westphal -
Mat Martineau -
Matthieu Baerts -
Paolo Abeni