On Sun, 30 Jun 2024, Chuck Lever wrote:
> Sorry, I guess I expected to have more time to learn about these
> patches before writing review comments. But if you want them to go
> in soon, I had better look more closely at them now.
>
>
> On Fri, Jun 28, 2024 at 05:10:59PM -0400, Mike Snitzer wrote:
> > Pass the stored cl_nfssvc_net from the client to the server as
>
> This is the only mention of cl_nfssvc_net I can find in this
> patch. I'm not sure what it is. Patch description should maybe
> provide some context.
>
>
> > first argument to nfsd_open_local_fh() to ensure the proper network
> > namespace is used for localio.
>
> Can the patch description say something about the distinct mount
> namespaces -- if the local application is running in a different
> container than the NFS server, are we using only the network
> namespaces for authorizing the file access? And is that OK to do?
> If yes, patch description should explain that NFS local I/O ignores
> the boundaries of mount namespaces and why that is OK to do.
>
>
> > Signed-off-by: Weston Andros Adamson <dros@xxxxxxxxxxxxxxx>
> > Signed-off-by: Peng Tao <tao.peng@xxxxxxxxxxxxxxx>
> > Signed-off-by: Lance Shelton <lance.shelton@xxxxxxxxxxxxxxx>
> > Signed-off-by: Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx>
> > Signed-off-by: Mike Snitzer <snitzer@xxxxxxxxxx>
> > ---
> > fs/nfsd/Makefile | 1 +
> > fs/nfsd/filecache.c | 2 +-
> > fs/nfsd/localio.c | 239 ++++++++++++++++++++++++++++++++++++++++++++
> > fs/nfsd/nfssvc.c | 1 +
> > fs/nfsd/trace.h | 3 +-
> > fs/nfsd/vfs.h | 9 ++
> > 6 files changed, 253 insertions(+), 2 deletions(-)
> > create mode 100644 fs/nfsd/localio.c
> >
> > diff --git a/fs/nfsd/Makefile b/fs/nfsd/Makefile
> > index b8736a82e57c..78b421778a79 100644
> > --- a/fs/nfsd/Makefile
> > +++ b/fs/nfsd/Makefile
> > @@ -23,3 +23,4 @@ nfsd-$(CONFIG_NFSD_PNFS) += nfs4layouts.o
> > nfsd-$(CONFIG_NFSD_BLOCKLAYOUT) += blocklayout.o blocklayoutxdr.o
> > nfsd-$(CONFIG_NFSD_SCSILAYOUT) += blocklayout.o blocklayoutxdr.o
> > nfsd-$(CONFIG_NFSD_FLEXFILELAYOUT) += flexfilelayout.o flexfilelayoutxdr.o
> > +nfsd-$(CONFIG_NFSD_LOCALIO) += localio.o
> > diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
> > index ad9083ca144b..99631fa56662 100644
> > --- a/fs/nfsd/filecache.c
> > +++ b/fs/nfsd/filecache.c
> > @@ -52,7 +52,7 @@
> > #define NFSD_FILE_CACHE_UP (0)
> >
> > /* We only care about NFSD_MAY_READ/WRITE for this cache */
> > -#define NFSD_FILE_MAY_MASK (NFSD_MAY_READ|NFSD_MAY_WRITE)
> > +#define NFSD_FILE_MAY_MASK (NFSD_MAY_READ|NFSD_MAY_WRITE|NFSD_MAY_LOCALIO)
> >
> > static DEFINE_PER_CPU(unsigned long, nfsd_file_cache_hits);
> > static DEFINE_PER_CPU(unsigned long, nfsd_file_acquisitions);
> > diff --git a/fs/nfsd/localio.c b/fs/nfsd/localio.c
> > new file mode 100644
> > index 000000000000..759a5cb79652
> > --- /dev/null
> > +++ b/fs/nfsd/localio.c
> > @@ -0,0 +1,239 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * NFS server support for local clients to bypass network stack
> > + *
> > + * Copyright (C) 2014 Weston Andros Adamson <dros@xxxxxxxxxxxxxxx>
> > + * Copyright (C) 2019 Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx>
> > + * Copyright (C) 2024 Mike Snitzer <snitzer@xxxxxxxxxxxxxxx>
> > + */
> > +
> > +#include <linux/exportfs.h>
> > +#include <linux/sunrpc/svcauth_gss.h>
> > +#include <linux/sunrpc/clnt.h>
> > +#include <linux/nfs.h>
> > +#include <linux/string.h>
> > +
> > +#include "nfsd.h"
> > +#include "vfs.h"
> > +#include "netns.h"
> > +#include "filecache.h"
> > +
> > +#define NFSDDBG_FACILITY NFSDDBG_FH
>
> With no more dprintk() call sites in this patch, you no longer need
> this macro definition.
>
>
> > +/*
> > + * We need to translate between nfs status return values and
> > + * the local errno values which may not be the same.
> > + * - duplicated from fs/nfs/nfs2xdr.c to avoid needless bloat of
> > + * all compiled nfs objects if it were in include/linux/nfs.h
> > + */
> > +static const struct {
> > + int stat;
> > + int errno;
> > +} nfs_common_errtbl[] = {
> > + { NFS_OK, 0 },
> > + { NFSERR_PERM, -EPERM },
> > + { NFSERR_NOENT, -ENOENT },
> > + { NFSERR_IO, -EIO },
> > + { NFSERR_NXIO, -ENXIO },
> > +/* { NFSERR_EAGAIN, -EAGAIN }, */
> > + { NFSERR_ACCES, -EACCES },
> > + { NFSERR_EXIST, -EEXIST },
> > + { NFSERR_XDEV, -EXDEV },
> > + { NFSERR_NODEV, -ENODEV },
> > + { NFSERR_NOTDIR, -ENOTDIR },
> > + { NFSERR_ISDIR, -EISDIR },
> > + { NFSERR_INVAL, -EINVAL },
> > + { NFSERR_FBIG, -EFBIG },
> > + { NFSERR_NOSPC, -ENOSPC },
> > + { NFSERR_ROFS, -EROFS },
> > + { NFSERR_MLINK, -EMLINK },
> > + { NFSERR_NAMETOOLONG, -ENAMETOOLONG },
> > + { NFSERR_NOTEMPTY, -ENOTEMPTY },
> > + { NFSERR_DQUOT, -EDQUOT },
> > + { NFSERR_STALE, -ESTALE },
> > + { NFSERR_REMOTE, -EREMOTE },
> > +#ifdef EWFLUSH
> > + { NFSERR_WFLUSH, -EWFLUSH },
> > +#endif
> > + { NFSERR_BADHANDLE, -EBADHANDLE },
> > + { NFSERR_NOT_SYNC, -ENOTSYNC },
> > + { NFSERR_BAD_COOKIE, -EBADCOOKIE },
> > + { NFSERR_NOTSUPP, -ENOTSUPP },
> > + { NFSERR_TOOSMALL, -ETOOSMALL },
> > + { NFSERR_SERVERFAULT, -EREMOTEIO },
> > + { NFSERR_BADTYPE, -EBADTYPE },
> > + { NFSERR_JUKEBOX, -EJUKEBOX },
> > + { -1, -EIO }
> > +};
> > +
> > +/**
> > + * nfs_stat_to_errno - convert an NFS status code to a local errno
> > + * @status: NFS status code to convert
> > + *
> > + * Returns a local errno value, or -EIO if the NFS status code is
> > + * not recognized. nfsd_file_acquire() returns an nfsstat that
> > + * needs to be translated to an errno before being returned to a
> > + * local client application.
> > + */
> > +static int nfs_stat_to_errno(enum nfs_stat status)
> > +{
> > + int i;
> > +
> > + for (i = 0; nfs_common_errtbl[i].stat != -1; i++) {
> > + if (nfs_common_errtbl[i].stat == (int)status)
> > + return nfs_common_errtbl[i].errno;
> > + }
> > + return nfs_common_errtbl[i].errno;
> > +}
> > +
> > +static void
> > +nfsd_local_fakerqst_destroy(struct svc_rqst *rqstp)
> > +{
> > + if (rqstp->rq_client)
> > + auth_domain_put(rqstp->rq_client);
> > + if (rqstp->rq_cred.cr_group_info)
> > + put_group_info(rqstp->rq_cred.cr_group_info);
> > + /* rpcauth_map_to_svc_cred_local() clears cr_principal */
> > + WARN_ON_ONCE(rqstp->rq_cred.cr_principal != NULL);
> > + kfree(rqstp->rq_xprt);
> > + kfree(rqstp);
> > +}
> > +
> > +static struct svc_rqst *
> > +nfsd_local_fakerqst_create(struct net *net, struct rpc_clnt *rpc_clnt,
> > + const struct cred *cred)
> > +{
> > + struct svc_rqst *rqstp;
> > + struct nfsd_net *nn = net_generic(net, nfsd_net_id);
> > + int status;
> > +
> > + /* FIXME: not running in nfsd context, must get reference on nfsd_serv */
> > + if (unlikely(!READ_ONCE(nn->nfsd_serv)))
> > + return ERR_PTR(-ENXIO);
> > +
> > + rqstp = kzalloc(sizeof(*rqstp), GFP_KERNEL);
> > + if (!rqstp)
> > + return ERR_PTR(-ENOMEM);
> > +
> > + rqstp->rq_xprt = kzalloc(sizeof(*rqstp->rq_xprt), GFP_KERNEL);
> > + if (!rqstp->rq_xprt) {
> > + status = -ENOMEM;
> > + goto out_err;
> > + }
>
> struct svc_rqst is pretty big (like, bigger than a couple of pages).
> What happens if this allocation fails?
>
> And how often does it occur -- does that add significant overhead?
>
>
> > +
> > + rqstp->rq_xprt->xpt_net = net;
> > + __set_bit(RQ_SECURE, &rqstp->rq_flags);
> > + rqstp->rq_proc = 1;
> > + rqstp->rq_vers = 3;
>
> IMO these need to be symbolic constants, not integers. Or, at least
> there needs to be some documenting comments that explain these are
> fake and why that's OK to do. Or, are there better choices?
>
>
> > + rqstp->rq_prot = IPPROTO_TCP;
> > + rqstp->rq_server = nn->nfsd_serv;
> > +
> > + /* Note: we're connecting to ourself, so source addr == peer addr */
> > + rqstp->rq_addrlen = rpc_peeraddr(rpc_clnt,
> > + (struct sockaddr *)&rqstp->rq_addr,
> > + sizeof(rqstp->rq_addr));
> > +
> > + rpcauth_map_to_svc_cred_local(rpc_clnt->cl_auth, cred, &rqstp->rq_cred);
> > +
> > + /*
> > + * set up enough for svcauth_unix_set_client to be able to wait
> > + * for the cache downcall. Note that we do _not_ want to allow the
> > + * request to be deferred for later revisit since this rqst and xprt
> > + * are not set up to run inside of the normal svc_rqst engine.
> > + */
> > + INIT_LIST_HEAD(&rqstp->rq_xprt->xpt_deferred);
> > + kref_init(&rqstp->rq_xprt->xpt_ref);
> > + spin_lock_init(&rqstp->rq_xprt->xpt_lock);
> > + rqstp->rq_chandle.thread_wait = 5 * HZ;
> > +
> > + status = svcauth_unix_set_client(rqstp);
> > + switch (status) {
> > + case SVC_OK:
> > + break;
> > + case SVC_DENIED:
> > + status = -ENXIO;
> > + goto out_err;
> > + default:
> > + status = -ETIMEDOUT;
> > + goto out_err;
> > + }
>
> Interesting. Why would svcauth_unix_set_client fail for a local I/O
> request? Wouldn't it only be because the local application is trying
> to open a file it doesn't have permission to?
>
I'm beginning to think this section of code is the of the sort where you
need to be twice as clever when debugging as you where when writing. It
is trying to get the client to use interfaces written for server-side
actions, and it isn't a good fit.
I think that instead we should modify fh_verify() so that it takes
explicit net, rq_vers, rq_cred, rq_client as well as the rqstp, and
the localio client passes in a NULL rqstp.
Getting the rq_client is an interesting challenge.
The above code (if I'm reading it correctly) gets the server-side
address of the IP connection, and passes that through to the sunrpc code
as though it is the client address. So as long as the server is
exporting to itself, and as long as no address translation is happening
on the path, this works. It feels messy though - and fragile.
I would rather we had some rq_client (struct auth_domain) that was
dedicated to localio. The client should be able to access it based on
the fact that it could rather the server UUID using the LOCALIO RPC
protocol.
I'm not sure what exactly this would look like, but the
'struct auth_domain *' should be something that can be accessed
directly, not looked up in a cache.
I can try to knock up a patch to allow fh_verify (and nfsd_file_acquire)
without an rqstp. I won't try the auth_domain change until I hear what
others think.
NeilBrown
