[PATCH v2 1/4] SUNRPC: Don't leak sockets in xs_local_connect()

[PATCH v2 1/4] SUNRPC: Don't leak sockets in xs_local_connect()

[trondmy]

From: Trond Myklebust <trond.myklebust@hammerspace.com>

If there is still a closed socket associated with the transport, then we
need to trigger an autoclose before we can set up a new connection.

Reported-by: wanghai (M) <wanghai38@huawei.com>
Fixes: f00432063db1 ("SUNRPC: Ensure we flush any closed sockets before xs_xprt_free()")
Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
---
 net/sunrpc/xprtsock.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
index 8ab64ea46870..f9849b297ea3 100644
--- a/net/sunrpc/xprtsock.c
+++ b/net/sunrpc/xprtsock.c
@@ -1950,6 +1950,9 @@ static void xs_local_connect(struct rpc_xprt *xprt, struct rpc_task *task)
 	struct sock_xprt *transport = container_of(xprt, struct sock_xprt, xprt);
 	int ret;
 
+	if (transport->file)
+		goto force_disconnect;
+
 	if (RPC_IS_ASYNC(task)) {
 		/*
 		 * We want the AF_LOCAL connect to be resolved in the
@@ -1962,11 +1965,17 @@ static void xs_local_connect(struct rpc_xprt *xprt, struct rpc_task *task)
 		 */
 		task->tk_rpc_status = -ENOTCONN;
 		rpc_exit(task, -ENOTCONN);
-		return;
+		goto out_wake;
 	}
 	ret = xs_local_setup_socket(transport);
 	if (ret && !RPC_IS_SOFTCONN(task))
 		msleep_interruptible(15000);
+	return;
+force_disconnect:
+	xprt_force_disconnect(xprt);
+out_wake:
+	xprt_clear_connecting(xprt);
+	xprt_wake_pending_tasks(xprt, -ENOTCONN);
 }
 
 #if IS_ENABLED(CONFIG_SUNRPC_SWAP)
-- 
2.35.1

[PATCH v2 2/4] SUNRPC: Ensure timely close of disconnected AF_LOCAL sockets

[trondmy]

From: Trond Myklebust <trond.myklebust@hammerspace.com>

When the rpcbind server closes the socket, we need to ensure that the
socket is closed by the kernel as soon as feasible, so add a
sk_state_change callback to trigger this close.

Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
---
 net/sunrpc/xprtsock.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
index f9849b297ea3..25b8a8ead56b 100644
--- a/net/sunrpc/xprtsock.c
+++ b/net/sunrpc/xprtsock.c
@@ -1418,6 +1418,26 @@ static size_t xs_tcp_bc_maxpayload(struct rpc_xprt *xprt)
 }
 #endif /* CONFIG_SUNRPC_BACKCHANNEL */
 
+/**
+ * xs_local_state_change - callback to handle AF_LOCAL socket state changes
+ * @sk: socket whose state has changed
+ *
+ */
+static void xs_local_state_change(struct sock *sk)
+{
+	struct rpc_xprt *xprt;
+	struct sock_xprt *transport;
+
+	if (!(xprt = xprt_from_sock(sk)))
+		return;
+	transport = container_of(xprt, struct sock_xprt, xprt);
+	if (sk->sk_shutdown & SHUTDOWN_MASK) {
+		clear_bit(XPRT_CONNECTED, &xprt->state);
+		/* Trigger the socket release */
+		xs_run_error_worker(transport, XPRT_SOCK_WAKE_DISCONNECT);
+	}
+}
+
 /**
  * xs_tcp_state_change - callback to handle TCP socket state changes
  * @sk: socket whose state has changed
@@ -1866,6 +1886,7 @@ static int xs_local_finish_connecting(struct rpc_xprt *xprt,
 		sk->sk_user_data = xprt;
 		sk->sk_data_ready = xs_data_ready;
 		sk->sk_write_space = xs_udp_write_space;
+		sk->sk_state_change = xs_local_state_change;
 		sk->sk_error_report = xs_error_report;
 
 		xprt_clear_connected(xprt);
-- 
2.35.1

[PATCH v2 3/4] SUNRPC: Ensure gss-proxy connects on setup

[trondmy]

From: Trond Myklebust <trond.myklebust@hammerspace.com>

For reasons best known to the author, gss-proxy does not implement a
NULL procedure, and returns RPC_PROC_UNAVAIL. However we still want to
ensure that we connect to the service at setup time.
So add a quirk-flag specially for this case.

Fixes: 1d658336b05f ("SUNRPC: Add RPC based upcall mechanism for RPCGSS auth")
Cc: stable@vger.kernel.org
Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
---
 include/linux/sunrpc/clnt.h          | 1 +
 net/sunrpc/auth_gss/gss_rpc_upcall.c | 2 +-
 net/sunrpc/clnt.c                    | 3 +++
 3 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/include/linux/sunrpc/clnt.h b/include/linux/sunrpc/clnt.h
index 267b7aeaf1a6..db5149567305 100644
--- a/include/linux/sunrpc/clnt.h
+++ b/include/linux/sunrpc/clnt.h
@@ -160,6 +160,7 @@ 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 61c276bddaf2..8ca1d809b78d 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_NOPING |
+		.flags		= RPC_CLNT_CREATE_IGNORE_NULL_UNAVAIL |
 				  RPC_CLNT_CREATE_NO_IDLE_TIMEOUT
 	};
 	struct rpc_clnt *clnt;
diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index 98133aa54f19..22c28cf43eba 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -479,6 +479,9 @@ 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.35.1

[PATCH v2 4/4] Revert "SUNRPC: attempt AF_LOCAL connect on setup"

[trondmy]

From: Trond Myklebust <trond.myklebust@hammerspace.com>

This reverts commit 7073ea8799a8cf73db60270986f14e4aae20fa80.

We must not try to connect the socket while the transport is under
construction, because the mechanisms to safely tear it down are not in
place.

Cc: stable@vger.kernel.org
Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
---
 net/sunrpc/xprtsock.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
index 25b8a8ead56b..650102a9c86a 100644
--- a/net/sunrpc/xprtsock.c
+++ b/net/sunrpc/xprtsock.c
@@ -2875,9 +2875,6 @@ 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.35.1

Re: [PATCH v2 4/4] Revert "SUNRPC: attempt AF_LOCAL connect on setup"

[Trond Myklebust]

On Fri, 2022-04-29 at 13:36 -0400, trondmy@kernel.org wrote:
> From: Trond Myklebust <trond.myklebust@hammerspace.com>
> 
> This reverts commit 7073ea8799a8cf73db60270986f14e4aae20fa80.
> 
> We must not try to connect the socket while the transport is under
> construction, because the mechanisms to safely tear it down are not
> in
> place.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>

Sorry. I intended to add a

"Reported-by: wanghai (M) <wanghai38@huawei.com>"

That has been added to the version in my "testing" branch.


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

Re: [PATCH v2 4/4] Revert "SUNRPC: attempt AF_LOCAL connect on setup"

[wanghai (M)]

在 2022/4/30 8:56, Trond Myklebust 写道:
> On Fri, 2022-04-29 at 13:36 -0400, trondmy@kernel.org wrote:
>> From: Trond Myklebust <trond.myklebust@hammerspace.com>
>>
>> This reverts commit 7073ea8799a8cf73db60270986f14e4aae20fa80.
>>
>> We must not try to connect the socket while the transport is under
>> construction, because the mechanisms to safely tear it down are not
>> in
>> place.
>>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> Sorry. I intended to add a
>
> "Reported-by: wanghai (M) <wanghai38@huawei.com>"
>
> That has been added to the version in my "testing" branch.
>
Thanks for your help, I tested it carefully and this patchset can solve 
my problem.

By the way, when can this patchset be applied to linux mainline?

-- 
Wang Hai

Re: [PATCH v2 4/4] Revert "SUNRPC: attempt AF_LOCAL connect on setup"

[wanghai (M)]

在 2022/4/30 8:56, Trond Myklebust 写道:
> On Fri, 2022-04-29 at 13:36 -0400, trondmy@kernel.org wrote:
>> From: Trond Myklebust <trond.myklebust@hammerspace.com>
>>
>> This reverts commit 7073ea8799a8cf73db60270986f14e4aae20fa80.
>>
>> We must not try to connect the socket while the transport is under
>> construction, because the mechanisms to safely tear it down are not
>> in
>> place.
>>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> Sorry. I intended to add a
>
> "Reported-by: wanghai (M) <wanghai38@huawei.com>"
>
> That has been added to the version in my "testing" branch.
>
Hi, Trond.

If it is just to fix my problem, is it enough to apply only
patch3 and patch4? I tested that if only patch3 and patch4
are applied, the problem seems to be fixed.

-- 
Wang Hai

Re: [PATCH v2 3/4] SUNRPC: Ensure gss-proxy connects on setup

[wanghai (M)]

在 2022/4/30 1:36, trondmy@kernel.org 写道:
> From: Trond Myklebust <trond.myklebust@hammerspace.com>
>
> For reasons best known to the author, gss-proxy does not implement a
> NULL procedure, and returns RPC_PROC_UNAVAIL. However we still want to
> ensure that we connect to the service at setup time.
> So add a quirk-flag specially for this case.
>
> Fixes: 1d658336b05f ("SUNRPC: Add RPC based upcall mechanism for RPCGSS auth")
> Cc: stable@vger.kernel.org
> Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> ---
>   include/linux/sunrpc/clnt.h          | 1 +
>   net/sunrpc/auth_gss/gss_rpc_upcall.c | 2 +-
>   net/sunrpc/clnt.c                    | 3 +++
>   3 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/sunrpc/clnt.h b/include/linux/sunrpc/clnt.h
> index 267b7aeaf1a6..db5149567305 100644
> --- a/include/linux/sunrpc/clnt.h
> +++ b/include/linux/sunrpc/clnt.h
> @@ -160,6 +160,7 @@ 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 61c276bddaf2..8ca1d809b78d 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_NOPING |
> +		.flags		= RPC_CLNT_CREATE_IGNORE_NULL_UNAVAIL |
>   				  RPC_CLNT_CREATE_NO_IDLE_TIMEOUT
>   	};
>   	struct rpc_clnt *clnt;
> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
> index 98133aa54f19..22c28cf43eba 100644
> --- a/net/sunrpc/clnt.c
> +++ b/net/sunrpc/clnt.c
> @@ -479,6 +479,9 @@ 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;

Hi, Trond and Bruce

After I apply this patch, gssproxy.service does not work properly.


The following is the abnormal working log
[root@localhost ~]# systemctl restart gssproxy.service
[root@localhost ~]# cat /proc/net/rpc/use-gss-proxy
-1
[root@localhost ~]#

On failure, rpc_ping() returns -EIO. should the following change be made 
here

diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index 22c28cf43eba..314b6fd60e2f 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -480,7 +480,7 @@ 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 == -EOPNOTSUPP || err == -EIO))
                         err = 0;
                 if (err != 0) {
                         rpc_shutdown_client(clnt);

>   		if (err != 0) {
>   			rpc_shutdown_client(clnt);
>   			return ERR_PTR(err);

-- 
Wang Hai

Re: [PATCH v2 3/4] SUNRPC: Ensure gss-proxy connects on setup

[wanghai (M)]

在 2022/5/9 18:28, wanghai (M) 写道:
>
> 在 2022/4/30 1:36, trondmy@kernel.org 写道:
>> From: Trond Myklebust <trond.myklebust@hammerspace.com>
>>
>> For reasons best known to the author, gss-proxy does not implement a
>> NULL procedure, and returns RPC_PROC_UNAVAIL. However we still want to
>> ensure that we connect to the service at setup time.
>> So add a quirk-flag specially for this case.
>>
>> Fixes: 1d658336b05f ("SUNRPC: Add RPC based upcall mechanism for 
>> RPCGSS auth")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
>> ---
>>   include/linux/sunrpc/clnt.h          | 1 +
>>   net/sunrpc/auth_gss/gss_rpc_upcall.c | 2 +-
>>   net/sunrpc/clnt.c                    | 3 +++
>>   3 files changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/linux/sunrpc/clnt.h b/include/linux/sunrpc/clnt.h
>> index 267b7aeaf1a6..db5149567305 100644
>> --- a/include/linux/sunrpc/clnt.h
>> +++ b/include/linux/sunrpc/clnt.h
>> @@ -160,6 +160,7 @@ 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 61c276bddaf2..8ca1d809b78d 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_NOPING |
>> +        .flags        = RPC_CLNT_CREATE_IGNORE_NULL_UNAVAIL |
>>                     RPC_CLNT_CREATE_NO_IDLE_TIMEOUT
>>       };
>>       struct rpc_clnt *clnt;
>> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
>> index 98133aa54f19..22c28cf43eba 100644
>> --- a/net/sunrpc/clnt.c
>> +++ b/net/sunrpc/clnt.c
>> @@ -479,6 +479,9 @@ 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;
>
> Hi, Trond and Bruce
>
> After I apply this patch, gssproxy.service does not work properly.
>
>
> The following is the abnormal working log
> [root@localhost ~]# systemctl restart gssproxy.service
> [root@localhost ~]# cat /proc/net/rpc/use-gss-proxy
> -1
> [root@localhost ~]#
>
> On failure, rpc_ping() returns -EIO. should the following change be 
> made here
>
> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
> index 22c28cf43eba..314b6fd60e2f 100644
> --- a/net/sunrpc/clnt.c
> +++ b/net/sunrpc/clnt.c
> @@ -480,7 +480,7 @@ 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 == -EOPNOTSUPP || err == -EIO))
>                         err = 0;
>                 if (err != 0) {
>                         rpc_shutdown_client(clnt);

Below is the rpc log

[root@localhost ~]# systemctl restart gssproxy.service

[  115.536788][ T2911] systemd-journald[2911]: Successfully sent stream 
file descriptor to service manager.
[  115.769042][ T6604] RPC:       set up xprt to /var/run/gssproxy.sock 
via AF_LOCAL
[  115.774253][ T6604] RPC:       worker connecting xprt 
ffff888116f24000 via AF_LOCAL to /var/run/gssproxy.sock
[  115.777719][ T6604] RPC:       xprt ffff888116f24000 connected to 
/var/run/gssproxy.sock
[  115.780634][ T6604] RPC:       xs_local_send_request(40) = 0

[  125.825668][ T6604] RPC:       failed to create AF_LOCAL gssproxy 
client (errno -5).
[  125.832021][   T31] RPC:       xs_destroy xprt ffff888116f24000
[  125.834805][   T31] RPC:       xs_close xprt ffff888116f24000
>
>>           if (err != 0) {
>>               rpc_shutdown_client(clnt);
>>               return ERR_PTR(err);
>
-- 
Wang Hai