On Wed, 12 Jun 2024, Mike Snitzer wrote:
> Pass the stored cl_nfssvc_net from the client to the server as first
> argument to nfsd_open_local_fh() to ensure the proper network
> namespace is used for localio.
>
> Otherwise, before this commit, the nfs_client's network namespace was
> used (as extracted from the client's cl_rpcclient). This is clearly
> not going to allow proper functionality if the client and server
> happen to have disjoint network namespaces.
>
> Elected to not rename the nfsd_uuid_t structure despite it growing a
> non-uuid member. Can revisit later.
>
> Signed-off-by: Mike Snitzer <snitzer@xxxxxxxxxx>
> ---
> fs/nfs/client.c | 1 +
> fs/nfs/localio.c | 7 +++++--
> fs/nfs_common/nfslocalio.c | 15 +++++++++------
> fs/nfsd/localio.c | 9 +++++----
> fs/nfsd/nfssvc.c | 1 +
> fs/nfsd/vfs.h | 3 ++-
> include/linux/nfs_fs_sb.h | 1 +
> include/linux/nfslocalio.h | 10 ++++++----
> 8 files changed, 30 insertions(+), 17 deletions(-)
>
> diff --git a/fs/nfs/client.c b/fs/nfs/client.c
> index 00044d7eda48..4ca2245c8e2c 100644
> --- a/fs/nfs/client.c
> +++ b/fs/nfs/client.c
> @@ -171,6 +171,7 @@ struct nfs_client *nfs_alloc_client(const struct nfs_client_initdata *cl_init)
>
> INIT_LIST_HEAD(&clp->cl_superblocks);
> clp->cl_rpcclient = clp->cl_rpcclient_localio = ERR_PTR(-EINVAL);
> + clp->cl_nfssvc_net = NULL;
> clp->nfsd_open_local_fh = NULL;
>
> clp->cl_flags = cl_init->init_flags;
> diff --git a/fs/nfs/localio.c b/fs/nfs/localio.c
> index bd81b0afdbda..2d3ed9953ae2 100644
> --- a/fs/nfs/localio.c
> +++ b/fs/nfs/localio.c
> @@ -189,6 +189,7 @@ static bool nfs_local_server_getuuid(struct nfs_client *clp, uuid_t *nfsd_uuid)
> void nfs_local_probe(struct nfs_client *clp)
> {
> uuid_t uuid;
> + struct net *net = NULL;
>
> if (!localio_enabled)
> return;
> @@ -204,8 +205,9 @@ void nfs_local_probe(struct nfs_client *clp)
> if (!nfs_local_server_getuuid(clp, &uuid))
> return;
> /* Verify client's nfsd, with specififed uuid, is local */
> - if (!nfsd_uuid_is_local(&uuid))
> + if (!nfsd_uuid_is_local(&uuid, &net))
> return;
> + clp->cl_nfssvc_net = net;
> break;
> default:
> return; /* localio not supported */
> @@ -231,7 +233,8 @@ nfs_local_open_fh(struct nfs_client *clp, const struct cred *cred,
> if (mode & ~(FMODE_READ | FMODE_WRITE))
> return ERR_PTR(-EINVAL);
>
> - status = clp->nfsd_open_local_fh(clp->cl_rpcclient, cred, fh, mode, &filp);
> + status = clp->nfsd_open_local_fh(clp->cl_nfssvc_net, clp->cl_rpcclient,
> + cred, fh, mode, &filp);
> if (status < 0) {
> dprintk("%s: open local file failed error=%d\n",
> __func__, status);
> diff --git a/fs/nfs_common/nfslocalio.c b/fs/nfs_common/nfslocalio.c
> index c454c4100976..086e09b3ec38 100644
> --- a/fs/nfs_common/nfslocalio.c
> +++ b/fs/nfs_common/nfslocalio.c
> @@ -12,29 +12,32 @@ MODULE_LICENSE("GPL");
> /*
> * Global list of nfsd_uuid_t instances, add/remove
> * is protected by fs/nfsd/nfssvc.c:nfsd_mutex.
> - * Reads are protected RCU read lock (see below).
> + * Reads are protected by RCU read lock (see below).
> */
> LIST_HEAD(nfsd_uuids);
> EXPORT_SYMBOL(nfsd_uuids);
>
> /* Must be called with RCU read lock held. */
> -static const uuid_t * nfsd_uuid_lookup(const uuid_t *uuid)
> +static const uuid_t * nfsd_uuid_lookup(const uuid_t *uuid,
> + struct net **netp)
> {
> nfsd_uuid_t *nfsd_uuid;
>
> list_for_each_entry_rcu(nfsd_uuid, &nfsd_uuids, list)
> - if (uuid_equal(&nfsd_uuid->uuid, uuid))
> + if (uuid_equal(&nfsd_uuid->uuid, uuid)) {
> + *netp = nfsd_uuid->net;
> return &nfsd_uuid->uuid;
> + }
You probably need a get_net() call here to be sure the netns doesn't
disappear on you. And a matching put_net() somewhere.
But if the server you were talking to were in some container, that might
stop you from shutting down and restarting that container.
I think it would be better to do the "fh + server uuid -> struct file *"
lookup on each request and if that is too slow, make it faster with some
caching and some rhashtable or whatever. i.e. don't hold a reference
to anything in the server. The longest ref you can hold is to the
struct file, and that only until the IO completes.
NeilBrown
>
> return &uuid_null;
> }
>
> -bool nfsd_uuid_is_local(const uuid_t *uuid)
> +bool nfsd_uuid_is_local(const uuid_t *uuid, struct net **netp)
> {
> const uuid_t *nfsd_uuid;
>
> rcu_read_lock();
> - nfsd_uuid = nfsd_uuid_lookup(uuid);
> + nfsd_uuid = nfsd_uuid_lookup(uuid, netp);
> rcu_read_unlock();
>
> return !uuid_is_null(nfsd_uuid);
> @@ -51,7 +54,7 @@ EXPORT_SYMBOL_GPL(nfsd_uuid_is_local);
> * This allows some sanity checking, like giving up on localio if nfsd isn't loaded.
> */
>
> -extern int nfsd_open_local_fh(struct rpc_clnt *rpc_clnt,
> +extern int nfsd_open_local_fh(struct net *, struct rpc_clnt *rpc_clnt,
> const struct cred *cred, const struct nfs_fh *nfs_fh,
> const fmode_t fmode, struct file **pfilp);
>
> diff --git a/fs/nfsd/localio.c b/fs/nfsd/localio.c
> index 866e8c8a5548..a8a18f940a7e 100644
> --- a/fs/nfsd/localio.c
> +++ b/fs/nfsd/localio.c
> @@ -39,10 +39,10 @@ nfsd_local_fakerqst_destroy(struct svc_rqst *rqstp)
> }
>
> static struct svc_rqst *
> -nfsd_local_fakerqst_create(struct rpc_clnt *rpc_clnt, const struct cred *cred)
> +nfsd_local_fakerqst_create(struct net *net, struct rpc_clnt *rpc_clnt,
> + const struct cred *cred)
> {
> struct svc_rqst *rqstp;
> - struct net *net = rpc_net_ns(rpc_clnt);
> struct nfsd_net *nn = net_generic(net, nfsd_net_id);
> int status;
>
> @@ -127,7 +127,8 @@ nfsd_local_fakerqst_create(struct rpc_clnt *rpc_clnt, const struct cred *cred)
> * dependency on knfsd. So, there is no forward declaration in a header file
> * for it.
> */
> -int nfsd_open_local_fh(struct rpc_clnt *rpc_clnt,
> +int nfsd_open_local_fh(struct net *net,
> + struct rpc_clnt *rpc_clnt,
> const struct cred *cred,
> const struct nfs_fh *nfs_fh,
> const fmode_t fmode,
> @@ -144,7 +145,7 @@ int nfsd_open_local_fh(struct rpc_clnt *rpc_clnt,
> /* Save creds before calling into nfsd */
> save_cred = get_current_cred();
>
> - rqstp = nfsd_local_fakerqst_create(rpc_clnt, cred);
> + rqstp = nfsd_local_fakerqst_create(net, rpc_clnt, cred);
> if (IS_ERR(rqstp)) {
> status = PTR_ERR(rqstp);
> goto out_revertcred;
> diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
> index 22fb16258d44..fbe072dc53c0 100644
> --- a/fs/nfsd/nfssvc.c
> +++ b/fs/nfsd/nfssvc.c
> @@ -473,6 +473,7 @@ static int nfsd_startup_net(struct net *net, const struct cred *cred)
> #endif
> #if defined(CONFIG_NFSD_V3_LOCALIO) || defined(CONFIG_NFSD_V4_LOCALIO)
> INIT_LIST_HEAD(&nn->nfsd_uuid.list);
> + nn->nfsd_uuid.net = net;
> list_add_tail_rcu(&nn->nfsd_uuid.list, &nfsd_uuids);
> #endif
> nn->nfsd_net_up = true;
> diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h
> index 91c50649a8c7..af07bb146e81 100644
> --- a/fs/nfsd/vfs.h
> +++ b/fs/nfsd/vfs.h
> @@ -160,7 +160,8 @@ __be32 nfsd_permission(struct svc_rqst *, struct svc_export *,
>
> void nfsd_filp_close(struct file *fp);
>
> -int nfsd_open_local_fh(struct rpc_clnt *rpc_clnt,
> +int nfsd_open_local_fh(struct net *net,
> + struct rpc_clnt *rpc_clnt,
> const struct cred *cred,
> const struct nfs_fh *nfs_fh,
> const fmode_t fmode,
> diff --git a/include/linux/nfs_fs_sb.h b/include/linux/nfs_fs_sb.h
> index f5760b05ec87..f47ea512eb0a 100644
> --- a/include/linux/nfs_fs_sb.h
> +++ b/include/linux/nfs_fs_sb.h
> @@ -132,6 +132,7 @@ struct nfs_client {
> struct timespec64 cl_nfssvc_boot;
> seqlock_t cl_boot_lock;
> struct rpc_clnt * cl_rpcclient_localio; /* localio RPC client handle */
> + struct net * cl_nfssvc_net;
> nfs_to_nfsd_open_t nfsd_open_local_fh;
> };
>
> diff --git a/include/linux/nfslocalio.h b/include/linux/nfslocalio.h
> index b8df1b9f248d..c9592ad0afe2 100644
> --- a/include/linux/nfslocalio.h
> +++ b/include/linux/nfslocalio.h
> @@ -8,6 +8,7 @@
> #include <linux/list.h>
> #include <linux/uuid.h>
> #include <linux/nfs.h>
> +#include <net/net_namespace.h>
>
> /*
> * Global list of nfsd_uuid_t instances, add/remove
> @@ -23,13 +24,14 @@ extern struct list_head nfsd_uuids;
> typedef struct {
> uuid_t uuid;
> struct list_head list;
> + struct net *net; /* nfsd's network namespace */
> } nfsd_uuid_t;
>
> -bool nfsd_uuid_is_local(const uuid_t *uuid);
> +bool nfsd_uuid_is_local(const uuid_t *uuid, struct net **netp);
>
> -typedef int (*nfs_to_nfsd_open_t)(struct rpc_clnt *, const struct cred *,
> - const struct nfs_fh *, const fmode_t,
> - struct file **);
> +typedef int (*nfs_to_nfsd_open_t)(struct net *, struct rpc_clnt *,
> + const struct cred *, const struct nfs_fh *,
> + const fmode_t, struct file **);
>
> nfs_to_nfsd_open_t get_nfsd_open_local_fh(void);
> void put_nfsd_open_local_fh(void);
> --
> 2.44.0
>
>
>
