diff mbox

[v2] nfsd: Disable NFSv2 timestamp workaround for NFSv3+

Message ID 1431124677-13388-1-git-send-email-andreas.gruenbacher@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Andreas Grünbacher May 8, 2015, 10:37 p.m. UTC 
NFSv2 can set the atime and/or mtime of a file to specific timestamps but not
to the server's current time.  To implement the equivalent of utimes("file",
NULL), it uses a heuristic.

NFSv3 and later do support setting the atime and/or mtime to the server's
current time directly.  The NFSv2 heuristic is still enabled, and causes
timestamps to be set wrong sometimes.

Fix this by moving the heuristic into the NFSv2 specific code.  We can leave it
out of the create code path: the owner can always set timestamps arbitrarily,
and the workaround would never trigger.

Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
---
 fs/nfsd/nfsproc.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++--
 fs/nfsd/vfs.c     | 36 ------------------------------------
 2 files changed, 50 insertions(+), 38 deletions(-)

Comments

Andreas Grünbacher May 8, 2015, 10:50 p.m. UTC | #1 
Here's a little test for this:

#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <unistd.h>
#include <time.h>
#include <utime.h>
#include <grp.h>
#include <errno.h>
#include <stdio.h>
#include <stdlib.h>
#include <stdbool.h>

static int touch(const char *path, mode_t mode, bool current_time)
{
        if (current_time)
                return utime(path, NULL);
        else {
                time_t now = time(NULL);
                struct utimbuf times = {
                        .actime = now,
                        .modtime = now,
                };
                return utime(path, &times);
        }
}

static int su(uid_t uid, gid_t gid)
{
        int ret;

        ret = seteuid(0);
        if (ret != 0)
                return ret;
        ret = setegid(gid);
        if (ret != 0)
                return ret;
        ret = setgroups(0, NULL);
        if (ret != 0)
                return ret;
        ret = seteuid(uid);
        return ret;
}

static void die(const char *str)
{
        perror(str);
        exit(1);
}

void cleanup(void)
{
        su(0, 0);
        unlink("foo");
}

int main(void)
{
        int ret;

        atexit(cleanup);

        ret = creat("foo", 0600);
        if (ret < 0)
                die("foo");
        close(ret);
        ret = chown("foo", 0, 12345);
        if (ret != 0)
                die("foo");

        ret = su(12345, 12345);
        if (ret != 0)
                die("su");
        errno = 0;
        ret = touch("foo", 0660, true);
        if (ret == 0)
                die("touch should have failed");

        ret = su(0, 0);
        if (ret != 0)
                die("su");
        ret = chmod("foo", 0660);
        if (ret != 0)
                die("foo");

        ret = su(12345, 12345);
        if (ret != 0)
                die("su");
        errno = 0;
        ret = touch("foo", 0660, true);
        if (ret != 0)
                die("touch should have succeeded");

        ret = touch("foo", 0660, false);
        if (ret == 0)
                die("touch should fail on NFSv3+");

        return 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
Christoph Hellwig May 11, 2015, 12:26 p.m. UTC | #2 
On Sat, May 09, 2015 at 12:37:57AM +0200, Andreas Gruenbacher wrote:
> NFSv2 can set the atime and/or mtime of a file to specific timestamps but not
> to the server's current time.  To implement the equivalent of utimes("file",
> NULL), it uses a heuristic.
> 
> NFSv3 and later do support setting the atime and/or mtime to the server's
> current time directly.  The NFSv2 heuristic is still enabled, and causes
> timestamps to be set wrong sometimes.
> 
> Fix this by moving the heuristic into the NFSv2 specific code.  We can leave it
> out of the create code path: the owner can always set timestamps arbitrarily,
> and the workaround would never trigger.
> 
> Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>

This looks fine,

Reviewed-by: Christoph Hellwig <hch@lst.de>
--
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
Christoph Hellwig May 11, 2015, 12:27 p.m. UTC | #3 
Can you add this to xfstests?  As it should fail for all filesystems
except for NFSv2 on which xfstests would fail horrible I think it can
become a generic test.
--
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
Andreas Grünbacher May 12, 2015, 6:38 p.m. UTC | #4 
2015-05-11 14:27 GMT+02:00 Christoph Hellwig <hch@lst.de>:
> Can you add this to xfstests?

Yes, that probably makes sense. I've sent two patches to the list.

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
J. Bruce Fields May 12, 2015, 7:17 p.m. UTC | #5 
On Mon, May 11, 2015 at 02:26:11PM +0200, Christoph Hellwig wrote:
> On Sat, May 09, 2015 at 12:37:57AM +0200, Andreas Gruenbacher wrote:
> > NFSv2 can set the atime and/or mtime of a file to specific timestamps but not
> > to the server's current time.  To implement the equivalent of utimes("file",
> > NULL), it uses a heuristic.
> > 
> > NFSv3 and later do support setting the atime and/or mtime to the server's
> > current time directly.  The NFSv2 heuristic is still enabled, and causes
> > timestamps to be set wrong sometimes.
> > 
> > Fix this by moving the heuristic into the NFSv2 specific code.  We can leave it
> > out of the create code path: the owner can always set timestamps arbitrarily,
> > and the workaround would never trigger.
> > 
> > Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
> 
> This looks fine,
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>

Yep, thanks, applying for 4.2.

--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 mbox

Patch

diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c
index aecbcd3..4cd78ef 100644
--- a/fs/nfsd/nfsproc.c
+++ b/fs/nfsd/nfsproc.c
@@ -59,13 +59,61 @@  static __be32
 nfsd_proc_setattr(struct svc_rqst *rqstp, struct nfsd_sattrargs *argp,
 					  struct nfsd_attrstat  *resp)
 {
+	struct iattr *iap = &argp->attrs;
+	struct svc_fh *fhp;
 	__be32 nfserr;
+
 	dprintk("nfsd: SETATTR  %s, valid=%x, size=%ld\n",
 		SVCFH_fmt(&argp->fh),
 		argp->attrs.ia_valid, (long) argp->attrs.ia_size);
 
-	fh_copy(&resp->fh, &argp->fh);
-	nfserr = nfsd_setattr(rqstp, &resp->fh, &argp->attrs,0, (time_t)0);
+	fhp = fh_copy(&resp->fh, &argp->fh);
+
+	/*
+	 * NFSv2 does not differentiate between "set-[ac]time-to-now"
+	 * which only requires access, and "set-[ac]time-to-X" which
+	 * requires ownership.
+	 * So if it looks like it might be "set both to the same time which
+	 * is close to now", and if inode_change_ok fails, then we
+	 * convert to "set to now" instead of "set to explicit time"
+	 *
+	 * We only call inode_change_ok as the last test as technically
+	 * it is not an interface that we should be using.
+	 */
+#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) {
+		/*
+		 * Looks probable.
+		 *
+		 * Now just make sure time is in the right ballpark.
+		 * Solaris, at least, doesn't seem to care what the time
+		 * request is.  We require it be within 30 minutes of now.
+		 */
+		time_t delta = iap->ia_atime.tv_sec - get_seconds();
+		struct inode *inode;
+
+		nfserr = fh_verify(rqstp, fhp, 0, NFSD_MAY_NOP);
+		if (nfserr)
+			goto done;
+		inode = d_inode(fhp->fh_dentry);
+
+		if (delta < 0)
+			delta = -delta;
+		if (delta < MAX_TOUCH_TIME_ERROR &&
+		    inode_change_ok(inode, iap) != 0) {
+			/*
+			 * Turn off ATTR_[AM]TIME_SET but leave ATTR_[AM]TIME.
+			 * This will cause notify_change to set these times
+			 * to "now"
+			 */
+			iap->ia_valid &= ~BOTH_TIME_SET;
+		}
+	}
+
+	nfserr = nfsd_setattr(rqstp, fhp, iap, 0, (time_t)0);
+done:
 	return nfsd_return_attrs(nfserr, resp);
 }
 
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index 84d770b..92de374 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -302,42 +302,6 @@  commit_metadata(struct svc_fh *fhp)
 static void
 nfsd_sanitize_attrs(struct inode *inode, struct iattr *iap)
 {
-	/*
-	 * NFSv2 does not differentiate between "set-[ac]time-to-now"
-	 * which only requires access, and "set-[ac]time-to-X" which
-	 * requires ownership.
-	 * So if it looks like it might be "set both to the same time which
-	 * is close to now", and if inode_change_ok fails, then we
-	 * convert to "set to now" instead of "set to explicit time"
-	 *
-	 * We only call inode_change_ok as the last test as technically
-	 * it is not an interface that we should be using.
-	 */
-#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) {
-		/*
-		 * Looks probable.
-		 *
-		 * Now just make sure time is in the right ballpark.
-		 * Solaris, at least, doesn't seem to care what the time
-		 * request is.  We require it be within 30 minutes of now.
-		 */
-		time_t delta = iap->ia_atime.tv_sec - get_seconds();
-		if (delta < 0)
-			delta = -delta;
-		if (delta < MAX_TOUCH_TIME_ERROR &&
-		    inode_change_ok(inode, iap) != 0) {
-			/*
-			 * Turn off ATTR_[AM]TIME_SET but leave ATTR_[AM]TIME.
-			 * This will cause notify_change to set these times
-			 * to "now"
-			 */
-			iap->ia_valid &= ~BOTH_TIME_SET;
-		}
-	}
-
 	/* sanitize the mode change */
 	if (iap->ia_valid & ATTR_MODE) {
 		iap->ia_mode &= S_IALLUGO;