* [PATCH mptcp-net] mptcp: prevent tcp diag from closing listener subflows
@ 2023-12-15 9:12 Paolo Abeni
2023-12-15 11:10 ` mptcp: prevent tcp diag from closing listener subflows: Tests Results MPTCP CI
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Paolo Abeni @ 2023-12-15 9:12 UTC (permalink / raw)
To: mptcp
The MPTCP protocol does not expect that any other entity could change
the first subflow status when such socket is listening.
Unfortunately the TCP diag interface allows aborting any TCP socket,
including MPTCP listeners subflows. As reported by syzbot, that trigger
a WARN() and could lead to later bigger trouble.
The MPTCP protocol needs to do some MPTCP-level cleanup actions to
properly shutdown the listener. To keep the fix simple, prevent
entirely the diag interface from stopping such listeners.
We could refine the diag callback in a later, larger patch targeting
net-next.
Fixes: 57fc0f1ceaa4 ("mptcp: ensure listener is unhashed before updating the sk status")
Closes: https://lore.kernel.org/netdev/0000000000004f4579060c68431b@google.com/
Reported-by: syzbot+5a01c3a666e726bc8752@syzkaller.appspotmail.com
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
net/mptcp/subflow.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 64302a1ac306..1f98bec264a6 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -1981,6 +1981,17 @@ static void tcp_release_cb_override(struct sock *ssk)
tcp_release_cb(ssk);
}
+static int tcp_abort_override(struct sock *ssk, int err)
+{
+ /* closing a listener subflow requires a great deal of care.
+ * keep it simple and just prevent such operation
+ */
+ if (inet_sk_state_load(ssk) == TCP_LISTEN)
+ return -EINVAL;
+
+ return tcp_abort(ssk, err);
+}
+
static struct tcp_ulp_ops subflow_ulp_ops __read_mostly = {
.name = "mptcp",
.owner = THIS_MODULE,
@@ -2025,6 +2036,7 @@ void __init mptcp_subflow_init(void)
tcp_prot_override = tcp_prot;
tcp_prot_override.release_cb = tcp_release_cb_override;
+ tcp_prot_override.diag_destroy = tcp_abort_override;
#if IS_ENABLED(CONFIG_MPTCP_IPV6)
/* In struct mptcp_subflow_request_sock, we assume the TCP request sock
@@ -2060,6 +2072,7 @@ void __init mptcp_subflow_init(void)
tcpv6_prot_override = tcpv6_prot;
tcpv6_prot_override.release_cb = tcp_release_cb_override;
+ tcpv6_prot_override.diag_destroy = tcp_abort_override;
#endif
mptcp_diag_subflow_init(&subflow_ulp_ops);
--
2.42.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: mptcp: prevent tcp diag from closing listener subflows: Tests Results
2023-12-15 9:12 [PATCH mptcp-net] mptcp: prevent tcp diag from closing listener subflows Paolo Abeni
@ 2023-12-15 11:10 ` MPTCP CI
2023-12-16 0:47 ` [PATCH mptcp-net] mptcp: prevent tcp diag from closing listener subflows Mat Martineau
2023-12-22 10:44 ` Matthieu Baerts
2 siblings, 0 replies; 6+ messages in thread
From: MPTCP CI @ 2023-12-15 11:10 UTC (permalink / raw)
To: Paolo Abeni; +Cc: mptcp
Hi Paolo,
Thank you for your modifications, that's great!
Our CI did some validations and here is its report:
- KVM Validation: normal (except selftest_mptcp_join):
- Success! ✅:
- Task: https://cirrus-ci.com/task/6646535530741760
- Summary: https://api.cirrus-ci.com/v1/artifact/task/6646535530741760/summary/summary.txt
- KVM Validation: normal (only selftest_mptcp_join):
- Success! ✅:
- Task: https://cirrus-ci.com/task/5666210654715904
- Summary: https://api.cirrus-ci.com/v1/artifact/task/5666210654715904/summary/summary.txt
- KVM Validation: debug (except selftest_mptcp_join):
- Success! ✅:
- Task: https://cirrus-ci.com/task/5731741856432128
- Summary: https://api.cirrus-ci.com/v1/artifact/task/5731741856432128/summary/summary.txt
- KVM Validation: debug (only selftest_mptcp_join):
- Unstable: 1 failed test(s): selftest_mptcp_join 🔴:
- Task: https://cirrus-ci.com/task/5168791903010816
- Summary: https://api.cirrus-ci.com/v1/artifact/task/5168791903010816/summary/summary.txt
Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/23b0f9732a26
If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:
$ cd [kernel source code]
$ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
--pull always mptcp/mptcp-upstream-virtme-docker:latest \
auto-debug
For more details:
https://github.com/multipath-tcp/mptcp-upstream-virtme-docker
Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)
Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (NGI0 Core)
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH mptcp-net] mptcp: prevent tcp diag from closing listener subflows
2023-12-15 9:12 [PATCH mptcp-net] mptcp: prevent tcp diag from closing listener subflows Paolo Abeni
2023-12-15 11:10 ` mptcp: prevent tcp diag from closing listener subflows: Tests Results MPTCP CI
@ 2023-12-16 0:47 ` Mat Martineau
2023-12-21 10:00 ` Paolo Abeni
2023-12-22 10:44 ` Matthieu Baerts
2 siblings, 1 reply; 6+ messages in thread
From: Mat Martineau @ 2023-12-16 0:47 UTC (permalink / raw)
To: Paolo Abeni; +Cc: mptcp
On Fri, 15 Dec 2023, Paolo Abeni wrote:
> The MPTCP protocol does not expect that any other entity could change
> the first subflow status when such socket is listening.
> Unfortunately the TCP diag interface allows aborting any TCP socket,
> including MPTCP listeners subflows. As reported by syzbot, that trigger
> a WARN() and could lead to later bigger trouble.
>
> The MPTCP protocol needs to do some MPTCP-level cleanup actions to
> properly shutdown the listener. To keep the fix simple, prevent
> entirely the diag interface from stopping such listeners.
>
> We could refine the diag callback in a later, larger patch targeting
> net-next.
>
> Fixes: 57fc0f1ceaa4 ("mptcp: ensure listener is unhashed before updating the sk status")
> Closes: https://lore.kernel.org/netdev/0000000000004f4579060c68431b@google.com/
> Reported-by: syzbot+5a01c3a666e726bc8752@syzkaller.appspotmail.com
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
> net/mptcp/subflow.c | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
>
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 64302a1ac306..1f98bec264a6 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -1981,6 +1981,17 @@ static void tcp_release_cb_override(struct sock *ssk)
> tcp_release_cb(ssk);
> }
>
> +static int tcp_abort_override(struct sock *ssk, int err)
> +{
> + /* closing a listener subflow requires a great deal of care.
> + * keep it simple and just prevent such operation
> + */
> + if (inet_sk_state_load(ssk) == TCP_LISTEN)
> + return -EINVAL;
> +
> + return tcp_abort(ssk, err);
Hi Paolo -
I looked around the code to see if there might be issues with an
unexpected tcp_abort on non-listening subflow sockets. Closest code I
found was mptcp_subflow_reset(), and the main difference seems to be
giving the msk a chance to clean up:
if (!test_and_set_bit(MPTCP_WORK_CLOSE_SUBFLOW, &mptcp_sk(sk)->flags))
mptcp_schedule_work(sk);
Worthwhile to add that in this override function?
- Mat
> +}
> +
> static struct tcp_ulp_ops subflow_ulp_ops __read_mostly = {
> .name = "mptcp",
> .owner = THIS_MODULE,
> @@ -2025,6 +2036,7 @@ void __init mptcp_subflow_init(void)
>
> tcp_prot_override = tcp_prot;
> tcp_prot_override.release_cb = tcp_release_cb_override;
> + tcp_prot_override.diag_destroy = tcp_abort_override;
>
> #if IS_ENABLED(CONFIG_MPTCP_IPV6)
> /* In struct mptcp_subflow_request_sock, we assume the TCP request sock
> @@ -2060,6 +2072,7 @@ void __init mptcp_subflow_init(void)
>
> tcpv6_prot_override = tcpv6_prot;
> tcpv6_prot_override.release_cb = tcp_release_cb_override;
> + tcpv6_prot_override.diag_destroy = tcp_abort_override;
> #endif
>
> mptcp_diag_subflow_init(&subflow_ulp_ops);
> --
> 2.42.0
>
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH mptcp-net] mptcp: prevent tcp diag from closing listener subflows
2023-12-16 0:47 ` [PATCH mptcp-net] mptcp: prevent tcp diag from closing listener subflows Mat Martineau
@ 2023-12-21 10:00 ` Paolo Abeni
2023-12-21 17:25 ` Mat Martineau
0 siblings, 1 reply; 6+ messages in thread
From: Paolo Abeni @ 2023-12-21 10:00 UTC (permalink / raw)
To: Mat Martineau; +Cc: mptcp
On Fri, 2023-12-15 at 16:47 -0800, Mat Martineau wrote:
> On Fri, 15 Dec 2023, Paolo Abeni wrote:
>
> > The MPTCP protocol does not expect that any other entity could change
> > the first subflow status when such socket is listening.
> > Unfortunately the TCP diag interface allows aborting any TCP socket,
> > including MPTCP listeners subflows. As reported by syzbot, that trigger
> > a WARN() and could lead to later bigger trouble.
> >
> > The MPTCP protocol needs to do some MPTCP-level cleanup actions to
> > properly shutdown the listener. To keep the fix simple, prevent
> > entirely the diag interface from stopping such listeners.
> >
> > We could refine the diag callback in a later, larger patch targeting
> > net-next.
> >
> > Fixes: 57fc0f1ceaa4 ("mptcp: ensure listener is unhashed before updating the sk status")
> > Closes: https://lore.kernel.org/netdev/0000000000004f4579060c68431b@google.com/
> > Reported-by: syzbot+5a01c3a666e726bc8752@syzkaller.appspotmail.com
> > Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> > ---
> > net/mptcp/subflow.c | 13 +++++++++++++
> > 1 file changed, 13 insertions(+)
> >
> > diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> > index 64302a1ac306..1f98bec264a6 100644
> > --- a/net/mptcp/subflow.c
> > +++ b/net/mptcp/subflow.c
> > @@ -1981,6 +1981,17 @@ static void tcp_release_cb_override(struct sock *ssk)
> > tcp_release_cb(ssk);
> > }
> >
> > +static int tcp_abort_override(struct sock *ssk, int err)
> > +{
> > + /* closing a listener subflow requires a great deal of care.
> > + * keep it simple and just prevent such operation
> > + */
> > + if (inet_sk_state_load(ssk) == TCP_LISTEN)
> > + return -EINVAL;
> > +
> > + return tcp_abort(ssk, err);
>
> Hi Paolo -
>
> I looked around the code to see if there might be issues with an
> unexpected tcp_abort on non-listening subflow sockets. Closest code I
> found was mptcp_subflow_reset(), and the main difference seems to be
> giving the msk a chance to clean up:
>
> if (!test_and_set_bit(MPTCP_WORK_CLOSE_SUBFLOW, &mptcp_sk(sk)->flags))
> mptcp_schedule_work(sk);
>
> Worthwhile to add that in this override function?
Reporting there the discussion from the last mtg for completeness.
Async closing of mptcp listener via the worker has proven to be quite
complex and bug prone. In this case it would probably be safe, but
given the troubled git history of mptcp_check_listen_stop(), I would
keep this version for net, and do the complete thing for net-next, with
no rush.
Cheers,
Paolo
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH mptcp-net] mptcp: prevent tcp diag from closing listener subflows
2023-12-21 10:00 ` Paolo Abeni
@ 2023-12-21 17:25 ` Mat Martineau
0 siblings, 0 replies; 6+ messages in thread
From: Mat Martineau @ 2023-12-21 17:25 UTC (permalink / raw)
To: Paolo Abeni; +Cc: mptcp
On Thu, 21 Dec 2023, Paolo Abeni wrote:
> On Fri, 2023-12-15 at 16:47 -0800, Mat Martineau wrote:
>> On Fri, 15 Dec 2023, Paolo Abeni wrote:
>>
>>> The MPTCP protocol does not expect that any other entity could change
>>> the first subflow status when such socket is listening.
>>> Unfortunately the TCP diag interface allows aborting any TCP socket,
>>> including MPTCP listeners subflows. As reported by syzbot, that trigger
>>> a WARN() and could lead to later bigger trouble.
>>>
>>> The MPTCP protocol needs to do some MPTCP-level cleanup actions to
>>> properly shutdown the listener. To keep the fix simple, prevent
>>> entirely the diag interface from stopping such listeners.
>>>
>>> We could refine the diag callback in a later, larger patch targeting
>>> net-next.
>>>
>>> Fixes: 57fc0f1ceaa4 ("mptcp: ensure listener is unhashed before updating the sk status")
>>> Closes: https://lore.kernel.org/netdev/0000000000004f4579060c68431b@google.com/
>>> Reported-by: syzbot+5a01c3a666e726bc8752@syzkaller.appspotmail.com
>>> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
>>> ---
>>> net/mptcp/subflow.c | 13 +++++++++++++
>>> 1 file changed, 13 insertions(+)
>>>
>>> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
>>> index 64302a1ac306..1f98bec264a6 100644
>>> --- a/net/mptcp/subflow.c
>>> +++ b/net/mptcp/subflow.c
>>> @@ -1981,6 +1981,17 @@ static void tcp_release_cb_override(struct sock *ssk)
>>> tcp_release_cb(ssk);
>>> }
>>>
>>> +static int tcp_abort_override(struct sock *ssk, int err)
>>> +{
>>> + /* closing a listener subflow requires a great deal of care.
>>> + * keep it simple and just prevent such operation
>>> + */
>>> + if (inet_sk_state_load(ssk) == TCP_LISTEN)
>>> + return -EINVAL;
>>> +
>>> + return tcp_abort(ssk, err);
>>
>> Hi Paolo -
>>
>> I looked around the code to see if there might be issues with an
>> unexpected tcp_abort on non-listening subflow sockets. Closest code I
>> found was mptcp_subflow_reset(), and the main difference seems to be
>> giving the msk a chance to clean up:
>>
>> if (!test_and_set_bit(MPTCP_WORK_CLOSE_SUBFLOW, &mptcp_sk(sk)->flags))
>> mptcp_schedule_work(sk);
>>
>> Worthwhile to add that in this override function?
>
> Reporting there the discussion from the last mtg for completeness.
> Async closing of mptcp listener via the worker has proven to be quite
> complex and bug prone. In this case it would probably be safe, but
> given the troubled git history of mptcp_check_listen_stop(), I would
> keep this version for net, and do the complete thing for net-next, with
> no rush.
>
Thanks Paolo, sounds like a good plan.
Reviewed-by: Mat Martineau <martineau@kernel.org>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH mptcp-net] mptcp: prevent tcp diag from closing listener subflows
2023-12-15 9:12 [PATCH mptcp-net] mptcp: prevent tcp diag from closing listener subflows Paolo Abeni
2023-12-15 11:10 ` mptcp: prevent tcp diag from closing listener subflows: Tests Results MPTCP CI
2023-12-16 0:47 ` [PATCH mptcp-net] mptcp: prevent tcp diag from closing listener subflows Mat Martineau
@ 2023-12-22 10:44 ` Matthieu Baerts
2 siblings, 0 replies; 6+ messages in thread
From: Matthieu Baerts @ 2023-12-22 10:44 UTC (permalink / raw)
To: Paolo Abeni; +Cc: mptcp
Hi Paolo, Mat,
On 15/12/2023 10:12, Paolo Abeni wrote:
> The MPTCP protocol does not expect that any other entity could change
> the first subflow status when such socket is listening.
> Unfortunately the TCP diag interface allows aborting any TCP socket,
> including MPTCP listeners subflows. As reported by syzbot, that trigger
> a WARN() and could lead to later bigger trouble.
>
> The MPTCP protocol needs to do some MPTCP-level cleanup actions to
> properly shutdown the listener. To keep the fix simple, prevent
> entirely the diag interface from stopping such listeners.
>
> We could refine the diag callback in a later, larger patch targeting
> net-next.
Thank you for this patch and the review!
> Fixes: 57fc0f1ceaa4 ("mptcp: ensure listener is unhashed before updating the sk status")
> Closes: https://lore.kernel.org/netdev/0000000000004f4579060c68431b@google.com/
> Reported-by: syzbot+5a01c3a666e726bc8752@syzkaller.appspotmail.com
(I just inverted these two lines to make checkpatch happy ;) )
(and it looks like b4 automatically adds '<>' around the email address)
Now in our tree (fixes for -net):
New patches for t/upstream-net and t/upstream:
- 5913b5d75d1f: mptcp: prevent tcp diag from closing listener subflows
- Results: 9293d696010b..b7f484870b63 (export-net)
- Results: 4720030d95d3..b52e904674ac (export)
(Tests are not in progress)
Cheers,
Matt
--
Sponsored by the NGI0 Core fund.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-12-22 10:44 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-12-15 9:12 [PATCH mptcp-net] mptcp: prevent tcp diag from closing listener subflows Paolo Abeni
2023-12-15 11:10 ` mptcp: prevent tcp diag from closing listener subflows: Tests Results MPTCP CI
2023-12-16 0:47 ` [PATCH mptcp-net] mptcp: prevent tcp diag from closing listener subflows Mat Martineau
2023-12-21 10:00 ` Paolo Abeni
2023-12-21 17:25 ` Mat Martineau
2023-12-22 10:44 ` Matthieu Baerts
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.