1

nfsd: Fix NFSv3 atomicity bugs in nfsd_setattr()

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@hammerspace.com>
Reviewed-by: Jeff Layton <jlayton@kernel.org>
Reviewed-by: NeilBrown <neilb@suse.de>
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
This commit is contained in:
Trond Myklebust 2024-02-15 20:24:51 -05:00 committed by Chuck Lever
parent 6412e44c40
commit 24d92de918
8 changed files with 25 additions and 21 deletions

View File

@ -71,13 +71,15 @@ nfsd3_proc_setattr(struct svc_rqst *rqstp)
struct nfsd_attrs attrs = { struct nfsd_attrs attrs = {
.na_iattr = &argp->attrs, .na_iattr = &argp->attrs,
}; };
const struct timespec64 *guardtime = NULL;
dprintk("nfsd: SETATTR(3) %s\n", dprintk("nfsd: SETATTR(3) %s\n",
SVCFH_fmt(&argp->fh)); SVCFH_fmt(&argp->fh));
fh_copy(&resp->fh, &argp->fh); fh_copy(&resp->fh, &argp->fh);
resp->status = nfsd_setattr(rqstp, &resp->fh, &attrs, if (argp->check_guard)
argp->check_guard, argp->guardtime); guardtime = &argp->guardtime;
resp->status = nfsd_setattr(rqstp, &resp->fh, &attrs, guardtime);
return rpc_success; return rpc_success;
} }

View File

@ -295,17 +295,14 @@ svcxdr_decode_sattr3(struct svc_rqst *rqstp, struct xdr_stream *xdr,
static bool static bool
svcxdr_decode_sattrguard3(struct xdr_stream *xdr, struct nfsd3_sattrargs *args) svcxdr_decode_sattrguard3(struct xdr_stream *xdr, struct nfsd3_sattrargs *args)
{ {
__be32 *p;
u32 check; u32 check;
if (xdr_stream_decode_bool(xdr, &check) < 0) if (xdr_stream_decode_bool(xdr, &check) < 0)
return false; return false;
if (check) { if (check) {
p = xdr_inline_decode(xdr, XDR_UNIT * 2); if (!svcxdr_decode_nfstime3(xdr, &args->guardtime))
if (!p)
return false; return false;
args->check_guard = 1; args->check_guard = 1;
args->guardtime = be32_to_cpup(p);
} else } else
args->check_guard = 0; args->check_guard = 0;

View File

@ -1171,8 +1171,7 @@ nfsd4_setattr(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
goto out; goto out;
save_no_wcc = cstate->current_fh.fh_no_wcc; save_no_wcc = cstate->current_fh.fh_no_wcc;
cstate->current_fh.fh_no_wcc = true; cstate->current_fh.fh_no_wcc = true;
status = nfsd_setattr(rqstp, &cstate->current_fh, &attrs, status = nfsd_setattr(rqstp, &cstate->current_fh, &attrs, NULL);
0, (time64_t)0);
cstate->current_fh.fh_no_wcc = save_no_wcc; cstate->current_fh.fh_no_wcc = save_no_wcc;
if (!status) if (!status)
status = nfserrno(attrs.na_labelerr); status = nfserrno(attrs.na_labelerr);

View File

@ -5460,7 +5460,7 @@ nfsd4_truncate(struct svc_rqst *rqstp, struct svc_fh *fh,
return 0; return 0;
if (!(open->op_share_access & NFS4_SHARE_ACCESS_WRITE)) if (!(open->op_share_access & NFS4_SHARE_ACCESS_WRITE))
return nfserr_inval; 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, static __be32 nfs4_get_vfs_file(struct svc_rqst *rqstp, struct nfs4_file *fp,

View File

@ -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) if (resp->status != nfs_ok)
goto out; goto out;
@ -390,8 +390,8 @@ nfsd_proc_create(struct svc_rqst *rqstp)
*/ */
attr->ia_valid &= ATTR_SIZE; attr->ia_valid &= ATTR_SIZE;
if (attr->ia_valid) if (attr->ia_valid)
resp->status = nfsd_setattr(rqstp, newfhp, &attrs, 0, resp->status = nfsd_setattr(rqstp, newfhp, &attrs,
(time64_t)0); NULL);
} }
out_unlock: out_unlock:

View File

@ -476,7 +476,6 @@ static int __nfsd_setattr(struct dentry *dentry, struct iattr *iap)
* @rqstp: controlling RPC transaction * @rqstp: controlling RPC transaction
* @fhp: filehandle of target * @fhp: filehandle of target
* @attr: attributes to set * @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 * @guardtime: do not act if ctime.tv_sec does not match this timestamp
* *
* This call may adjust the contents of @attr (in particular, this * 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 __be32
nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp,
struct nfsd_attrs *attr, struct nfsd_attrs *attr, const struct timespec64 *guardtime)
int check_guard, time64_t guardtime)
{ {
struct dentry *dentry; struct dentry *dentry;
struct inode *inode; struct inode *inode;
@ -538,9 +536,6 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp,
nfsd_sanitize_attrs(inode, iap); 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 * 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 * 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); err = fh_fill_pre_attrs(fhp);
if (err) if (err)
goto out_unlock; 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;;) { for (retries = 1;;) {
struct iattr attrs; 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, attr->na_aclerr = set_posix_acl(&nop_mnt_idmap,
dentry, ACL_TYPE_DEFAULT, dentry, ACL_TYPE_DEFAULT,
attr->na_dpacl); attr->na_dpacl);
out_fill_attrs:
fh_fill_post_attrs(fhp); fh_fill_post_attrs(fhp);
out_unlock: out_unlock:
inode_unlock(inode); inode_unlock(inode);
@ -1411,7 +1417,7 @@ nfsd_create_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp,
* if the attributes have not changed. * if the attributes have not changed.
*/ */
if (iap->ia_valid) if (iap->ia_valid)
status = nfsd_setattr(rqstp, resfhp, attrs, 0, (time64_t)0); status = nfsd_setattr(rqstp, resfhp, attrs, NULL);
else else
status = nfserrno(commit_metadata(resfhp)); status = nfserrno(commit_metadata(resfhp));

View File

@ -69,7 +69,7 @@ __be32 nfsd_lookup_dentry(struct svc_rqst *, struct svc_fh *,
const char *, unsigned int, const char *, unsigned int,
struct svc_export **, struct dentry **); struct svc_export **, struct dentry **);
__be32 nfsd_setattr(struct svc_rqst *, struct svc_fh *, __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 *); int nfsd_mountpoint(struct dentry *, struct svc_export *);
#ifdef CONFIG_NFSD_V4 #ifdef CONFIG_NFSD_V4
__be32 nfsd4_vfs_fallocate(struct svc_rqst *, struct svc_fh *, __be32 nfsd4_vfs_fallocate(struct svc_rqst *, struct svc_fh *,

View File

@ -14,7 +14,7 @@ struct nfsd3_sattrargs {
struct svc_fh fh; struct svc_fh fh;
struct iattr attrs; struct iattr attrs;
int check_guard; int check_guard;
time64_t guardtime; struct timespec64 guardtime;
}; };
struct nfsd3_diropargs { struct nfsd3_diropargs {