[PATCH RFC 0/9] nfs/sunrpc: stop holding netns references in client-side NFS and RPC objects

[PATCH RFC 0/9] nfs/sunrpc: stop holding netns references in client-side NFS and RPC objects

[Jeff Layton]

We have a long-standing problem with containers that have NFS mounts in
them. Best practice is to unmount gracefully, of course, but sometimes
containers just spontaneously die (e.g. SIGSEGV in the init task in the
container). When that happens the orchestrator will see that all of the
tasks are dead, and will detach the mount namespace and kill off the
network connection.

If there are RPCs in flight at the time, the rpc_clnt will try to
retransmit them indefinitely, but there is no hope of them ever
contacting the server since nothing in userland can reach the netns
at that point to fix anything.

This patchset takes the approach of changing various nfs client and
sunrpc objects to not hold a netns reference. Instead, when a nfs_net or
sunrpc_net is exiting, all nfs_server, nfs_client and rpc_clnt objects
associated with it are shut down, and the pre_exit functions block
until they are gone.

With this approach, when the last userland task in the container exits,
the NFS and RPC clients get cleaned up automatically. As a bonus, this
fixes another bug with the gssproxy RPC client that causes net namespace
leaks in any container where it runs (details in the patch
descriptions).

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
Jeff Layton (9):
      sunrpc: transplant shutdown_client() to sunrpc module
      lockd: add a helper to shut down rpc_clnt in nlm_host
      lockd: don't #include debug.h from lockd.h
      nfs: transplant nfs_server shutdown into a helper function
      nfs: don't hold a reference to struct net in struct nfs_client
      auth_gss: shut down gssproxy rpc_clnt in net pre_exit
      auth_gss: don't hold a net reference in gss_auth
      sunrpc: don't hold a struct net reference in rpc_xprt
      sunrpc: don't upgrade passive net reference in xs_create_sock

 fs/lockd/clnt4xdr.c                |  1 +
 fs/lockd/clntlock.c                |  1 +
 fs/lockd/clntproc.c                |  1 +
 fs/lockd/clntxdr.c                 |  1 +
 fs/lockd/host.c                    |  8 ++++++++
 fs/lockd/mon.c                     |  1 +
 fs/lockd/svc.c                     |  1 +
 fs/lockd/svc4proc.c                |  1 +
 fs/lockd/svclock.c                 |  1 +
 fs/lockd/svcproc.c                 |  1 +
 fs/lockd/svcsubs.c                 |  1 +
 fs/nfs/client.c                    |  6 ++++--
 fs/nfs/inode.c                     | 28 ++++++++++++++++++++++++++++
 fs/nfs/internal.h                  |  1 +
 fs/nfs/super.c                     | 18 ++++++++++++++++++
 fs/nfs/sysfs.c                     | 27 ++-------------------------
 include/linux/lockd/lockd.h        |  2 +-
 include/linux/sunrpc/sched.h       |  1 +
 include/linux/sunrpc/svcauth_gss.h |  1 +
 include/linux/sunrpc/xprt.h        |  1 -
 net/sunrpc/auth_gss/auth_gss.c     | 15 ++++++++-------
 net/sunrpc/auth_gss/svcauth_gss.c  |  7 ++++++-
 net/sunrpc/clnt.c                  | 14 ++++++++++++++
 net/sunrpc/sunrpc_syms.c           | 29 +++++++++++++++++++++++++++++
 net/sunrpc/xprt.c                  |  3 +--
 net/sunrpc/xprtsock.c              |  3 ---
 26 files changed, 132 insertions(+), 42 deletions(-)
---
base-commit: 80e54e84911a923c40d7bee33a34c1b4be148d7a
change-id: 20250317-rpc-shutdown-1519aacd1db3

Best regards,
-- 
Jeff Layton <jlayton@kernel.org>

[PATCH RFC 1/9] sunrpc: transplant shutdown_client() to sunrpc module

[Jeff Layton]

This is really a sunrpc-level function, and in later patches we'll need
to call this routine from lockd as well. Move it to the sunrpc module,
rename and export it.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/nfs/sysfs.c               | 19 ++++---------------
 include/linux/sunrpc/sched.h |  1 +
 net/sunrpc/clnt.c            | 12 ++++++++++++
 3 files changed, 17 insertions(+), 15 deletions(-)

diff --git a/fs/nfs/sysfs.c b/fs/nfs/sysfs.c
index 7b59a40d40c061a41b0fbde91aa006314f02c1fb..c29c5fd639554461bdcd9ff612726194910d85b5 100644
--- a/fs/nfs/sysfs.c
+++ b/fs/nfs/sysfs.c
@@ -217,17 +217,6 @@ void nfs_netns_sysfs_destroy(struct nfs_net *netns)
 	}
 }
 
-static bool shutdown_match_client(const struct rpc_task *task, const void *data)
-{
-	return true;
-}
-
-static void shutdown_client(struct rpc_clnt *clnt)
-{
-	clnt->cl_shutdown = 1;
-	rpc_cancel_tasks(clnt, -EIO, shutdown_match_client, NULL);
-}
-
 static ssize_t
 shutdown_show(struct kobject *kobj, struct kobj_attribute *attr,
 				char *buf)
@@ -258,14 +247,14 @@ shutdown_store(struct kobject *kobj, struct kobj_attribute *attr,
 		goto out;
 
 	server->flags |= NFS_MOUNT_SHUTDOWN;
-	shutdown_client(server->client);
-	shutdown_client(server->nfs_client->cl_rpcclient);
+	rpc_clnt_shutdown(server->client);
+	rpc_clnt_shutdown(server->nfs_client->cl_rpcclient);
 
 	if (!IS_ERR(server->client_acl))
-		shutdown_client(server->client_acl);
+		rpc_clnt_shutdown(server->client_acl);
 
 	if (server->nlm_host)
-		shutdown_client(server->nlm_host->h_rpcclnt);
+		rpc_clnt_shutdown(server->nlm_host->h_rpcclnt);
 out:
 	return count;
 }
diff --git a/include/linux/sunrpc/sched.h b/include/linux/sunrpc/sched.h
index eac57914dcf3200c1a6ed39ab030e3fe8b4da3e1..fe7c39a17ce44ec68c0cf057133d0f8e7f0ae797 100644
--- a/include/linux/sunrpc/sched.h
+++ b/include/linux/sunrpc/sched.h
@@ -232,6 +232,7 @@ unsigned long	rpc_cancel_tasks(struct rpc_clnt *clnt, int error,
 				 bool (*fnmatch)(const struct rpc_task *,
 						 const void *),
 				 const void *data);
+void		rpc_clnt_shutdown(struct rpc_clnt *clnt);
 void		rpc_execute(struct rpc_task *);
 void		rpc_init_priority_wait_queue(struct rpc_wait_queue *, const char *);
 void		rpc_init_wait_queue(struct rpc_wait_queue *, const char *);
diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index 2fe88ea79a70c134e58abfb03fca230883eddf1f..0028858b12d97e7b45f4c24cfbd761ba2a734b32 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -934,6 +934,18 @@ unsigned long rpc_cancel_tasks(struct rpc_clnt *clnt, int error,
 }
 EXPORT_SYMBOL_GPL(rpc_cancel_tasks);
 
+static bool shutdown_match_client(const struct rpc_task *task, const void *data)
+{
+	return true;
+}
+
+void rpc_clnt_shutdown(struct rpc_clnt *clnt)
+{
+	clnt->cl_shutdown = 1;
+	rpc_cancel_tasks(clnt, -EIO, shutdown_match_client, NULL);
+}
+EXPORT_SYMBOL_GPL(rpc_clnt_shutdown);
+
 static int rpc_clnt_disconnect_xprt(struct rpc_clnt *clnt,
 				    struct rpc_xprt *xprt, void *dummy)
 {

-- 
2.48.1

[PATCH RFC 2/9] lockd: add a helper to shut down rpc_clnt in nlm_host

[Jeff Layton]

NFS reaching into struct nlm_host is at least a minor layering
violation. Make an exported helper function and have nfs call that
instead.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/lockd/host.c             | 7 +++++++
 fs/nfs/sysfs.c              | 2 +-
 include/linux/lockd/lockd.h | 1 +
 3 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/fs/lockd/host.c b/fs/lockd/host.c
index 5e6877c37f7303e7141fbb96bc278a207c8e5ddc..ed88c102eca0f999a9c5351467d823b806c30962 100644
--- a/fs/lockd/host.c
+++ b/fs/lockd/host.c
@@ -692,3 +692,10 @@ nlm_gc_hosts(struct net *net)
 		ln->next_gc = jiffies + NLM_HOST_COLLECT;
 	}
 }
+
+void
+nlm_host_shutdown_rpc(struct nlm_host *host)
+{
+	rpc_clnt_shutdown(host->h_rpcclnt);
+}
+EXPORT_SYMBOL_GPL(nlm_host_shutdown_rpc);
diff --git a/fs/nfs/sysfs.c b/fs/nfs/sysfs.c
index c29c5fd639554461bdcd9ff612726194910d85b5..c0bfe6df53b51c0fcc541c33ab7590813114d7ec 100644
--- a/fs/nfs/sysfs.c
+++ b/fs/nfs/sysfs.c
@@ -254,7 +254,7 @@ shutdown_store(struct kobject *kobj, struct kobj_attribute *attr,
 		rpc_clnt_shutdown(server->client_acl);
 
 	if (server->nlm_host)
-		rpc_clnt_shutdown(server->nlm_host->h_rpcclnt);
+		nlm_host_shutdown_rpc(server->nlm_host);
 out:
 	return count;
 }
diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
index c8f0f9458f2cc035fd9161f8f2486ba76084abf1..6b8c912f443c3b4130f49b8170070d0b794abb94 100644
--- a/include/linux/lockd/lockd.h
+++ b/include/linux/lockd/lockd.h
@@ -248,6 +248,7 @@ void		  nlm_shutdown_hosts(void);
 void		  nlm_shutdown_hosts_net(struct net *net);
 void		  nlm_host_rebooted(const struct net *net,
 					const struct nlm_reboot *);
+void		  nlm_host_shutdown_rpc(struct nlm_host *host);
 
 /*
  * Host monitoring

-- 
2.48.1

[PATCH RFC 3/9] lockd: don't #include debug.h from lockd.h

[Jeff Layton]

We need to #include lockd.h in some files in fs/nfs, but the dprintk
definitions collide with the ones in NFS. Include it directly in the
files in fs/lockd/ that need it.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/lockd/clnt4xdr.c         | 1 +
 fs/lockd/clntlock.c         | 1 +
 fs/lockd/clntproc.c         | 1 +
 fs/lockd/clntxdr.c          | 1 +
 fs/lockd/host.c             | 1 +
 fs/lockd/mon.c              | 1 +
 fs/lockd/svc.c              | 1 +
 fs/lockd/svc4proc.c         | 1 +
 fs/lockd/svclock.c          | 1 +
 fs/lockd/svcproc.c          | 1 +
 fs/lockd/svcsubs.c          | 1 +
 include/linux/lockd/lockd.h | 1 -
 12 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/fs/lockd/clnt4xdr.c b/fs/lockd/clnt4xdr.c
index 527458db4525af3e76d9119feb6e5e5b62890741..d824c8f89aeedf5903052bf4ed045b079adadaa1 100644
--- a/fs/lockd/clnt4xdr.c
+++ b/fs/lockd/clnt4xdr.c
@@ -14,6 +14,7 @@
 #include <linux/sunrpc/clnt.h>
 #include <linux/sunrpc/stats.h>
 #include <linux/lockd/lockd.h>
+#include <linux/lockd/debug.h>
 
 #include <uapi/linux/nfs3.h>
 
diff --git a/fs/lockd/clntlock.c b/fs/lockd/clntlock.c
index a7e0519ec024a9f73ca05d0d4a0ca29f22d0eb23..5f593d367c1f1781cf68842bb00610234c436524 100644
--- a/fs/lockd/clntlock.c
+++ b/fs/lockd/clntlock.c
@@ -16,6 +16,7 @@
 #include <linux/sunrpc/svc.h>
 #include <linux/sunrpc/svc_xprt.h>
 #include <linux/lockd/lockd.h>
+#include <linux/lockd/debug.h>
 #include <linux/kthread.h>
 
 #include "trace.h"
diff --git a/fs/lockd/clntproc.c b/fs/lockd/clntproc.c
index cebcc283b7ce2e813944d9037de2a7462585a2c9..2cc331c8b2a294f4feae38d1ca0da240b14008bc 100644
--- a/fs/lockd/clntproc.c
+++ b/fs/lockd/clntproc.c
@@ -19,6 +19,7 @@
 #include <linux/sunrpc/clnt.h>
 #include <linux/sunrpc/svc.h>
 #include <linux/lockd/lockd.h>
+#include <linux/lockd/debug.h>
 
 #include "trace.h"
 
diff --git a/fs/lockd/clntxdr.c b/fs/lockd/clntxdr.c
index 6ea3448d2d31ea54451d6ec0fd01c5f31c35e123..effcef151b60a929a8e96d5560aa5efa84e00b74 100644
--- a/fs/lockd/clntxdr.c
+++ b/fs/lockd/clntxdr.c
@@ -16,6 +16,7 @@
 #include <linux/sunrpc/clnt.h>
 #include <linux/sunrpc/stats.h>
 #include <linux/lockd/lockd.h>
+#include <linux/lockd/debug.h>
 
 #include <uapi/linux/nfs2.h>
 
diff --git a/fs/lockd/host.c b/fs/lockd/host.c
index ed88c102eca0f999a9c5351467d823b806c30962..931a24abd78462ae12b18b7799e1c060748ff276 100644
--- a/fs/lockd/host.c
+++ b/fs/lockd/host.c
@@ -17,6 +17,7 @@
 #include <linux/sunrpc/addr.h>
 #include <linux/sunrpc/svc.h>
 #include <linux/lockd/lockd.h>
+#include <linux/lockd/debug.h>
 #include <linux/mutex.h>
 
 #include <linux/sunrpc/svc_xprt.h>
diff --git a/fs/lockd/mon.c b/fs/lockd/mon.c
index b8fc732e1c677063a0d0a1385a64ce22fe216ae4..79364ffa7b2f2aa43c1a504b59d2de77691ae378 100644
--- a/fs/lockd/mon.c
+++ b/fs/lockd/mon.c
@@ -17,6 +17,7 @@
 #include <linux/sunrpc/xprtsock.h>
 #include <linux/sunrpc/svc.h>
 #include <linux/lockd/lockd.h>
+#include <linux/lockd/debug.h>
 
 #include <linux/unaligned.h>
 
diff --git a/fs/lockd/svc.c b/fs/lockd/svc.c
index 2c8eedc6c2cc9ebcfe521d90364de84e00eae40f..1cd1bdb06566c70c77728e9a3bab19364c155cdc 100644
--- a/fs/lockd/svc.c
+++ b/fs/lockd/svc.c
@@ -37,6 +37,7 @@
 #include <net/addrconf.h>
 #include <net/ipv6.h>
 #include <linux/lockd/lockd.h>
+#include <linux/lockd/debug.h>
 #include <linux/nfs.h>
 
 #include "netns.h"
diff --git a/fs/lockd/svc4proc.c b/fs/lockd/svc4proc.c
index 109e5caae8c7045487d32bf35b5b80456a421f57..6fdaac8e6877f148927bdc9c0ba613de2dbf2be0 100644
--- a/fs/lockd/svc4proc.c
+++ b/fs/lockd/svc4proc.c
@@ -11,6 +11,7 @@
 #include <linux/types.h>
 #include <linux/time.h>
 #include <linux/lockd/lockd.h>
+#include <linux/lockd/debug.h>
 #include <linux/lockd/share.h>
 #include <linux/sunrpc/svc_xprt.h>
 
diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c
index c1315df4b350bbd753305b5c08550d50f67b92aa..d73ba3959aac3160904b55e77cc10d59604b725c 100644
--- a/fs/lockd/svclock.c
+++ b/fs/lockd/svclock.c
@@ -30,6 +30,7 @@
 #include <linux/sunrpc/svc_xprt.h>
 #include <linux/lockd/nlm.h>
 #include <linux/lockd/lockd.h>
+#include <linux/lockd/debug.h>
 
 #define NLMDBG_FACILITY		NLMDBG_SVCLOCK
 
diff --git a/fs/lockd/svcproc.c b/fs/lockd/svcproc.c
index f53d5177f26732092242fcabe32f0f8931fc1fb6..68c08bf21d300a1c5739c659ceffe375b465c16b 100644
--- a/fs/lockd/svcproc.c
+++ b/fs/lockd/svcproc.c
@@ -11,6 +11,7 @@
 #include <linux/types.h>
 #include <linux/time.h>
 #include <linux/lockd/lockd.h>
+#include <linux/lockd/debug.h>
 #include <linux/lockd/share.h>
 #include <linux/sunrpc/svc_xprt.h>
 
diff --git a/fs/lockd/svcsubs.c b/fs/lockd/svcsubs.c
index 9103896164f6886eec5adf65a55f4c29433dcb29..e3b63708f7872d5a97df0d1379db40b2fbd3ac5d 100644
--- a/fs/lockd/svcsubs.c
+++ b/fs/lockd/svcsubs.c
@@ -16,6 +16,7 @@
 #include <linux/sunrpc/svc.h>
 #include <linux/sunrpc/addr.h>
 #include <linux/lockd/lockd.h>
+#include <linux/lockd/debug.h>
 #include <linux/lockd/share.h>
 #include <linux/module.h>
 #include <linux/mount.h>
diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
index 6b8c912f443c3b4130f49b8170070d0b794abb94..21cd2cd85e537708ac83d658e40265cec327197b 100644
--- a/include/linux/lockd/lockd.h
+++ b/include/linux/lockd/lockd.h
@@ -24,7 +24,6 @@
 #ifdef CONFIG_LOCKD_V4
 #include <linux/lockd/xdr4.h>
 #endif
-#include <linux/lockd/debug.h>
 #include <linux/sunrpc/svc.h>
 
 /*

-- 
2.48.1

[PATCH RFC 4/9] nfs: transplant nfs_server shutdown into a helper function

[Jeff Layton]

Move the guts of shutdown_store() into a new helper for shutting down a
nfs_server.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/nfs/internal.h |  1 +
 fs/nfs/super.c    | 18 ++++++++++++++++++
 fs/nfs/sysfs.c    | 16 ++--------------
 3 files changed, 21 insertions(+), 14 deletions(-)

diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
index fae2c7ae4acc286e1c5ad2b2225b1e9a6930b56b..968c8c845f49f5b7ca6f78e391ba2a3024ce64ff 100644
--- a/fs/nfs/internal.h
+++ b/fs/nfs/internal.h
@@ -507,6 +507,7 @@ bool nfs_auth_info_match(const struct nfs_auth_info *, rpc_authflavor_t);
 int nfs_try_get_tree(struct fs_context *);
 int nfs_get_tree_common(struct fs_context *);
 void nfs_kill_super(struct super_block *);
+void nfs_server_shutdown(struct nfs_server *server);
 
 extern int __init register_nfs_fs(void);
 extern void __exit unregister_nfs_fs(void);
diff --git a/fs/nfs/super.c b/fs/nfs/super.c
index aeb715b4a6902dc907b4b4cf2712232b080c497f..b78251dde6b717738847ebf4b75f6de10e7ea644 100644
--- a/fs/nfs/super.c
+++ b/fs/nfs/super.c
@@ -56,6 +56,7 @@
 #include <linux/parser.h>
 #include <linux/nsproxy.h>
 #include <linux/rcupdate.h>
+#include <linux/lockd/lockd.h>
 
 #include <linux/uaccess.h>
 #include <linux/nfs_ssc.h>
@@ -1386,6 +1387,23 @@ void nfs_kill_super(struct super_block *s)
 }
 EXPORT_SYMBOL_GPL(nfs_kill_super);
 
+void nfs_server_shutdown(struct nfs_server *server)
+{
+	/* already shut down? */
+	if (server->flags & NFS_MOUNT_SHUTDOWN)
+		return;
+
+	server->flags |= NFS_MOUNT_SHUTDOWN;
+	rpc_clnt_shutdown(server->client);
+	rpc_clnt_shutdown(server->nfs_client->cl_rpcclient);
+
+	if (!IS_ERR(server->client_acl))
+		rpc_clnt_shutdown(server->client_acl);
+
+	if (server->nlm_host)
+		nlm_host_shutdown_rpc(server->nlm_host);
+}
+
 #if IS_ENABLED(CONFIG_NFS_V4)
 
 /*
diff --git a/fs/nfs/sysfs.c b/fs/nfs/sysfs.c
index c0bfe6df53b51c0fcc541c33ab7590813114d7ec..1b2e2ed10a1bf8d4ad06dc676ec5831d6eba99b4 100644
--- a/fs/nfs/sysfs.c
+++ b/fs/nfs/sysfs.c
@@ -14,6 +14,7 @@
 #include <linux/rcupdate.h>
 #include <linux/lockd/lockd.h>
 
+#include "internal.h"
 #include "nfs4_fs.h"
 #include "netns.h"
 #include "sysfs.h"
@@ -242,20 +243,7 @@ shutdown_store(struct kobject *kobj, struct kobj_attribute *attr,
 	if (val != 1)
 		return -EINVAL;
 
-	/* already shut down? */
-	if (server->flags & NFS_MOUNT_SHUTDOWN)
-		goto out;
-
-	server->flags |= NFS_MOUNT_SHUTDOWN;
-	rpc_clnt_shutdown(server->client);
-	rpc_clnt_shutdown(server->nfs_client->cl_rpcclient);
-
-	if (!IS_ERR(server->client_acl))
-		rpc_clnt_shutdown(server->client_acl);
-
-	if (server->nlm_host)
-		nlm_host_shutdown_rpc(server->nlm_host);
-out:
+	nfs_server_shutdown(server);
 	return count;
 }
 

-- 
2.48.1

[PATCH RFC 5/9] nfs: don't hold a reference to struct net in struct nfs_client

[Jeff Layton]

An NFS client holds a reference to the net namespace. This can cause
nfs_clients to stick around after a container dies spontaneously.

The container orchestrator sees that there are no more tasks in the
container, detaches the filesystems and tears down the networking.
Unfortunately, there can still be RPCs in flight that will end up
attempting to retransmit indefinitely. No userland tasks have a way
to reach that net namespace any longer though, so there is no hope of it
being rescued.

Instead of keeping a net reference in struct nfs_client, add a nfs_net
pre_exit routine that kills off the nfs_server and any remaining
nfs_clients, and then waits for the nfs_client_list to go empty.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/nfs/client.c |  6 ++++--
 fs/nfs/inode.c  | 28 ++++++++++++++++++++++++++++
 2 files changed, 32 insertions(+), 2 deletions(-)

diff --git a/fs/nfs/client.c b/fs/nfs/client.c
index 3b0918ade53cd331d76baaa86fd2adec5d945b78..8f41cb88f88c2b4d7f10424484b6e70ac2b8835a 100644
--- a/fs/nfs/client.c
+++ b/fs/nfs/client.c
@@ -180,7 +180,7 @@ struct nfs_client *nfs_alloc_client(const struct nfs_client_initdata *cl_init)
 	clp->cl_proto = cl_init->proto;
 	clp->cl_nconnect = cl_init->nconnect;
 	clp->cl_max_connect = cl_init->max_connect ? cl_init->max_connect : 1;
-	clp->cl_net = get_net(cl_init->net);
+	clp->cl_net = cl_init->net;
 
 #if IS_ENABLED(CONFIG_NFS_LOCALIO)
 	seqlock_init(&clp->cl_boot_lock);
@@ -244,13 +244,15 @@ static void pnfs_init_server(struct nfs_server *server)
  */
 void nfs_free_client(struct nfs_client *clp)
 {
+	struct nfs_net *nn = net_generic(clp->cl_net, nfs_net_id);
+
 	nfs_localio_disable_client(clp);
 
 	/* -EIO all pending I/O */
 	if (!IS_ERR(clp->cl_rpcclient))
 		rpc_shutdown_client(clp->cl_rpcclient);
 
-	put_net(clp->cl_net);
+	wake_up_var(&nn->nfs_client_list);
 	put_nfs_version(clp->cl_nfs_mod);
 	kfree(clp->cl_hostname);
 	kfree(clp->cl_acceptor);
diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index 1aa67fca69b2fbd8afb1c51be78198220b1e13c7..0352e7e6ee270562a971d031ba02bdec96496288 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -2562,9 +2562,37 @@ static void nfs_net_exit(struct net *net)
 	nfs_clients_exit(net);
 }
 
+static bool all_clients_gone(struct nfs_net *nn)
+{
+	bool	gone;
+
+	spin_lock(&nn->nfs_client_lock);
+	gone = list_empty(&nn->nfs_client_list);
+	spin_unlock(&nn->nfs_client_lock);
+
+	return gone;
+}
+
+static void nfs_net_pre_exit(struct net *net)
+{
+	struct nfs_net *nn = net_generic(net, nfs_net_id);
+	struct nfs_server *server;
+	struct nfs_client *clp;
+
+	spin_lock(&nn->nfs_client_lock);
+	list_for_each_entry(server, &nn->nfs_volume_list, master_link)
+		nfs_server_shutdown(server);
+	list_for_each_entry(clp, &nn->nfs_client_list, cl_share_link)
+		rpc_clnt_shutdown(clp->cl_rpcclient);
+	spin_unlock(&nn->nfs_client_lock);
+
+	wait_var_event(&nn->nfs_client_list, all_clients_gone(nn));
+}
+
 static struct pernet_operations nfs_net_ops = {
 	.init = nfs_net_init,
 	.exit = nfs_net_exit,
+	.pre_exit = nfs_net_pre_exit,
 	.id   = &nfs_net_id,
 	.size = sizeof(struct nfs_net),
 };

-- 
2.48.1

[PATCH RFC 6/9] auth_gss: shut down gssproxy rpc_clnt in net pre_exit

[Jeff Layton]

With a coming change to rpc_xprt's not holding a reference to the netns,
it will be necessary to shut down the gssp_clnt in pre_exit rather than
the netns exit routine.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 include/linux/sunrpc/svcauth_gss.h | 1 +
 net/sunrpc/auth_gss/auth_gss.c     | 6 ++++++
 net/sunrpc/auth_gss/svcauth_gss.c  | 7 ++++++-
 3 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/include/linux/sunrpc/svcauth_gss.h b/include/linux/sunrpc/svcauth_gss.h
index f09c82b0a7aefed0b4ce6ab60258a2d34cea8e27..0dfda2f25f7fdb16a6bc9e537399548e47dfde56 100644
--- a/include/linux/sunrpc/svcauth_gss.h
+++ b/include/linux/sunrpc/svcauth_gss.h
@@ -19,6 +19,7 @@
 int gss_svc_init(void);
 void gss_svc_shutdown(void);
 int gss_svc_init_net(struct net *net);
+void gss_svc_pre_shutdown_net(struct net *net);
 void gss_svc_shutdown_net(struct net *net);
 struct auth_domain *svcauth_gss_register_pseudoflavor(u32 pseudoflavor,
 						      char *name);
diff --git a/net/sunrpc/auth_gss/auth_gss.c b/net/sunrpc/auth_gss/auth_gss.c
index 369310909fc98596c8a06db8ac3c976a719ca7b2..78571776f446e2097bf25642c182d57546502803 100644
--- a/net/sunrpc/auth_gss/auth_gss.c
+++ b/net/sunrpc/auth_gss/auth_gss.c
@@ -2242,9 +2242,15 @@ static __net_exit void rpcsec_gss_exit_net(struct net *net)
 	gss_svc_shutdown_net(net);
 }
 
+static __net_exit void rpcsec_gss_pre_exit_net(struct net *net)
+{
+	gss_svc_pre_shutdown_net(net);
+}
+
 static struct pernet_operations rpcsec_gss_net_ops = {
 	.init = rpcsec_gss_init_net,
 	.exit = rpcsec_gss_exit_net,
+	.pre_exit = rpcsec_gss_pre_exit_net,
 };
 
 /*
diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
index 73a90ad873fb9da659ba76184b2e2a0e5324ce0d..624e88be6eb3163b131902c4800e4842e4c0808e 100644
--- a/net/sunrpc/auth_gss/svcauth_gss.c
+++ b/net/sunrpc/auth_gss/svcauth_gss.c
@@ -2102,11 +2102,16 @@ gss_svc_init_net(struct net *net)
 	return rv;
 }
 
+void
+gss_svc_pre_shutdown_net(struct net *net)
+{
+	destroy_use_gss_proxy_proc_entry(net);
+}
+
 void
 gss_svc_shutdown_net(struct net *net)
 {
 	destroy_krb5_enctypes_proc_entry(net);
-	destroy_use_gss_proxy_proc_entry(net);
 	rsi_cache_destroy_net(net);
 	rsc_cache_destroy_net(net);
 }

-- 
2.48.1

[PATCH RFC 7/9] auth_gss: don't hold a net reference in gss_auth

[Jeff Layton]

It's not clear to me that these net references were ever needed. They
were added in commit e726340ac9cf ("RPCSEC_GSS: Further cleanups"), but
there no is explanation for taking it in the patch description and it's
not clear what race this prevents.

Now that the gssproxy client is shut down in pre_exit, there should be
no need to keep a reference here.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 net/sunrpc/auth_gss/auth_gss.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/net/sunrpc/auth_gss/auth_gss.c b/net/sunrpc/auth_gss/auth_gss.c
index 78571776f446e2097bf25642c182d57546502803..9698914d7ed3e5674351f9cc4f43c08d7b746bc3 100644
--- a/net/sunrpc/auth_gss/auth_gss.c
+++ b/net/sunrpc/auth_gss/auth_gss.c
@@ -89,7 +89,6 @@ struct gss_auth {
 	enum rpc_gss_svc service;
 	struct rpc_clnt *client;
 	struct net	*net;
-	netns_tracker	ns_tracker;
 	/*
 	 * There are two upcall pipes; dentry[1], named "gssd", is used
 	 * for the new text-based upcall; dentry[0] is named after the
@@ -1045,12 +1044,11 @@ gss_create_new(const struct rpc_auth_create_args *args, struct rpc_clnt *clnt)
 			goto err_free;
 	}
 	gss_auth->client = clnt;
-	gss_auth->net = get_net_track(rpc_net_ns(clnt), &gss_auth->ns_tracker,
-				      GFP_KERNEL);
+	gss_auth->net = rpc_net_ns(clnt);
 	err = -EINVAL;
 	gss_auth->mech = gss_mech_get_by_pseudoflavor(flavor);
 	if (!gss_auth->mech)
-		goto err_put_net;
+		goto err_free;
 	gss_auth->service = gss_pseudoflavor_to_service(gss_auth->mech, flavor);
 	if (gss_auth->service == 0)
 		goto err_put_mech;
@@ -1101,8 +1099,6 @@ gss_create_new(const struct rpc_auth_create_args *args, struct rpc_clnt *clnt)
 	rpcauth_destroy_credcache(auth);
 err_put_mech:
 	gss_mech_put(gss_auth->mech);
-err_put_net:
-	put_net_track(gss_auth->net, &gss_auth->ns_tracker);
 err_free:
 	kfree(gss_auth->target_name);
 	kfree(gss_auth);
@@ -1118,7 +1114,6 @@ gss_free(struct gss_auth *gss_auth)
 	gss_pipe_free(gss_auth->gss_pipe[0]);
 	gss_pipe_free(gss_auth->gss_pipe[1]);
 	gss_mech_put(gss_auth->mech);
-	put_net_track(gss_auth->net, &gss_auth->ns_tracker);
 	kfree(gss_auth->target_name);
 
 	kfree(gss_auth);

-- 
2.48.1

[PATCH RFC 8/9] sunrpc: don't hold a struct net reference in rpc_xprt

[Jeff Layton]

Currently each rpc_xprt holds a reference to struct net. This is
problematic for at least a couple of reasons:

1/ a container with an nfs mount inside can spontaneously die and take
network connectivity with it. When this happens, the netns and rpc_clnt
stick around and the client continually tries to retransmit any RPCs in
a net namespace that is otherwise defunct.

2/ the gssproxy rpc_clnt is torn down when the netns exits, but that
can never happen due to the circular dependency. The result is that any
container that runs gssproxy will leak the netns on exit.

Instead of holding a reference to the netns in rpc_xprt, add a pre_exit
routine that will shut down all rpc_clnt's associated with the netns,
and then wait for the list to go empty.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 include/linux/sunrpc/xprt.h |  1 -
 net/sunrpc/clnt.c           |  2 ++
 net/sunrpc/sunrpc_syms.c    | 29 +++++++++++++++++++++++++++++
 net/sunrpc/xprt.c           |  3 +--
 4 files changed, 32 insertions(+), 3 deletions(-)

diff --git a/include/linux/sunrpc/xprt.h b/include/linux/sunrpc/xprt.h
index 81b952649d35e3ad4fa8c7e77388ac2ceb44ce60..67c41917e3d83fc0b5c2240f2f1243a2b67da0ec 100644
--- a/include/linux/sunrpc/xprt.h
+++ b/include/linux/sunrpc/xprt.h
@@ -296,7 +296,6 @@ struct rpc_xprt {
 	} stat;
 
 	struct net		*xprt_net;
-	netns_tracker		ns_tracker;
 	const char		*servername;
 	const char		*address_strings[RPC_DISPLAY_MAX];
 #if IS_ENABLED(CONFIG_SUNRPC_DEBUG)
diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index 0028858b12d97e7b45f4c24cfbd761ba2a734b32..80cd1ddd155db64fc5b2449ebf54714d80a2838c 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -91,6 +91,8 @@ static void rpc_unregister_client(struct rpc_clnt *clnt)
 
 	spin_lock(&sn->rpc_client_lock);
 	list_del(&clnt->cl_clients);
+	if (list_empty(&sn->all_clients))
+		wake_up_var(&sn->all_clients);
 	spin_unlock(&sn->rpc_client_lock);
 }
 
diff --git a/net/sunrpc/sunrpc_syms.c b/net/sunrpc/sunrpc_syms.c
index bab6cab2940524a970422b62b3fa4212c61c4f43..d919f77522a7c6f7c477b2674f0b3a54155c91d9 100644
--- a/net/sunrpc/sunrpc_syms.c
+++ b/net/sunrpc/sunrpc_syms.c
@@ -77,9 +77,38 @@ static __net_exit void sunrpc_exit_net(struct net *net)
 	WARN_ON_ONCE(!list_empty(&sn->all_clients));
 }
 
+static void shutdown_all_clients(struct sunrpc_net *sn)
+{
+	struct rpc_clnt *clnt;
+
+	lockdep_assert_held(&sn->rpc_client_lock);
+
+	list_for_each_entry(clnt, &sn->all_clients, cl_clients)
+		rpc_clnt_shutdown(clnt);
+}
+
+static bool all_clients_gone(struct sunrpc_net *sn)
+{
+	bool empty;
+
+	spin_lock(&sn->rpc_client_lock);
+	empty = list_empty(&sn->all_clients);
+	spin_unlock(&sn->rpc_client_lock);
+	return empty;
+}
+
+static void sunrpc_pre_exit_net(struct net *net)
+{
+	struct sunrpc_net *sn = net_generic(net, sunrpc_net_id);
+
+	shutdown_all_clients(sn);
+	wait_var_event(&sn->all_clients, all_clients_gone(sn));
+}
+
 static struct pernet_operations sunrpc_net_ops = {
 	.init = sunrpc_init_net,
 	.exit = sunrpc_exit_net,
+	.pre_exit = sunrpc_pre_exit_net,
 	.id = &sunrpc_net_id,
 	.size = sizeof(struct sunrpc_net),
 };
diff --git a/net/sunrpc/xprt.c b/net/sunrpc/xprt.c
index 09f245cda5262a572c450237419c80b183a83568..040cc9bf92cfa8f28edaee707a09a9e8d9955c41 100644
--- a/net/sunrpc/xprt.c
+++ b/net/sunrpc/xprt.c
@@ -1849,7 +1849,6 @@ EXPORT_SYMBOL_GPL(xprt_alloc);
 
 void xprt_free(struct rpc_xprt *xprt)
 {
-	put_net_track(xprt->xprt_net, &xprt->ns_tracker);
 	xprt_free_all_slots(xprt);
 	xprt_free_id(xprt);
 	rpc_sysfs_xprt_destroy(xprt);
@@ -2047,7 +2046,7 @@ static void xprt_init(struct rpc_xprt *xprt, struct net *net)
 
 	xprt_init_xid(xprt);
 
-	xprt->xprt_net = get_net_track(net, &xprt->ns_tracker, GFP_KERNEL);
+	xprt->xprt_net = net;
 }
 
 /**

-- 
2.48.1

[PATCH RFC 9/9] sunrpc: don't upgrade passive net reference in xs_create_sock

[Jeff Layton]

With the move to having sunrpc client xprts not hold active references
to the net namespace, there is no need to upgrade the socket's reference
in xs_create_sock. Just keep the passive reference instead.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 net/sunrpc/xprtsock.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
index 83cc095846d356f24aed26e2f98525662a6cff1f..0c3d7552f772d6f8477a3aed8f0c513b62cdf589 100644
--- a/net/sunrpc/xprtsock.c
+++ b/net/sunrpc/xprtsock.c
@@ -1941,9 +1941,6 @@ static struct socket *xs_create_sock(struct rpc_xprt *xprt,
 		goto out;
 	}
 
-	if (protocol == IPPROTO_TCP)
-		sk_net_refcnt_upgrade(sock->sk);
-
 	filp = sock_alloc_file(sock, O_NONBLOCK, NULL);
 	if (IS_ERR(filp))
 		return ERR_CAST(filp);

-- 
2.48.1

Re: [PATCH RFC 9/9] sunrpc: don't upgrade passive net reference in xs_create_sock

[Trond Myklebust]

On Mon, 2025-03-17 at 17:00 -0400, Jeff Layton wrote:
> With the move to having sunrpc client xprts not hold active
> references
> to the net namespace, there is no need to upgrade the socket's
> reference
> in xs_create_sock. Just keep the passive reference instead.
> 
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  net/sunrpc/xprtsock.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
> index
> 83cc095846d356f24aed26e2f98525662a6cff1f..0c3d7552f772d6f8477a3aed8f0
> c513b62cdf589 100644
> --- a/net/sunrpc/xprtsock.c
> +++ b/net/sunrpc/xprtsock.c
> @@ -1941,9 +1941,6 @@ static struct socket *xs_create_sock(struct
> rpc_xprt *xprt,
>  		goto out;
>  	}
>  
> -	if (protocol == IPPROTO_TCP)
> -		sk_net_refcnt_upgrade(sock->sk);
> -
>  	filp = sock_alloc_file(sock, O_NONBLOCK, NULL);
>  	if (IS_ERR(filp))
>  		return ERR_CAST(filp);
> 

Is this not going to reintroduce the bug described by
https://lore.kernel.org/netdev/67b72aeb.050a0220.14d86d.0283.GAE@google.com/T/#u
?

As I understand it, the problem has nothing to do with whether or not
NFS or the RPC layer holds a reference to the net namespace, but rather
whether there are still packets in the socket queues at the time when
that net namespace is being freed.

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

Re: [PATCH RFC 0/9] nfs/sunrpc: stop holding netns references in client-side NFS and RPC objects

[Trond Myklebust]

On Mon, 2025-03-17 at 16:59 -0400, Jeff Layton wrote:
> We have a long-standing problem with containers that have NFS mounts
> in
> them. Best practice is to unmount gracefully, of course, but
> sometimes
> containers just spontaneously die (e.g. SIGSEGV in the init task in
> the
> container). When that happens the orchestrator will see that all of
> the
> tasks are dead, and will detach the mount namespace and kill off the
> network connection.
> 
> If there are RPCs in flight at the time, the rpc_clnt will try to
> retransmit them indefinitely, but there is no hope of them ever
> contacting the server since nothing in userland can reach the netns
> at that point to fix anything.
> 
> This patchset takes the approach of changing various nfs client and
> sunrpc objects to not hold a netns reference. Instead, when a nfs_net
> or
> sunrpc_net is exiting, all nfs_server, nfs_client and rpc_clnt
> objects
> associated with it are shut down, and the pre_exit functions block
> until they are gone.
> 
> With this approach, when the last userland task in the container
> exits,
> the NFS and RPC clients get cleaned up automatically. As a bonus,
> this
> fixes another bug with the gssproxy RPC client that causes net
> namespace
> leaks in any container where it runs (details in the patch
> descriptions).
> 

So with this approach, what happens if the NFS mount was created in a
container, but got bind mounted somewhere else?

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

Re: [PATCH RFC 9/9] sunrpc: don't upgrade passive net reference in xs_create_sock

[Jeff Layton]

On Mon, 2025-03-17 at 21:28 +0000, Trond Myklebust wrote:
> On Mon, 2025-03-17 at 17:00 -0400, Jeff Layton wrote:
> > With the move to having sunrpc client xprts not hold active
> > references
> > to the net namespace, there is no need to upgrade the socket's
> > reference
> > in xs_create_sock. Just keep the passive reference instead.
> > 
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >  net/sunrpc/xprtsock.c | 3 ---
> >  1 file changed, 3 deletions(-)
> > 
> > diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
> > index
> > 83cc095846d356f24aed26e2f98525662a6cff1f..0c3d7552f772d6f8477a3aed8f0
> > c513b62cdf589 100644
> > --- a/net/sunrpc/xprtsock.c
> > +++ b/net/sunrpc/xprtsock.c
> > @@ -1941,9 +1941,6 @@ static struct socket *xs_create_sock(struct
> > rpc_xprt *xprt,
> >  		goto out;
> >  	}
> >  
> > -	if (protocol == IPPROTO_TCP)
> > -		sk_net_refcnt_upgrade(sock->sk);
> > -
> >  	filp = sock_alloc_file(sock, O_NONBLOCK, NULL);
> >  	if (IS_ERR(filp))
> >  		return ERR_CAST(filp);
> > 
> 
> Is this not going to reintroduce the bug described by
> https://lore.kernel.org/netdev/67b72aeb.050a0220.14d86d.0283.GAE@google.com/T/#u
> ?
> 
> As I understand it, the problem has nothing to do with whether or not
> NFS or the RPC layer holds a reference to the net namespace, but rather
> whether there are still packets in the socket queues at the time when
> that net namespace is being freed.
> 
> 

I don't think so. That syzkaller report was closed by this patch:

    5c70eb5c593d net: better track kernel sockets lifetime

That says:

    "To fix this, make sure that kernel sockets own a reference on net
passive."

With this, we still do keep a passive reference on the net in the
socket, which I think is enough.

That said, I'd appreciate a look at this from the netdev folks.
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH RFC 9/9] sunrpc: don't upgrade passive net reference in xs_create_sock

[Trond Myklebust]

On Mon, 2025-03-17 at 17:36 -0400, Jeff Layton wrote:
> On Mon, 2025-03-17 at 21:28 +0000, Trond Myklebust wrote:
> > On Mon, 2025-03-17 at 17:00 -0400, Jeff Layton wrote:
> > > With the move to having sunrpc client xprts not hold active
> > > references
> > > to the net namespace, there is no need to upgrade the socket's
> > > reference
> > > in xs_create_sock. Just keep the passive reference instead.
> > > 
> > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > > ---
> > >  net/sunrpc/xprtsock.c | 3 ---
> > >  1 file changed, 3 deletions(-)
> > > 
> > > diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
> > > index
> > > 83cc095846d356f24aed26e2f98525662a6cff1f..0c3d7552f772d6f8477a3ae
> > > d8f0
> > > c513b62cdf589 100644
> > > --- a/net/sunrpc/xprtsock.c
> > > +++ b/net/sunrpc/xprtsock.c
> > > @@ -1941,9 +1941,6 @@ static struct socket *xs_create_sock(struct
> > > rpc_xprt *xprt,
> > >  		goto out;
> > >  	}
> > >  
> > > -	if (protocol == IPPROTO_TCP)
> > > -		sk_net_refcnt_upgrade(sock->sk);
> > > -
> > >  	filp = sock_alloc_file(sock, O_NONBLOCK, NULL);
> > >  	if (IS_ERR(filp))
> > >  		return ERR_CAST(filp);
> > > 
> > 
> > Is this not going to reintroduce the bug described by
> > https://lore.kernel.org/netdev/67b72aeb.050a0220.14d86d.0283.GAE@google.com/T/#u
> > ?
> > 
> > As I understand it, the problem has nothing to do with whether or
> > not
> > NFS or the RPC layer holds a reference to the net namespace, but
> > rather
> > whether there are still packets in the socket queues at the time
> > when
> > that net namespace is being freed.
> > 
> > 
> 
> I don't think so. That syzkaller report was closed by this patch:
> 
>     5c70eb5c593d net: better track kernel sockets lifetime
> 
> That says:
> 
>     "To fix this, make sure that kernel sockets own a reference on
> net
> passive."
> 
> With this, we still do keep a passive reference on the net in the
> socket, which I think is enough.

No. You just removed that by reverting the patch that assigns the
passive reference.

> 
> That said, I'd appreciate a look at this from the netdev folks.

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

Re: [PATCH RFC 9/9] sunrpc: don't upgrade passive net reference in xs_create_sock

[Jeff Layton]

On Mon, 2025-03-17 at 21:37 +0000, Trond Myklebust wrote:
> On Mon, 2025-03-17 at 17:36 -0400, Jeff Layton wrote:
> > On Mon, 2025-03-17 at 21:28 +0000, Trond Myklebust wrote:
> > > On Mon, 2025-03-17 at 17:00 -0400, Jeff Layton wrote:
> > > > With the move to having sunrpc client xprts not hold active
> > > > references
> > > > to the net namespace, there is no need to upgrade the socket's
> > > > reference
> > > > in xs_create_sock. Just keep the passive reference instead.
> > > > 
> > > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > > > ---
> > > >  net/sunrpc/xprtsock.c | 3 ---
> > > >  1 file changed, 3 deletions(-)
> > > > 
> > > > diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
> > > > index
> > > > 83cc095846d356f24aed26e2f98525662a6cff1f..0c3d7552f772d6f8477a3ae
> > > > d8f0
> > > > c513b62cdf589 100644
> > > > --- a/net/sunrpc/xprtsock.c
> > > > +++ b/net/sunrpc/xprtsock.c
> > > > @@ -1941,9 +1941,6 @@ static struct socket *xs_create_sock(struct
> > > > rpc_xprt *xprt,
> > > >  		goto out;
> > > >  	}
> > > >  
> > > > -	if (protocol == IPPROTO_TCP)
> > > > -		sk_net_refcnt_upgrade(sock->sk);
> > > > -
> > > >  	filp = sock_alloc_file(sock, O_NONBLOCK, NULL);
> > > >  	if (IS_ERR(filp))
> > > >  		return ERR_CAST(filp);
> > > > 
> > > 
> > > Is this not going to reintroduce the bug described by
> > > https://lore.kernel.org/netdev/67b72aeb.050a0220.14d86d.0283.GAE@google.com/T/#u
> > > ?
> > > 
> > > As I understand it, the problem has nothing to do with whether or
> > > not
> > > NFS or the RPC layer holds a reference to the net namespace, but
> > > rather
> > > whether there are still packets in the socket queues at the time
> > > when
> > > that net namespace is being freed.
> > > 
> > > 
> > 
> > I don't think so. That syzkaller report was closed by this patch:
> > 
> >     5c70eb5c593d net: better track kernel sockets lifetime
> > 
> > That says:
> > 
> >     "To fix this, make sure that kernel sockets own a reference on
> > net
> > passive."
> > 
> > With this, we still do keep a passive reference on the net in the
> > socket, which I think is enough.
> 
> No. You just removed that by reverting the patch that assigns the
> passive reference.
> 

That's not how I read sk_net_refcnt_upgrade(). The socket already holds
a passive reference on the netns. sk_net_refcnt_upgrade() puts that
reference and then gets an active reference to the netns.

With this patchset, we just need to keep the passive one (I think).
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH RFC 0/9] nfs/sunrpc: stop holding netns references in client-side NFS and RPC objects

[Jeff Layton]

On Mon, 2025-03-17 at 21:35 +0000, Trond Myklebust wrote:
> On Mon, 2025-03-17 at 16:59 -0400, Jeff Layton wrote:
> > We have a long-standing problem with containers that have NFS mounts
> > in
> > them. Best practice is to unmount gracefully, of course, but
> > sometimes
> > containers just spontaneously die (e.g. SIGSEGV in the init task in
> > the
> > container). When that happens the orchestrator will see that all of
> > the
> > tasks are dead, and will detach the mount namespace and kill off the
> > network connection.
> > 
> > If there are RPCs in flight at the time, the rpc_clnt will try to
> > retransmit them indefinitely, but there is no hope of them ever
> > contacting the server since nothing in userland can reach the netns
> > at that point to fix anything.
> > 
> > This patchset takes the approach of changing various nfs client and
> > sunrpc objects to not hold a netns reference. Instead, when a nfs_net
> > or
> > sunrpc_net is exiting, all nfs_server, nfs_client and rpc_clnt
> > objects
> > associated with it are shut down, and the pre_exit functions block
> > until they are gone.
> > 
> > With this approach, when the last userland task in the container
> > exits,
> > the NFS and RPC clients get cleaned up automatically. As a bonus,
> > this
> > fixes another bug with the gssproxy RPC client that causes net
> > namespace
> > leaks in any container where it runs (details in the patch
> > descriptions).
> > 
> 
> So with this approach, what happens if the NFS mount was created in a
> container, but got bind mounted somewhere else?
> 

The lifetime of these objects are tied to the net namespace. If it gets
bind-mounted into a different mount namespace, while the tasks are
setns()'ed into the correct net namespace, then I expect the mount
would end up shut down at that point and be unusable, just like if you
echo 1 into the shutdown file in sysfs.

Hopefully no one is doing anything that silly. You wouldn't be able to
upcall, for one thing, since there wouldn't be any more userland
processes attached to the netns.

I'll test that scenario and get back to you though. I do want to make
sure that that's not going to lead to a crash or anything.
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH RFC 0/9] nfs/sunrpc: stop holding netns references in client-side NFS and RPC objects

[Trond Myklebust]

On Mon, 2025-03-17 at 17:57 -0400, Jeff Layton wrote:
> On Mon, 2025-03-17 at 21:35 +0000, Trond Myklebust wrote:
> > On Mon, 2025-03-17 at 16:59 -0400, Jeff Layton wrote:
> > > We have a long-standing problem with containers that have NFS
> > > mounts
> > > in
> > > them. Best practice is to unmount gracefully, of course, but
> > > sometimes
> > > containers just spontaneously die (e.g. SIGSEGV in the init task
> > > in
> > > the
> > > container). When that happens the orchestrator will see that all
> > > of
> > > the
> > > tasks are dead, and will detach the mount namespace and kill off
> > > the
> > > network connection.
> > > 
> > > If there are RPCs in flight at the time, the rpc_clnt will try to
> > > retransmit them indefinitely, but there is no hope of them ever
> > > contacting the server since nothing in userland can reach the
> > > netns
> > > at that point to fix anything.
> > > 
> > > This patchset takes the approach of changing various nfs client
> > > and
> > > sunrpc objects to not hold a netns reference. Instead, when a
> > > nfs_net
> > > or
> > > sunrpc_net is exiting, all nfs_server, nfs_client and rpc_clnt
> > > objects
> > > associated with it are shut down, and the pre_exit functions
> > > block
> > > until they are gone.
> > > 
> > > With this approach, when the last userland task in the container
> > > exits,
> > > the NFS and RPC clients get cleaned up automatically. As a bonus,
> > > this
> > > fixes another bug with the gssproxy RPC client that causes net
> > > namespace
> > > leaks in any container where it runs (details in the patch
> > > descriptions).
> > > 
> > 
> > So with this approach, what happens if the NFS mount was created in
> > a
> > container, but got bind mounted somewhere else?
> > 
> 
> The lifetime of these objects are tied to the net namespace. If it
> gets
> bind-mounted into a different mount namespace, while the tasks are
> setns()'ed into the correct net namespace, then I expect the mount
> would end up shut down at that point and be unusable, just like if
> you
> echo 1 into the shutdown file in sysfs.
> 
> Hopefully no one is doing anything that silly. You wouldn't be able
> to
> upcall, for one thing, since there wouldn't be any more userland
> processes attached to the netns.
> 
> I'll test that scenario and get back to you though. I do want to make
> sure that that's not going to lead to a crash or anything.

I agree with you that it's not a sane scenario, and that there is no
need to try to make it work. However the user space tools are there to
allow it to happen, so we need to ensure that the kernel won't panic or
cause any new exotic hangs.

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

Re: [PATCH RFC 0/9] nfs/sunrpc: stop holding netns references in client-side NFS and RPC objects

[Jeff Layton]

On Mon, 2025-03-17 at 22:11 +0000, Trond Myklebust wrote:
> On Mon, 2025-03-17 at 17:57 -0400, Jeff Layton wrote:
> > On Mon, 2025-03-17 at 21:35 +0000, Trond Myklebust wrote:
> > > On Mon, 2025-03-17 at 16:59 -0400, Jeff Layton wrote:
> > > > We have a long-standing problem with containers that have NFS
> > > > mounts
> > > > in
> > > > them. Best practice is to unmount gracefully, of course, but
> > > > sometimes
> > > > containers just spontaneously die (e.g. SIGSEGV in the init task
> > > > in
> > > > the
> > > > container). When that happens the orchestrator will see that all
> > > > of
> > > > the
> > > > tasks are dead, and will detach the mount namespace and kill off
> > > > the
> > > > network connection.
> > > > 
> > > > If there are RPCs in flight at the time, the rpc_clnt will try to
> > > > retransmit them indefinitely, but there is no hope of them ever
> > > > contacting the server since nothing in userland can reach the
> > > > netns
> > > > at that point to fix anything.
> > > > 
> > > > This patchset takes the approach of changing various nfs client
> > > > and
> > > > sunrpc objects to not hold a netns reference. Instead, when a
> > > > nfs_net
> > > > or
> > > > sunrpc_net is exiting, all nfs_server, nfs_client and rpc_clnt
> > > > objects
> > > > associated with it are shut down, and the pre_exit functions
> > > > block
> > > > until they are gone.
> > > > 
> > > > With this approach, when the last userland task in the container
> > > > exits,
> > > > the NFS and RPC clients get cleaned up automatically. As a bonus,
> > > > this
> > > > fixes another bug with the gssproxy RPC client that causes net
> > > > namespace
> > > > leaks in any container where it runs (details in the patch
> > > > descriptions).
> > > > 
> > > 
> > > So with this approach, what happens if the NFS mount was created in
> > > a
> > > container, but got bind mounted somewhere else?
> > > 
> > 
> > The lifetime of these objects are tied to the net namespace. If it
> > gets
> > bind-mounted into a different mount namespace, while the tasks are
> > setns()'ed into the correct net namespace, then I expect the mount
> > would end up shut down at that point and be unusable, just like if
> > you
> > echo 1 into the shutdown file in sysfs.
> > 
> > Hopefully no one is doing anything that silly. You wouldn't be able
> > to
> > upcall, for one thing, since there wouldn't be any more userland
> > processes attached to the netns.
> > 
> > I'll test that scenario and get back to you though. I do want to make
> > sure that that's not going to lead to a crash or anything.
> 
> I agree with you that it's not a sane scenario, and that there is no
> need to try to make it work. However the user space tools are there to
> allow it to happen, so we need to ensure that the kernel won't panic or
> cause any new exotic hangs.

Unfortunately, this does create a hang.

Bind-mounting it will cause the superblock's refcount to increase,
which keeps the nfs_server struct active. That holds a reference to the
nfs_client, which prevents everything from coming down properly in
pre_exit.

I'll have to think about how we can solve that. Let me know if you have
ideas.
-- 
Jeff Layton <jlayton@kernel.org>