Re: [PATCH 6/6] nfsd: add nfsd_file_acquire_local().

Jeff Layton
2024-07-01T11:21:21Z

On Mon, 2024-07-01 at 12:53 +1000, NeilBrown wrote:
> nfsd_file_acquire_local() can be used to look up a file by filehandle
> without having a struct rqst.  This can be used by NFS LOCALIO to allow
> the NFS client to by the NFS protocol to directly access a file provided
> by the NFS server which is running in the same kernel.
>
> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
>  fs/nfsd/filecache.c | 54 ++++++++++++++++++++++++++++++++++++++++-----
>  fs/nfsd/filecache.h |  4 ++++
>  fs/nfsd/nfsfh.c     |  2 +-
>  fs/nfsd/nfsfh.h     |  5 +++++
>  4 files changed, 59 insertions(+), 6 deletions(-)
>
> diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
> index ad9083ca144b..87f965d2574b 100644
> --- a/fs/nfsd/filecache.c
> +++ b/fs/nfsd/filecache.c
> @@ -977,7 +977,10 @@ nfsd_file_is_cached(struct inode *inode)
>  }
>
>  static __be32
> -nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp,
> +nfsd_file_do_acquire(struct svc_rqst *rqstp, struct nfsd_net *nn,
> +      struct svc_cred *cred, int nfs_vers,
> +      struct auth_domain *client,
> +      struct svc_fh *fhp,
>        unsigned int may_flags, struct file *file,
>        struct nfsd_file **pnf, bool want_gc)
>  {
> @@ -991,7 +994,7 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp,
>   int ret;
>
>  retry:
> - status = fh_verify(rqstp, fhp, S_IFREG,
> + status = __fh_verify(rqstp, nn, cred, nfs_vers, client, fhp, S_IFREG,
>   may_flags|NFSD_MAY_OWNER_OVERRIDE);
>   if (status != nfs_ok)
>   return status;
> @@ -1139,7 +1142,8 @@ __be32
>  nfsd_file_acquire_gc(struct svc_rqst *rqstp, struct svc_fh *fhp,
>        unsigned int may_flags, struct nfsd_file **pnf)
>  {
> - return nfsd_file_do_acquire(rqstp, fhp, may_flags, NULL, pnf, true);
> + return nfsd_file_do_acquire(rqstp, NULL, NULL, 0, NULL,
> +     fhp, may_flags, NULL, pnf, true);
>  }
>
>  /**
> @@ -1163,7 +1167,46 @@ __be32
>  nfsd_file_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp,
>     unsigned int may_flags, struct nfsd_file **pnf)
>  {
> - return nfsd_file_do_acquire(rqstp, fhp, may_flags, NULL, pnf, false);
> + return nfsd_file_do_acquire(rqstp, NULL, NULL, 0, NULL, fhp,
> +     may_flags, NULL, pnf, false);
> +}
> +
> +/**
> + * nfsd_file_acquire_local - Get a struct nfsd_file with an open file for localio
> + * @nn: The nfsd network namespace in which to perform a lookup
> + * @cred: the user credential with which to validate access
> + * @nfs_vers: NFS version number to assume for request
> + * @client: the auth_domain for LOCALIO lookup
> + * @fhp: the NFS filehandle of the file to be opened
> + * @may_flags: NFSD_MAY_ settings for the file
> + * @pnf: OUT: new or found "struct nfsd_file" object
> + *
> + * This file lookup interface provide access to a file given the
> + * filehandle and credential.  No connection-based authorisation
> + * is performed and in that way it is quite different to other
> + * file access mediated by nfsd.  It allows a kernel module such as the NFS
> + * client to reach across network and filesystem namespaces to access
> + * a file.  The security implications of this should be carefully
> + * considered before use.
> + *
> + * The nfsd_file_object returned by this API is reference-counted
> + * but not garbage-collected. The object is unhashed after the
> + * final nfsd_file_put().
> + *
> + * Return values:
> + *   %nfs_ok - @pnf points to an nfsd_file with its reference
> + *   count boosted.
> + *
> + * On error, an nfsstat value in network byte order is returned.
> + */
> +__be32
> +nfsd_file_acquire_local(struct nfsd_net *nn, struct svc_cred *cred,
> + int nfs_vers, struct auth_domain *client,
> + struct svc_fh *fhp,
> + unsigned int may_flags, struct nfsd_file **pnf)
> +{
> + return nfsd_file_do_acquire(NULL, nn, cred, nfs_vers, client,
> +     fhp, may_flags, NULL, pnf, false);

It still seems to me like these should be garbage-collected.

Neil, in an earlier email you mentioned that the client could hold onto
the nfsd_file reference over several operations and then put it. That
would be fine too, but I'm unclear on how the client will manage this.
Does the client have a way to keep track of the nfsd_file over several
operations to the same inode?

Even then, I still think we're probably better off just garbage
collecting thse, since it seems likely that they will end up being
reused in many cases.

>  }
>
>  /**
> @@ -1189,7 +1232,8 @@ nfsd_file_acquire_opened(struct svc_rqst *rqstp, struct svc_fh *fhp,
>   unsigned int may_flags, struct file *file,
>   struct nfsd_file **pnf)
>  {
> - return nfsd_file_do_acquire(rqstp, fhp, may_flags, file, pnf, false);
> + return nfsd_file_do_acquire(rqstp, NULL, NULL, 0, NULL,
> +     fhp, may_flags, file, pnf, false);
>  }
>
>  /*
> diff --git a/fs/nfsd/filecache.h b/fs/nfsd/filecache.h
> index c61884def906..d179dbae98e3 100644
> --- a/fs/nfsd/filecache.h
> +++ b/fs/nfsd/filecache.h
> @@ -65,5 +65,9 @@ __be32 nfsd_file_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  __be32 nfsd_file_acquire_opened(struct svc_rqst *rqstp, struct svc_fh *fhp,
>     unsigned int may_flags, struct file *file,
>     struct nfsd_file **nfp);
> +__be32 nfsd_file_acquire_local(struct nfsd_net *nn, struct svc_cred *cred,
> +        int nfs_vers, struct auth_domain *client,
> +        struct svc_fh *fhp,
> +        unsigned int may_flags, struct nfsd_file **pnf);
>  int nfsd_file_cache_stats_show(struct seq_file *m, void *v);
>  #endif /* _FS_NFSD_FILECACHE_H */
> diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
> index fb5a23060a4c..fa7e358d91ab 100644
> --- a/fs/nfsd/nfsfh.c
> +++ b/fs/nfsd/nfsfh.c
> @@ -328,7 +328,7 @@ static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct nfsd_net *nn,
>   * @access is formed from the NFSD_MAY_* constants defined in
>   * fs/nfsd/vfs.h.
>   */
> -static __be32
> +__be32
>  __fh_verify(struct svc_rqst *rqstp,
>       struct nfsd_net *nn, struct svc_cred *cred,
>       int nfs_vers, struct auth_domain *client,
> diff --git a/fs/nfsd/nfsfh.h b/fs/nfsd/nfsfh.h
> index 6ebdf7ea27bf..a2d9962f1bf8 100644
> --- a/fs/nfsd/nfsfh.h
> +++ b/fs/nfsd/nfsfh.h
> @@ -214,7 +214,12 @@ extern char * SVCFH_fmt(struct svc_fh *fhp);
>  /*
>   * Function prototypes
>   */
> +struct nfsd_net;
>  __be32 fh_verify(struct svc_rqst *, struct svc_fh *, umode_t, int);
> +__be32 __fh_verify(struct svc_rqst *rqstp,
> +     struct nfsd_net *nn, struct svc_cred *cred,
> +     int nfs_vers, struct auth_domain *client,
> +     struct svc_fh *fhp, umode_t type, int access);
>  __be32 fh_compose(struct svc_fh *, struct svc_export *, struct dentry *, struct svc_fh *);
>  __be32 fh_update(struct svc_fh *);
>  void fh_put(struct svc_fh *);

--
Jeff Layton <jlayton@kernel.org>