Re: [PATCH v7 05/20] nfs_common: add NFS LOCALIO auxiliary protocol enablement

NeilBrown
2024-06-25T23:33:11Z

On Tue, 25 Jun 2024, Mike Snitzer wrote:
> First use is in nfsd, to add access to a global nfsd_uuids list that
> will be used to identify local nfsd instances.
>
> nfsd_uuids is protected by nfsd_mutex or RCU read lock. List is
> composed of nfsd_uuid_t instances that are managed as nfsd creates
> them (per network namespace).
>
> nfsd_uuid_is_local() will be used to search all local nfsd for the
> client specified nfsd uuid.
>
> This commit also adds all the nfs_client members required to implement
> the entire localio feature (which depends on the LOCALIO protocol).
>
> Signed-off-by: Mike Snitzer <snitzer@kernel.org>
> ---
> fs/nfs/client.c | 8 +++++
> fs/nfs_common/Makefile | 3 ++
> fs/nfs_common/nfslocalio.c | 72 ++++++++++++++++++++++++++++++++++++++
> fs/nfsd/netns.h | 4 +++
> fs/nfsd/nfssvc.c | 12 ++++++-
> include/linux/nfs_fs_sb.h | 9 +++++
> include/linux/nfslocalio.h | 39 +++++++++++++++++++++
> 7 files changed, 146 insertions(+), 1 deletion(-)
> create mode 100644 fs/nfs_common/nfslocalio.c
> create mode 100644 include/linux/nfslocalio.h
>
> diff --git a/fs/nfs/client.c b/fs/nfs/client.c
> index de77848ae654..bcdf8d42cbc7 100644
> --- a/fs/nfs/client.c
> +++ b/fs/nfs/client.c
> @@ -178,6 +178,14 @@ struct nfs_client *nfs_alloc_client(const struct nfs_client_initdata *cl_init)
> clp->cl_max_connect = cl_init->max_connect ? cl_init->max_connect : 1;
> clp->cl_net = get_net(cl_init->net);
>
> +#if IS_ENABLED(CONFIG_NFS_LOCALIO)
> + seqlock_init(&clp->cl_boot_lock);
> + ktime_get_real_ts64(&clp->cl_nfssvc_boot);
> + clp->cl_rpcclient_localio = ERR_PTR(-EINVAL);
> + clp->nfsd_open_local_fh = NULL;
> + clp->cl_nfssvc_net = NULL;
> +#endif /* CONFIG_NFS_LOCALIO */
> +
> clp->cl_principal = "*";
> clp->cl_xprtsec = cl_init->xprtsec;
> return clp;
> diff --git a/fs/nfs_common/Makefile b/fs/nfs_common/Makefile
> index 119c75ab9fd0..d81623b76aba 100644
> --- a/fs/nfs_common/Makefile
> +++ b/fs/nfs_common/Makefile
> @@ -6,5 +6,8 @@
> obj-$(CONFIG_NFS_ACL_SUPPORT) += nfs_acl.o
> nfs_acl-objs := nfsacl.o
>
> +obj-$(CONFIG_NFS_COMMON_LOCALIO_SUPPORT) += nfs_localio.o
> +nfs_localio-objs := nfslocalio.o
> +
> obj-$(CONFIG_GRACE_PERIOD) += grace.o
> obj-$(CONFIG_NFS_V4_2_SSC_HELPER) += nfs_ssc.o
> diff --git a/fs/nfs_common/nfslocalio.c b/fs/nfs_common/nfslocalio.c
> new file mode 100644
> index 000000000000..755b84b742a6
> --- /dev/null
> +++ b/fs/nfs_common/nfslocalio.c
> @@ -0,0 +1,72 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2024 Mike Snitzer <snitzer@hammerspace.com>
> + */
> +
> +#include <linux/module.h>
> +#include <linux/rculist.h>
> +#include <linux/nfslocalio.h>
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("NFS localio protocol bypass support");
> +
> +/*
> + * Global list of nfsd_uuid_t instances, add/remove
> + * is protected by fs/nfsd/nfssvc.c:nfsd_mutex.
> + * 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,
> + 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)) {
> + *netp = nfsd_uuid->net;
> + return &nfsd_uuid->uuid;
> + }
> +
> + return &uuid_null;
> +}
> +
> +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, netp);
> + rcu_read_unlock();
> +
> + return !uuid_is_null(nfsd_uuid);

This is still unsafe. You can only safely dereference nfsd_uuid while
still holding the rcu_read_lock.

NeilBrown

> +}
> +EXPORT_SYMBOL_GPL(nfsd_uuid_is_local);
> +
> +/*
> + * The nfs localio code needs to call into nfsd to do the filehandle -> struct path
> + * mapping, but cannot be statically linked, because that will make the nfs module
> + * depend on the nfsd module.
> + *
> + * Instead, do dynamic linking to the nfsd module (via nfs_common module). The
> + * nfs_common module will only hold a reference on nfsd when localio is in use.
> + * This allows some sanity checking, like giving up on localio if nfsd isn't loaded.
> + */
> +
> +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);
> +
> +nfs_to_nfsd_open_t get_nfsd_open_local_fh(void)
> +{
> + return symbol_request(nfsd_open_local_fh);
> +}
> +EXPORT_SYMBOL_GPL(get_nfsd_open_local_fh);
> +
> +void put_nfsd_open_local_fh(void)
> +{
> + symbol_put(nfsd_open_local_fh);
> +}
> +EXPORT_SYMBOL_GPL(put_nfsd_open_local_fh);
> diff --git a/fs/nfsd/netns.h b/fs/nfsd/netns.h
> index 14ec15656320..0c5a1d97e4ac 100644
> --- a/fs/nfsd/netns.h
> +++ b/fs/nfsd/netns.h
> @@ -15,6 +15,7 @@
> #include <linux/percpu_counter.h>
> #include <linux/siphash.h>
> #include <linux/sunrpc/stats.h>
> +#include <linux/nfslocalio.h>
>
> /* Hash tables for nfs4_clientid state */
> #define CLIENT_HASH_BITS 4
> @@ -213,6 +214,9 @@ struct nfsd_net {
> /* last time an admin-revoke happened for NFSv4.0 */
> time64_t nfs40_last_revoke;
>
> +#if IS_ENABLED(CONFIG_NFSD_LOCALIO)
> + nfsd_uuid_t nfsd_uuid;
> +#endif
> };
>
> /* Simple check to find out if a given net was properly initialized */
> diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
> index 9edb4f7c4cc2..1222a0a33fe1 100644
> --- a/fs/nfsd/nfssvc.c
> +++ b/fs/nfsd/nfssvc.c
> @@ -19,6 +19,7 @@
> #include <linux/sunrpc/svc_xprt.h>
> #include <linux/lockd/bind.h>
> #include <linux/nfsacl.h>
> +#include <linux/nfslocalio.h>
> #include <linux/seq_file.h>
> #include <linux/inetdevice.h>
> #include <net/addrconf.h>
> @@ -427,6 +428,10 @@ static int nfsd_startup_net(struct net *net, const struct cred *cred)
>
> #ifdef CONFIG_NFSD_V4_2_INTER_SSC
> nfsd4_ssc_init_umount_work(nn);
> +#endif
> +#if IS_ENABLED(CONFIG_NFSD_LOCALIO)
> + INIT_LIST_HEAD(&nn->nfsd_uuid.list);
> + list_add_tail_rcu(&nn->nfsd_uuid.list, &nfsd_uuids);
> #endif
> nn->nfsd_net_up = true;
> return 0;
> @@ -456,6 +461,9 @@ static void nfsd_shutdown_net(struct net *net)
> lockd_down(net);
> nn->lockd_up = false;
> }
> +#if IS_ENABLED(CONFIG_NFSD_LOCALIO)
> + list_del_rcu(&nn->nfsd_uuid.list);
> +#endif
> nn->nfsd_net_up = false;
> nfsd_shutdown_generic();
> }
> @@ -802,7 +810,9 @@ nfsd_svc(int n, int *nthreads, struct net *net, const struct cred *cred, const c
>
> strscpy(nn->nfsd_name, scope ? scope : utsname()->nodename,
> sizeof(nn->nfsd_name));
> -
> +#if IS_ENABLED(CONFIG_NFSD_LOCALIO)
> + uuid_gen(&nn->nfsd_uuid.uuid);
> +#endif
> error = nfsd_create_serv(net);
> if (error)
> goto out;
> diff --git a/include/linux/nfs_fs_sb.h b/include/linux/nfs_fs_sb.h
> index 92de074e63b9..e58e706a6503 100644
> --- a/include/linux/nfs_fs_sb.h
> +++ b/include/linux/nfs_fs_sb.h
> @@ -8,6 +8,7 @@
> #include <linux/wait.h>
> #include <linux/nfs_xdr.h>
> #include <linux/sunrpc/xprt.h>
> +#include <linux/nfslocalio.h>
>
> #include <linux/atomic.h>
> #include <linux/refcount.h>
> @@ -125,6 +126,14 @@ struct nfs_client {
> struct net *cl_net;
> struct list_head pending_cb_stateids;
> struct rcu_head rcu;
> +
> +#if IS_ENABLED(CONFIG_NFS_LOCALIO)
> + struct timespec64 cl_nfssvc_boot;
> + seqlock_t cl_boot_lock;
> + struct rpc_clnt * cl_rpcclient_localio;
> + struct net * cl_nfssvc_net;
> + nfs_to_nfsd_open_t nfsd_open_local_fh;
> +#endif /* CONFIG_NFS_LOCALIO */
> };
>
> /*
> diff --git a/include/linux/nfslocalio.h b/include/linux/nfslocalio.h
> new file mode 100644
> index 000000000000..c9592ad0afe2
> --- /dev/null
> +++ b/include/linux/nfslocalio.h
> @@ -0,0 +1,39 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) 2024 Mike Snitzer <snitzer@hammerspace.com>
> + */
> +#ifndef __LINUX_NFSLOCALIO_H
> +#define __LINUX_NFSLOCALIO_H
> +
> +#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
> + * is protected by fs/nfsd/nfssvc.c:nfsd_mutex.
> + */
> +extern struct list_head nfsd_uuids;
> +
> +/*
> + * Each nfsd instance has an nfsd_uuid_t that is accessible through the
> + * global nfsd_uuids list. Useful to allow a client to negotiate if localio
> + * possible with its server.
> + */
> +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, struct net **netp);
> +
> +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);
> +
> +#endif /* __LINUX_NFSLOCALIO_H */
> --
> 2.44.0
>
>
>