* [PATCH mptcp-net v3] mptcp: use entry->lsk directly
@ 2022-11-04 11:53 Geliang Tang
2022-11-04 13:53 ` mptcp: use entry->lsk directly: Tests Results MPTCP CI
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Geliang Tang @ 2022-11-04 11:53 UTC (permalink / raw)
To: mptcp; +Cc: Geliang Tang
entry->lsk should be used as the PM listening socket directly in
mptcp_pm_nl_create_listen_socket(), not the initial subflow socket.
This patch fixes this, use entry->lsk directly instead of ssock.
Fixes: 1729cf186d8a ("mptcp: create the listening socket for new port")
Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
v3:
- use mptcp-net tag, add Fixes tag, update commit log.
---
net/mptcp/pm_netlink.c | 18 ++----------------
1 file changed, 2 insertions(+), 16 deletions(-)
diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index 9813ed0fde9b..52a5847b9d74 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -992,8 +992,6 @@ static int mptcp_pm_nl_create_listen_socket(struct sock *sk,
{
int addrlen = sizeof(struct sockaddr_in);
struct sockaddr_storage addr;
- struct mptcp_sock *msk;
- struct socket *ssock;
int backlog = 1024;
int err;
@@ -1002,30 +1000,18 @@ static int mptcp_pm_nl_create_listen_socket(struct sock *sk,
if (err)
return err;
- msk = mptcp_sk(entry->lsk->sk);
- if (!msk) {
- err = -EINVAL;
- goto out;
- }
-
- ssock = __mptcp_nmpc_socket(msk);
- if (!ssock) {
- err = -EINVAL;
- goto out;
- }
-
mptcp_info2sockaddr(&entry->addr, &addr, entry->addr.family);
#if IS_ENABLED(CONFIG_MPTCP_IPV6)
if (entry->addr.family == AF_INET6)
addrlen = sizeof(struct sockaddr_in6);
#endif
- err = kernel_bind(ssock, (struct sockaddr *)&addr, addrlen);
+ err = kernel_bind(entry->lsk, (struct sockaddr *)&addr, addrlen);
if (err) {
pr_warn("kernel_bind error, err=%d", err);
goto out;
}
- err = kernel_listen(ssock, backlog);
+ err = kernel_listen(entry->lsk, backlog);
if (err) {
pr_warn("kernel_listen error, err=%d", err);
goto out;
--
2.35.3
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: mptcp: use entry->lsk directly: Tests Results
2022-11-04 11:53 [PATCH mptcp-net v3] mptcp: use entry->lsk directly Geliang Tang
@ 2022-11-04 13:53 ` MPTCP CI
2022-11-04 14:28 ` [PATCH mptcp-net v3] mptcp: use entry->lsk directly Matthieu Baerts
2022-11-04 15:57 ` Paolo Abeni
2 siblings, 0 replies; 5+ messages in thread
From: MPTCP CI @ 2022-11-04 13:53 UTC (permalink / raw)
To: Geliang Tang; +Cc: mptcp
Hi Geliang,
Thank you for your modifications, that's great!
Our CI did some validations and here is its report:
- KVM Validation: normal:
- Success! ✅:
- Task: https://cirrus-ci.com/task/5039064876318720
- Summary: https://api.cirrus-ci.com/v1/artifact/task/5039064876318720/summary/summary.txt
- KVM Validation: debug:
- Critical: 1 Call Trace(s) ❌:
- Task: https://cirrus-ci.com/task/6164964783161344
- Summary: https://api.cirrus-ci.com/v1/artifact/task/6164964783161344/summary/summary.txt
Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/3dd3db3d01db
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 (Tessares)
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH mptcp-net v3] mptcp: use entry->lsk directly
2022-11-04 11:53 [PATCH mptcp-net v3] mptcp: use entry->lsk directly Geliang Tang
2022-11-04 13:53 ` mptcp: use entry->lsk directly: Tests Results MPTCP CI
@ 2022-11-04 14:28 ` Matthieu Baerts
2022-11-04 15:57 ` Paolo Abeni
2 siblings, 0 replies; 5+ messages in thread
From: Matthieu Baerts @ 2022-11-04 14:28 UTC (permalink / raw)
To: Geliang Tang; +Cc: mptcp
Hi Geliang,
On 04/11/2022 12:53, Geliang Tang wrote:
> entry->lsk should be used as the PM listening socket directly in
> mptcp_pm_nl_create_listen_socket(), not the initial subflow socket.
>
> This patch fixes this, use entry->lsk directly instead of ssock.
I'm sorry but the commit message is still unclear to me: it doesn't
explain why "entry->lsk" should be used directly instead of ssock. Do
you mind adding this justification?
(The "reason" why a commit is needed is for me the most important info
to add in the commit message because it is not always obvious: here I
can clearly and quickly see you are using 'entry->lsk' directly when
looking at the diff but I cannot extract the "reason" that easily).
Please also note the public CI reported a possible recursive locking
issue with this patch when creating a listening socket on a different
port (single address with port):
- KVM Validation: debug:
- Critical: 1 Call Trace(s) ❌:
- Task: https://cirrus-ci.com/task/6164964783161344
- Summary:
https://api.cirrus-ci.com/v1/artifact/task/6164964783161344/summary/summary.txt
Cheers,
Matt
--
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH mptcp-net v3] mptcp: use entry->lsk directly
2022-11-04 11:53 [PATCH mptcp-net v3] mptcp: use entry->lsk directly Geliang Tang
2022-11-04 13:53 ` mptcp: use entry->lsk directly: Tests Results MPTCP CI
2022-11-04 14:28 ` [PATCH mptcp-net v3] mptcp: use entry->lsk directly Matthieu Baerts
@ 2022-11-04 15:57 ` Paolo Abeni
2022-11-04 18:15 ` Mat Martineau
2 siblings, 1 reply; 5+ messages in thread
From: Paolo Abeni @ 2022-11-04 15:57 UTC (permalink / raw)
To: Geliang Tang, mptcp
Hello,
On Fri, 2022-11-04 at 19:53 +0800, Geliang Tang wrote:
> entry->lsk should be used as the PM listening socket directly in
> mptcp_pm_nl_create_listen_socket(), not the initial subflow socket.
>
> This patch fixes this, use entry->lsk directly instead of ssock.
>
> Fixes: 1729cf186d8a ("mptcp: create the listening socket for new port")
> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
Sorry for the late review. I also don't understand exactly what bug is
fixed here. Could you please elaborate more - even in this thread,
before an eventual next version?
AFAICS, this patch introduces a functional change: before the msk
created by the NL PM was left in TCP_CLOSE status, while the tcp
subflow entered into the TCP_LISTENER mode.
After this patch even the msk status is updated to TCP_LISTENER, which
is possibly cleaner, and looks safe, but I don't see a bug fixed
here?!?
Cheers,
Paolo
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH mptcp-net v3] mptcp: use entry->lsk directly
2022-11-04 15:57 ` Paolo Abeni
@ 2022-11-04 18:15 ` Mat Martineau
0 siblings, 0 replies; 5+ messages in thread
From: Mat Martineau @ 2022-11-04 18:15 UTC (permalink / raw)
To: Paolo Abeni, Geliang Tang; +Cc: mptcp
On Fri, 4 Nov 2022, Paolo Abeni wrote:
> Hello,
>
> On Fri, 2022-11-04 at 19:53 +0800, Geliang Tang wrote:
>> entry->lsk should be used as the PM listening socket directly in
>> mptcp_pm_nl_create_listen_socket(), not the initial subflow socket.
>>
>> This patch fixes this, use entry->lsk directly instead of ssock.
>>
>> Fixes: 1729cf186d8a ("mptcp: create the listening socket for new port")
>> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
>
> Sorry for the late review. I also don't understand exactly what bug is
> fixed here. Could you please elaborate more - even in this thread,
> before an eventual next version?
>
> AFAICS, this patch introduces a functional change: before the msk
> created by the NL PM was left in TCP_CLOSE status, while the tcp
> subflow entered into the TCP_LISTENER mode.
>
> After this patch even the msk status is updated to TCP_LISTENER, which
> is possibly cleaner, and looks safe, but I don't see a bug fixed
> here?!?
>
I think this patch came up because Geliang was working on
https://github.com/multipath-tcp/mptcp_net-next/issues/313 and found that
the in-kernel listening sockets were only listening at the subflow level
and not the msk level.
In the v2 review I misread where the msk was coming from and thought there
was something buggy here - but now I see this code is dealing with the
MPTCP-level socket that was just created and added to the new
mptcp_pm_addr_entry. So, I agree (now) that this is not a bug fix, and
that it's "possibly cleaner and looks safe".
That still leaves an open question of whether to make this change in
mptcp-next for #313. An alternative is to have
mptcp_pm_nl_create_listen_socket() trigger MPTCP_EVENT_LISTENER_CREATED
after listening on the subflow rather than depending on
kernel_listen(msk, ...) to do it using this patch.
--
Mat Martineau
Intel
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-11-04 18:15 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-04 11:53 [PATCH mptcp-net v3] mptcp: use entry->lsk directly Geliang Tang
2022-11-04 13:53 ` mptcp: use entry->lsk directly: Tests Results MPTCP CI
2022-11-04 14:28 ` [PATCH mptcp-net v3] mptcp: use entry->lsk directly Matthieu Baerts
2022-11-04 15:57 ` Paolo Abeni
2022-11-04 18:15 ` Mat Martineau
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.