netdev - Re: [PATCH net] SUNRPC: Fix local socket leak in xs_local_setup

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <d013bdc75085e380250cb79edf2b27680cbc9f7e.camel@hammerspace.com>
Date:   Tue, 26 Apr 2022 18:51:45 +0000
From:   Trond Myklebust <trondmy@...merspace.com>
To:     "anna@...nel.org" <anna@...nel.org>,
        "pabeni@...hat.com" <pabeni@...hat.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "wanghai38@...wei.com" <wanghai38@...wei.com>,
        "chuck.lever@...cle.com" <chuck.lever@...cle.com>,
        "kuba@...nel.org" <kuba@...nel.org>
CC:     "linux-nfs@...r.kernel.org" <linux-nfs@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net] SUNRPC: Fix local socket leak in
 xs_local_setup_socket()

On Tue, 2022-04-26 at 21:20 +0800, Wang Hai wrote:
> If the connection to a local endpoint in xs_local_setup_socket()
> fails,
> fput() is missing in the error path, which will result in a socket
> leak.
> It can be reproduced in simple script below.
> 
> while true
> do
>         systemctl stop rpcbind.service
>         systemctl stop rpc-statd.service
>         systemctl stop nfs-server.service
> 
>         systemctl restart rpcbind.service
>         systemctl restart rpc-statd.service
>         systemctl restart nfs-server.service
> done
> 
> When executing the script, you can observe that the
> "cat /proc/net/unix | wc -l" count keeps growing.
> 
> Add the missing fput(), and restore transport to old socket.
> 
> Signed-off-by: Wang Hai <wanghai38@...wei.com>
> ---
>  net/sunrpc/xprtsock.c | 20 ++++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
> index 0f39e08ee580..7219c545385e 100644
> --- a/net/sunrpc/xprtsock.c
> +++ b/net/sunrpc/xprtsock.c
> @@ -1819,6 +1819,9 @@ static int xs_local_finish_connecting(struct
> rpc_xprt *xprt,
>  {
>         struct sock_xprt *transport = container_of(xprt, struct
> sock_xprt,
>                                                                      
>    xprt);
> +       struct socket *trans_sock = NULL;
> +       struct sock *trans_inet = NULL;
> +       int ret;
>  
>         if (!transport->inet) {
>                 struct sock *sk = sock->sk;
> @@ -1835,6 +1838,9 @@ static int xs_local_finish_connecting(struct
> rpc_xprt *xprt,
>  
>                 xprt_clear_connected(xprt);
>  
> +               trans_sock = transport->sock;
> +               trans_inet = transport->inet;
> +

Both values are NULL here

>                 /* Reset to new socket */
>                 transport->sock = sock;
>                 transport->inet = sk;
> @@ -1844,7 +1850,14 @@ static int xs_local_finish_connecting(struct
> rpc_xprt *xprt,
>  
>         xs_stream_start_connect(transport);
>  
> -       return kernel_connect(sock, xs_addr(xprt), xprt->addrlen, 0);
> +       ret = kernel_connect(sock, xs_addr(xprt), xprt->addrlen, 0);
> +       /* Restore to old socket */
> +       if (ret && trans_inet) {
> +               transport->sock = trans_sock;
> +               transport->inet = trans_inet;
> +       }
> +
> +       return ret;
>  }
>  
>  /**
> @@ -1887,7 +1900,7 @@ static int xs_local_setup_socket(struct
> sock_xprt *transport)
>                 xprt->stat.connect_time += (long)jiffies -
>                                            xprt->stat.connect_start;
>                 xprt_set_connected(xprt);
> -               break;
> +               goto out;
>         case -ENOBUFS:
>                 break;
>         case -ENOENT:
> @@ -1904,6 +1917,9 @@ static int xs_local_setup_socket(struct
> sock_xprt *transport)
>                                 xprt-
> >address_strings[RPC_DISPLAY_ADDR]);
>         }
>  
> +       transport->file = NULL;
> +       fput(filp);

Please just call xprt_force_disconnect() so that this can be cleaned up
from a safe context.

> +
>  out:
>         xprt_clear_connecting(xprt);
>         xprt_wake_pending_tasks(xprt, status);

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@...merspace.com