* Re: [PATCH] nfsd: special case truncates some more
2017-02-20 6:21 ` [PATCH] nfsd: special case truncates some more Christoph Hellwig
@ 2017-02-20 22:23 ` J. Bruce Fields
2017-02-21 15:07 ` Chuck Lever
1 sibling, 0 replies; 5+ messages in thread
From: J. Bruce Fields @ 2017-02-20 22:23 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: bfields, jlayton, linux-nfs, stable
Thanks! I split out the cleanup into a separate patch just to make sure
I understood what the important change was.... Looks good to me.
--b.
On Mon, Feb 20, 2017 at 07:21:33AM +0100, Christoph Hellwig wrote:
> Both the NFS protocols and the Linux VFS use a setattr operation with a
> bitmap of attributs to set to set various file attributes including the
> file size and the uid/gid.
>
> The Linux syscalls never mixes size updates with unrelated updates like
> the uid/gid, and some file systems like XFS and GFS2 rely on the fact
> that truncates might not update random other attributes, and many other
> file systems handle the case but do not update the different attributes
> in the same transaction. NFSD on the other hand passes the attributes
> it gets on the wire more or less directly through to the VFS, leading to
> updates the file systems don't expect. XFS at least has an assert on
> the allowed attributes, which caught an unusual NFS client setting the
> size and group at the same time.
>
> To handle this issue properly this splits the notify_change call in
> nfsd_setattr into two separate ones.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Cc: stable@kernel.org
> ---
> fs/nfsd/vfs.c | 59 +++++++++++++++++++++++++++++++++++++----------------------
> 1 file changed, 37 insertions(+), 22 deletions(-)
>
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index 26c6fdb4bf67..3c36ed5a1f07 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -377,7 +377,7 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, struct iattr *iap,
> __be32 err;
> int host_err;
> bool get_write_count;
> - int size_change = 0;
> + bool size_change = (iap->ia_valid & ATTR_SIZE);
>
> if (iap->ia_valid & (ATTR_ATIME | ATTR_MTIME | ATTR_SIZE))
> accmode |= NFSD_MAY_WRITE|NFSD_MAY_OWNER_OVERRIDE;
> @@ -390,11 +390,11 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, struct iattr *iap,
> /* Get inode */
> err = fh_verify(rqstp, fhp, ftype, accmode);
> if (err)
> - goto out;
> + return err;
> if (get_write_count) {
> host_err = fh_want_write(fhp);
> if (host_err)
> - return nfserrno(host_err);
> + goto out;
> }
>
> dentry = fhp->fh_dentry;
> @@ -405,20 +405,28 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, struct iattr *iap,
> iap->ia_valid &= ~ATTR_MODE;
>
> if (!iap->ia_valid)
> - goto out;
> + return 0;
>
> nfsd_sanitize_attrs(inode, iap);
>
> + if (check_guard && guardtime != inode->i_ctime.tv_sec)
> + return nfserr_notsync;
> +
> /*
> * The size case is special, it changes the file in addition to the
> - * attributes.
> + * attributes, and file systems don't expect it to be mixed with
> + * "random" attribute changes. We thus split out the size change
> + * into a separate call to ->setattr, and do the rest as a separate
> + * setattr call.
> */
> - if (iap->ia_valid & ATTR_SIZE) {
> + if (size_change) {
> err = nfsd_get_write_access(rqstp, fhp, iap);
> if (err)
> - goto out;
> - size_change = 1;
> + return err;
> + }
>
> + fh_lock(fhp);
> + if (size_change) {
> /*
> * RFC5661, Section 18.30.4:
> * Changing the size of a file with SETATTR indirectly
> @@ -426,29 +434,36 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, struct iattr *iap,
> *
> * (and similar for the older RFCs)
> */
> - if (iap->ia_size != i_size_read(inode))
> - iap->ia_valid |= ATTR_MTIME;
> - }
> + struct iattr size_attr = {
> + .ia_valid = ATTR_SIZE | ATTR_CTIME | ATTR_MTIME,
> + .ia_size = iap->ia_size,
> + };
>
> - iap->ia_valid |= ATTR_CTIME;
> + host_err = notify_change(dentry, &size_attr, NULL);
> + if (host_err)
> + goto out_unlock;
> + iap->ia_valid &= ~ATTR_SIZE;
>
> - if (check_guard && guardtime != inode->i_ctime.tv_sec) {
> - err = nfserr_notsync;
> - goto out_put_write_access;
> + /*
> + * Avoid the additional setattr call below if the only other
> + * attribute that the client sends is the mtime, as we update
> + * it as part of the size change above.
> + */
> + if ((iap->ia_valid & ~ATTR_MTIME) == 0)
> + goto out_unlock;
> }
>
> - fh_lock(fhp);
> + iap->ia_valid |= ATTR_CTIME;
> host_err = notify_change(dentry, iap, NULL);
> - fh_unlock(fhp);
> - err = nfserrno(host_err);
>
> -out_put_write_access:
> +out_unlock:
> + fh_unlock(fhp);
> if (size_change)
> put_write_access(inode);
> - if (!err)
> - err = nfserrno(commit_metadata(fhp));
> out:
> - return err;
> + if (!host_err)
> + host_err = commit_metadata(fhp);
> + return nfserrno(host_err);
> }
>
> #if defined(CONFIG_NFSD_V4)
> --
> 2.11.0
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] nfsd: special case truncates some more
2017-02-20 6:21 ` [PATCH] nfsd: special case truncates some more Christoph Hellwig
2017-02-20 22:23 ` J. Bruce Fields
@ 2017-02-21 15:07 ` Chuck Lever
2017-02-21 15:14 ` J. Bruce Fields
1 sibling, 1 reply; 5+ messages in thread
From: Chuck Lever @ 2017-02-21 15:07 UTC (permalink / raw)
To: Christoph Hellwig
Cc: J. Bruce Fields, Jeff Layton, Linux NFS Mailing List, stable
> On Feb 20, 2017, at 1:21 AM, Christoph Hellwig <hch@lst.de> wrote:
>
> Both the NFS protocols and the Linux VFS use a setattr operation with a
> bitmap of attributs to set to set various file attributes including the
> file size and the uid/gid.
>
> The Linux syscalls never mixes size updates with unrelated updates like
> the uid/gid, and some file systems like XFS and GFS2 rely on the fact
> that truncates might not update random other attributes, and many other
> file systems handle the case but do not update the different attributes
> in the same transaction. NFSD on the other hand passes the attributes
> it gets on the wire more or less directly through to the VFS, leading to
> updates the file systems don't expect. XFS at least has an assert on
> the allowed attributes, which caught an unusual NFS client setting the
> size and group at the same time.
>
> To handle this issue properly this splits the notify_change call in
> nfsd_setattr into two separate ones.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Cc: stable@kernel.org
Tested-by: Chuck Lever <chuck.lever@oracle.com>
> ---
> fs/nfsd/vfs.c | 59 +++++++++++++++++++++++++++++++++++++----------------------
> 1 file changed, 37 insertions(+), 22 deletions(-)
>
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index 26c6fdb4bf67..3c36ed5a1f07 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -377,7 +377,7 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, struct iattr *iap,
> __be32 err;
> int host_err;
> bool get_write_count;
> - int size_change = 0;
> + bool size_change = (iap->ia_valid & ATTR_SIZE);
>
> if (iap->ia_valid & (ATTR_ATIME | ATTR_MTIME | ATTR_SIZE))
> accmode |= NFSD_MAY_WRITE|NFSD_MAY_OWNER_OVERRIDE;
> @@ -390,11 +390,11 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, struct iattr *iap,
> /* Get inode */
> err = fh_verify(rqstp, fhp, ftype, accmode);
> if (err)
> - goto out;
> + return err;
> if (get_write_count) {
> host_err = fh_want_write(fhp);
> if (host_err)
> - return nfserrno(host_err);
> + goto out;
> }
>
> dentry = fhp->fh_dentry;
> @@ -405,20 +405,28 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, struct iattr *iap,
> iap->ia_valid &= ~ATTR_MODE;
>
> if (!iap->ia_valid)
> - goto out;
> + return 0;
>
> nfsd_sanitize_attrs(inode, iap);
>
> + if (check_guard && guardtime != inode->i_ctime.tv_sec)
> + return nfserr_notsync;
> +
> /*
> * The size case is special, it changes the file in addition to the
> - * attributes.
> + * attributes, and file systems don't expect it to be mixed with
> + * "random" attribute changes. We thus split out the size change
> + * into a separate call to ->setattr, and do the rest as a separate
> + * setattr call.
> */
> - if (iap->ia_valid & ATTR_SIZE) {
> + if (size_change) {
> err = nfsd_get_write_access(rqstp, fhp, iap);
> if (err)
> - goto out;
> - size_change = 1;
> + return err;
> + }
>
> + fh_lock(fhp);
> + if (size_change) {
> /*
> * RFC5661, Section 18.30.4:
> * Changing the size of a file with SETATTR indirectly
> @@ -426,29 +434,36 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, struct iattr *iap,
> *
> * (and similar for the older RFCs)
> */
> - if (iap->ia_size != i_size_read(inode))
> - iap->ia_valid |= ATTR_MTIME;
> - }
> + struct iattr size_attr = {
> + .ia_valid = ATTR_SIZE | ATTR_CTIME | ATTR_MTIME,
> + .ia_size = iap->ia_size,
> + };
>
> - iap->ia_valid |= ATTR_CTIME;
> + host_err = notify_change(dentry, &size_attr, NULL);
> + if (host_err)
> + goto out_unlock;
> + iap->ia_valid &= ~ATTR_SIZE;
>
> - if (check_guard && guardtime != inode->i_ctime.tv_sec) {
> - err = nfserr_notsync;
> - goto out_put_write_access;
> + /*
> + * Avoid the additional setattr call below if the only other
> + * attribute that the client sends is the mtime, as we update
> + * it as part of the size change above.
> + */
> + if ((iap->ia_valid & ~ATTR_MTIME) == 0)
> + goto out_unlock;
> }
>
> - fh_lock(fhp);
> + iap->ia_valid |= ATTR_CTIME;
> host_err = notify_change(dentry, iap, NULL);
> - fh_unlock(fhp);
> - err = nfserrno(host_err);
>
> -out_put_write_access:
> +out_unlock:
> + fh_unlock(fhp);
> if (size_change)
> put_write_access(inode);
> - if (!err)
> - err = nfserrno(commit_metadata(fhp));
> out:
> - return err;
> + if (!host_err)
> + host_err = commit_metadata(fhp);
> + return nfserrno(host_err);
> }
>
> #if defined(CONFIG_NFSD_V4)
> --
> 2.11.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Chuck Lever
^ permalink raw reply [flat|nested] 5+ messages in thread