Re: CVS commit: syssrc/sys/ufs/ffs

[enami tsugutomo <enami%sm.sony.co.jp@localhost>]

Christos Zoulas <christos%netbsd.org@localhost> writes:

> Module Name:  syssrc
> Committed By: christos
> Date:         Sun Mar 31 20:53:26 UTC 2002
> 
> Modified Files:
>       syssrc/sys/ufs/ffs: ffs_vfsops.c
> 
> Log Message:
> PR/16136: Chris Jepeway: Bogus entry in /etc/fstab can panic kernel.
> 
> 
> To generate a diff of this commit:
> cvs rdiff -r1.94 -r1.95 syssrc/sys/ufs/ffs/ffs_vfsops.c
> 
> Please note that diffs are not public domain; they are subject to the
> copyright notices on the relevant files.

I suspect how well this change is reviewed.  It at least introduces
one bug and one bogus check and points out some ambiguity.

- If VOP_ACCESS fails when updating mount, we will vrele() twice.

- The check for update-only flags in mp->mnt_flag when not updating
  case is bogus.  If we really want to check, we need to see flags in
  ufs_args, but I'm not sure if it is really necessary.

- The credential passed to ffs_reload was credential of when looking
  up mount point, but now it is credential of when looking up device
  node.  Anyway, it may be current process's credential.

enami.

c.f.:

Index: ffs_vfsops.c
===================================================================
RCS file: /cvsroot/syssrc/sys/ufs/ffs/ffs_vfsops.c,v
retrieving revision 1.95
diff -u -r1.95 ffs_vfsops.c
--- ffs_vfsops.c        2002/03/31 20:53:25     1.95
+++ ffs_vfsops.c        2002/04/01 01:03:53
@@ -207,27 +207,9 @@
        } else {
                /* New mounts must have a filename for the device */
                if (args.fspec == NULL)
-                       return EINVAL;
-
-               /* Check for update-only flags */
-               if (mp->mnt_flag &
-                   (MNT_WANTRDWR |     /*
-                                        * Upgrading from read-only to
-                                        * read-write can only occur after
-                                        * the initial mount
-                                        */
-                    MNT_EXRDONLY |
-                    MNT_DEFEXPORTED |
-                    MNT_EXPORTANON |
-                    MNT_EXKERB |       /* Only update mounts are allowed */
-                    MNT_EXNORESPORT |  /* to affect exporting            */
-                    MNT_EXPUBLIC |
-                    MNT_DELEXPORT))
-               return EINVAL;
+                       return (EINVAL);
        }
 
-       error = 0;
-
        if (args.fspec) {
                /*
                 * Look up the name and verify that it's sane.
@@ -252,13 +234,6 @@
                         */
                        if (devvp != ump->um_devvp)
                                error = EINVAL;
-                       else
-                               /*
-                                * The initial mount got a reference on this
-                                * device, so drop the one obtained via
-                                * namei(), above
-                                */
-                               vrele(devvp);
                }
        }
 
@@ -268,8 +243,9 @@
         */
        if (error == 0 && p->p_ucred->cr_uid != 0) {
                accessmode = VREAD;
-               if ((!update && (mp->mnt_flag & MNT_RDONLY) == 0) ||
-                    (update && (mp->mnt_flag & MNT_WANTRDWR)))
+               if (update ?
+                   (mp->mnt_flag & MNT_WANTRDWR) != 0 :
+                   (mp->mnt_flag & MNT_RDONLY) == 0)
                        accessmode |= VWRITE;
                vn_lock(devvp, LK_EXCLUSIVE | LK_RETRY);
                error = VOP_ACCESS(devvp, accessmode, p->p_ucred, p);
@@ -293,14 +269,22 @@
                if ((mp->mnt_flag & (MNT_SOFTDEP | MNT_ASYNC)) ==
                    (MNT_SOFTDEP | MNT_ASYNC)) {
                        printf("%s fs uses soft updates, "
-                              "ignoring async mode\n",
-                              fs->fs_fsmnt);
+                           "ignoring async mode\n",
+                           fs->fs_fsmnt);
                        mp->mnt_flag &= ~MNT_ASYNC;
                }
        } else {
+               /*
+                * Update the mount.
+                */
+
                /*
-                * Update the mount
+                * The initial mount got a reference on this
+                * device, so drop the one obtained via
+                * namei(), above.
                 */
+               vrele(devvp);
+
                fs = ump->um_fs;
                if (fs->fs_ronly == 0 && (mp->mnt_flag & MNT_RDONLY)) {
                        /*
@@ -372,7 +356,7 @@
                }
 
                if (mp->mnt_flag & MNT_RELOAD) {
-                       error = ffs_reload(mp, ndp->ni_cnd.cn_cred, p);
+                       error = ffs_reload(mp, p->p_ucred, p);
                        if (error)
                                return (error);
                }