diff for duplicates of <20170123153615.GA32201@lst.de> diff --git a/a/1.txt b/N1/1.txt index fdaacf0..0602dd4 100644 --- a/a/1.txt +++ b/N1/1.txt @@ -23,3 +23,180 @@ back to nfsd_setattr? This would avoid the additional setattr call, but make me feel dirty :) --- +>From 0e06e2fc6157bb97692ed47c21e36120efb9f15c Mon Sep 17 00:00:00 2001 +From: Christoph Hellwig <hch@lst.de> +Date: Sun, 22 Jan 2017 17:17:48 +0100 +Subject: nfsd: special case truncates some more +MIME-Version: 1.0 +Content-Type: text/plain; charset=UTF-8 +Content-Transfer-Encoding: 8bit + +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 mixe 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 cought an NFS +client sets the size and group ІD at the same time. + +To handles this issue properly this switches nfsd to call vfs_truncate +for size changes, and then handling all other attributes through +notify_change. As a side effect this also means less boilerplace +code around the size change as we can now reuse the VFS code. + +Signed-off-by: Christoph Hellwig <hch@lst.de> +--- + fs/nfsd/vfs.c | 92 +++++++++++++++++++---------------------------------------- + 1 file changed, 30 insertions(+), 62 deletions(-) + +diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c +index 26c6fdb..4ca5b92 100644 +--- a/fs/nfsd/vfs.c ++++ b/fs/nfsd/vfs.c +@@ -332,37 +332,6 @@ nfsd_sanitize_attrs(struct inode *inode, struct iattr *iap) + } + } + +-static __be32 +-nfsd_get_write_access(struct svc_rqst *rqstp, struct svc_fh *fhp, +- struct iattr *iap) +-{ +- struct inode *inode = d_inode(fhp->fh_dentry); +- int host_err; +- +- if (iap->ia_size < inode->i_size) { +- __be32 err; +- +- err = nfsd_permission(rqstp, fhp->fh_export, fhp->fh_dentry, +- NFSD_MAY_TRUNC | NFSD_MAY_OWNER_OVERRIDE); +- if (err) +- return err; +- } +- +- host_err = get_write_access(inode); +- if (host_err) +- goto out_nfserrno; +- +- host_err = locks_verify_truncate(inode, NULL, iap->ia_size); +- if (host_err) +- goto out_put_write_access; +- return 0; +- +-out_put_write_access: +- put_write_access(inode); +-out_nfserrno: +- return nfserrno(host_err); +-} +- + /* + * Set various file attributes. After this call fhp needs an fh_put. + */ +@@ -377,7 +346,6 @@ 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; + + if (iap->ia_valid & (ATTR_ATIME | ATTR_MTIME | ATTR_SIZE)) + accmode |= NFSD_MAY_WRITE|NFSD_MAY_OWNER_OVERRIDE; +@@ -390,11 +358,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_host_err; + } + + dentry = fhp->fh_dentry; +@@ -405,50 +373,50 @@ 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 calo for vfs_truncate, and do the rest as a ++ * a separate setattr call. ++ * ++ * Note that vfs_truncate will also update ctime and mtime if ++ * the file size changes. + */ + if (iap->ia_valid & ATTR_SIZE) { +- err = nfsd_get_write_access(rqstp, fhp, iap); +- if (err) +- goto out; +- size_change = 1; ++ struct path path = { ++ .mnt = fhp->fh_export->ex_path.mnt, ++ .dentry = dentry, ++ }; + +- /* +- * RFC5661, Section 18.30.4: +- * Changing the size of a file with SETATTR indirectly +- * changes the time_modify and change attributes. +- * +- * (and similar for the older RFCs) +- */ +- if (iap->ia_size != i_size_read(inode)) +- iap->ia_valid |= ATTR_MTIME; ++ host_err = vfs_truncate(&path, iap->ia_size); ++ if (host_err) ++ goto out_host_err; ++ ++ iap->ia_valid &= ~ATTR_SIZE; ++ if (!iap->ia_valid) ++ goto done; + } + + iap->ia_valid |= ATTR_CTIME; + +- if (check_guard && guardtime != inode->i_ctime.tv_sec) { +- err = nfserr_notsync; +- goto out_put_write_access; +- } +- + fh_lock(fhp); + host_err = notify_change(dentry, iap, NULL); + fh_unlock(fhp); +- err = nfserrno(host_err); ++ if (host_err) ++ goto out_host_err; + +-out_put_write_access: +- if (size_change) +- put_write_access(inode); +- if (!err) +- err = nfserrno(commit_metadata(fhp)); +-out: +- return err; ++done: ++ host_err = commit_metadata(fhp); ++out_host_err: ++ return nfserrno(host_err); + } + + #if defined(CONFIG_NFSD_V4) +-- +2.1.4 diff --git a/a/content_digest b/N1/content_digest index e41d285..603d07c 100644 --- a/a/content_digest +++ b/N1/content_digest @@ -36,6 +36,183 @@ "back to nfsd_setattr? This would avoid the additional setattr call,\n" "but make me feel dirty :)\n" "\n" - --- + "---\n" + ">From 0e06e2fc6157bb97692ed47c21e36120efb9f15c Mon Sep 17 00:00:00 2001\n" + "From: Christoph Hellwig <hch@lst.de>\n" + "Date: Sun, 22 Jan 2017 17:17:48 +0100\n" + "Subject: nfsd: special case truncates some more\n" + "MIME-Version: 1.0\n" + "Content-Type: text/plain; charset=UTF-8\n" + "Content-Transfer-Encoding: 8bit\n" + "\n" + "Both the NFS protocols and the Linux VFS use a setattr operation with a\n" + "bitmap of attributs to set to set various file attributes including the\n" + "file size and the uid/gid.\n" + "\n" + "The Linux syscalls never mixe size updates with unrelated updates like\n" + "the uid/gid, and some file systems like XFS and GFS2 rely on the fact\n" + "that truncates might not update random other attributes, and many\n" + "other file systems handle the case but do not update the different\n" + "attributes in the same transaction. NFSD on the other hand passes\n" + "the attributes it gets on the wire more or less directly through to\n" + "the VFS, leading to updates the file systems don't expect. XFS at\n" + "least has an assert on the allowed attributes, which cought an NFS\n" + "client sets the size and group \320\206D at the same time.\n" + "\n" + "To handles this issue properly this switches nfsd to call vfs_truncate\n" + "for size changes, and then handling all other attributes through\n" + "notify_change. As a side effect this also means less boilerplace\n" + "code around the size change as we can now reuse the VFS code.\n" + "\n" + "Signed-off-by: Christoph Hellwig <hch@lst.de>\n" + "---\n" + " fs/nfsd/vfs.c | 92 +++++++++++++++++++----------------------------------------\n" + " 1 file changed, 30 insertions(+), 62 deletions(-)\n" + "\n" + "diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c\n" + "index 26c6fdb..4ca5b92 100644\n" + "--- a/fs/nfsd/vfs.c\n" + "+++ b/fs/nfsd/vfs.c\n" + "@@ -332,37 +332,6 @@ nfsd_sanitize_attrs(struct inode *inode, struct iattr *iap)\n" + " \t}\n" + " }\n" + " \n" + "-static __be32\n" + "-nfsd_get_write_access(struct svc_rqst *rqstp, struct svc_fh *fhp,\n" + "-\t\tstruct iattr *iap)\n" + "-{\n" + "-\tstruct inode *inode = d_inode(fhp->fh_dentry);\n" + "-\tint host_err;\n" + "-\n" + "-\tif (iap->ia_size < inode->i_size) {\n" + "-\t\t__be32 err;\n" + "-\n" + "-\t\terr = nfsd_permission(rqstp, fhp->fh_export, fhp->fh_dentry,\n" + "-\t\t\t\tNFSD_MAY_TRUNC | NFSD_MAY_OWNER_OVERRIDE);\n" + "-\t\tif (err)\n" + "-\t\t\treturn err;\n" + "-\t}\n" + "-\n" + "-\thost_err = get_write_access(inode);\n" + "-\tif (host_err)\n" + "-\t\tgoto out_nfserrno;\n" + "-\n" + "-\thost_err = locks_verify_truncate(inode, NULL, iap->ia_size);\n" + "-\tif (host_err)\n" + "-\t\tgoto out_put_write_access;\n" + "-\treturn 0;\n" + "-\n" + "-out_put_write_access:\n" + "-\tput_write_access(inode);\n" + "-out_nfserrno:\n" + "-\treturn nfserrno(host_err);\n" + "-}\n" + "-\n" + " /*\n" + " * Set various file attributes. After this call fhp needs an fh_put.\n" + " */\n" + "@@ -377,7 +346,6 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, struct iattr *iap,\n" + " \t__be32\t\terr;\n" + " \tint\t\thost_err;\n" + " \tbool\t\tget_write_count;\n" + "-\tint\t\tsize_change = 0;\n" + " \n" + " \tif (iap->ia_valid & (ATTR_ATIME | ATTR_MTIME | ATTR_SIZE))\n" + " \t\taccmode |= NFSD_MAY_WRITE|NFSD_MAY_OWNER_OVERRIDE;\n" + "@@ -390,11 +358,11 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, struct iattr *iap,\n" + " \t/* Get inode */\n" + " \terr = fh_verify(rqstp, fhp, ftype, accmode);\n" + " \tif (err)\n" + "-\t\tgoto out;\n" + "+\t\treturn err;\n" + " \tif (get_write_count) {\n" + " \t\thost_err = fh_want_write(fhp);\n" + " \t\tif (host_err)\n" + "-\t\t\treturn nfserrno(host_err);\n" + "+\t\t\tgoto out_host_err;\n" + " \t}\n" + " \n" + " \tdentry = fhp->fh_dentry;\n" + "@@ -405,50 +373,50 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, struct iattr *iap,\n" + " \t\tiap->ia_valid &= ~ATTR_MODE;\n" + " \n" + " \tif (!iap->ia_valid)\n" + "-\t\tgoto out;\n" + "+\t\treturn 0;\n" + " \n" + " \tnfsd_sanitize_attrs(inode, iap);\n" + " \n" + "+\tif (check_guard && guardtime != inode->i_ctime.tv_sec)\n" + "+\t\treturn nfserr_notsync;\n" + "+\n" + " \t/*\n" + " \t * The size case is special, it changes the file in addition to the\n" + "-\t * attributes.\n" + "+\t * attributes, and file systems don't expect it to be mixed with\n" + "+\t * \"random\" attribute changes. We thus split out the size change\n" + "+\t * into a separate calo for vfs_truncate, and do the rest as a\n" + "+\t * a separate setattr call.\n" + "+\t *\n" + "+\t * Note that vfs_truncate will also update ctime and mtime if\n" + "+\t * the file size changes.\n" + " \t */\n" + " \tif (iap->ia_valid & ATTR_SIZE) {\n" + "-\t\terr = nfsd_get_write_access(rqstp, fhp, iap);\n" + "-\t\tif (err)\n" + "-\t\t\tgoto out;\n" + "-\t\tsize_change = 1;\n" + "+\t\tstruct path path = {\n" + "+\t\t\t.mnt\t= fhp->fh_export->ex_path.mnt,\n" + "+\t\t\t.dentry\t= dentry,\n" + "+\t\t};\n" + " \n" + "-\t\t/*\n" + "-\t\t * RFC5661, Section 18.30.4:\n" + "-\t\t * Changing the size of a file with SETATTR indirectly\n" + "-\t\t * changes the time_modify and change attributes.\n" + "-\t\t *\n" + "-\t\t * (and similar for the older RFCs)\n" + "-\t\t */\n" + "-\t\tif (iap->ia_size != i_size_read(inode))\n" + "-\t\t\tiap->ia_valid |= ATTR_MTIME;\n" + "+\t\thost_err = vfs_truncate(&path, iap->ia_size);\n" + "+\t\tif (host_err)\n" + "+\t\t\tgoto out_host_err;\n" + "+\n" + "+\t\tiap->ia_valid &= ~ATTR_SIZE;\n" + "+\t\tif (!iap->ia_valid)\n" + "+\t\t\tgoto done;\n" + " \t}\n" + " \n" + " \tiap->ia_valid |= ATTR_CTIME;\n" + " \n" + "-\tif (check_guard && guardtime != inode->i_ctime.tv_sec) {\n" + "-\t\terr = nfserr_notsync;\n" + "-\t\tgoto out_put_write_access;\n" + "-\t}\n" + "-\n" + " \tfh_lock(fhp);\n" + " \thost_err = notify_change(dentry, iap, NULL);\n" + " \tfh_unlock(fhp);\n" + "-\terr = nfserrno(host_err);\n" + "+\tif (host_err)\n" + "+\t\tgoto out_host_err;\n" + " \n" + "-out_put_write_access:\n" + "-\tif (size_change)\n" + "-\t\tput_write_access(inode);\n" + "-\tif (!err)\n" + "-\t\terr = nfserrno(commit_metadata(fhp));\n" + "-out:\n" + "-\treturn err;\n" + "+done:\n" + "+\thost_err = commit_metadata(fhp);\n" + "+out_host_err:\n" + "+\treturn nfserrno(host_err);\n" + " }\n" + " \n" + " #if defined(CONFIG_NFSD_V4)\n" + "-- \n" + 2.1.4 -401fb4c7990d37b73aad22aedbb38489dcc50d5c5bec1b37ef3b6bbe4e9bf5f2 +da6ea300f94ec8cdb41f0746f996555e39ef67adf5759bf0376d20fa4d5b77da
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.