Re: [PATCH] nfsd: change nfsd_create_setattr() to call nfsd_setattr() unconditionally

[Trond Myklebust <trondmy@hammerspace.com>]

On Mon, 2024-05-13 at 15:42 +1000, NeilBrown wrote:
> 
> A recent change improved the guard on calling nfsd_setattr() from
> nfsd_create_setattr() so that it could be called even if ia_valid was
> zero - if there was a security label that needed to be set.
> 
> Unfortunately this is not sufficient as there could be an ACL that
> needs
> to be set.  Most likely in this case there would also be mode bits so
> ->ia_valid would not be zero, but it isn't safe to depend on that.
> 
> Rather than making nfsd_attrs_valid() more complete, this patch
> removes
> it and places the code in-line at the top of nfsd_setattr().  If
> there
> is nothing to be set, that function now short-circuits to the end
> where
> commit_metadata() is called.
> 
> With this change it is appropriate to call nfsd_setattr()
> unconditionally.
> 
> Reported-by: Stephen Smalley <stephen.smalley.work@gmail.com>
> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
>  fs/nfsd/vfs.c | 17 +++++++++--------
>  fs/nfsd/vfs.h |  8 --------
>  2 files changed, 9 insertions(+), 16 deletions(-)
> 
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index 29b1f3613800..e63f870696ad 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -499,6 +499,14 @@ nfsd_setattr(struct svc_rqst *rqstp, struct
> svc_fh *fhp,
>  	bool		size_change = (iap->ia_valid & ATTR_SIZE);
>  	int		retries;
>  
> +	if (!(iap->ia_valid ||
> +	      (attr->na_seclabel && attr->na_seclabel->len) ||
> +	      (IS_ENABLED(CONFIG_FS_POSIX_ACL) && attr->na_pacl) ||
> +	      (IS_ENABLED(CONFIG_FS_POSIX_ACL) &&
> +	       !attr->na_aclerr && attr->na_dpacl && S_ISDIR(inode-
> >i_mode))))
> +		/* Don't bother with inode_lock() */
> +		goto out;

Hmm... With NFSv4 being one of the filesystems that can now be re-
exported by knfsd, I feel somewhat queasy when I see POSIX acl-specific
code being added to a generic function. We'll want to push it down
closer to the filesystem-specific code when we fix re-exporting.

So can we please put that, at least, in its own function?

> +
>  	if (iap->ia_valid & ATTR_SIZE) {
>  		accmode |= NFSD_MAY_WRITE|NFSD_MAY_OWNER_OVERRIDE;
>  		ftype = S_IFREG;
> @@ -1418,14 +1426,7 @@ nfsd_create_setattr(struct svc_rqst *rqstp,
> struct svc_fh *fhp,
>  	if (!uid_eq(current_fsuid(), GLOBAL_ROOT_UID))
>  		iap->ia_valid &= ~(ATTR_UID|ATTR_GID);
>  
> -	/*
> -	 * Callers expect new file metadata to be committed even
> -	 * if the attributes have not changed.
> -	 */
> -	if (nfsd_attrs_valid(attrs))
> -		status = nfsd_setattr(rqstp, resfhp, attrs, NULL);
> -	else
> -		status = nfserrno(commit_metadata(resfhp));
> +	status = nfsd_setattr(rqstp, resfhp, attrs, NULL);
>  
>  	/*
>  	 * Transactional filesystems had a chance to commit changes
> diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h
> index 57cd70062048..c60fdb6200fd 100644
> --- a/fs/nfsd/vfs.h
> +++ b/fs/nfsd/vfs.h
> @@ -60,14 +60,6 @@ static inline void nfsd_attrs_free(struct
> nfsd_attrs *attrs)
>  	posix_acl_release(attrs->na_dpacl);
>  }
>  
> -static inline bool nfsd_attrs_valid(struct nfsd_attrs *attrs)
> -{
> -	struct iattr *iap = attrs->na_iattr;
> -
> -	return (iap->ia_valid || (attrs->na_seclabel &&
> -		attrs->na_seclabel->len));
> -}
> -
>  __be32		nfserrno (int errno);
>  int		nfsd_cross_mnt(struct svc_rqst *rqstp, struct dentry
> **dpp,
>  		                struct svc_export **expp);

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com