NFSD SGID permission problem

Hello,

I've been running the Linux Test Package on a system with NFS root, and one of the errors it found I think is a bug in the kernel NFSD.

The test is chown02, and it tests two things. Quoting from the comments in the test case, it tests:

* Test Description:

 *  Verify that, when chown(2) invoked by super-user to change the owner and
 *  group of a file specified by path to any numeric owner(uid)/group(gid)
 *  values,

* - clears setuid and setgid bits set on an executable file.
* - preserves setgid bit set on a non-group-executable file.

The second test case is the one that fails. In the example, if a file has these permissions and owners:
-rwx--S--- 1 root root 0 Dec 10 17:39 /tmp/testfile2

and chown(2) is called to change the owner/group to 700:701, the resulting file should look like this:
-rwx--S--- 1 700 701 0 Dec 10 17:39 /tmp/testfile2

On an NFS mounted filesystem (with no_root_squash enabled, so I'm still root across the mount) I get this:
-rwx------ 1 700 701 0 Dec 10 17:39 /tmp/testfile2

After a brief search for SGID in the nfsd code, I removed a few lines of code from linux/fs/nfsd/vfs.c (see attached patch) and I started to get the right results. The underlying filesystem appears to properly enforce clearing of the SGID bit when the group execute bit is set, and leave the SGID alone when the group execute bit is not set. I did this work on my Redhat 9.0 workstation which was the NFS server for these tests, so the patch is against a 2.4.20 kernel, but the same code is in 2.4.28, and similar code is in 2.6.9. The underlying filesystem NFSD was exporting was ext3.

So, the question is, is this patch the proper fix? The code to keep the SGID bit if group execute is not present is in linux/fs/open.c, and the comment says:
/*
* Likewise, if the user or group of a non-directory has been changed
* by a non-root user, remove the setgid bit UNLESS there is no group
* execute bit (this would be a file marked for mandatory locking).
* 19981026 David C Niemi <[email protected]>
*

so it appears to be common code, and not dependent on the underlying filesystem.

I'm no NFS expert, so I'm looking for comments and feedback.

Thanks!

Ken Sumrall
[email protected]

Index: linux/fs/nfsd/vfs.c
===================================================================
RCS file: /cvsdev/mvl-kernel/linux/fs/nfsd/vfs.c,v retrieving revision 1.3

diff -u -r1.3 vfs.c
@@ -277,18 +277,6 @@
 		imode = iap->ia_mode |= (imode & ~S_IALLUGO);
 	}
 
-	/* Revoke setuid/setgid bit on chown/chgrp */
-	if ((iap->ia_valid & ATTR_UID) && (imode & S_ISUID)
-	 && iap->ia_uid != inode->i_uid) {
-		iap->ia_valid |= ATTR_MODE;
-		iap->ia_mode = imode &= ~S_ISUID;
-	}
-	if ((iap->ia_valid & ATTR_GID) && (imode & S_ISGID)
-	 && iap->ia_gid != inode->i_gid) {
-		iap->ia_valid |= ATTR_MODE;
-		iap->ia_mode = imode &= ~S_ISGID;
-	}
-
 	/* Change the attributes. */