Re: [PATCH 2/6] nfsd: add cred parameter to __fh_verify()

Jeff Layton
2024-07-01T11:02:37Z

On Mon, 2024-07-01 at 12:53 +1000, NeilBrown wrote:
> __fh_verify() now takes a 'cred' parameter and never dereferences
> rqstp->rq_cred.
>
> nfsd_permission(), nfsd_setuser() and nfsexp_flags() only never needed

"only ever"

> the cred out of rqstp, so we now pass in the cred explicitly and not the
> rqstp.
>
> nfsd_originating_port_ok() is NOT passed a cred despite that it uses
> one.  This function is not useful when rqstp is NULL and a future patch
> will address that.
>
> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
>  fs/nfsd/auth.c      | 14 +++++++-------
>  fs/nfsd/auth.h      |  2 +-
>  fs/nfsd/export.h    |  3 ++-
>  fs/nfsd/nfs4state.c |  3 ++-
>  fs/nfsd/nfsfh.c     | 18 +++++++++++-------
>  fs/nfsd/nfsproc.c   |  9 +++++----
>  fs/nfsd/vfs.c       | 21 ++++++++++++---------
>  fs/nfsd/vfs.h       |  2 +-
>  8 files changed, 41 insertions(+), 31 deletions(-)
>
> diff --git a/fs/nfsd/auth.c b/fs/nfsd/auth.c
> index e6beaaf4f170..93e33d1ee891 100644
> --- a/fs/nfsd/auth.c
> +++ b/fs/nfsd/auth.c
> @@ -5,26 +5,26 @@
>  #include "nfsd.h"
>  #include "auth.h"
>
> -int nfsexp_flags(struct svc_rqst *rqstp, struct svc_export *exp)
> +int nfsexp_flags(struct svc_cred *cred, struct svc_export *exp)
>  {
>   struct exp_flavor_info *f;
>   struct exp_flavor_info *end = exp->ex_flavors + exp->ex_nflavors;
>
>   for (f = exp->ex_flavors; f < end; f++) {
> - if (f->pseudoflavor == rqstp->rq_cred.cr_flavor)
> + if (f->pseudoflavor == cred->cr_flavor)
>   return f->flags;
>   }
>   return exp->ex_flags;
>
>  }
>
> -int nfsd_setuser(struct svc_rqst *rqstp, struct svc_export *exp)
> +int nfsd_setuser(struct svc_cred *cred, struct svc_export *exp)
>  {
>   struct group_info *rqgi;
>   struct group_info *gi;
>   struct cred *new;
>   int i;
> - int flags = nfsexp_flags(rqstp, exp);
> + int flags = nfsexp_flags(cred, exp);
>
>   /* discard any old override before preparing the new set */
>   revert_creds(get_cred(current_real_cred()));
> @@ -32,10 +32,10 @@ int nfsd_setuser(struct svc_rqst *rqstp, struct svc_export *exp)
>   if (!new)
>   return -ENOMEM;
>
> - new->fsuid = rqstp->rq_cred.cr_uid;
> - new->fsgid = rqstp->rq_cred.cr_gid;
> + new->fsuid = cred->cr_uid;
> + new->fsgid = cred->cr_gid;
>
> - rqgi = rqstp->rq_cred.cr_group_info;
> + rqgi = cred->cr_group_info;
>
>   if (flags & NFSEXP_ALLSQUASH) {
>   new->fsuid = exp->ex_anon_uid;
> diff --git a/fs/nfsd/auth.h b/fs/nfsd/auth.h
> index dbd66424f600..fc75c5d90be4 100644
> --- a/fs/nfsd/auth.h
> +++ b/fs/nfsd/auth.h
> @@ -12,6 +12,6 @@
>   * Set the current process's fsuid/fsgid etc to those of the NFS
>   * client user
>   */
> -int nfsd_setuser(struct svc_rqst *, struct svc_export *);
> +int nfsd_setuser(struct svc_cred *, struct svc_export *);
>
>  #endif /* LINUX_NFSD_AUTH_H */
> diff --git a/fs/nfsd/export.h b/fs/nfsd/export.h
> index 1a54d388d58d..2dbd15704a86 100644
> --- a/fs/nfsd/export.h
> +++ b/fs/nfsd/export.h
> @@ -99,7 +99,8 @@ struct svc_expkey {
>  #define EX_NOHIDE(exp) ((exp)->ex_flags & NFSEXP_NOHIDE)
>  #define EX_WGATHER(exp) ((exp)->ex_flags & NFSEXP_GATHERED_WRITES)
>
> -int nfsexp_flags(struct svc_rqst *rqstp, struct svc_export *exp);
> +struct svc_cred;
> +int nfsexp_flags(struct svc_cred *cred, struct svc_export *exp);
>  __be32 check_nfsd_access(struct svc_export *exp, struct svc_rqst *rqstp);
>
>  /*
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index a20c2c9d7d45..eadb7d1a7f13 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -6889,7 +6889,8 @@ nfs4_check_file(struct svc_rqst *rqstp, struct svc_fh *fhp, struct nfs4_stid *s,
>
>   nf = nfs4_find_file(s, flags);
>   if (nf) {
> - status = nfsd_permission(rqstp, fhp->fh_export, fhp->fh_dentry,
> + status = nfsd_permission(&rqstp->rq_cred,
> + fhp->fh_export, fhp->fh_dentry,
>   acc | NFSD_MAY_OWNER_OVERRIDE);
>   if (status) {
>   nfsd_file_put(nf);
> diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
> index e27ed27054ab..760684fa4b50 100644
> --- a/fs/nfsd/nfsfh.c
> +++ b/fs/nfsd/nfsfh.c
> @@ -100,9 +100,10 @@ static bool nfsd_originating_port_ok(struct svc_rqst *rqstp, int flags)
>  }
>
>  static __be32 nfsd_setuser_and_check_port(struct svc_rqst *rqstp,
> +   struct svc_cred *cred,
>     struct svc_export *exp)
>  {
> - int flags = nfsexp_flags(rqstp, exp);
> + int flags = nfsexp_flags(cred, exp);
>
>   /* Check if the request originated from a secure port. */
>   if (!nfsd_originating_port_ok(rqstp, flags)) {
> @@ -113,7 +114,7 @@ static __be32 nfsd_setuser_and_check_port(struct svc_rqst *rqstp,
>   }
>
>   /* Set user creds for this exportpoint */
> - return nfserrno(nfsd_setuser(rqstp, exp));
> + return nfserrno(nfsd_setuser(cred, exp));
>  }
>
>  static inline __be32 check_pseudo_root(struct svc_rqst *rqstp,
> @@ -152,6 +153,7 @@ static inline __be32 check_pseudo_root(struct svc_rqst *rqstp,
>   * fh_dentry.
>   */
>  static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct nfsd_net *nn,
> + struct svc_cred *cred,
>   struct svc_fh *fhp)
>  {
>   struct knfsd_fh *fh = &fhp->fh_handle;
> @@ -230,7 +232,7 @@ static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct nfsd_net *nn,
>   put_cred(override_creds(new));
>   put_cred(new);
>   } else {
> - error = nfsd_setuser_and_check_port(rqstp, exp);
> + error = nfsd_setuser_and_check_port(rqstp, cred, exp);
>   if (error)
>   goto out;
>   }
> @@ -326,7 +328,8 @@ static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct nfsd_net *nn,
>   * fs/nfsd/vfs.h.
>   */
>  static __be32
> -__fh_verify(struct svc_rqst *rqstp, struct nfsd_net *nn,
> +__fh_verify(struct svc_rqst *rqstp,
> +     struct nfsd_net *nn, struct svc_cred *cred,
>       struct svc_fh *fhp, umode_t type, int access)
>  {
>   struct svc_export *exp = NULL;
> @@ -334,7 +337,7 @@ __fh_verify(struct svc_rqst *rqstp, struct nfsd_net *nn,
>   __be32 error;
>
>   if (!fhp->fh_dentry) {
> - error = nfsd_set_fh_dentry(rqstp, nn, fhp);
> + error = nfsd_set_fh_dentry(rqstp, nn, cred, fhp);
>   if (error)
>   goto out;
>   }
> @@ -363,7 +366,7 @@ __fh_verify(struct svc_rqst *rqstp, struct nfsd_net *nn,
>   if (error)
>   goto out;
>
> - error = nfsd_setuser_and_check_port(rqstp, exp);
> + error = nfsd_setuser_and_check_port(rqstp, cred, exp);
>   if (error)
>   goto out;
>
> @@ -393,7 +396,7 @@ __fh_verify(struct svc_rqst *rqstp, struct nfsd_net *nn,
>
>  skip_pseudoflavor_check:
>   /* Finally, check access permissions. */
> - error = nfsd_permission(rqstp, exp, dentry, access);
> + error = nfsd_permission(cred, exp, dentry, access);
>  out:
>   trace_nfsd_fh_verify_err(rqstp, fhp, type, access, error);
>   if (error == nfserr_stale)
> @@ -405,6 +408,7 @@ __be32
>  fh_verify(struct svc_rqst *rqstp, struct svc_fh *fhp, umode_t type, int access)
>  {
>   return __fh_verify(rqstp, net_generic(SVC_NET(rqstp), nfsd_net_id),
> +    &rqstp->rq_cred,
>      fhp, type, access);
>  }
>
> diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c
> index 36370b957b63..97aab34593ef 100644
> --- a/fs/nfsd/nfsproc.c
> +++ b/fs/nfsd/nfsproc.c
> @@ -331,10 +331,11 @@ nfsd_proc_create(struct svc_rqst *rqstp)
>   *   echo thing > device-special-file-or-pipe
>   * by doing a CREATE with type==0
>   */
> - resp->status = nfsd_permission(rqstp,
> - newfhp->fh_export,
> - newfhp->fh_dentry,
> - NFSD_MAY_WRITE|NFSD_MAY_LOCAL_ACCESS);
> + resp->status = nfsd_permission(
> + &rqstp->rq_cred,
> + newfhp->fh_export,
> + newfhp->fh_dentry,
> + NFSD_MAY_WRITE|NFSD_MAY_LOCAL_ACCESS);
>   if (resp->status && resp->status != nfserr_rofs)
>   goto out_unlock;
>   }
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index 29b1f3613800..0862f6ae86a9 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -421,8 +421,9 @@ nfsd_get_write_access(struct svc_rqst *rqstp, struct svc_fh *fhp,
>   if (iap->ia_size < inode->i_size) {
>   __be32 err;
>
> - err = nfsd_permission(rqstp, fhp->fh_export, fhp->fh_dentry,
> - NFSD_MAY_TRUNC | NFSD_MAY_OWNER_OVERRIDE);
> + err = nfsd_permission(&rqstp->rq_cred,
> +       fhp->fh_export, fhp->fh_dentry,
> +       NFSD_MAY_TRUNC | NFSD_MAY_OWNER_OVERRIDE);
>   if (err)
>   return err;
>   }
> @@ -814,7 +815,8 @@ nfsd_access(struct svc_rqst *rqstp, struct svc_fh *fhp, u32 *access, u32 *suppor
>
>   sresult |= map->access;
>
> - err2 = nfsd_permission(rqstp, export, dentry, map->how);
> + err2 = nfsd_permission(&rqstp->rq_cred, export,
> +        dentry, map->how);
>   switch (err2) {
>   case nfs_ok:
>   result |= map->access;
> @@ -1475,7 +1477,8 @@ nfsd_create_locked(struct svc_rqst *rqstp, struct svc_fh *fhp,
>   dirp = d_inode(dentry);
>
>   dchild = dget(resfhp->fh_dentry);
> - err = nfsd_permission(rqstp, fhp->fh_export, dentry, NFSD_MAY_CREATE);
> + err = nfsd_permission(&rqstp->rq_cred, fhp->fh_export, dentry,
> +       NFSD_MAY_CREATE);
>   if (err)
>   goto out;
>
> @@ -2255,9 +2258,9 @@ nfsd_statfs(struct svc_rqst *rqstp, struct svc_fh *fhp, struct kstatfs *stat, in
>   return err;
>  }
>
> -static int exp_rdonly(struct svc_rqst *rqstp, struct svc_export *exp)
> +static int exp_rdonly(struct svc_cred *cred, struct svc_export *exp)
>  {
> - return nfsexp_flags(rqstp, exp) & NFSEXP_READONLY;
> + return nfsexp_flags(cred, exp) & NFSEXP_READONLY;
>  }
>
>  #ifdef CONFIG_NFSD_V4
> @@ -2501,8 +2504,8 @@ nfsd_setxattr(struct svc_rqst *rqstp, struct svc_fh *fhp, char *name,
>   * Check for a user's access permissions to this inode.
>   */
>  __be32
> -nfsd_permission(struct svc_rqst *rqstp, struct svc_export *exp,
> - struct dentry *dentry, int acc)
> +nfsd_permission(struct svc_cred *cred, struct svc_export *exp,
> + struct dentry *dentry, int acc)
>  {
>   struct inode *inode = d_inode(dentry);
>   int err;
> @@ -2533,7 +2536,7 @@ nfsd_permission(struct svc_rqst *rqstp, struct svc_export *exp,
>   */
>   if (!(acc & NFSD_MAY_LOCAL_ACCESS))
>   if (acc & (NFSD_MAY_WRITE | NFSD_MAY_SATTR | NFSD_MAY_TRUNC)) {
> - if (exp_rdonly(rqstp, exp) ||
> + if (exp_rdonly(cred, exp) ||
>       __mnt_is_readonly(exp->ex_path.mnt))
>   return nfserr_rofs;
>   if (/* (acc & NFSD_MAY_WRITE) && */ IS_IMMUTABLE(inode))
> diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h
> index 57cd70062048..1565c1dc28b6 100644
> --- a/fs/nfsd/vfs.h
> +++ b/fs/nfsd/vfs.h
> @@ -153,7 +153,7 @@ __be32 nfsd_readdir(struct svc_rqst *, struct svc_fh *,
>  __be32 nfsd_statfs(struct svc_rqst *, struct svc_fh *,
>   struct kstatfs *, int access);
>
> -__be32 nfsd_permission(struct svc_rqst *, struct svc_export *,
> +__be32 nfsd_permission(struct svc_cred *, struct svc_export *,
>   struct dentry *, int);
>
>  void nfsd_filp_close(struct file *fp);

Patch itself looks fine though.
--
Jeff Layton <jlayton@kernel.org>