From: Geliang Tang <tanggeliang@kylinos.cn>
The client-side function 'connect_one_server()' correctly closes the IPC
descriptor (a pipe or UNIX socket) after use. However, the server-side
functions 'process_one_client()' in both 'mptcp_sockopt.c' and 'mptcp_inq.c'
were missing the corresponding 'close()' call for their IPC descriptors.
This omission could lead to resource leaks (file descriptors) in the test
server processes over time.
This patch adds the missing 'close(pipefd)' and 'close(unixfd)' calls in the
server-side code, ensuring symmetric and correct resource cleanup.
Fixes: ce9979129a0b ("selftests: mptcp: add mptcp getsockopt test cases")
Fixes: b51880568f20 ("selftests: mptcp: add inq test case")
Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
tools/testing/selftests/net/mptcp/mptcp_inq.c | 1 +
tools/testing/selftests/net/mptcp/mptcp_sockopt.c | 1 +
2 files changed, 2 insertions(+)
diff --git a/tools/testing/selftests/net/mptcp/mptcp_inq.c b/tools/testing/selftests/net/mptcp/mptcp_inq.c
index 40f2a1b24763..6a282ec21fd7 100644
--- a/tools/testing/selftests/net/mptcp/mptcp_inq.c
+++ b/tools/testing/selftests/net/mptcp/mptcp_inq.c
@@ -462,6 +462,7 @@ static void process_one_client(int fd, int unixfd)
get_tcp_inq(&msg, &tcp_inq);
assert(tcp_inq == 1);
+ close(unixfd);
close(fd);
}
diff --git a/tools/testing/selftests/net/mptcp/mptcp_sockopt.c b/tools/testing/selftests/net/mptcp/mptcp_sockopt.c
index b44b6c9b0550..b616af36c16f 100644
--- a/tools/testing/selftests/net/mptcp/mptcp_sockopt.c
+++ b/tools/testing/selftests/net/mptcp/mptcp_sockopt.c
@@ -685,6 +685,7 @@ static void process_one_client(int fd, int pipefd)
s.last_sample.mptcpi_bytes_acked - ret2);
}
+ close(pipefd);
close(fd);
}
--
2.48.1Hi Geliang, On 03/09/2025 06:08, Geliang Tang wrote: > From: Geliang Tang <tanggeliang@kylinos.cn> > > The client-side function 'connect_one_server()' correctly closes the IPC > descriptor (a pipe or UNIX socket) after use. However, the server-side > functions 'process_one_client()' in both 'mptcp_sockopt.c' and 'mptcp_inq.c' > were missing the corresponding 'close()' call for their IPC descriptors. > > This omission could lead to resource leaks (file descriptors) in the test > server processes over time. > > This patch adds the missing 'close(pipefd)' and 'close(unixfd)' calls in the > server-side code, ensuring symmetric and correct resource cleanup. I don't know if we need such patch. I mean: yes, that's better to close such FD before closing the application, but then: - it is strange to close it in the middle of a function (process_one_client()), and not where it has been created (main()) - if I'm not mistaken, unixfds[0] / pipefds[0] are not closed in the server process. So if you really want to fix that, I think it would be better to close all these unix FD in the 'main()' function, no? Cheers, Matt -- Sponsored by the NGI0 Core fund.
Hi Matt,
On Wed, 2025-09-03 at 13:57 +0200, Matthieu Baerts wrote:
> Hi Geliang,
>
> On 03/09/2025 06:08, Geliang Tang wrote:
> > From: Geliang Tang <tanggeliang@kylinos.cn>
> >
> > The client-side function 'connect_one_server()' correctly closes
> > the IPC
> > descriptor (a pipe or UNIX socket) after use. However, the server-
> > side
> > functions 'process_one_client()' in both 'mptcp_sockopt.c' and
> > 'mptcp_inq.c'
> > were missing the corresponding 'close()' call for their IPC
> > descriptors.
> >
> > This omission could lead to resource leaks (file descriptors) in
> > the test
> > server processes over time.
> >
> > This patch adds the missing 'close(pipefd)' and 'close(unixfd)'
> > calls in the
> > server-side code, ensuring symmetric and correct resource cleanup.
>
> I don't know if we need such patch. I mean: yes, that's better to
> close
> such FD before closing the application, but then:
>
> - it is strange to close it in the middle of a function
> (process_one_client()), and not where it has been created (main())
>
Yes, close it in main() is much better. I added close(pipefds[1]) after
calling server(pipefds[1]) in the new version instead of adding
close(pipefd) in process_one_client().
> - if I'm not mistaken, unixfds[0] / pipefds[0] are not closed in the
> server process.
Yes, indeed. close(pipefds[0]) is missing in the server process too.
> So if you really want to fix that, I think it would be better to
> close
> all these unix FD in the 'main()' function, no?
I also moved 'close(pipefd)' from connect_one_server() to main(), just
after calling client(pipefds[0]). And here's the new version:
e1 = pipe(pipefds);
if (e1 < 0)
die_perror("pipe");
s = xfork();
if (s == 0) {
close(pipefds[0]);
ret = server(pipefds[1]);
close(pipefds[1]);
return ret;
}
close(pipefds[1]);
/* wait until server bound a socket */
e1 = read(pipefds[0], &e1, 4);
assert(e1 == 4);
c = xfork();
if (c == 0) {
ret = client(pipefds[0]);
close(pipefds[0]);
return ret;
}
close(pipefds[0]);
Is this better?
Thanks,
-Geliang
>
> Cheers,
> Matt
Hi Geliang,
On 04/09/2025 10:26, Geliang Tang wrote:
> Hi Matt,
>
> On Wed, 2025-09-03 at 13:57 +0200, Matthieu Baerts wrote:
>> Hi Geliang,
>>
>> On 03/09/2025 06:08, Geliang Tang wrote:
>>> From: Geliang Tang <tanggeliang@kylinos.cn>
>>>
>>> The client-side function 'connect_one_server()' correctly closes
>>> the IPC
>>> descriptor (a pipe or UNIX socket) after use. However, the server-
>>> side
>>> functions 'process_one_client()' in both 'mptcp_sockopt.c' and
>>> 'mptcp_inq.c'
>>> were missing the corresponding 'close()' call for their IPC
>>> descriptors.
>>>
>>> This omission could lead to resource leaks (file descriptors) in
>>> the test
>>> server processes over time.
>>>
>>> This patch adds the missing 'close(pipefd)' and 'close(unixfd)'
>>> calls in the
>>> server-side code, ensuring symmetric and correct resource cleanup.
>>
>> I don't know if we need such patch. I mean: yes, that's better to
>> close
>> such FD before closing the application, but then:
>>
>> - it is strange to close it in the middle of a function
>> (process_one_client()), and not where it has been created (main())
>>
>
> Yes, close it in main() is much better. I added close(pipefds[1]) after
> calling server(pipefds[1]) in the new version instead of adding
> close(pipefd) in process_one_client().
>
>> - if I'm not mistaken, unixfds[0] / pipefds[0] are not closed in the
>> server process.
>
> Yes, indeed. close(pipefds[0]) is missing in the server process too.
>
>> So if you really want to fix that, I think it would be better to
>> close
>> all these unix FD in the 'main()' function, no?
>
> I also moved 'close(pipefd)' from connect_one_server() to main(), just
> after calling client(pipefds[0]). And here's the new version:
>
> e1 = pipe(pipefds);
> if (e1 < 0)
> die_perror("pipe");
>
> s = xfork();
> if (s == 0) {
> close(pipefds[0]);
> ret = server(pipefds[1]);
> close(pipefds[1]);
> return ret;
> }
>
> close(pipefds[1]);
>
> /* wait until server bound a socket */
> e1 = read(pipefds[0], &e1, 4);
> assert(e1 == 4);
>
> c = xfork();
> if (c == 0) {
> ret = client(pipefds[0]);
> close(pipefds[0]);
> return ret;
> }
>
> close(pipefds[0]);
>
> Is this better?
(sorry, I thought I replied...)
Yes, that looks more manageable like that: all close() by the "function
owner" (where they have been opened).
Also, please don't bother about closing all FD in the error exit paths,
e.g. in xfork(), etc. when xerror() is called. These are more unexpected
errors, and closing everything properly is this case, for the selftests
is not a priority, and might cause issues if the code is more complex (+
issues for the backports, etc.)
Cheers,
Matt
--
Sponsored by the NGI0 Core fund.
Hi Matt,
On Wed, 2025-09-10 at 19:00 +0200, Matthieu Baerts wrote:
> Hi Geliang,
>
> On 04/09/2025 10:26, Geliang Tang wrote:
> > Hi Matt,
> >
> > On Wed, 2025-09-03 at 13:57 +0200, Matthieu Baerts wrote:
> > > Hi Geliang,
> > >
> > > On 03/09/2025 06:08, Geliang Tang wrote:
> > > > From: Geliang Tang <tanggeliang@kylinos.cn>
> > > >
> > > > The client-side function 'connect_one_server()' correctly
> > > > closes
> > > > the IPC
> > > > descriptor (a pipe or UNIX socket) after use. However, the
> > > > server-
> > > > side
> > > > functions 'process_one_client()' in both 'mptcp_sockopt.c' and
> > > > 'mptcp_inq.c'
> > > > were missing the corresponding 'close()' call for their IPC
> > > > descriptors.
> > > >
> > > > This omission could lead to resource leaks (file descriptors)
> > > > in
> > > > the test
> > > > server processes over time.
> > > >
> > > > This patch adds the missing 'close(pipefd)' and 'close(unixfd)'
> > > > calls in the
> > > > server-side code, ensuring symmetric and correct resource
> > > > cleanup.
> > >
> > > I don't know if we need such patch. I mean: yes, that's better to
> > > close
> > > such FD before closing the application, but then:
> > >
> > > - it is strange to close it in the middle of a function
> > > (process_one_client()), and not where it has been created
> > > (main())
> > >
> >
> > Yes, close it in main() is much better. I added close(pipefds[1])
> > after
> > calling server(pipefds[1]) in the new version instead of adding
> > close(pipefd) in process_one_client().
> >
> > > - if I'm not mistaken, unixfds[0] / pipefds[0] are not closed in
> > > the
> > > server process.
> >
> > Yes, indeed. close(pipefds[0]) is missing in the server process
> > too.
> >
> > > So if you really want to fix that, I think it would be better to
> > > close
> > > all these unix FD in the 'main()' function, no?
> >
> > I also moved 'close(pipefd)' from connect_one_server() to main(),
> > just
> > after calling client(pipefds[0]). And here's the new version:
> >
> > e1 = pipe(pipefds);
> > if (e1 < 0)
> > die_perror("pipe");
> >
> > s = xfork();
> > if (s == 0) {
> > close(pipefds[0]);
> > ret = server(pipefds[1]);
> > close(pipefds[1]);
> > return ret;
> > }
> >
> > close(pipefds[1]);
> >
> > /* wait until server bound a socket */
> > e1 = read(pipefds[0], &e1, 4);
> > assert(e1 == 4);
> >
> > c = xfork();
> > if (c == 0) {
> > ret = client(pipefds[0]);
> > close(pipefds[0]);
> > return ret;
> > }
> >
> > close(pipefds[0]);
> >
> > Is this better?
>
> (sorry, I thought I replied...)
>
> Yes, that looks more manageable like that: all close() by the
> "function
> owner" (where they have been opened).
>
> Also, please don't bother about closing all FD in the error exit
> paths,
> e.g. in xfork(), etc. when xerror() is called. These are more
> unexpected
> errors, and closing everything properly is this case, for the
> selftests
> is not a priority, and might cause issues if the code is more complex
> (+
> issues for the backports, etc.)
I agree, the priority is really low. I think there is no need to
backport these three patches. Should I change them to -next from -net
in v3 and treat them as cleanups instead of fixes?
Thanks,
-Geliang
>
> Cheers,
> Matt
Hi Geliang,
On 11/09/2025 11:13, Geliang Tang wrote:
> Hi Matt,
>
> On Wed, 2025-09-10 at 19:00 +0200, Matthieu Baerts wrote:
>> Hi Geliang,
>>
>> On 04/09/2025 10:26, Geliang Tang wrote:
>>> Hi Matt,
>>>
>>> On Wed, 2025-09-03 at 13:57 +0200, Matthieu Baerts wrote:
>>>> Hi Geliang,
>>>>
>>>> On 03/09/2025 06:08, Geliang Tang wrote:
>>>>> From: Geliang Tang <tanggeliang@kylinos.cn>
>>>>>
>>>>> The client-side function 'connect_one_server()' correctly
>>>>> closes
>>>>> the IPC
>>>>> descriptor (a pipe or UNIX socket) after use. However, the
>>>>> server-
>>>>> side
>>>>> functions 'process_one_client()' in both 'mptcp_sockopt.c' and
>>>>> 'mptcp_inq.c'
>>>>> were missing the corresponding 'close()' call for their IPC
>>>>> descriptors.
>>>>>
>>>>> This omission could lead to resource leaks (file descriptors)
>>>>> in
>>>>> the test
>>>>> server processes over time.
>>>>>
>>>>> This patch adds the missing 'close(pipefd)' and 'close(unixfd)'
>>>>> calls in the
>>>>> server-side code, ensuring symmetric and correct resource
>>>>> cleanup.
>>>>
>>>> I don't know if we need such patch. I mean: yes, that's better to
>>>> close
>>>> such FD before closing the application, but then:
>>>>
>>>> - it is strange to close it in the middle of a function
>>>> (process_one_client()), and not where it has been created
>>>> (main())
>>>>
>>>
>>> Yes, close it in main() is much better. I added close(pipefds[1])
>>> after
>>> calling server(pipefds[1]) in the new version instead of adding
>>> close(pipefd) in process_one_client().
>>>
>>>> - if I'm not mistaken, unixfds[0] / pipefds[0] are not closed in
>>>> the
>>>> server process.
>>>
>>> Yes, indeed. close(pipefds[0]) is missing in the server process
>>> too.
>>>
>>>> So if you really want to fix that, I think it would be better to
>>>> close
>>>> all these unix FD in the 'main()' function, no?
>>>
>>> I also moved 'close(pipefd)' from connect_one_server() to main(),
>>> just
>>> after calling client(pipefds[0]). And here's the new version:
>>>
>>> e1 = pipe(pipefds);
>>> if (e1 < 0)
>>> die_perror("pipe");
>>>
>>> s = xfork();
>>> if (s == 0) {
>>> close(pipefds[0]);
>>> ret = server(pipefds[1]);
>>> close(pipefds[1]);
>>> return ret;
>>> }
>>>
>>> close(pipefds[1]);
>>>
>>> /* wait until server bound a socket */
>>> e1 = read(pipefds[0], &e1, 4);
>>> assert(e1 == 4);
>>>
>>> c = xfork();
>>> if (c == 0) {
>>> ret = client(pipefds[0]);
>>> close(pipefds[0]);
>>> return ret;
>>> }
>>>
>>> close(pipefds[0]);
>>>
>>> Is this better?
>>
>> (sorry, I thought I replied...)
>>
>> Yes, that looks more manageable like that: all close() by the
>> "function
>> owner" (where they have been opened).
>>
>> Also, please don't bother about closing all FD in the error exit
>> paths,
>> e.g. in xfork(), etc. when xerror() is called. These are more
>> unexpected
>> errors, and closing everything properly is this case, for the
>> selftests
>> is not a priority, and might cause issues if the code is more complex
>> (+
>> issues for the backports, etc.)
>
> I agree, the priority is really low. I think there is no need to
> backport these three patches. Should I change them to -next from -net
> in v3 and treat them as cleanups instead of fixes?
Good point. For the selftests, if the problems you identified are not
causing issues or confusions, probably best not to target net then.
So here, if fd's should have been closed just before exiting, that's
fine, no real harm. We would have backported a patch if fd were reused
but not closed, or many connections were created one after the other,
but not closed, etc. → there the impact could be visible.
For the 3rd patch, it is different, it might be interesting to backport
it if error messages are wrong or confusing.
Cheers,
Matt
--
Sponsored by the NGI0 Core fund.
Hi Geliang, On 03/09/2025 13:57, Matthieu Baerts wrote: > Hi Geliang, > > On 03/09/2025 06:08, Geliang Tang wrote: >> From: Geliang Tang <tanggeliang@kylinos.cn> >> >> The client-side function 'connect_one_server()' correctly closes the IPC >> descriptor (a pipe or UNIX socket) after use. However, the server-side >> functions 'process_one_client()' in both 'mptcp_sockopt.c' and 'mptcp_inq.c' FYI, checkpatch is complaining that this line is too long. https://github.com/multipath-tcp/mptcp_net-next/actions/runs/17423040753 Because it is not an output of a log, can you go to the next line after max 72 chars please? (I didn't check, but there are other similar errors reported by checkpatch. *If they make sense*, don't hesitate to fix them as well) Cheers, Matt -- Sponsored by the NGI0 Core fund.
© 2016 - 2026 Red Hat, Inc.