[PATCH v2 0/2] nfs: don't share pNFS DS connections between net namespaces

[PATCH v2 0/2] nfs: don't share pNFS DS connections between net namespaces

[Jeff Layton]

Sargun noted that he had seen some cases where a dead netns with a stuck
NFS mount in it would affect other containers. Omar took a look last
week and noted that there was a global list of DS connections and there
was no segregation by namespace.

The first patch in the series fixes this in a minimal way by tracking
struct net in the nfs4_pnfs_ds structure and not matching it when the
caller's net is different. The second patch goes the rest of the way,
and makes the nfs4_data_server_cache and lock be per-net.

My thought was that the first patch should be suitable for stable
kernels, and both could go to mainline. If you think the risk is low
though, we could just squash the two together.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
Changes in v2:
- fix build break when IS_ENABLED(CONFIG_NFS_V4_1) is false
- Link to v1: https://lore.kernel.org/r/20250410-nfs-ds-netns-v1-0-cc6236e84190@kernel.org

---
Jeff Layton (2):
      nfs: don't share pNFS DS connections between net namespaces
      nfs: move the nfs4_data_server_cache into struct nfs_net

 fs/nfs/client.c                           |  7 +++++++
 fs/nfs/filelayout/filelayoutdev.c         |  6 +++---
 fs/nfs/flexfilelayout/flexfilelayoutdev.c |  6 +++---
 fs/nfs/netns.h                            |  6 +++++-
 fs/nfs/pnfs.h                             |  4 +++-
 fs/nfs/pnfs_nfs.c                         | 32 +++++++++++++++++--------------
 6 files changed, 39 insertions(+), 22 deletions(-)
---
base-commit: cf03f570936ac96ed4775eb2e4f1a6ab6a13f143
change-id: 20250410-nfs-ds-netns-321c78c16a79

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

[PATCH v2 1/2] nfs: don't share pNFS DS connections between net namespaces

[Jeff Layton]

Currently, different NFS clients can share the same DS connections, even
when they are in different net namespaces. If a containerized client
creates a DS connection, another container can find and use it. When the
first client exits, the connection will which can lead to stalls in
other clients.

Add a net namespace pointer to struct nfs4_pnfs_ds, and compare those
value to the caller's netns in _data_server_lookup_locked() when
searching for a nfs4_pnfs_ds to match.

Reported-by: Omar Sandoval <osandov@osandov.com>
Reported-by: Sargun Dillon <sargun@sargun.me>
Closes: https://lore.kernel.org/linux-nfs/Z_ArpQC_vREh_hEA@telecaster/
Tested-by: Sargun Dillon <sargun@sargun.me>
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/nfs/filelayout/filelayoutdev.c         | 6 +++---
 fs/nfs/flexfilelayout/flexfilelayoutdev.c | 6 +++---
 fs/nfs/pnfs.h                             | 4 +++-
 fs/nfs/pnfs_nfs.c                         | 9 +++++----
 4 files changed, 14 insertions(+), 11 deletions(-)

diff --git a/fs/nfs/filelayout/filelayoutdev.c b/fs/nfs/filelayout/filelayoutdev.c
index 4fa304fa5bc4b2346458877c39a558936a49317a..29d9234d5c085f3feda9abf6a98cd5e272f22613 100644
--- a/fs/nfs/filelayout/filelayoutdev.c
+++ b/fs/nfs/filelayout/filelayoutdev.c
@@ -76,6 +76,7 @@ nfs4_fl_alloc_deviceid_node(struct nfs_server *server, struct pnfs_device *pdev,
 	struct page *scratch;
 	struct list_head dsaddrs;
 	struct nfs4_pnfs_ds_addr *da;
+	struct net *net = server->nfs_client->cl_net;
 
 	/* set up xdr stream */
 	scratch = alloc_page(gfp_flags);
@@ -159,8 +160,7 @@ nfs4_fl_alloc_deviceid_node(struct nfs_server *server, struct pnfs_device *pdev,
 
 		mp_count = be32_to_cpup(p); /* multipath count */
 		for (j = 0; j < mp_count; j++) {
-			da = nfs4_decode_mp_ds_addr(server->nfs_client->cl_net,
-						    &stream, gfp_flags);
+			da = nfs4_decode_mp_ds_addr(net, &stream, gfp_flags);
 			if (da)
 				list_add_tail(&da->da_node, &dsaddrs);
 		}
@@ -170,7 +170,7 @@ nfs4_fl_alloc_deviceid_node(struct nfs_server *server, struct pnfs_device *pdev,
 			goto out_err_free_deviceid;
 		}
 
-		dsaddr->ds_list[i] = nfs4_pnfs_ds_add(&dsaddrs, gfp_flags);
+		dsaddr->ds_list[i] = nfs4_pnfs_ds_add(net, &dsaddrs, gfp_flags);
 		if (!dsaddr->ds_list[i])
 			goto out_err_drain_dsaddrs;
 		trace_fl_getdevinfo(server, &pdev->dev_id, dsaddr->ds_list[i]->ds_remotestr);
diff --git a/fs/nfs/flexfilelayout/flexfilelayoutdev.c b/fs/nfs/flexfilelayout/flexfilelayoutdev.c
index e58bedfb1dcc14307e0cb3bc7011ed3f199eecc6..4a304cf17c4b07a07b9e25e48af9d83c345d2ac1 100644
--- a/fs/nfs/flexfilelayout/flexfilelayoutdev.c
+++ b/fs/nfs/flexfilelayout/flexfilelayoutdev.c
@@ -49,6 +49,7 @@ nfs4_ff_alloc_deviceid_node(struct nfs_server *server, struct pnfs_device *pdev,
 	struct nfs4_pnfs_ds_addr *da;
 	struct nfs4_ff_layout_ds *new_ds = NULL;
 	struct nfs4_ff_ds_version *ds_versions = NULL;
+	struct net *net = server->nfs_client->cl_net;
 	u32 mp_count;
 	u32 version_count;
 	__be32 *p;
@@ -80,8 +81,7 @@ nfs4_ff_alloc_deviceid_node(struct nfs_server *server, struct pnfs_device *pdev,
 
 	for (i = 0; i < mp_count; i++) {
 		/* multipath ds */
-		da = nfs4_decode_mp_ds_addr(server->nfs_client->cl_net,
-					    &stream, gfp_flags);
+		da = nfs4_decode_mp_ds_addr(net, &stream, gfp_flags);
 		if (da)
 			list_add_tail(&da->da_node, &dsaddrs);
 	}
@@ -149,7 +149,7 @@ nfs4_ff_alloc_deviceid_node(struct nfs_server *server, struct pnfs_device *pdev,
 	new_ds->ds_versions = ds_versions;
 	new_ds->ds_versions_cnt = version_count;
 
-	new_ds->ds = nfs4_pnfs_ds_add(&dsaddrs, gfp_flags);
+	new_ds->ds = nfs4_pnfs_ds_add(net, &dsaddrs, gfp_flags);
 	if (!new_ds->ds)
 		goto out_err_drain_dsaddrs;
 
diff --git a/fs/nfs/pnfs.h b/fs/nfs/pnfs.h
index 30d2613e912b8804c994fc19e25b767f360ce51a..91ff877185c8afe462eb81f6571afd3ade14ffb4 100644
--- a/fs/nfs/pnfs.h
+++ b/fs/nfs/pnfs.h
@@ -60,6 +60,7 @@ struct nfs4_pnfs_ds {
 	struct list_head	ds_node;  /* nfs4_pnfs_dev_hlist dev_dslist */
 	char			*ds_remotestr;	/* comma sep list of addrs */
 	struct list_head	ds_addrs;
+	const struct net	*ds_net;
 	struct nfs_client	*ds_clp;
 	refcount_t		ds_count;
 	unsigned long		ds_state;
@@ -415,7 +416,8 @@ int pnfs_generic_commit_pagelist(struct inode *inode,
 int pnfs_generic_scan_commit_lists(struct nfs_commit_info *cinfo, int max);
 void pnfs_generic_write_commit_done(struct rpc_task *task, void *data);
 void nfs4_pnfs_ds_put(struct nfs4_pnfs_ds *ds);
-struct nfs4_pnfs_ds *nfs4_pnfs_ds_add(struct list_head *dsaddrs,
+struct nfs4_pnfs_ds *nfs4_pnfs_ds_add(const struct net *net,
+				      struct list_head *dsaddrs,
 				      gfp_t gfp_flags);
 void nfs4_pnfs_v3_ds_connect_unload(void);
 int nfs4_pnfs_ds_connect(struct nfs_server *mds_srv, struct nfs4_pnfs_ds *ds,
diff --git a/fs/nfs/pnfs_nfs.c b/fs/nfs/pnfs_nfs.c
index dbef837e871ad44e401461c39f04b159ec43f2f6..2ee20a0f0b36d3b38e35c4cad966b9d58fa822f4 100644
--- a/fs/nfs/pnfs_nfs.c
+++ b/fs/nfs/pnfs_nfs.c
@@ -604,12 +604,12 @@ _same_data_server_addrs_locked(const struct list_head *dsaddrs1,
  * Lookup DS by addresses.  nfs4_ds_cache_lock is held
  */
 static struct nfs4_pnfs_ds *
-_data_server_lookup_locked(const struct list_head *dsaddrs)
+_data_server_lookup_locked(const struct net *net, const struct list_head *dsaddrs)
 {
 	struct nfs4_pnfs_ds *ds;
 
 	list_for_each_entry(ds, &nfs4_data_server_cache, ds_node)
-		if (_same_data_server_addrs_locked(&ds->ds_addrs, dsaddrs))
+		if (ds->ds_net == net && _same_data_server_addrs_locked(&ds->ds_addrs, dsaddrs))
 			return ds;
 	return NULL;
 }
@@ -716,7 +716,7 @@ nfs4_pnfs_remotestr(struct list_head *dsaddrs, gfp_t gfp_flags)
  * uncached and return cached struct nfs4_pnfs_ds.
  */
 struct nfs4_pnfs_ds *
-nfs4_pnfs_ds_add(struct list_head *dsaddrs, gfp_t gfp_flags)
+nfs4_pnfs_ds_add(const struct net *net, struct list_head *dsaddrs, gfp_t gfp_flags)
 {
 	struct nfs4_pnfs_ds *tmp_ds, *ds = NULL;
 	char *remotestr;
@@ -734,13 +734,14 @@ nfs4_pnfs_ds_add(struct list_head *dsaddrs, gfp_t gfp_flags)
 	remotestr = nfs4_pnfs_remotestr(dsaddrs, gfp_flags);
 
 	spin_lock(&nfs4_ds_cache_lock);
-	tmp_ds = _data_server_lookup_locked(dsaddrs);
+	tmp_ds = _data_server_lookup_locked(net, dsaddrs);
 	if (tmp_ds == NULL) {
 		INIT_LIST_HEAD(&ds->ds_addrs);
 		list_splice_init(dsaddrs, &ds->ds_addrs);
 		ds->ds_remotestr = remotestr;
 		refcount_set(&ds->ds_count, 1);
 		INIT_LIST_HEAD(&ds->ds_node);
+		ds->ds_net = net;
 		ds->ds_clp = NULL;
 		list_add(&ds->ds_node, &nfs4_data_server_cache);
 		dprintk("%s add new data server %s\n", __func__,

-- 
2.49.0

Re: [PATCH v2 1/2] nfs: don't share pNFS DS connections between net namespaces

[Benjamin Coddington]

On 10 Apr 2025, at 16:42, Jeff Layton wrote:

> Currently, different NFS clients can share the same DS connections, even
> when they are in different net namespaces. If a containerized client
> creates a DS connection, another container can find and use it. When the
> first client exits, the connection will which can lead to stalls in

                                         ^^ close ?

> other clients.
>
> Add a net namespace pointer to struct nfs4_pnfs_ds, and compare those
> value to the caller's netns in _data_server_lookup_locked() when
> searching for a nfs4_pnfs_ds to match.
>
> Reported-by: Omar Sandoval <osandov@osandov.com>
> Reported-by: Sargun Dillon <sargun@sargun.me>
> Closes: https://lore.kernel.org/linux-nfs/Z_ArpQC_vREh_hEA@telecaster/
> Tested-by: Sargun Dillon <sargun@sargun.me>
> Signed-off-by: Jeff Layton <jlayton@kernel.org>

Looks good to me,

Reviewed-by: Benjamin Coddington <bcodding@redhat.com>

Ben

Re: [PATCH v2 1/2] nfs: don't share pNFS DS connections between net namespaces

[Jeff Layton]

On Fri, 2025-04-11 at 09:57 -0400, Benjamin Coddington wrote:
> On 10 Apr 2025, at 16:42, Jeff Layton wrote:
> 
> > Currently, different NFS clients can share the same DS connections, even
> > when they are in different net namespaces. If a containerized client
> > creates a DS connection, another container can find and use it. When the
> > first client exits, the connection will which can lead to stalls in
> 
>                                          ^^ close ?
> 

Yes, thanks. Trond/Anna, can you fix before merging?

> > other clients.
> > 
> > Add a net namespace pointer to struct nfs4_pnfs_ds, and compare those
> > value to the caller's netns in _data_server_lookup_locked() when
> > searching for a nfs4_pnfs_ds to match.
> > 
> > Reported-by: Omar Sandoval <osandov@osandov.com>
> > Reported-by: Sargun Dillon <sargun@sargun.me>
> > Closes: https://lore.kernel.org/linux-nfs/Z_ArpQC_vREh_hEA@telecaster/
> > Tested-by: Sargun Dillon <sargun@sargun.me>
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> 
> Looks good to me,
> 
> Reviewed-by: Benjamin Coddington <bcodding@redhat.com>
> 
> Ben
> 

Thank you!
-- 
Jeff Layton <jlayton@kernel.org>

[PATCH v2 2/2] nfs: move the nfs4_data_server_cache into struct nfs_net

[Jeff Layton]

Since struct nfs4_pnfs_ds should not be shared between net namespaces,
move from a global list of objects to a per-netns list and spinlock.

Tested-by: Sargun Dillon <sargun@sargun.me>
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/nfs/client.c   |  7 +++++++
 fs/nfs/netns.h    |  6 +++++-
 fs/nfs/pnfs_nfs.c | 31 +++++++++++++++++--------------
 3 files changed, 29 insertions(+), 15 deletions(-)

diff --git a/fs/nfs/client.c b/fs/nfs/client.c
index 9500b46005b0148a5a9a7d464095ca944de06bb5..d975123530506cd789a55337212a38ee93e84858 100644
--- a/fs/nfs/client.c
+++ b/fs/nfs/client.c
@@ -1199,6 +1199,10 @@ void nfs_clients_init(struct net *net)
 	INIT_LIST_HEAD(&nn->nfs_volume_list);
 #if IS_ENABLED(CONFIG_NFS_V4)
 	idr_init(&nn->cb_ident_idr);
+#endif
+#if IS_ENABLED(CONFIG_NFS_V4_1)
+	INIT_LIST_HEAD(&nn->nfs4_data_server_cache);
+	spin_lock_init(&nn->nfs4_data_server_lock);
 #endif
 	spin_lock_init(&nn->nfs_client_lock);
 	nn->boot_time = ktime_get_real();
@@ -1216,6 +1220,9 @@ void nfs_clients_exit(struct net *net)
 	nfs_cleanup_cb_ident_idr(net);
 	WARN_ON_ONCE(!list_empty(&nn->nfs_client_list));
 	WARN_ON_ONCE(!list_empty(&nn->nfs_volume_list));
+#if IS_ENABLED(CONFIG_NFS_V4_1)
+	WARN_ON_ONCE(!list_empty(&nn->nfs4_data_server_cache));
+#endif
 }
 
 #ifdef CONFIG_PROC_FS
diff --git a/fs/nfs/netns.h b/fs/nfs/netns.h
index a68b21603ea9a867ba513e2a667b08fbc6d80dd8..6ba3ea39e928c066003c6db29dd59afc2cfa9f85 100644
--- a/fs/nfs/netns.h
+++ b/fs/nfs/netns.h
@@ -31,7 +31,11 @@ struct nfs_net {
 	unsigned short nfs_callback_tcpport;
 	unsigned short nfs_callback_tcpport6;
 	int cb_users[NFS4_MAX_MINOR_VERSION + 1];
-#endif
+#endif /* CONFIG_NFS_V4 */
+#if IS_ENABLED(CONFIG_NFS_V4_1)
+	struct list_head nfs4_data_server_cache;
+	spinlock_t nfs4_data_server_lock;
+#endif /* CONFIG_NFS_V4_1 */
 	struct nfs_netns_client *nfs_client;
 	spinlock_t nfs_client_lock;
 	ktime_t boot_time;
diff --git a/fs/nfs/pnfs_nfs.c b/fs/nfs/pnfs_nfs.c
index 2ee20a0f0b36d3b38e35c4cad966b9d58fa822f4..91ef486f40b943a1dc55164e610378ef73781e55 100644
--- a/fs/nfs/pnfs_nfs.c
+++ b/fs/nfs/pnfs_nfs.c
@@ -16,6 +16,7 @@
 #include "nfs4session.h"
 #include "internal.h"
 #include "pnfs.h"
+#include "netns.h"
 
 #define NFSDBG_FACILITY		NFSDBG_PNFS
 
@@ -504,14 +505,14 @@ EXPORT_SYMBOL_GPL(pnfs_generic_commit_pagelist);
 /*
  * Data server cache
  *
- * Data servers can be mapped to different device ids.
- * nfs4_pnfs_ds reference counting
+ * Data servers can be mapped to different device ids, but should
+ * never be shared between net namespaces.
+ *
+ * nfs4_pnfs_ds reference counting:
  *   - set to 1 on allocation
  *   - incremented when a device id maps a data server already in the cache.
  *   - decremented when deviceid is removed from the cache.
  */
-static DEFINE_SPINLOCK(nfs4_ds_cache_lock);
-static LIST_HEAD(nfs4_data_server_cache);
 
 /* Debug routines */
 static void
@@ -604,12 +605,12 @@ _same_data_server_addrs_locked(const struct list_head *dsaddrs1,
  * Lookup DS by addresses.  nfs4_ds_cache_lock is held
  */
 static struct nfs4_pnfs_ds *
-_data_server_lookup_locked(const struct net *net, const struct list_head *dsaddrs)
+_data_server_lookup_locked(const struct nfs_net *nn, const struct list_head *dsaddrs)
 {
 	struct nfs4_pnfs_ds *ds;
 
-	list_for_each_entry(ds, &nfs4_data_server_cache, ds_node)
-		if (ds->ds_net == net && _same_data_server_addrs_locked(&ds->ds_addrs, dsaddrs))
+	list_for_each_entry(ds, &nn->nfs4_data_server_cache, ds_node)
+		if (_same_data_server_addrs_locked(&ds->ds_addrs, dsaddrs))
 			return ds;
 	return NULL;
 }
@@ -653,10 +654,11 @@ static void destroy_ds(struct nfs4_pnfs_ds *ds)
 
 void nfs4_pnfs_ds_put(struct nfs4_pnfs_ds *ds)
 {
-	if (refcount_dec_and_lock(&ds->ds_count,
-				&nfs4_ds_cache_lock)) {
+	struct nfs_net *nn = net_generic(ds->ds_net, nfs_net_id);
+
+	if (refcount_dec_and_lock(&ds->ds_count, &nn->nfs4_data_server_lock)) {
 		list_del_init(&ds->ds_node);
-		spin_unlock(&nfs4_ds_cache_lock);
+		spin_unlock(&nn->nfs4_data_server_lock);
 		destroy_ds(ds);
 	}
 }
@@ -718,6 +720,7 @@ nfs4_pnfs_remotestr(struct list_head *dsaddrs, gfp_t gfp_flags)
 struct nfs4_pnfs_ds *
 nfs4_pnfs_ds_add(const struct net *net, struct list_head *dsaddrs, gfp_t gfp_flags)
 {
+	struct nfs_net *nn = net_generic(net, nfs_net_id);
 	struct nfs4_pnfs_ds *tmp_ds, *ds = NULL;
 	char *remotestr;
 
@@ -733,8 +736,8 @@ nfs4_pnfs_ds_add(const struct net *net, struct list_head *dsaddrs, gfp_t gfp_fla
 	/* this is only used for debugging, so it's ok if its NULL */
 	remotestr = nfs4_pnfs_remotestr(dsaddrs, gfp_flags);
 
-	spin_lock(&nfs4_ds_cache_lock);
-	tmp_ds = _data_server_lookup_locked(net, dsaddrs);
+	spin_lock(&nn->nfs4_data_server_lock);
+	tmp_ds = _data_server_lookup_locked(nn, dsaddrs);
 	if (tmp_ds == NULL) {
 		INIT_LIST_HEAD(&ds->ds_addrs);
 		list_splice_init(dsaddrs, &ds->ds_addrs);
@@ -743,7 +746,7 @@ nfs4_pnfs_ds_add(const struct net *net, struct list_head *dsaddrs, gfp_t gfp_fla
 		INIT_LIST_HEAD(&ds->ds_node);
 		ds->ds_net = net;
 		ds->ds_clp = NULL;
-		list_add(&ds->ds_node, &nfs4_data_server_cache);
+		list_add(&ds->ds_node, &nn->nfs4_data_server_cache);
 		dprintk("%s add new data server %s\n", __func__,
 			ds->ds_remotestr);
 	} else {
@@ -755,7 +758,7 @@ nfs4_pnfs_ds_add(const struct net *net, struct list_head *dsaddrs, gfp_t gfp_fla
 			refcount_read(&tmp_ds->ds_count));
 		ds = tmp_ds;
 	}
-	spin_unlock(&nfs4_ds_cache_lock);
+	spin_unlock(&nn->nfs4_data_server_lock);
 out:
 	return ds;
 }

-- 
2.49.0

Re: [PATCH v2 2/2] nfs: move the nfs4_data_server_cache into struct nfs_net

[Benjamin Coddington]

On 10 Apr 2025, at 16:42, Jeff Layton wrote:

> Since struct nfs4_pnfs_ds should not be shared between net namespaces,
> move from a global list of objects to a per-netns list and spinlock.
>
> Tested-by: Sargun Dillon <sargun@sargun.me>
> Signed-off-by: Jeff Layton <jlayton@kernel.org>

Reviewed-by: Benjamin Coddington <bcodding@redhat.com>

Ben

Re: [PATCH v2 0/2] nfs: don't share pNFS DS connections between net namespaces

[Jeff Layton]

On Thu, 2025-04-10 at 16:42 -0400, Jeff Layton wrote:
> Sargun noted that he had seen some cases where a dead netns with a stuck
> NFS mount in it would affect other containers. Omar took a look last
> week and noted that there was a global list of DS connections and there
> was no segregation by namespace.
> 
> The first patch in the series fixes this in a minimal way by tracking
> struct net in the nfs4_pnfs_ds structure and not matching it when the
> caller's net is different. The second patch goes the rest of the way,
> and makes the nfs4_data_server_cache and lock be per-net.
> 
> My thought was that the first patch should be suitable for stable
> kernels, and both could go to mainline. If you think the risk is low
> though, we could just squash the two together.
> 
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
> Changes in v2:
> - fix build break when IS_ENABLED(CONFIG_NFS_V4_1) is false
> - Link to v1: https://lore.kernel.org/r/20250410-nfs-ds-netns-v1-0-cc6236e84190@kernel.org
> 
> ---
> Jeff Layton (2):
>       nfs: don't share pNFS DS connections between net namespaces
>       nfs: move the nfs4_data_server_cache into struct nfs_net
> 
>  fs/nfs/client.c                           |  7 +++++++
>  fs/nfs/filelayout/filelayoutdev.c         |  6 +++---
>  fs/nfs/flexfilelayout/flexfilelayoutdev.c |  6 +++---
>  fs/nfs/netns.h                            |  6 +++++-
>  fs/nfs/pnfs.h                             |  4 +++-
>  fs/nfs/pnfs_nfs.c                         | 32 +++++++++++++++++--------------
>  6 files changed, 39 insertions(+), 22 deletions(-)
> ---
> base-commit: cf03f570936ac96ed4775eb2e4f1a6ab6a13f143
> change-id: 20250410-nfs-ds-netns-321c78c16a79
> 
> Best regards,

Trond,

Does this set look reasonable for v6.15?

Thanks,
-- 
Jeff Layton <jlayton@kernel.org>