Re: [PATCH mptcp-net v2 2/3] selftests: mptcp: close IPC descriptor on server side

[Matthieu Baerts <matttbe@kernel.org>]

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.