[PATCH 0/3] Refix the socket leak in xs_setup_local()

[PATCH 0/3] Refix the socket leak in xs_setup_local()

[Wang Hai]

Patch1 and patch2 roll back the wrong solution to fix socket leaks.

Patch3 adds safe teardown mechanism to re-fix socket leaks.

Wang Hai (3):
  Revert "SUNRPC: Ensure gss-proxy connects on setup"
  Revert "Revert "SUNRPC: attempt AF_LOCAL connect on setup""
  SUNRPC: Fix local socket leak in xs_setup_local()

 include/linux/sunrpc/clnt.h          |  1 -
 net/sunrpc/auth_gss/gss_rpc_upcall.c |  2 +-
 net/sunrpc/clnt.c                    |  3 ---
 net/sunrpc/xprtsock.c                | 19 ++++++++++++++++++-
 4 files changed, 19 insertions(+), 6 deletions(-)

-- 
2.17.1

[PATCH 1/3] Revert "SUNRPC: Ensure gss-proxy connects on setup"

[Wang Hai]

This reverts commit 892de36fd4a98fab3298d417c051d9099af5448d.

In fact, gss-proxy can't respond to rpc_ping() at setup time.
After the timeout, rpc_ping() will return -EIO instead of -EOPNOTSUPP.
The reverted patch can't ensure that gssp connects to the service
at setup time.

Signed-off-by: Wang Hai <wanghai38@huawei.com>
---
 include/linux/sunrpc/clnt.h          | 1 -
 net/sunrpc/auth_gss/gss_rpc_upcall.c | 2 +-
 net/sunrpc/clnt.c                    | 3 ---
 3 files changed, 1 insertion(+), 5 deletions(-)

diff --git a/include/linux/sunrpc/clnt.h b/include/linux/sunrpc/clnt.h
index db5149567305..267b7aeaf1a6 100644
--- a/include/linux/sunrpc/clnt.h
+++ b/include/linux/sunrpc/clnt.h
@@ -160,7 +160,6 @@ struct rpc_add_xprt_test {
 #define RPC_CLNT_CREATE_NO_RETRANS_TIMEOUT	(1UL << 9)
 #define RPC_CLNT_CREATE_SOFTERR		(1UL << 10)
 #define RPC_CLNT_CREATE_REUSEPORT	(1UL << 11)
-#define RPC_CLNT_CREATE_IGNORE_NULL_UNAVAIL (1UL << 12)
 
 struct rpc_clnt *rpc_create(struct rpc_create_args *args);
 struct rpc_clnt	*rpc_bind_new_program(struct rpc_clnt *,
diff --git a/net/sunrpc/auth_gss/gss_rpc_upcall.c b/net/sunrpc/auth_gss/gss_rpc_upcall.c
index 8ca1d809b78d..61c276bddaf2 100644
--- a/net/sunrpc/auth_gss/gss_rpc_upcall.c
+++ b/net/sunrpc/auth_gss/gss_rpc_upcall.c
@@ -97,7 +97,7 @@ static int gssp_rpc_create(struct net *net, struct rpc_clnt **_clnt)
 		 * timeout, which would result in reconnections being
 		 * done without the correct namespace:
 		 */
-		.flags		= RPC_CLNT_CREATE_IGNORE_NULL_UNAVAIL |
+		.flags		= RPC_CLNT_CREATE_NOPING |
 				  RPC_CLNT_CREATE_NO_IDLE_TIMEOUT
 	};
 	struct rpc_clnt *clnt;
diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index 22c28cf43eba..98133aa54f19 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -479,9 +479,6 @@ static struct rpc_clnt *rpc_create_xprt(struct rpc_create_args *args,
 
 	if (!(args->flags & RPC_CLNT_CREATE_NOPING)) {
 		int err = rpc_ping(clnt);
-		if ((args->flags & RPC_CLNT_CREATE_IGNORE_NULL_UNAVAIL) &&
-		    err == -EOPNOTSUPP)
-			err = 0;
 		if (err != 0) {
 			rpc_shutdown_client(clnt);
 			return ERR_PTR(err);
-- 
2.17.1

[PATCH 2/3] Revert "Revert "SUNRPC: attempt AF_LOCAL connect on setup""

[Wang Hai]

This reverts commit a3d0562d4dc039bca39445e1cddde7951662e17d.

There is currently no better way to ensure that gss-proxy connects when
setup. Therefore, it is still necessary to connect in the construction.
The mechanism to safely tear xprt down needs to be implemented later.

Signed-off-by: Wang Hai <wanghai38@huawei.com>
---
 net/sunrpc/xprtsock.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
index 650102a9c86a..25b8a8ead56b 100644
--- a/net/sunrpc/xprtsock.c
+++ b/net/sunrpc/xprtsock.c
@@ -2875,6 +2875,9 @@ static struct rpc_xprt *xs_setup_local(struct xprt_create *args)
 		}
 		xprt_set_bound(xprt);
 		xs_format_peer_addresses(xprt, "local", RPCBIND_NETID_LOCAL);
+		ret = ERR_PTR(xs_local_setup_socket(transport));
+		if (ret)
+			goto out_err;
 		break;
 	default:
 		ret = ERR_PTR(-EAFNOSUPPORT);
-- 
2.17.1

[PATCH 3/3] SUNRPC: Fix local socket leak in xs_setup_local()

[Wang Hai]

Add mechanism in xs_setup_local() to safely tear xprt down and
solve the socket leak problem.

Signed-off-by: Wang Hai <wanghai38@huawei.com>
---
 net/sunrpc/xprtsock.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
index 25b8a8ead56b..7b9a62750571 100644
--- a/net/sunrpc/xprtsock.c
+++ b/net/sunrpc/xprtsock.c
@@ -1264,6 +1264,18 @@ static void xs_xprt_free(struct rpc_xprt *xprt)
 	xprt_free(xprt);
 }
 
+/**
+ * xs_destroy_xprt - close xprt socket and destroy it.
+ */
+static void xs_destroy_xprt(struct work_struct *work)
+{
+	struct rpc_xprt *xprt =
+		container_of(work, struct rpc_xprt, task_cleanup);
+
+	xs_close(xprt);
+	xs_xprt_free(xprt);
+}
+
 /**
  * xs_destroy - prepare to shutdown a transport
  * @xprt: doomed transport
@@ -2891,7 +2903,9 @@ static struct rpc_xprt *xs_setup_local(struct xprt_create *args)
 		return xprt;
 	ret = ERR_PTR(-EINVAL);
 out_err:
-	xs_xprt_free(xprt);
+
+	INIT_WORK(&xprt->task_cleanup, xs_destroy_xprt);
+	schedule_work(&xprt->task_cleanup);
 	return ret;
 }
 
-- 
2.17.1

Re: [PATCH 0/3] Refix the socket leak in xs_setup_local()

[Trond Myklebust]

On Wed, 2022-05-11 at 21:22 +0800, Wang Hai wrote:
> Patch1 and patch2 roll back the wrong solution to fix socket leaks.
> 
> Patch3 adds safe teardown mechanism to re-fix socket leaks.
> 
> Wang Hai (3):
>   Revert "SUNRPC: Ensure gss-proxy connects on setup"
>   Revert "Revert "SUNRPC: attempt AF_LOCAL connect on setup""
>   SUNRPC: Fix local socket leak in xs_setup_local()
> 
>  include/linux/sunrpc/clnt.h          |  1 -
>  net/sunrpc/auth_gss/gss_rpc_upcall.c |  2 +-
>  net/sunrpc/clnt.c                    |  3 ---
>  net/sunrpc/xprtsock.c                | 19 ++++++++++++++++++-
>  4 files changed, 19 insertions(+), 6 deletions(-)
> 

Thanks, but there is already a fix for this queued up in the linux-next
branch.

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

Re: [PATCH 0/3] Refix the socket leak in xs_setup_local()

[wanghai (M)]

在 2022/5/12 0:14, Trond Myklebust 写道:
> On Wed, 2022-05-11 at 21:22 +0800, Wang Hai wrote:
>> Patch1 and patch2 roll back the wrong solution to fix socket leaks.
>>
>> Patch3 adds safe teardown mechanism to re-fix socket leaks.
>>
>> Wang Hai (3):
>>    Revert "SUNRPC: Ensure gss-proxy connects on setup"
>>    Revert "Revert "SUNRPC: attempt AF_LOCAL connect on setup""
>>    SUNRPC: Fix local socket leak in xs_setup_local()
>>
>>   include/linux/sunrpc/clnt.h          |  1 -
>>   net/sunrpc/auth_gss/gss_rpc_upcall.c |  2 +-
>>   net/sunrpc/clnt.c                    |  3 ---
>>   net/sunrpc/xprtsock.c                | 19 ++++++++++++++++++-
>>   4 files changed, 19 insertions(+), 6 deletions(-)
>>
> Thanks, but there is already a fix for this queued up in the linux-next
> branch.
Thanks, sorry to bother you, I found it.

I only subscribed to the linux-kernel@vger.kernel.org mailing list,
not the linux-nfs@vger.kernel.org mailing list separately. So I
didn't notice it was fixed.

-- 
Wang Hai