From: Trond Myklebust <[email protected]>
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 <[email protected]>
---
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
On Wed, 2024-02-14 at 17:35 -0500, [email protected] wrote:
> From: Trond Myklebust <[email protected]>
>
> 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 <[email protected]>
> ---
> 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)
Nice, I like not having a separate check_guard arg.
> {
> 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 {
Reviewed-by: Jeff Layton <[email protected]>
On Wed, Feb 14, 2024 at 05:35:01PM -0500, [email protected] wrote:
> From: Trond Myklebust <[email protected]>
>
> 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 <[email protected]>
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
On Thu, 2024-02-15 at 11:02 -0500, Chuck Lever wrote:
> On Wed, Feb 14, 2024 at 05:35:01PM -0500, [email protected] wrote:
> > From: Trond Myklebust <[email protected]>
> >
> > 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 <[email protected]>
>
> LGTM! Applied to nfsd-next.
Sigh... Unfortunately this patch wasn't sufficient. I just found a
rather nasty regression in the same area of code. v2 is on its way,
with the fix for the regression as a first standalone patch.
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
[email protected]