* [MPTCP] [PATCH net 0/2] mptcp: a couple of fixes
@ 2021-01-12 17:25 ` Paolo Abeni
0 siblings, 0 replies; 17+ messages in thread
From: Paolo Abeni @ 2021-01-12 17:25 UTC (permalink / raw)
To: mptcp
[-- Attachment #1: Type: text/plain, Size: 347 bytes --]
This series includes two related fixes addressing potential divide by 0
bugs in the MPTCP datapath.
Paolo Abeni (2):
mptcp: more strict state checking for acks
mptcp: better msk-level shutdown.
net/mptcp/protocol.c | 64 +++++++++++++-------------------------------
1 file changed, 18 insertions(+), 46 deletions(-)
--
2.26.2
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH net 0/2] mptcp: a couple of fixes
@ 2021-01-12 17:25 ` Paolo Abeni
0 siblings, 0 replies; 17+ messages in thread
From: Paolo Abeni @ 2021-01-12 17:25 UTC (permalink / raw)
To: netdev; +Cc: mptcp, David S. Miller, Jakub Kicinski
This series includes two related fixes addressing potential divide by 0
bugs in the MPTCP datapath.
Paolo Abeni (2):
mptcp: more strict state checking for acks
mptcp: better msk-level shutdown.
net/mptcp/protocol.c | 64 +++++++++++++-------------------------------
1 file changed, 18 insertions(+), 46 deletions(-)
--
2.26.2
^ permalink raw reply [flat|nested] 17+ messages in thread
* [MPTCP] [PATCH net 1/2] mptcp: more strict state checking for acks
2021-01-12 17:25 ` Paolo Abeni
@ 2021-01-12 17:25 ` Paolo Abeni
-1 siblings, 0 replies; 17+ messages in thread
From: Paolo Abeni @ 2021-01-12 17:25 UTC (permalink / raw)
To: mptcp
[-- Attachment #1: Type: text/plain, Size: 1088 bytes --]
Syzkaller found a way to trigger division by zero
in mptcp_subflow_cleanup_rbuf().
The current checks implemented into tcp_can_send_ack()
are too week, let's be more accurate.
Reported-by: Christoph Paasch <cpaasch(a)apple.com>
Fixes: ea4ca586b16f ("mptcp: refine MPTCP-level ack scheduling")
Fixes: fd8976790a6c ("mptcp: be careful on MPTCP-level ack.")
Signed-off-by: Paolo Abeni <pabeni(a)redhat.com>
---
net/mptcp/protocol.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 6628d8d74203..2ff8c7caf74f 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -427,7 +427,7 @@ static bool mptcp_subflow_active(struct mptcp_subflow_context *subflow)
static bool tcp_can_send_ack(const struct sock *ssk)
{
return !((1 << inet_sk_state_load(ssk)) &
- (TCPF_SYN_SENT | TCPF_SYN_RECV | TCPF_TIME_WAIT | TCPF_CLOSE));
+ (TCPF_SYN_SENT | TCPF_SYN_RECV | TCPF_TIME_WAIT | TCPF_CLOSE | TCPF_LISTEN));
}
static void mptcp_send_ack(struct mptcp_sock *msk)
--
2.26.2
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH net 1/2] mptcp: more strict state checking for acks
@ 2021-01-12 17:25 ` Paolo Abeni
0 siblings, 0 replies; 17+ messages in thread
From: Paolo Abeni @ 2021-01-12 17:25 UTC (permalink / raw)
To: netdev; +Cc: mptcp, David S. Miller, Jakub Kicinski
Syzkaller found a way to trigger division by zero
in mptcp_subflow_cleanup_rbuf().
The current checks implemented into tcp_can_send_ack()
are too week, let's be more accurate.
Reported-by: Christoph Paasch <cpaasch@apple.com>
Fixes: ea4ca586b16f ("mptcp: refine MPTCP-level ack scheduling")
Fixes: fd8976790a6c ("mptcp: be careful on MPTCP-level ack.")
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
net/mptcp/protocol.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 6628d8d74203..2ff8c7caf74f 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -427,7 +427,7 @@ static bool mptcp_subflow_active(struct mptcp_subflow_context *subflow)
static bool tcp_can_send_ack(const struct sock *ssk)
{
return !((1 << inet_sk_state_load(ssk)) &
- (TCPF_SYN_SENT | TCPF_SYN_RECV | TCPF_TIME_WAIT | TCPF_CLOSE));
+ (TCPF_SYN_SENT | TCPF_SYN_RECV | TCPF_TIME_WAIT | TCPF_CLOSE | TCPF_LISTEN));
}
static void mptcp_send_ack(struct mptcp_sock *msk)
--
2.26.2
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [MPTCP] [PATCH net 2/2] mptcp: better msk-level shutdown.
2021-01-12 17:25 ` Paolo Abeni
@ 2021-01-12 17:25 ` Paolo Abeni
-1 siblings, 0 replies; 17+ messages in thread
From: Paolo Abeni @ 2021-01-12 17:25 UTC (permalink / raw)
To: mptcp
[-- Attachment #1: Type: text/plain, Size: 3867 bytes --]
Instead of re-implementing most of inet_shutdown, re-use
such helper, and implement the MPTCP-specific bits at the
'proto' level.
The msk-level disconnect() can now be invoked, lets provide a
suitable implementation.
As a side effect, this fixes bad state management for listener
sockets. The latter could lead to division by 0 oops since
commit ea4ca586b16f ("mptcp: refine MPTCP-level ack scheduling").
Fixes: 43b54c6ee382 ("mptcp: Use full MPTCP-level disconnect state machine")
Fixes: ea4ca586b16f ("mptcp: refine MPTCP-level ack scheduling")
Signed-off-by: Paolo Abeni <pabeni(a)redhat.com>
---
net/mptcp/protocol.c | 62 ++++++++++++--------------------------------
1 file changed, 17 insertions(+), 45 deletions(-)
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 2ff8c7caf74f..81faeff8f3bb 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -2642,11 +2642,12 @@ static void mptcp_copy_inaddrs(struct sock *msk, const struct sock *ssk)
static int mptcp_disconnect(struct sock *sk, int flags)
{
- /* Should never be called.
- * inet_stream_connect() calls ->disconnect, but that
- * refers to the subflow socket, not the mptcp one.
- */
- WARN_ON_ONCE(1);
+ struct mptcp_subflow_context *subflow;
+ struct mptcp_sock *msk = mptcp_sk(sk);
+
+ __mptcp_flush_join_list(msk);
+ mptcp_for_each_subflow(msk, subflow)
+ tcp_disconnect(mptcp_subflow_tcp_sock(subflow), flags);
return 0;
}
@@ -3089,6 +3090,14 @@ bool mptcp_finish_join(struct sock *ssk)
return true;
}
+static void mptcp_shutdown(struct sock *sk, int how)
+{
+ pr_debug("sk=%p, how=%d", sk, how);
+
+ if ((how & SEND_SHUTDOWN) && mptcp_close_state(sk))
+ __mptcp_wr_shutdown(sk);
+}
+
static struct proto mptcp_prot = {
.name = "MPTCP",
.owner = THIS_MODULE,
@@ -3098,7 +3107,7 @@ static struct proto mptcp_prot = {
.accept = mptcp_accept,
.setsockopt = mptcp_setsockopt,
.getsockopt = mptcp_getsockopt,
- .shutdown = tcp_shutdown,
+ .shutdown = mptcp_shutdown,
.destroy = mptcp_destroy,
.sendmsg = mptcp_sendmsg,
.recvmsg = mptcp_recvmsg,
@@ -3344,43 +3353,6 @@ static __poll_t mptcp_poll(struct file *file, struct socket *sock,
return mask;
}
-static int mptcp_shutdown(struct socket *sock, int how)
-{
- struct mptcp_sock *msk = mptcp_sk(sock->sk);
- struct sock *sk = sock->sk;
- int ret = 0;
-
- pr_debug("sk=%p, how=%d", msk, how);
-
- lock_sock(sk);
-
- how++;
- if ((how & ~SHUTDOWN_MASK) || !how) {
- ret = -EINVAL;
- goto out_unlock;
- }
-
- if (sock->state == SS_CONNECTING) {
- if ((1 << sk->sk_state) &
- (TCPF_SYN_SENT | TCPF_SYN_RECV | TCPF_CLOSE))
- sock->state = SS_DISCONNECTING;
- else
- sock->state = SS_CONNECTED;
- }
-
- sk->sk_shutdown |= how;
- if ((how & SEND_SHUTDOWN) && mptcp_close_state(sk))
- __mptcp_wr_shutdown(sk);
-
- /* Wake up anyone sleeping in poll. */
- sk->sk_state_change(sk);
-
-out_unlock:
- release_sock(sk);
-
- return ret;
-}
-
static const struct proto_ops mptcp_stream_ops = {
.family = PF_INET,
.owner = THIS_MODULE,
@@ -3394,7 +3366,7 @@ static const struct proto_ops mptcp_stream_ops = {
.ioctl = inet_ioctl,
.gettstamp = sock_gettstamp,
.listen = mptcp_listen,
- .shutdown = mptcp_shutdown,
+ .shutdown = inet_shutdown,
.setsockopt = sock_common_setsockopt,
.getsockopt = sock_common_getsockopt,
.sendmsg = inet_sendmsg,
@@ -3444,7 +3416,7 @@ static const struct proto_ops mptcp_v6_stream_ops = {
.ioctl = inet6_ioctl,
.gettstamp = sock_gettstamp,
.listen = mptcp_listen,
- .shutdown = mptcp_shutdown,
+ .shutdown = inet_shutdown,
.setsockopt = sock_common_setsockopt,
.getsockopt = sock_common_getsockopt,
.sendmsg = inet6_sendmsg,
--
2.26.2
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH net 2/2] mptcp: better msk-level shutdown.
@ 2021-01-12 17:25 ` Paolo Abeni
0 siblings, 0 replies; 17+ messages in thread
From: Paolo Abeni @ 2021-01-12 17:25 UTC (permalink / raw)
To: netdev; +Cc: mptcp, David S. Miller, Jakub Kicinski
Instead of re-implementing most of inet_shutdown, re-use
such helper, and implement the MPTCP-specific bits at the
'proto' level.
The msk-level disconnect() can now be invoked, lets provide a
suitable implementation.
As a side effect, this fixes bad state management for listener
sockets. The latter could lead to division by 0 oops since
commit ea4ca586b16f ("mptcp: refine MPTCP-level ack scheduling").
Fixes: 43b54c6ee382 ("mptcp: Use full MPTCP-level disconnect state machine")
Fixes: ea4ca586b16f ("mptcp: refine MPTCP-level ack scheduling")
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
net/mptcp/protocol.c | 62 ++++++++++++--------------------------------
1 file changed, 17 insertions(+), 45 deletions(-)
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 2ff8c7caf74f..81faeff8f3bb 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -2642,11 +2642,12 @@ static void mptcp_copy_inaddrs(struct sock *msk, const struct sock *ssk)
static int mptcp_disconnect(struct sock *sk, int flags)
{
- /* Should never be called.
- * inet_stream_connect() calls ->disconnect, but that
- * refers to the subflow socket, not the mptcp one.
- */
- WARN_ON_ONCE(1);
+ struct mptcp_subflow_context *subflow;
+ struct mptcp_sock *msk = mptcp_sk(sk);
+
+ __mptcp_flush_join_list(msk);
+ mptcp_for_each_subflow(msk, subflow)
+ tcp_disconnect(mptcp_subflow_tcp_sock(subflow), flags);
return 0;
}
@@ -3089,6 +3090,14 @@ bool mptcp_finish_join(struct sock *ssk)
return true;
}
+static void mptcp_shutdown(struct sock *sk, int how)
+{
+ pr_debug("sk=%p, how=%d", sk, how);
+
+ if ((how & SEND_SHUTDOWN) && mptcp_close_state(sk))
+ __mptcp_wr_shutdown(sk);
+}
+
static struct proto mptcp_prot = {
.name = "MPTCP",
.owner = THIS_MODULE,
@@ -3098,7 +3107,7 @@ static struct proto mptcp_prot = {
.accept = mptcp_accept,
.setsockopt = mptcp_setsockopt,
.getsockopt = mptcp_getsockopt,
- .shutdown = tcp_shutdown,
+ .shutdown = mptcp_shutdown,
.destroy = mptcp_destroy,
.sendmsg = mptcp_sendmsg,
.recvmsg = mptcp_recvmsg,
@@ -3344,43 +3353,6 @@ static __poll_t mptcp_poll(struct file *file, struct socket *sock,
return mask;
}
-static int mptcp_shutdown(struct socket *sock, int how)
-{
- struct mptcp_sock *msk = mptcp_sk(sock->sk);
- struct sock *sk = sock->sk;
- int ret = 0;
-
- pr_debug("sk=%p, how=%d", msk, how);
-
- lock_sock(sk);
-
- how++;
- if ((how & ~SHUTDOWN_MASK) || !how) {
- ret = -EINVAL;
- goto out_unlock;
- }
-
- if (sock->state == SS_CONNECTING) {
- if ((1 << sk->sk_state) &
- (TCPF_SYN_SENT | TCPF_SYN_RECV | TCPF_CLOSE))
- sock->state = SS_DISCONNECTING;
- else
- sock->state = SS_CONNECTED;
- }
-
- sk->sk_shutdown |= how;
- if ((how & SEND_SHUTDOWN) && mptcp_close_state(sk))
- __mptcp_wr_shutdown(sk);
-
- /* Wake up anyone sleeping in poll. */
- sk->sk_state_change(sk);
-
-out_unlock:
- release_sock(sk);
-
- return ret;
-}
-
static const struct proto_ops mptcp_stream_ops = {
.family = PF_INET,
.owner = THIS_MODULE,
@@ -3394,7 +3366,7 @@ static const struct proto_ops mptcp_stream_ops = {
.ioctl = inet_ioctl,
.gettstamp = sock_gettstamp,
.listen = mptcp_listen,
- .shutdown = mptcp_shutdown,
+ .shutdown = inet_shutdown,
.setsockopt = sock_common_setsockopt,
.getsockopt = sock_common_getsockopt,
.sendmsg = inet_sendmsg,
@@ -3444,7 +3416,7 @@ static const struct proto_ops mptcp_v6_stream_ops = {
.ioctl = inet6_ioctl,
.gettstamp = sock_gettstamp,
.listen = mptcp_listen,
- .shutdown = mptcp_shutdown,
+ .shutdown = inet_shutdown,
.setsockopt = sock_common_setsockopt,
.getsockopt = sock_common_getsockopt,
.sendmsg = inet6_sendmsg,
--
2.26.2
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [MPTCP] Re: [PATCH net 1/2] mptcp: more strict state checking for acks
2021-01-12 17:25 ` Paolo Abeni
@ 2021-01-12 21:37 ` Mat Martineau
-1 siblings, 0 replies; 17+ messages in thread
From: Mat Martineau @ 2021-01-12 21:37 UTC (permalink / raw)
To: mptcp
[-- Attachment #1: Type: text/plain, Size: 662 bytes --]
On Tue, 12 Jan 2021, Paolo Abeni wrote:
> Syzkaller found a way to trigger division by zero
> in mptcp_subflow_cleanup_rbuf().
>
> The current checks implemented into tcp_can_send_ack()
> are too week, let's be more accurate.
>
> Reported-by: Christoph Paasch <cpaasch(a)apple.com>
> Fixes: ea4ca586b16f ("mptcp: refine MPTCP-level ack scheduling")
> Fixes: fd8976790a6c ("mptcp: be careful on MPTCP-level ack.")
> Signed-off-by: Paolo Abeni <pabeni(a)redhat.com>
> ---
> net/mptcp/protocol.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
Reviewed-by: Mat Martineau <mathew.j.martineau(a)linux.intel.com>
--
Mat Martineau
Intel
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [MPTCP] [PATCH net 1/2] mptcp: more strict state checking for acks
@ 2021-01-12 21:37 ` Mat Martineau
0 siblings, 0 replies; 17+ messages in thread
From: Mat Martineau @ 2021-01-12 21:37 UTC (permalink / raw)
To: Paolo Abeni; +Cc: netdev, mptcp, David S. Miller, Jakub Kicinski
On Tue, 12 Jan 2021, Paolo Abeni wrote:
> Syzkaller found a way to trigger division by zero
> in mptcp_subflow_cleanup_rbuf().
>
> The current checks implemented into tcp_can_send_ack()
> are too week, let's be more accurate.
>
> Reported-by: Christoph Paasch <cpaasch@apple.com>
> Fixes: ea4ca586b16f ("mptcp: refine MPTCP-level ack scheduling")
> Fixes: fd8976790a6c ("mptcp: be careful on MPTCP-level ack.")
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
> net/mptcp/protocol.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
--
Mat Martineau
Intel
^ permalink raw reply [flat|nested] 17+ messages in thread
* [MPTCP] Re: [PATCH net 2/2] mptcp: better msk-level shutdown.
2021-01-12 17:25 ` Paolo Abeni
@ 2021-01-12 21:38 ` Mat Martineau
-1 siblings, 0 replies; 17+ messages in thread
From: Mat Martineau @ 2021-01-12 21:38 UTC (permalink / raw)
To: mptcp
[-- Attachment #1: Type: text/plain, Size: 913 bytes --]
On Tue, 12 Jan 2021, Paolo Abeni wrote:
> Instead of re-implementing most of inet_shutdown, re-use
> such helper, and implement the MPTCP-specific bits at the
> 'proto' level.
>
> The msk-level disconnect() can now be invoked, lets provide a
> suitable implementation.
>
> As a side effect, this fixes bad state management for listener
> sockets. The latter could lead to division by 0 oops since
> commit ea4ca586b16f ("mptcp: refine MPTCP-level ack scheduling").
>
> Fixes: 43b54c6ee382 ("mptcp: Use full MPTCP-level disconnect state machine")
> Fixes: ea4ca586b16f ("mptcp: refine MPTCP-level ack scheduling")
> Signed-off-by: Paolo Abeni <pabeni(a)redhat.com>
> ---
> net/mptcp/protocol.c | 62 ++++++++++++--------------------------------
> 1 file changed, 17 insertions(+), 45 deletions(-)
>
Reviewed-by: Mat Martineau <mathew.j.martineau(a)linux.intel.com>
--
Mat Martineau
Intel
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [MPTCP] [PATCH net 2/2] mptcp: better msk-level shutdown.
@ 2021-01-12 21:38 ` Mat Martineau
0 siblings, 0 replies; 17+ messages in thread
From: Mat Martineau @ 2021-01-12 21:38 UTC (permalink / raw)
To: Paolo Abeni; +Cc: netdev, mptcp, David S. Miller, Jakub Kicinski
On Tue, 12 Jan 2021, Paolo Abeni wrote:
> Instead of re-implementing most of inet_shutdown, re-use
> such helper, and implement the MPTCP-specific bits at the
> 'proto' level.
>
> The msk-level disconnect() can now be invoked, lets provide a
> suitable implementation.
>
> As a side effect, this fixes bad state management for listener
> sockets. The latter could lead to division by 0 oops since
> commit ea4ca586b16f ("mptcp: refine MPTCP-level ack scheduling").
>
> Fixes: 43b54c6ee382 ("mptcp: Use full MPTCP-level disconnect state machine")
> Fixes: ea4ca586b16f ("mptcp: refine MPTCP-level ack scheduling")
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
> net/mptcp/protocol.c | 62 ++++++++++++--------------------------------
> 1 file changed, 17 insertions(+), 45 deletions(-)
>
Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
--
Mat Martineau
Intel
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH net 0/2] mptcp: a couple of fixes
2021-01-12 17:25 ` Paolo Abeni
(?)
@ 2021-01-13 4:20 ` patchwork-bot+netdevbpf
-1 siblings, 0 replies; 17+ messages in thread
From: patchwork-bot+netdevbpf @ 2021-01-13 4:20 UTC (permalink / raw)
To: Paolo Abeni; +Cc: netdev, mptcp, davem, kuba
Hello:
This series was applied to netdev/net.git (refs/heads/master):
On Tue, 12 Jan 2021 18:25:22 +0100 you wrote:
> This series includes two related fixes addressing potential divide by 0
> bugs in the MPTCP datapath.
>
> Paolo Abeni (2):
> mptcp: more strict state checking for acks
> mptcp: better msk-level shutdown.
>
> [...]
Here is the summary with links:
- [net,1/2] mptcp: more strict state checking for acks
https://git.kernel.org/netdev/net/c/20bc80b6f582
- [net,2/2] mptcp: better msk-level shutdown.
https://git.kernel.org/netdev/net/c/76e2a55d1625
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 17+ messages in thread
* [MPTCP] Re: [PATCH net 2/2] mptcp: better msk-level shutdown.
2021-01-12 17:25 ` Paolo Abeni
@ 2021-01-13 10:21 ` Eric Dumazet
-1 siblings, 0 replies; 17+ messages in thread
From: Eric Dumazet @ 2021-01-13 10:21 UTC (permalink / raw)
To: mptcp
[-- Attachment #1: Type: text/plain, Size: 1709 bytes --]
On 1/12/21 6:25 PM, Paolo Abeni wrote:
> Instead of re-implementing most of inet_shutdown, re-use
> such helper, and implement the MPTCP-specific bits at the
> 'proto' level.
>
> The msk-level disconnect() can now be invoked, lets provide a
> suitable implementation.
>
> As a side effect, this fixes bad state management for listener
> sockets. The latter could lead to division by 0 oops since
> commit ea4ca586b16f ("mptcp: refine MPTCP-level ack scheduling").
>
> Fixes: 43b54c6ee382 ("mptcp: Use full MPTCP-level disconnect state machine")
> Fixes: ea4ca586b16f ("mptcp: refine MPTCP-level ack scheduling")
> Signed-off-by: Paolo Abeni <pabeni(a)redhat.com>
> ---
> net/mptcp/protocol.c | 62 ++++++++++++--------------------------------
> 1 file changed, 17 insertions(+), 45 deletions(-)
>
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 2ff8c7caf74f..81faeff8f3bb 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -2642,11 +2642,12 @@ static void mptcp_copy_inaddrs(struct sock *msk, const struct sock *ssk)
>
> static int mptcp_disconnect(struct sock *sk, int flags)
> {
> - /* Should never be called.
> - * inet_stream_connect() calls ->disconnect, but that
> - * refers to the subflow socket, not the mptcp one.
> - */
> - WARN_ON_ONCE(1);
> + struct mptcp_subflow_context *subflow;
> + struct mptcp_sock *msk = mptcp_sk(sk);
> +
> + __mptcp_flush_join_list(msk);
> + mptcp_for_each_subflow(msk, subflow)
> + tcp_disconnect(mptcp_subflow_tcp_sock(subflow), flags);
Ouch.
tcp_disconnect() is supposed to be called with socket lock being held.
Really, CONFIG_LOCKDEP=y should have warned you :/
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH net 2/2] mptcp: better msk-level shutdown.
@ 2021-01-13 10:21 ` Eric Dumazet
0 siblings, 0 replies; 17+ messages in thread
From: Eric Dumazet @ 2021-01-13 10:21 UTC (permalink / raw)
To: Paolo Abeni, netdev; +Cc: mptcp, David S. Miller, Jakub Kicinski
On 1/12/21 6:25 PM, Paolo Abeni wrote:
> Instead of re-implementing most of inet_shutdown, re-use
> such helper, and implement the MPTCP-specific bits at the
> 'proto' level.
>
> The msk-level disconnect() can now be invoked, lets provide a
> suitable implementation.
>
> As a side effect, this fixes bad state management for listener
> sockets. The latter could lead to division by 0 oops since
> commit ea4ca586b16f ("mptcp: refine MPTCP-level ack scheduling").
>
> Fixes: 43b54c6ee382 ("mptcp: Use full MPTCP-level disconnect state machine")
> Fixes: ea4ca586b16f ("mptcp: refine MPTCP-level ack scheduling")
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
> net/mptcp/protocol.c | 62 ++++++++++++--------------------------------
> 1 file changed, 17 insertions(+), 45 deletions(-)
>
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 2ff8c7caf74f..81faeff8f3bb 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -2642,11 +2642,12 @@ static void mptcp_copy_inaddrs(struct sock *msk, const struct sock *ssk)
>
> static int mptcp_disconnect(struct sock *sk, int flags)
> {
> - /* Should never be called.
> - * inet_stream_connect() calls ->disconnect, but that
> - * refers to the subflow socket, not the mptcp one.
> - */
> - WARN_ON_ONCE(1);
> + struct mptcp_subflow_context *subflow;
> + struct mptcp_sock *msk = mptcp_sk(sk);
> +
> + __mptcp_flush_join_list(msk);
> + mptcp_for_each_subflow(msk, subflow)
> + tcp_disconnect(mptcp_subflow_tcp_sock(subflow), flags);
Ouch.
tcp_disconnect() is supposed to be called with socket lock being held.
Really, CONFIG_LOCKDEP=y should have warned you :/
^ permalink raw reply [flat|nested] 17+ messages in thread
* [MPTCP] Re: [PATCH net 2/2] mptcp: better msk-level shutdown.
2021-01-13 10:21 ` Eric Dumazet
@ 2021-01-13 10:26 ` Eric Dumazet
-1 siblings, 0 replies; 17+ messages in thread
From: Eric Dumazet @ 2021-01-13 10:26 UTC (permalink / raw)
To: mptcp
[-- Attachment #1: Type: text/plain, Size: 1865 bytes --]
On 1/13/21 11:21 AM, Eric Dumazet wrote:
>
>
> On 1/12/21 6:25 PM, Paolo Abeni wrote:
>> Instead of re-implementing most of inet_shutdown, re-use
>> such helper, and implement the MPTCP-specific bits at the
>> 'proto' level.
>>
>> The msk-level disconnect() can now be invoked, lets provide a
>> suitable implementation.
>>
>> As a side effect, this fixes bad state management for listener
>> sockets. The latter could lead to division by 0 oops since
>> commit ea4ca586b16f ("mptcp: refine MPTCP-level ack scheduling").
>>
>> Fixes: 43b54c6ee382 ("mptcp: Use full MPTCP-level disconnect state machine")
>> Fixes: ea4ca586b16f ("mptcp: refine MPTCP-level ack scheduling")
>> Signed-off-by: Paolo Abeni <pabeni(a)redhat.com>
>> ---
>> net/mptcp/protocol.c | 62 ++++++++++++--------------------------------
>> 1 file changed, 17 insertions(+), 45 deletions(-)
>>
>> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
>> index 2ff8c7caf74f..81faeff8f3bb 100644
>> --- a/net/mptcp/protocol.c
>> +++ b/net/mptcp/protocol.c
>> @@ -2642,11 +2642,12 @@ static void mptcp_copy_inaddrs(struct sock *msk, const struct sock *ssk)
>>
>> static int mptcp_disconnect(struct sock *sk, int flags)
>> {
>> - /* Should never be called.
>> - * inet_stream_connect() calls ->disconnect, but that
>> - * refers to the subflow socket, not the mptcp one.
>> - */
>> - WARN_ON_ONCE(1);
>> + struct mptcp_subflow_context *subflow;
>> + struct mptcp_sock *msk = mptcp_sk(sk);
>> +
>> + __mptcp_flush_join_list(msk);
>> + mptcp_for_each_subflow(msk, subflow)
>> + tcp_disconnect(mptcp_subflow_tcp_sock(subflow), flags);
>
> Ouch.
>
> tcp_disconnect() is supposed to be called with socket lock being held.
>
> Really, CONFIG_LOCKDEP=y should have warned you :/
Or maybe CONFIG_PROVE_RCU=y is needed to catch the bug.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH net 2/2] mptcp: better msk-level shutdown.
@ 2021-01-13 10:26 ` Eric Dumazet
0 siblings, 0 replies; 17+ messages in thread
From: Eric Dumazet @ 2021-01-13 10:26 UTC (permalink / raw)
To: Paolo Abeni, netdev; +Cc: mptcp, David S. Miller, Jakub Kicinski
On 1/13/21 11:21 AM, Eric Dumazet wrote:
>
>
> On 1/12/21 6:25 PM, Paolo Abeni wrote:
>> Instead of re-implementing most of inet_shutdown, re-use
>> such helper, and implement the MPTCP-specific bits at the
>> 'proto' level.
>>
>> The msk-level disconnect() can now be invoked, lets provide a
>> suitable implementation.
>>
>> As a side effect, this fixes bad state management for listener
>> sockets. The latter could lead to division by 0 oops since
>> commit ea4ca586b16f ("mptcp: refine MPTCP-level ack scheduling").
>>
>> Fixes: 43b54c6ee382 ("mptcp: Use full MPTCP-level disconnect state machine")
>> Fixes: ea4ca586b16f ("mptcp: refine MPTCP-level ack scheduling")
>> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
>> ---
>> net/mptcp/protocol.c | 62 ++++++++++++--------------------------------
>> 1 file changed, 17 insertions(+), 45 deletions(-)
>>
>> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
>> index 2ff8c7caf74f..81faeff8f3bb 100644
>> --- a/net/mptcp/protocol.c
>> +++ b/net/mptcp/protocol.c
>> @@ -2642,11 +2642,12 @@ static void mptcp_copy_inaddrs(struct sock *msk, const struct sock *ssk)
>>
>> static int mptcp_disconnect(struct sock *sk, int flags)
>> {
>> - /* Should never be called.
>> - * inet_stream_connect() calls ->disconnect, but that
>> - * refers to the subflow socket, not the mptcp one.
>> - */
>> - WARN_ON_ONCE(1);
>> + struct mptcp_subflow_context *subflow;
>> + struct mptcp_sock *msk = mptcp_sk(sk);
>> +
>> + __mptcp_flush_join_list(msk);
>> + mptcp_for_each_subflow(msk, subflow)
>> + tcp_disconnect(mptcp_subflow_tcp_sock(subflow), flags);
>
> Ouch.
>
> tcp_disconnect() is supposed to be called with socket lock being held.
>
> Really, CONFIG_LOCKDEP=y should have warned you :/
Or maybe CONFIG_PROVE_RCU=y is needed to catch the bug.
^ permalink raw reply [flat|nested] 17+ messages in thread
* [MPTCP] Re: [PATCH net 2/2] mptcp: better msk-level shutdown.
2021-01-13 10:26 ` Eric Dumazet
@ 2021-01-13 15:26 ` Paolo Abeni
-1 siblings, 0 replies; 17+ messages in thread
From: Paolo Abeni @ 2021-01-13 15:26 UTC (permalink / raw)
To: mptcp
[-- Attachment #1: Type: text/plain, Size: 2252 bytes --]
On Wed, 2021-01-13 at 11:26 +0100, Eric Dumazet wrote:
>
> On 1/13/21 11:21 AM, Eric Dumazet wrote:
> >
> > On 1/12/21 6:25 PM, Paolo Abeni wrote:
> > > Instead of re-implementing most of inet_shutdown, re-use
> > > such helper, and implement the MPTCP-specific bits at the
> > > 'proto' level.
> > >
> > > The msk-level disconnect() can now be invoked, lets provide a
> > > suitable implementation.
> > >
> > > As a side effect, this fixes bad state management for listener
> > > sockets. The latter could lead to division by 0 oops since
> > > commit ea4ca586b16f ("mptcp: refine MPTCP-level ack scheduling").
> > >
> > > Fixes: 43b54c6ee382 ("mptcp: Use full MPTCP-level disconnect state machine")
> > > Fixes: ea4ca586b16f ("mptcp: refine MPTCP-level ack scheduling")
> > > Signed-off-by: Paolo Abeni <pabeni(a)redhat.com>
> > > ---
> > > net/mptcp/protocol.c | 62 ++++++++++++--------------------------------
> > > 1 file changed, 17 insertions(+), 45 deletions(-)
> > >
> > > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> > > index 2ff8c7caf74f..81faeff8f3bb 100644
> > > --- a/net/mptcp/protocol.c
> > > +++ b/net/mptcp/protocol.c
> > > @@ -2642,11 +2642,12 @@ static void mptcp_copy_inaddrs(struct sock *msk, const struct sock *ssk)
> > >
> > > static int mptcp_disconnect(struct sock *sk, int flags)
> > > {
> > > - /* Should never be called.
> > > - * inet_stream_connect() calls ->disconnect, but that
> > > - * refers to the subflow socket, not the mptcp one.
> > > - */
> > > - WARN_ON_ONCE(1);
> > > + struct mptcp_subflow_context *subflow;
> > > + struct mptcp_sock *msk = mptcp_sk(sk);
> > > +
> > > + __mptcp_flush_join_list(msk);
> > > + mptcp_for_each_subflow(msk, subflow)
> > > + tcp_disconnect(mptcp_subflow_tcp_sock(subflow), flags);
> >
> > Ouch.
> >
> > tcp_disconnect() is supposed to be called with socket lock being held.
> >
> > Really, CONFIG_LOCKDEP=y should have warned you :/
>
> Or maybe CONFIG_PROVE_RCU=y is needed to catch the bug.
Thank you for catching this!
Yep, CONFIG_PROVE_RCU=y triggers a 'suspicious RCU usage' warning. I
should really enable 'panic_on_warn' in batch tests.
I'll send a patch.
Thanks,
Paolo
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH net 2/2] mptcp: better msk-level shutdown.
@ 2021-01-13 15:26 ` Paolo Abeni
0 siblings, 0 replies; 17+ messages in thread
From: Paolo Abeni @ 2021-01-13 15:26 UTC (permalink / raw)
To: Eric Dumazet, netdev; +Cc: mptcp, David S. Miller, Jakub Kicinski
On Wed, 2021-01-13 at 11:26 +0100, Eric Dumazet wrote:
>
> On 1/13/21 11:21 AM, Eric Dumazet wrote:
> >
> > On 1/12/21 6:25 PM, Paolo Abeni wrote:
> > > Instead of re-implementing most of inet_shutdown, re-use
> > > such helper, and implement the MPTCP-specific bits at the
> > > 'proto' level.
> > >
> > > The msk-level disconnect() can now be invoked, lets provide a
> > > suitable implementation.
> > >
> > > As a side effect, this fixes bad state management for listener
> > > sockets. The latter could lead to division by 0 oops since
> > > commit ea4ca586b16f ("mptcp: refine MPTCP-level ack scheduling").
> > >
> > > Fixes: 43b54c6ee382 ("mptcp: Use full MPTCP-level disconnect state machine")
> > > Fixes: ea4ca586b16f ("mptcp: refine MPTCP-level ack scheduling")
> > > Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> > > ---
> > > net/mptcp/protocol.c | 62 ++++++++++++--------------------------------
> > > 1 file changed, 17 insertions(+), 45 deletions(-)
> > >
> > > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> > > index 2ff8c7caf74f..81faeff8f3bb 100644
> > > --- a/net/mptcp/protocol.c
> > > +++ b/net/mptcp/protocol.c
> > > @@ -2642,11 +2642,12 @@ static void mptcp_copy_inaddrs(struct sock *msk, const struct sock *ssk)
> > >
> > > static int mptcp_disconnect(struct sock *sk, int flags)
> > > {
> > > - /* Should never be called.
> > > - * inet_stream_connect() calls ->disconnect, but that
> > > - * refers to the subflow socket, not the mptcp one.
> > > - */
> > > - WARN_ON_ONCE(1);
> > > + struct mptcp_subflow_context *subflow;
> > > + struct mptcp_sock *msk = mptcp_sk(sk);
> > > +
> > > + __mptcp_flush_join_list(msk);
> > > + mptcp_for_each_subflow(msk, subflow)
> > > + tcp_disconnect(mptcp_subflow_tcp_sock(subflow), flags);
> >
> > Ouch.
> >
> > tcp_disconnect() is supposed to be called with socket lock being held.
> >
> > Really, CONFIG_LOCKDEP=y should have warned you :/
>
> Or maybe CONFIG_PROVE_RCU=y is needed to catch the bug.
Thank you for catching this!
Yep, CONFIG_PROVE_RCU=y triggers a 'suspicious RCU usage' warning. I
should really enable 'panic_on_warn' in batch tests.
I'll send a patch.
Thanks,
Paolo
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2021-01-13 15:28 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-12 21:37 [MPTCP] Re: [PATCH net 1/2] mptcp: more strict state checking for acks Mat Martineau
2021-01-12 21:37 ` [MPTCP] " Mat Martineau
-- strict thread matches above, loose matches on Subject: below --
2021-01-13 15:26 [MPTCP] Re: [PATCH net 2/2] mptcp: better msk-level shutdown Paolo Abeni
2021-01-13 15:26 ` Paolo Abeni
2021-01-13 10:26 [MPTCP] " Eric Dumazet
2021-01-13 10:26 ` Eric Dumazet
2021-01-13 10:21 [MPTCP] " Eric Dumazet
2021-01-13 10:21 ` Eric Dumazet
2021-01-12 21:38 [MPTCP] " Mat Martineau
2021-01-12 21:38 ` [MPTCP] " Mat Martineau
2021-01-12 17:25 Paolo Abeni
2021-01-12 17:25 ` Paolo Abeni
2021-01-12 17:25 [MPTCP] [PATCH net 1/2] mptcp: more strict state checking for acks Paolo Abeni
2021-01-12 17:25 ` Paolo Abeni
2021-01-12 17:25 [MPTCP] [PATCH net 0/2] mptcp: a couple of fixes Paolo Abeni
2021-01-12 17:25 ` Paolo Abeni
2021-01-13 4:20 ` patchwork-bot+netdevbpf
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.