On Wed, Feb 14, 2024 at 05:35:01PM -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>
LGTM! Applied to nfsd-next.
I'm open to suggestion for Fixes: / Cc: stable tags, but for now
I'm leaving those off.
> ---
> 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 14712fa08f76..0294f5cce5dd 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -1168,8 +1168,7 @@ nfsd4_setattr(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>
> if (status)
> goto out;
> - status = nfsd_setattr(rqstp, &cstate->current_fh, &attrs,
> - 0, (time64_t)0);
> + status = nfsd_setattr(rqstp, &cstate->current_fh, &attrs, NULL);
> if (!status)
> status = nfserrno(attrs.na_labelerr);
> if (!status)
> 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 6e7e37192461..cc17eb8633ea 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
> @@ -555,6 +550,17 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp,
> }
>
> inode_lock(inode);
> + if (guardtime) {
> + struct timespec64 ctime = inode_get_ctime(inode);
> + if ((u32)guardtime->tv_sec != (u32)ctime.tv_sec ||
> + guardtime->tv_nsec != ctime.tv_nsec) {
> + inode_unlock(inode);
> + if (size_change)
> + put_write_access(inode);
> + return nfserr_notsync;
> + }
> + }
> +
> for (retries = 1;;) {
> struct iattr attrs;
>
> @@ -1404,7 +1410,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 {
> --
> 2.43.0
>
--
Chuck Lever
