| Message ID | 1430895024-1403-1-git-send-email-andreas.gruenbacher@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, May 06, 2015 at 08:50:24AM +0200, Andreas Gruenbacher wrote: > The NFSv2 protocol does not have a way to set the atime or mtime of a > file to the server's current time, only to specific timestamps. To make > up for that, when a client sets both atime and mtime to the same > timestamp and that timestamp is within the last half hour, the server > sets them to its own current time instead. > > The NFSv3 and later protocols do support setting atime or mtime to the > server's current time and clients do use that, so skip the NFSv2 > workaround there. > > With this change, clients which have write access but are not the owner > can still do the equivalent of utimes("file", NULL), for example with > "touch", but setting atime or mtime to any other value will now > consistently fail. This is also the local, non-NFS behavior. How about moving the workaround into the NFSv2 specific code? Looks like the call from nfsd_create() should never be used for this workaround, and the call from nfsd_proc_create isn't ever used for anything but size updates, leaving nfsd_proc_setattr as the only caller for which we should apply this hack. -- 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
2015-05-06 8:56 GMT+02:00 Christoph Hellwig <hch@lst.de>: > How about moving the workaround into the NFSv2 specific code? Not trivially, we would have to fh_verify() the file handle in nfsd_proc_setattr() first. Is that preferable? > Looks like the call from nfsd_create() should never be used for this > workaround, Right, the workaround won't trigger there because setting the times arbitrarily will already succeed because we are the owner (nfsd_sanitize_attrs tries that first with inode_change_ok). Thanks, Andreas -- 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
On Wed, May 06, 2015 at 12:12:13PM +0200, Andreas Grünbacher wrote: > 2015-05-06 8:56 GMT+02:00 Christoph Hellwig <hch@lst.de>: > > How about moving the workaround into the NFSv2 specific code? > > Not trivially, we would have to fh_verify() the file handle in > nfsd_proc_setattr() first. Is that preferable? Sounds better to me than making this workaround even more invasive in the core. Bruce, what do you think? -- 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
On Thu, May 07, 2015 at 09:53:02AM +0200, Christoph Hellwig wrote: > On Wed, May 06, 2015 at 12:12:13PM +0200, Andreas Grünbacher wrote: > > 2015-05-06 8:56 GMT+02:00 Christoph Hellwig <hch@lst.de>: > > > How about moving the workaround into the NFSv2 specific code? > > > > Not trivially, we would have to fh_verify() the file handle in > > nfsd_proc_setattr() first. Is that preferable? > > Sounds better to me than making this workaround even more invasive > in the core. Bruce, what do you think? I haven't looked into it, but I certainly don't see a reason why an extra call to nfsd_proc_setattr would be a problem. And I agree that it would be nice to have this in NFSv2-specific code. --b. -- 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
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c index 84d770b..41d3359 100644 --- a/fs/nfsd/vfs.c +++ b/fs/nfsd/vfs.c @@ -300,7 +300,7 @@ commit_metadata(struct svc_fh *fhp) * NFS semantics and what Linux expects. */ static void -nfsd_sanitize_attrs(struct inode *inode, struct iattr *iap) +nfsd_sanitize_attrs(struct svc_rqst *rqstp, struct inode *inode, struct iattr *iap) { /* * NFSv2 does not differentiate between "set-[ac]time-to-now" @@ -316,7 +316,8 @@ nfsd_sanitize_attrs(struct inode *inode, struct iattr *iap) #define BOTH_TIME_SET (ATTR_ATIME_SET | ATTR_MTIME_SET) #define MAX_TOUCH_TIME_ERROR (30*60) if ((iap->ia_valid & BOTH_TIME_SET) == BOTH_TIME_SET && - iap->ia_mtime.tv_sec == iap->ia_atime.tv_sec) { + iap->ia_mtime.tv_sec == iap->ia_atime.tv_sec && + rqstp->rq_vers == 2) { /* * Looks probable. * @@ -435,7 +436,7 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, struct iattr *iap, if (!iap->ia_valid) goto out; - nfsd_sanitize_attrs(inode, iap); + nfsd_sanitize_attrs(rqstp, inode, iap); /* * The size case is special, it changes the file in addition to the
The NFSv2 protocol does not have a way to set the atime or mtime of a file to the server's current time, only to specific timestamps. To make up for that, when a client sets both atime and mtime to the same timestamp and that timestamp is within the last half hour, the server sets them to its own current time instead. The NFSv3 and later protocols do support setting atime or mtime to the server's current time and clients do use that, so skip the NFSv2 workaround there. With this change, clients which have write access but are not the owner can still do the equivalent of utimes("file", NULL), for example with "touch", but setting atime or mtime to any other value will now consistently fail. This is also the local, non-NFS behavior. Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com> --- fs/nfsd/vfs.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)