On Thu, 2024-02-15 at 20:24 -0500, trondmy@xxxxxxxxxx wrote:
> From: Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx>
>
> The main point of the guarded SETATTR is to prevent races with other
> WRITE and SETATTR calls. That requires that the check of the guard time
> against the inode ctime be done after taking the inode lock.
>
> Furthermore, we need to take into account the 32-bit nature of
> timestamps in NFSv3, and the possibility that files may change at a
> faster rate than once a second.
>
> Signed-off-by: Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx>
> ---
> fs/nfsd/nfs3proc.c | 6 ++++--
> fs/nfsd/nfs3xdr.c | 5 +----
> fs/nfsd/nfs4proc.c | 3 +--
> fs/nfsd/nfs4state.c | 2 +-
> fs/nfsd/nfsproc.c | 6 +++---
> fs/nfsd/vfs.c | 20 +++++++++++++-------
> fs/nfsd/vfs.h | 2 +-
> fs/nfsd/xdr3.h | 2 +-
> 8 files changed, 25 insertions(+), 21 deletions(-)
>
> diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c
> index b78eceebd945..dfcc957e460d 100644
> --- a/fs/nfsd/nfs3proc.c
> +++ b/fs/nfsd/nfs3proc.c
> @@ -71,13 +71,15 @@ nfsd3_proc_setattr(struct svc_rqst *rqstp)
> struct nfsd_attrs attrs = {
> .na_iattr = &argp->attrs,
> };
> + const struct timespec64 *guardtime = NULL;
>
> dprintk("nfsd: SETATTR(3) %s\n",
> SVCFH_fmt(&argp->fh));
>
> fh_copy(&resp->fh, &argp->fh);
> - resp->status = nfsd_setattr(rqstp, &resp->fh, &attrs,
> - argp->check_guard, argp->guardtime);
> + if (argp->check_guard)
> + guardtime = &argp->guardtime;
> + resp->status = nfsd_setattr(rqstp, &resp->fh, &attrs, guardtime);
> return rpc_success;
> }
>
> diff --git a/fs/nfsd/nfs3xdr.c b/fs/nfsd/nfs3xdr.c
> index f32128955ec8..a7a07470c1f8 100644
> --- a/fs/nfsd/nfs3xdr.c
> +++ b/fs/nfsd/nfs3xdr.c
> @@ -295,17 +295,14 @@ svcxdr_decode_sattr3(struct svc_rqst *rqstp, struct xdr_stream *xdr,
> static bool
> svcxdr_decode_sattrguard3(struct xdr_stream *xdr, struct nfsd3_sattrargs *args)
> {
> - __be32 *p;
> u32 check;
>
> if (xdr_stream_decode_bool(xdr, &check) < 0)
> return false;
> if (check) {
> - p = xdr_inline_decode(xdr, XDR_UNIT * 2);
> - if (!p)
> + if (!svcxdr_decode_nfstime3(xdr, &args->guardtime))
> return false;
> args->check_guard = 1;
> - args->guardtime = be32_to_cpup(p);
> } else
> args->check_guard = 0;
>
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index e6d8624efc83..ae48690f4c7c 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -1171,8 +1171,7 @@ nfsd4_setattr(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> goto out;
> save_no_wcc = cstate->current_fh.fh_no_wcc;
> cstate->current_fh.fh_no_wcc = true;
> - status = nfsd_setattr(rqstp, &cstate->current_fh, &attrs,
> - 0, (time64_t)0);
> + status = nfsd_setattr(rqstp, &cstate->current_fh, &attrs, NULL);
> cstate->current_fh.fh_no_wcc = save_no_wcc;
> if (!status)
> status = nfserrno(attrs.na_labelerr);
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 2fa54cfd4882..538edd85b51e 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -5191,7 +5191,7 @@ nfsd4_truncate(struct svc_rqst *rqstp, struct svc_fh *fh,
> return 0;
> if (!(open->op_share_access & NFS4_SHARE_ACCESS_WRITE))
> return nfserr_inval;
> - return nfsd_setattr(rqstp, fh, &attrs, 0, (time64_t)0);
> + return nfsd_setattr(rqstp, fh, &attrs, NULL);
> }
>
> static __be32 nfs4_get_vfs_file(struct svc_rqst *rqstp, struct nfs4_file *fp,
> diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c
> index a7315928a760..36370b957b63 100644
> --- a/fs/nfsd/nfsproc.c
> +++ b/fs/nfsd/nfsproc.c
> @@ -103,7 +103,7 @@ nfsd_proc_setattr(struct svc_rqst *rqstp)
> }
> }
>
> - resp->status = nfsd_setattr(rqstp, fhp, &attrs, 0, (time64_t)0);
> + resp->status = nfsd_setattr(rqstp, fhp, &attrs, NULL);
> if (resp->status != nfs_ok)
> goto out;
>
> @@ -390,8 +390,8 @@ nfsd_proc_create(struct svc_rqst *rqstp)
> */
> attr->ia_valid &= ATTR_SIZE;
> if (attr->ia_valid)
> - resp->status = nfsd_setattr(rqstp, newfhp, &attrs, 0,
> - (time64_t)0);
> + resp->status = nfsd_setattr(rqstp, newfhp, &attrs,
> + NULL);
> }
>
> out_unlock:
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index 58fab461bc00..3602e35e83d2 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -476,7 +476,6 @@ static int __nfsd_setattr(struct dentry *dentry, struct iattr *iap)
> * @rqstp: controlling RPC transaction
> * @fhp: filehandle of target
> * @attr: attributes to set
> - * @check_guard: set to 1 if guardtime is a valid timestamp
> * @guardtime: do not act if ctime.tv_sec does not match this timestamp
> *
> * This call may adjust the contents of @attr (in particular, this
> @@ -488,8 +487,7 @@ static int __nfsd_setattr(struct dentry *dentry, struct iattr *iap)
> */
> __be32
> nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp,
> - struct nfsd_attrs *attr,
> - int check_guard, time64_t guardtime)
> + struct nfsd_attrs *attr, const struct timespec64 *guardtime)
> {
> struct dentry *dentry;
> struct inode *inode;
> @@ -538,9 +536,6 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp,
>
> nfsd_sanitize_attrs(inode, iap);
>
> - if (check_guard && guardtime != inode_get_ctime_sec(inode))
> - return nfserr_notsync;
> -
> /*
> * The size case is special, it changes the file in addition to the
> * attributes, and file systems don't expect it to be mixed with
> @@ -558,6 +553,16 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp,
> err = fh_fill_pre_attrs(fhp);
> if (err)
> goto out_unlock;
> +
> + if (guardtime) {
> + struct timespec64 ctime = inode_get_ctime(inode);
> + if ((u32)guardtime->tv_sec != (u32)ctime.tv_sec ||
> + guardtime->tv_nsec != ctime.tv_nsec) {
> + err = nfserr_notsync;
> + goto out_fill_attrs;
> + }
> + }
> +
> for (retries = 1;;) {
> struct iattr attrs;
>
> @@ -585,6 +590,7 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp,
> attr->na_aclerr = set_posix_acl(&nop_mnt_idmap,
> dentry, ACL_TYPE_DEFAULT,
> attr->na_dpacl);
> +out_fill_attrs:
> fh_fill_post_attrs(fhp);
> out_unlock:
> inode_unlock(inode);
> @@ -1409,7 +1415,7 @@ nfsd_create_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp,
> * if the attributes have not changed.
> */
> if (iap->ia_valid)
> - status = nfsd_setattr(rqstp, resfhp, attrs, 0, (time64_t)0);
> + status = nfsd_setattr(rqstp, resfhp, attrs, NULL);
> else
> status = nfserrno(commit_metadata(resfhp));
>
> diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h
> index 702fbc4483bf..7d77303ef5f7 100644
> --- a/fs/nfsd/vfs.h
> +++ b/fs/nfsd/vfs.h
> @@ -69,7 +69,7 @@ __be32 nfsd_lookup_dentry(struct svc_rqst *, struct svc_fh *,
> const char *, unsigned int,
> struct svc_export **, struct dentry **);
> __be32 nfsd_setattr(struct svc_rqst *, struct svc_fh *,
> - struct nfsd_attrs *, int, time64_t);
> + struct nfsd_attrs *, const struct timespec64 *);
> int nfsd_mountpoint(struct dentry *, struct svc_export *);
> #ifdef CONFIG_NFSD_V4
> __be32 nfsd4_vfs_fallocate(struct svc_rqst *, struct svc_fh *,
> diff --git a/fs/nfsd/xdr3.h b/fs/nfsd/xdr3.h
> index 03fe4e21306c..522067b7fd75 100644
> --- a/fs/nfsd/xdr3.h
> +++ b/fs/nfsd/xdr3.h
> @@ -14,7 +14,7 @@ struct nfsd3_sattrargs {
> struct svc_fh fh;
> struct iattr attrs;
> int check_guard;
> - time64_t guardtime;
> + struct timespec64 guardtime;
> };
>
> struct nfsd3_diropargs {
I thought I sent this the other day, but...
Reviewed-by: Jeff Layton <jlayton@xxxxxxxxxx>
