From 604a3c407026d6162d15300478e63f901e435efc Mon Sep 17 00:00:00 2001 From: Christoph Hellwig Date: Mon, 20 Feb 2017 07:21:33 +0100 Subject: [PATCH] nfsd: special case truncates some more commit 783112f7401ff449d979530209b3f6c2594fdb4e upstream. Both the NFS protocols and the Linux VFS use a setattr operation with a bitmap of attributes to set to set various file attributes including the file size and the uid/gid. The Linux syscalls never mix size updates with unrelated updates like the uid/gid, and some file systems like XFS and GFS2 rely on the fact that truncates don't update random other attributes, and many other file systems handle the case but do not update the other 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 Tested-by: Chuck Lever Signed-off-by: J. Bruce Fields [bwh: Backported to 3.2: - notify_change() doesn't take a struct inode ** parameter - Move call to nfsd_break_lease() up along with fh_lock() - Adjust context] Signed-off-by: Ben Hutchings --- fs/nfsd/vfs.c | 40 ++++++++++++++++++++++++++++++---------- 1 file changed, 30 insertions(+), 10 deletions(-) diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c index 56bdccc9df96..acabafd9d0e0 100644 --- a/fs/nfsd/vfs.c +++ b/fs/nfsd/vfs.c @@ -446,13 +446,23 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, struct iattr *iap, /* * 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 (size_change) { err = nfsd_get_write_access(rqstp, fhp, iap); if (err) return err; + } + + host_err = nfsd_break_lease(inode); + if (host_err) + goto out_put_write_access_nfserror; + fh_lock(fhp); + if (size_change) { /* * RFC5661, Section 18.30.4: * Changing the size of a file with SETATTR indirectly @@ -460,20 +470,30 @@ 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); + if (host_err) + goto out_unlock; + iap->ia_valid &= ~ATTR_SIZE; - host_err = nfsd_break_lease(inode); - if (host_err) - goto out_put_write_access_nfserror; + /* + * 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); - fh_unlock(fhp); +out_unlock: + fh_unlock(fhp); out_put_write_access_nfserror: if (size_change) put_write_access(inode); -- 2.39.2