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

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

[Andreas Gruenbacher]

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(-)

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

Re: [PATCH] nfsd: Disable NFSv2 timestamp workaround for NFSv3+

[Christoph Hellwig]

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.

Re: [PATCH] nfsd: Disable NFSv2 timestamp workaround for NFSv3+

[Andreas Grünbacher]

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

Re: [PATCH] nfsd: Disable NFSv2 timestamp workaround for NFSv3+

[Christoph Hellwig]

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?

Re: [PATCH] nfsd: Disable NFSv2 timestamp workaround for NFSv3+

[J. Bruce Fields]

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.

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

[Andreas Gruenbacher]

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(-)

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;
-- 
2.1.0

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

[Andreas Grünbacher]

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;
}

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

[Christoph Hellwig]

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.

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

[Andreas Grünbacher]

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

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

[Christoph Hellwig]

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>

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

[J. Bruce Fields]

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.