Re: [NFS] [PATCH] Bug in NFS - should init be allowed to set umask???

[Neil Brown]

On Friday July 13, [e-mail] wrote:
> So we have several options:
> 
> 1/ Claim that redhat is broken. Leave them to fix SysVinit.
> 2/ Have nfsd over-write the umask setting that /sbin/init imposed.
>    This is effectively what your patch does.
> 3/ Decide that it is inappropriate for nfsd to share the current->fs
>    fs_struct with init.  Unfortunately this means changing or
>    replacing daemonize().
> 
> None of these seem ideal.  (3) is probably most correct (i.e. protect
> the kernel from user space mucking about) but is the most work.

I've found a 4th option.  We make it so that fs->umask does not affect
nfsd, just as the rest of the fs_struct has no effect on nfsd.
This means moving the " & ~current->fs->umask" up out of the vfs_*
routines and into the sys_* (or open_namei) routines.

The attached patch does this.
The only part that I'm not 100% confident of is the call to vfs_mknod
in net/unix/af_unix.c:unix_bind.  I haven't put an "& ~current->fs->umask"
there, as I suspect that is actually the right thing.  Any comments on
that?

Al, as this is a vfs change, I have cc:ed you.  Are you happy with it?
It allows nfsd to not be effected by any change the init might make to
umask.

This patch also puts the umask for init back to 0022.

NeilBrown

--- ./fs/namei.c	2001/07/13 21:43:03	1.1
+++ ./fs/namei.c	2001/07/13 22:38:31	1.2
@@ -886,7 +886,7 @@
 {
 	int error;
 
-	mode &= S_IALLUGO & ~current->fs->umask;
+	mode &= S_IALLUGO;
 	mode |= S_IFREG;
 
 	down(&dir->i_zombie);
@@ -975,7 +975,8 @@
 
 	/* Negative dentry, just create the file */
 	if (!dentry->d_inode) {
-		error = vfs_create(dir->d_inode, dentry, mode);
+		error = vfs_create(dir->d_inode, dentry,
+				   mode & ~current->fs->umask);
 		up(&dir->d_inode->i_sem);
 		dput(nd->dentry);
 		nd->dentry = dentry;
@@ -1164,8 +1165,6 @@
 {
 	int error = -EPERM;
 
-	mode &= ~current->fs->umask;
-
 	down(&dir->i_zombie);
 	if ((S_ISCHR(mode) || S_ISBLK(mode)) && !capable(CAP_MKNOD))
 		goto exit_lock;
@@ -1208,6 +1207,8 @@
 		goto out;
 	dentry = lookup_create(&nd, 0);
 	error = PTR_ERR(dentry);
+
+	mode &= ~current->fs->umask;
 	if (!IS_ERR(dentry)) {
 		switch (mode & S_IFMT) {
 		case 0: case S_IFREG:
@@ -1246,7 +1247,7 @@
 		goto exit_lock;
 
 	DQUOT_INIT(dir);
-	mode &= (S_IRWXUGO|S_ISVTX) & ~current->fs->umask;
+	mode &= (S_IRWXUGO|S_ISVTX);
 	lock_kernel();
 	error = dir->i_op->mkdir(dir, dentry, mode);
 	unlock_kernel();
@@ -1276,7 +1277,8 @@
 		dentry = lookup_create(&nd, 1);
 		error = PTR_ERR(dentry);
 		if (!IS_ERR(dentry)) {
-			error = vfs_mkdir(nd.dentry->d_inode, dentry, mode);
+			error = vfs_mkdir(nd.dentry->d_inode, dentry,
+					  mode & ~current->fs->umask);
 			dput(dentry);
 		}
 		up(&nd.dentry->d_inode->i_sem);

--- ./include/linux/fs_struct.h	2001/07/13 22:46:19	1.1
+++ ./include/linux/fs_struct.h	2001/07/13 22:46:40	1.2
@@ -13,7 +13,7 @@
 #define INIT_FS { \
 	ATOMIC_INIT(1), \
 	RW_LOCK_UNLOCKED, \
-	0000, \
+	0022, \
 	NULL, NULL, NULL, NULL, NULL, NULL \
 }