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@xxxxxxxxx>
> Signed-off-by: NeilBrown <neilb@xxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxx
