[PATCH] Bug in NFS

[PATCH] Bug in NFS

[Abramo Bagnara]

[-- Attachment #1: Type: text/plain, Size: 670 bytes --]


I have found a bug in NFSv2.

[root@igor /tmp]# mount igor:/u u
[root@igor /tmp]# cd u
[root@igor u]# umask 000
[root@igor u]# ls -l q
ls: q: File o directory inesistente
[root@igor u]# touch q
[root@igor u]# ls -l q
-rw-r--r--    1 root     root            0 lug 13 07:56 q

This seems to be caused by use of unitialized current->fs->umask via
vfs_create called by nfsd_create.

Patch for 2.4.6 follows.

-- 
Abramo Bagnara                       mailto:abramo@alsa-project.org

Opera Unica                          Phone: +39.546.656023
Via Emilia Interna, 140
48014 Castel Bolognese (RA) - Italy

ALSA project               http://www.alsa-project.org
It sounds good!

[-- Attachment #2: nfs.diff --]
[-- Type: text/plain, Size: 282 bytes --]

--- linux-2.4/fs/nfsd/auth.c.~1~	Mon Jul 24 08:04:10 2000
+++ linux-2.4/fs/nfsd/auth.c	Fri Jul 13 08:00:10 2001
@@ -34,6 +34,7 @@
 				cred->cr_groups[i] = exp->ex_anon_gid;
 	}
 
+	current->fs->umask = 0;
 	if (cred->cr_uid != (uid_t) -1)
 		current->fsuid = cred->cr_uid;
 	else

Re: [NFS] [PATCH] Bug in NFS

[Neil Brown]

On Friday July 13, abramo@alsa-project.org wrote:
> 
> I have found a bug in NFSv2.
> 
> [root@igor /tmp]# mount igor:/u u
> [root@igor /tmp]# cd u
> [root@igor u]# umask 000
> [root@igor u]# ls -l q
> ls: q: File o directory inesistente
> [root@igor u]# touch q
> [root@igor u]# ls -l q
> -rw-r--r--    1 root     root            0 lug 13 07:56 q
> 
> This seems to be caused by use of unitialized current->fs->umask via
> vfs_create called by nfsd_create.
> 

Hmmm..  I think there is more here than immediately meets the eye.

current->fs->umask is initialised, to 0, in include/linux/fs_struct.h
The "INIT_FS" define is used to set the initial value of the fs_struct
(see arch/i386/kernel/init_task.c - other archs are the same).
The third field here is the umask field.

This is the fs_struct for init, and for every kernel thread that call
daemonise, as the nfsd threads do ever since 2.4.3pre5 or there abouts.

If the umask for nfsd is getting set to 022, as it would appear from
your experiment, then either:
  - your init process is setting it, or
  - you are using some odd architecture that doesn't use INIT_FS

So: what init program are you running, what architecture, any other
patches, anything else that might explain why your machine is
different from mine.  Because on mine, the touched file gets the right
permissions.

NeilBrown


> Patch for 2.4.6 follows.
> 
> -- 
> Abramo Bagnara                       mailto:abramo@alsa-project.org
> 
> Opera Unica                          Phone: +39.546.656023
> Via Emilia Interna, 140
> 48014 Castel Bolognese (RA) - Italy
> 
> ALSA project               http://www.alsa-project.org
> It sounds good!--- linux-2.4/fs/nfsd/auth.c.~1~	Mon Jul 24 08:04:10 2000
> +++ linux-2.4/fs/nfsd/auth.c	Fri Jul 13 08:00:10 2001
> @@ -34,6 +34,7 @@
>  				cred->cr_groups[i] = exp->ex_anon_gid;
>  	}
>  
> +	current->fs->umask = 0;
>  	if (cred->cr_uid != (uid_t) -1)
>  		current->fsuid = cred->cr_uid;
>  	else
> 

Re: [NFS] [PATCH] Bug in NFS

[Abramo Bagnara]

Neil Brown wrote:
> 
> On Friday July 13, abramo@alsa-project.org wrote:
> >
> > I have found a bug in NFSv2.
> >
> > [root@igor /tmp]# mount igor:/u u
> > [root@igor /tmp]# cd u
> > [root@igor u]# umask 000
> > [root@igor u]# ls -l q
> > ls: q: File o directory inesistente
> > [root@igor u]# touch q
> > [root@igor u]# ls -l q
> > -rw-r--r--    1 root     root            0 lug 13 07:56 q
> >
> > This seems to be caused by use of unitialized current->fs->umask via
> > vfs_create called by nfsd_create.
> >
> 
> Hmmm..  I think there is more here than immediately meets the eye.
> 
> current->fs->umask is initialised, to 0, in include/linux/fs_struct.h
> The "INIT_FS" define is used to set the initial value of the fs_struct
> (see arch/i386/kernel/init_task.c - other archs are the same).
> The third field here is the umask field.
> 
> This is the fs_struct for init, and for every kernel thread that call
> daemonise, as the nfsd threads do ever since 2.4.3pre5 or there abouts.
> 
> If the umask for nfsd is getting set to 022, as it would appear from
> your experiment, then either:
>   - your init process is setting it, or
>   - you are using some odd architecture that doesn't use INIT_FS
> 
> So: what init program are you running, what architecture, any other
> patches, anything else that might explain why your machine is
> different from mine.  Because on mine, the touched file gets the right
> permissions.

I've seen that on several systems with the following characteristics:

- Torvalds linux-2.4.6, Torvalds linux-2.4.4, linux-2.4.4+xfs from sgi
- nfsd compiled as a module (I suppose this make the difference we see)
- stock redhat-7.1 with updates applied

$ rpm -qf /sbin/init
SysVinit-2.78-17

-- 
Abramo Bagnara                       mailto:abramo@alsa-project.org

Opera Unica                          Phone: +39.546.656023
Via Emilia Interna, 140
48014 Castel Bolognese (RA) - Italy

ALSA project               http://www.alsa-project.org
It sounds good!

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

[Neil Brown]

[ Background for Linus who has been added to the To: list
  Abramo noticed that knfsd appeared to be running with a umask of 022.
  I commented that nfsd has the same umask as /sbin/init as they share
  current->fs, courtesy of daemonize().  I asked what "init" he was
  running]

On Friday July 13, abramo@alsa-project.org wrote:
> $ rpm -qf /sbin/init
> SysVinit-2.78-17

Ok, I found SysVinit-2.78-15 which is probably close enough, managed
to find a redhat system to explode it on, and found this patch:

--- sysvinit-2.78/src/init.c.foo        Wed Apr  4 01:42:27 2001
+++ sysvinit-2.78/src/init.c    Wed Apr  4 01:42:49 2001
@@ -2451,6 +2451,8 @@
        p = argv[0];
        
 
+  umask(022);
+       
   /*
    *   Is this telinit or init ?
    */
@@ -2523,8 +2525,6 @@
   /* Check syntax. */
   if (argc - optind != 1 || strlen(argv[optind]) != 1) Usage(p);
   if (!strchr("0123456789SsQqAaBbCcUu", argv[optind][0])) Usage(p);
-
-  umask(022);
 
   /* Open the fifo and write a command. */
   memset(&request, 0, sizeof(request));


what this does is that instead of just setting umask when running as
"telinit", init always sets umask to 022.  This is setting the umask
for nfsd. This completely explains your observations.
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.

Suggestions please ??? Linus??

NeilBrown

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

[Alan Cox]

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

#3 seems right. Of course its not clear whose fs struct should be shared

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

[Trond Myklebust]

>>>>> " " == Alan Cox <alan@redhat.com> writes:

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

     > #3 seems right. Of course its not clear whose fs struct should
     > #be shared

Well, you can either use the fs_struct from init, or that of the first
process to call nfsd. I'm not sure if there's any real point in having
a chrooted nfsd, but it's easy to implement.

In either case, the principle is the same: use copy_fs_struct() on
whatever you want to clone, then have all the nfsd daemons and the
lockd daemon attach to the new shared fs_struct when they get set up.
No need to replace daemonize...

Cheers,
  Trond

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

[Neil Brown]

On Friday July 13, neilb@cse.unsw.edu.au 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 \
 }
 

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

[Linus Torvalds]

On Sat, 14 Jul 2001, Neil Brown wrote:
>
> I've found a 4th option.  We make it so that fs->umask does not affect
> nfsd

Me likee.

Applied. I'd only like to double-check that you made sure you changed all
callers?

		Linus

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

[Neil Brown]

On Friday July 13, torvalds@transmeta.com wrote:
> 
> On Sat, 14 Jul 2001, Neil Brown wrote:
> >
> > I've found a 4th option.  We make it so that fs->umask does not affect
> > nfsd
> 
> Me likee.
> 
> Applied. I'd only like to double-check that you made sure you changed all
> callers?

There is just the call to vfs_mknod in net/unix/af_unix.c that I
mentioned.  I'm not sure what to do about that one.

A
    find -name '*.[ch]' | xargs egrep 'vfs_(mkdir|mknod|create)'

found:
  4 matches in fs/nfsd/vfs.c       which I explicitly didn't want to change
  6 matches in fs/devfs/base.c     which were really matches for devfs_(mkdir|mknod)
  7 matches in fs/namei.c          which the patch changed
  3 matches in kernel/ksyms.c      which are EXPORT_SYMBOLs, not calls
  3 matches in include/linux/fs.h  which are declatations
  2 matches in net/unix/af_unix.c  one is a comment, the other is the
                                   one in question

To be maximally conservative, you might want to apply this patch,
just in case it is important.

NeilBrown

--- ./net/unix/af_unix.c	2001/07/14 04:43:58	1.1
+++ ./net/unix/af_unix.c	2001/07/14 04:46:39
@@ -714,7 +714,7 @@
 		 * All right, let's create it.
 		 */
 		err = vfs_mknod(nd.dentry->d_inode, dentry,
-			S_IFSOCK|sock->inode->i_mode, 0);
+			(S_IFSOCK|sock->inode->i_mode) & ~current->fs->umask, 0);
 		if (err)
 			goto out_mknod_dput;
 		up(&nd.dentry->d_inode->i_sem);

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

[Alexander Viro]

On Sat, 14 Jul 2001, Neil Brown wrote:

> On Friday July 13, torvalds@transmeta.com wrote:
> > 
> > On Sat, 14 Jul 2001, Neil Brown wrote:
> > >
> > > I've found a 4th option.  We make it so that fs->umask does not affect
> > > nfsd
> > 
> > Me likee.
> > 
> > Applied. I'd only like to double-check that you made sure you changed all
> > callers?
> 
> There is just the call to vfs_mknod in net/unix/af_unix.c that I
> mentioned.  I'm not sure what to do about that one.
> 
> A
>     find -name '*.[ch]' | xargs egrep 'vfs_(mkdir|mknod|create)'

RTFM grep(1). \< is your friend...

>   2 matches in net/unix/af_unix.c  one is a comment, the other is the
>                                    one in question
> 
> To be maximally conservative, you might want to apply this patch,
> just in case it is important.

It is. Ability to connect == write permissions on AF_UNIX socket. So
umask matters.

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

[Neil Brown]

On Saturday July 14, viro@math.psu.edu wrote:
> 
> 
> On Sat, 14 Jul 2001, Neil Brown wrote:
> 
> > On Friday July 13, torvalds@transmeta.com wrote:
> > > 
> > > On Sat, 14 Jul 2001, Neil Brown wrote:
> > > >
> > > > I've found a 4th option.  We make it so that fs->umask does not affect
> > > > nfsd
> > > 
> > > Me likee.
> > > 
> > > Applied. I'd only like to double-check that you made sure you changed all
> > > callers?
> > 
> > There is just the call to vfs_mknod in net/unix/af_unix.c that I
> > mentioned.  I'm not sure what to do about that one.
> > 
> > A
> >     find -name '*.[ch]' | xargs egrep 'vfs_(mkdir|mknod|create)'
> 
> RTFM grep(1). \< is your friend...

But then you don't get to see and inspect random samplings of the
kernel, and thereby increase your general knowledge.
The false-positive of devfs_mknod gives a remarkable (if inacurate)
inside into the relationship between vfs and de-vfs :-)

Similarly searching for "umask" finds lots of matches for cpumask and
so it a steping stone into learning more about SMP infrastructure....
:-)

It's call "stratified sampling" and can be a more effective way to
sample a large body of data than put random sampling.

> 
> >   2 matches in net/unix/af_unix.c  one is a comment, the other is the
> >                                    one in question
> > 
> > To be maximally conservative, you might want to apply this patch,
> > just in case it is important.
> 
> It is. Ability to connect == write permissions on AF_UNIX socket. So
> umask matters.

I certainly appreciate that permissions on an AF_UNIX socket matter,
but wondered why they were set to "sock->inode->i_mode" rather than
simply 0666.  Maybe - I thought - sock->inode->i_mode already has the
umask applied in some way, and so re-appling it was not necessary.
Where-from comes the mode that is in sock->inode->i_mode ?

NeilBrown

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

[Alexander Viro]

On Sat, 14 Jul 2001, Neil Brown wrote:

> > It is. Ability to connect == write permissions on AF_UNIX socket. So
> > umask matters.
> 
> I certainly appreciate that permissions on an AF_UNIX socket matter,
> but wondered why they were set to "sock->inode->i_mode" rather than
> simply 0666.  Maybe - I thought - sock->inode->i_mode already has the
> umask applied in some way, and so re-appling it was not necessary.
> Where-from comes the mode that is in sock->inode->i_mode ?

net/socket.c::sock_alloc().