From: Jeff Layton <jlayton@kernel.org>
To: trondmy@kernel.org, Chuck Lever <chuck.lever@oracle.com>
Cc: linux-nfs@vger.kernel.org
Subject: Re: [PATCH] nfsd: Fix NFSv3 atomicity bugs in nfsd_setattr()
Date: Thu, 15 Feb 2024 07:33:42 -0500 [thread overview]
Message-ID: <e7cbf7f79baa1fc932084940476ee9cee48c279a.camel@kernel.org> (raw)
In-Reply-To: <20240214223501.205822-1-trondmy@kernel.org>
On Wed, 2024-02-14 at 17:35 -0500, trondmy@kernel.org wrote:
> From: Trond Myklebust <trond.myklebust@hammerspace.com>
>
> 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>
> ---
> 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 <jlayton@kernel.org>
next prev parent reply other threads:[~2024-02-15 12:33 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-14 22:35 [PATCH] nfsd: Fix NFSv3 atomicity bugs in nfsd_setattr() trondmy
2024-02-15 12:33 ` Jeff Layton [this message]
2024-02-15 16:02 ` Chuck Lever
2024-02-16 1:29 ` Trond Myklebust
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=e7cbf7f79baa1fc932084940476ee9cee48c279a.camel@kernel.org \
--to=jlayton@kernel.org \
--cc=chuck.lever@oracle.com \
--cc=linux-nfs@vger.kernel.org \
--cc=trondmy@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).