[PATCH] usbip: Close all listening sockets in request workers

[PATCH] usbip: Close all listening sockets in request workers

[raoxu]

From: Xu Rao <raoxu@uniontech.com>

process_request() forks a worker after accepting a connection, but the
child only closes the listener used for that connection. All other
listening sockets are inherited and remain open until the worker exits.
This occurs when usbipd listens on multiple addresses, including the
default IPv4 and IPv6 setup.

A worker waiting for a client request can therefore keep an unrelated
listener alive after the main daemon exits. Restarting usbipd then fails
to bind that address with EADDRINUSE, or starts with only the addresses
that are still available. A single-listener configuration is unaffected.

Pass the complete listening socket array and its count to
process_request(), and close every listener in the child before handling
the request. The accepted connection remains open, and the parent's
descriptors are unaffected. Make process_request() static as its only
caller is in the same file.

Fixes: 328f7f8a0a1c ("staging: usbip: userspace: eliminate glib dependency")
Cc: stable@vger.kernel.org
Signed-off-by: Xu Rao <raoxu@uniontech.com>
---
 tools/usb/usbip/src/usbipd.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/tools/usb/usbip/src/usbipd.c b/tools/usb/usbip/src/usbipd.c
index 3e22b651c754..5ff5b46674a2 100644
--- a/tools/usb/usbip/src/usbipd.c
+++ b/tools/usb/usbip/src/usbipd.c
@@ -339,17 +339,20 @@ static int do_accept(int listenfd)
 	return connfd;
 }

-int process_request(int listenfd)
+static int process_request(int listenfd, const int *listenfds, int nlistenfds)
 {
 	pid_t childpid;
 	int connfd;
+	int i;

 	connfd = do_accept(listenfd);
 	if (connfd < 0)
 		return -1;
 	childpid = fork();
 	if (childpid == 0) {
-		close(listenfd);
+		/* Do not keep unrelated listening sockets alive in workers. */
+		for (i = 0; i < nlistenfds; i++)
+			close(listenfds[i]);
 		recv_pdu(connfd);
 		exit(0);
 	}
@@ -568,7 +571,8 @@ static int do_standalone_mode(int daemonize, int ipv4, int ipv6)
 				if (fds[i].revents & POLLIN) {
 					dbg("read event on fd[%d]=%d",
 					    i, sockfdlist[i]);
-					process_request(sockfdlist[i]);
+					process_request(sockfdlist[i], sockfdlist,
+							nsockfd);
 				}
 			}
 		} else {
--
2.50.1

Re: [PATCH] usbip: Close all listening sockets in request workers

[Shuah Khan]

On 11/12/26 05:16, raoxu wrote:
> From: Xu Rao <raoxu@uniontech.com>
> 
> process_request() forks a worker after accepting a connection, but the
> child only closes the listener used for that connection. All other
> listening sockets are inherited and remain open until the worker exits.
> This occurs when usbipd listens on multiple addresses, including the
> default IPv4 and IPv6 setup.
> 
> A worker waiting for a client request can therefore keep an unrelated
> listener alive after the main daemon exits. Restarting usbipd then fails
> to bind that address with EADDRINUSE, or starts with only the addresses
> that are still available. A single-listener configuration is unaffected.
> 
> Pass the complete listening socket array and its count to
> process_request(), and close every listener in the child before handling
> the request. The accepted connection remains open, and the parent's
> descriptors are unaffected. Make process_request() static as its only
> caller is in the same file.

Looks reasonable to me. How did you find the problem? Can you share
how to reproduce this problem?

> 
> Fixes: 328f7f8a0a1c ("staging: usbip: userspace: eliminate glib dependency")
> Cc: stable@vger.kernel.org
> Signed-off-by: Xu Rao <raoxu@uniontech.com>
> ---
>   tools/usb/usbip/src/usbipd.c | 10 +++++++---
>   1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/usb/usbip/src/usbipd.c b/tools/usb/usbip/src/usbipd.c
> index 3e22b651c754..5ff5b46674a2 100644
> --- a/tools/usb/usbip/src/usbipd.c
> +++ b/tools/usb/usbip/src/usbipd.c
> @@ -339,17 +339,20 @@ static int do_accept(int listenfd)
>   	return connfd;
>   }
> 
> -int process_request(int listenfd)
> +static int process_request(int listenfd, const int *listenfds, int nlistenfds)
>   {
>   	pid_t childpid;
>   	int connfd;
> +	int i;
> 
>   	connfd = do_accept(listenfd);
>   	if (connfd < 0)
>   		return -1;
>   	childpid = fork();
>   	if (childpid == 0) {
> -		close(listenfd);
> +		/* Do not keep unrelated listening sockets alive in workers. */
> +		for (i = 0; i < nlistenfds; i++)
> +			close(listenfds[i]);
>   		recv_pdu(connfd);
>   		exit(0);
>   	}
> @@ -568,7 +571,8 @@ static int do_standalone_mode(int daemonize, int ipv4, int ipv6)
>   				if (fds[i].revents & POLLIN) {
>   					dbg("read event on fd[%d]=%d",
>   					    i, sockfdlist[i]);
> -					process_request(sockfdlist[i]);
> +					process_request(sockfdlist[i], sockfdlist,
> +							nsockfd);
>   				}
>   			}
>   		} else {
> --
> 2.50.1
> 

thanks,
-- Shuah

Re: [PATCH] usbip: Close all listening sockets in request workers

[raoxu]

Hi Shuah,

Thanks for the review.

I found this while reviewing the listening socket handling across fork() in
usbipd.c, and then reproduced the issue with the unmodified usbipd binary on
two UOS test machines.

Both machines have usbip 2.0+6.5.10-2 installed, with usbipd located at
/usr/sbin/usbipd. This package contains the original listener handling.

I used separate test ports to avoid interfering with the default USB/IP
service.

On 10.20.12.188, the reproduction steps were:

1. Start usbipd as root with both IPv4 and IPv6 listeners:

       sudo /usr/sbin/usbipd -d -4 -6 -t 43241

2. From a client machine, establish an IPv4 TCP connection and keep it open
   without sending a USB/IP request:

       python3 -c 'import socket,time; s=socket.create_connection(("10.20.12.188",43241)); time.sleep(300)'

3. Terminate only the main usbipd process with SIGTERM, leaving the worker
   running.

4. Inspect the listening sockets:

       ss -lntp 'sport = :43241'

5. Restart usbipd on the same port.

Before the connection, usbipd had two listeners:

    0.0.0.0:43241 -> parent PID 62216, fd 3
    [::]:43241    -> parent PID 62216, fd 4

After accepting the IPv4 connection, the forked worker inherited the IPv6
listener:

    [::]:43241 -> parent PID 62216, fd 4
                 worker PID 62223, fd 4

After terminating the parent, the worker continued holding that listener:

    [::]:43241 -> worker PID 62223, fd 4

Restarting usbipd then produced:

    usbipd: info: listening on 0.0.0.0:43241
    usbipd: error: bind: :::43241: 98 (Address already in use)
    usbipd: debug: listening on 1 address

The restarted daemon therefore had only the IPv4 listener available.

I reproduced the same behavior independently on 10.20.12.199 using port
43242:

    usbipd: info: listening on 0.0.0.0:43242
    usbipd: error: bind: :::43242: 98 (Address already in use)
    usbipd: debug: listening on 1 address

This confirms that the worker inherits the unrelated IPv6 listening socket
and keeps it alive after the parent daemon exits.

I also used a separate TCP socket/fork reproducer to compare the original and
patched process_request() logic. The original implementation left the
unrelated listening socket open in the worker, causing the rebind to fail
with EADDRINUSE. With the patched logic, the listener could be rebound while
the worker was still alive. The accepted connection remained usable and the
parent's descriptors were unchanged.

The affected usbipd source also builds successfully with:

    make -C tools/usb/usbip/src usbipd

These tests confirm the inherited listening socket issue and support closing
all listening sockets in the child after fork().

Thanks,
Xu

Re: [PATCH] usbip: Close all listening sockets in request workers

[Shuah Khan]

On 11/12/26 19:37, raoxu wrote:
> Hi Shuah,
> 
> Thanks for the review.
> 
> I found this while reviewing the listening socket handling across fork() in
> usbipd.c, and then reproduced the issue with the unmodified usbipd binary on
> two UOS test machines.
> 
> Both machines have usbip 2.0+6.5.10-2 installed, with usbipd located at
> /usr/sbin/usbipd. This package contains the original listener handling.
> 
> I used separate test ports to avoid interfering with the default USB/IP
> service.
> 
> On 10.20.12.188, the reproduction steps were:
> 
> 1. Start usbipd as root with both IPv4 and IPv6 listeners:
> 
>         sudo /usr/sbin/usbipd -d -4 -6 -t 43241
> 
> 2. From a client machine, establish an IPv4 TCP connection and keep it open
>     without sending a USB/IP request:
> 
>         python3 -c 'import socket,time; s=socket.create_connection(("10.20.12.188",43241)); time.sleep(300)'
> 
> 3. Terminate only the main usbipd process with SIGTERM, leaving the worker
>     running.

You removed all the context from the thread :(

In the following path in process_request(), child exits after
handling the pdu - all open fds get closed.

        childpid = fork();
         if (childpid == 0) {
                 close(listenfd);
                 recv_pdu(connfd);
                 exit(0);
         }

How does the problem you are describing happen in the normal
use-case when a child exits after handling the request. If
it doesn't there is no need for this change you are proposing.

thanks,
-- Shuah