[PATCH] PATCH 3/7: knfsd cleanups - incorrect use of inode_change_ok

Get nfsd_setattr to not put too much weight on inode_change_ok

nfsd_currently calls inode_change_ok and does not try setattr if this fails.
However this is wrong.  If a filesystem defines it's own i_op->setattr, then
it might use a completely different mechanisim for determining what is ok.
nfsd shouldn't assume...

We still use inode_change_ok when normalising NFSv2 "touch" requests, but
only in passing.

fs/nfsd/vfs.c

@@ -209,36 +209,37 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, struct iattr *iap,
 	dentry = fhp->fh_dentry;
 	inode = dentry->d_inode;
 
-	err = inode_change_ok(inode, iap);
-	/* could be a "touch" (utimes) request where the user is not the owner but does
-	 * have write permission. In this case the user should be allowed to set
-	 * both times to the current time.  We could just assume any such SETATTR
-	 * is intended to set the times to "now", but we do a couple of simple tests
-	 * to increase our confidence.
+	/* 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.  It is only
+	 * valid if the filesystem does not define it's own i_op->setattr.
 	 */
 #define BOTH_TIME_SET (ATTR_ATIME_SET | ATTR_MTIME_SET)
 #define	MAX_TOUCH_TIME_ERROR (30*60)
-	if (err
-	    && (iap->ia_valid & BOTH_TIME_SET) == BOTH_TIME_SET
+	if ((iap->ia_valid & BOTH_TIME_SET) == BOTH_TIME_SET
 	    && iap->ia_mtime == iap->ia_atime
 	    ) {
-	    /* looks good.  now just make sure time is in the right ballpark.
-	     * solaris, at least, doesn't seem to care what the time request is
+	    /* 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 - CURRENT_TIME;
 	    if (delta<0) delta = -delta;
-	    if (delta < MAX_TOUCH_TIME_ERROR) {
+	    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;
-		err = inode_change_ok(inode, iap);
 	    }
 	}
 	    
-	if (err)
-		goto out_nfserr;
-
 	/* The size case is special. It changes the file as well as the attributes.  */
 	if (iap->ia_valid & ATTR_SIZE) {
 		if (iap->ia_size < inode->i_size) {