* split setattr operations take 2
@ 2017-02-20 6:21 Christoph Hellwig
2017-02-20 6:21 ` [PATCH] nfsd: special case truncates some more Christoph Hellwig
0 siblings, 1 reply; 5+ messages in thread
From: Christoph Hellwig @ 2017-02-20 6:21 UTC (permalink / raw)
To: bfields, jlayton; +Cc: linux-nfs
This just splits the setattr operations and doesn't try to use more
of the VFS infrastructure.
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] nfsd: special case truncates some more
2017-02-20 6:21 split setattr operations take 2 Christoph Hellwig
@ 2017-02-20 6:21 ` Christoph Hellwig
2017-02-20 22:23 ` J. Bruce Fields
2017-02-21 15:07 ` Chuck Lever
0 siblings, 2 replies; 5+ messages in thread
From: Christoph Hellwig @ 2017-02-20 6:21 UTC (permalink / raw)
To: bfields, jlayton; +Cc: linux-nfs, stable
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 related [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
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
* Re: [PATCH] nfsd: special case truncates some more
2017-02-21 15:07 ` Chuck Lever
@ 2017-02-21 15:14 ` J. Bruce Fields
0 siblings, 0 replies; 5+ messages in thread
From: J. Bruce Fields @ 2017-02-21 15:14 UTC (permalink / raw)
To: Chuck Lever
Cc: Christoph Hellwig, Jeff Layton, Linux NFS Mailing List, stable
On Tue, Feb 21, 2017 at 10:07:51AM -0500, Chuck Lever wrote:
>
> > 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>
Thanks.--b.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-02-21 15:14 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-02-20 6:21 split setattr operations take 2 Christoph Hellwig
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
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).