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

[Mike Snitzer <snitzer@kernel.org>]

On Wed, Jun 26, 2024 at 09:33:03AM +1000, NeilBrown wrote:
> 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.

Fixed, thanks.