Re: amd64 -current crashs at boot

[Christoph Egger <Christoph_Egger%gmx.de@localhost>]

Patrick Welche wrote:
> On Sun, Dec 21, 2008 at 05:55:54PM +0100, Christoph Egger wrote:
>> Christoph Egger wrote:
>>> Hi,
>>>
>>> a amd64 -current kernel from today crashes at boot
>>> when sshd starts:
>>>
>>> uvm_fault(0xffffffff80d1e180, 0x0, 1) -> e
>>> fatal page fault in supervisor mode
>>> trap type 6 code 0 rip ffffffff802abbe4 cs 8 rflags 10282 cr2  60 cpl 0
>>> rsp ffff80004d832b20
>>> kernel: page fault trap, code=0
>>> Stopped in pid 0x46 (system) at netbsd:ffs_update+0x24: testb
>>> $0x1,0x60(%ray)
>>> db{0}> bt
>>> ffs_update() at netbsd:ffs_update+0x24
>>> ffs_full_fsync() at netbsd:ffs_full_fsync+0x54b
>>> spec_fsync() at netbsd:spec_fsync+0x59
>>> VOP_FSYNC() at netbsd:VOP_FSYNC+0x71
>>> sched_sync() at netbsd:sched_sync+0x14f
>>> db{0}> ps /l
>>> [...]
>>>  PID  LID S FLAGS     STRUCT LWP *         NAME WAIT
>>>> 0    49 3   204  ffff80004e1e7400     physiod physiod
>>>        48 3   204  ffff80004d7127c0 vmem_rehash vmem_rehash
>>>        47 3   204  ffff80004d712ba0    aiodoned aiodoned
>>>      > 46 7   204  ffff80004d700000     ioflush
>>> [...]
>> I found the commit which causes this:
>>
>> It is ffs_vnops.c, rev. 1.105. Going back to rev. 1.104 makes
>> the machine boot again.
>>
>> With rev. 1.105, when ffs_full_fsync() calls ffs_update in line 580,
>> vp->v_mount is a NULL pointer. ffs_update() dereferences it w/o
>> checking if the pointer is valid.
> 
> I seem to hit the other branch - NULL v_specmountpoint rather
> than NULL v_mount:
> With source of Dec 21 18:40, on i386, hit the assertion in
> /sys/ufs/ffs/ffs_vnops.c:414
> 
>    413          if ((flags & FSYNC_VFS) != 0) {
>    414                  KASSERT(vp->v_specmountpoint != NULL);
>    415                  mp = vp->v_specmountpoint;
>    416                  ffsino = (mp->mnt_op == &ffs_vfsops);
>    417                  KASSERT(vp->v_type == VBLK);
>    418          } else {
>    419                  mp = vp->v_mount;
>    420                  ffsino = true;
>    421                  KASSERT(vp->v_tag == VT_UFS);
>    422          }


No, I also hit the same branch. mp is set to vp->v_specmountpoint
which is a valid pointer. vp->v_mount is the NULL pointer
which is deferenced in ffs_update() w/o any validity checks before.

Attached patch sprinkles some KASSERTs which explicitely check
for assumptions not check otherwise in the code path.

The last hunk in the diff to ffs_vnode.c fixes the crash for me
and makes my machine boot again.

Patch ok to commit ?


Christoph

Index: sys/ufs/ffs/ffs_inode.c
===================================================================
RCS file: /cvsroot/src/sys/ufs/ffs/ffs_inode.c,v
retrieving revision 1.100
diff -u -p -r1.100 ffs_inode.c
--- sys/ufs/ffs/ffs_inode.c     17 Dec 2008 20:51:38 -0000      1.100
+++ sys/ufs/ffs/ffs_inode.c     22 Dec 2008 11:37:42 -0000
@@ -118,8 +118,12 @@ ffs_update(struct vnode *vp, const struc
        void *cp;
        int waitfor, flags;
 
+       KASSERT(vp);
+       KASSERT(vp->v_mount);
+
        if (vp->v_mount->mnt_flag & MNT_RDONLY)
                return (0);
+       KASSERT(VTOI(vp));
        ip = VTOI(vp);
        FFS_ITIMES(ip, acc, mod, NULL);
        if (updflags & UPDATE_CLOSE)
Index: sys/ufs/ffs/ffs_vnops.c
===================================================================
RCS file: /cvsroot/src/sys/ufs/ffs/ffs_vnops.c,v
retrieving revision 1.105
diff -u -p -r1.105 ffs_vnops.c
--- sys/ufs/ffs/ffs_vnops.c     21 Dec 2008 10:44:32 -0000      1.105
+++ sys/ufs/ffs/ffs_vnops.c     22 Dec 2008 11:37:42 -0000
@@ -410,6 +410,7 @@ ffs_full_fsync(struct vnode *vp, int fla
 
        error = 0;
 
+       KASSERT(vp != NULL);
        if ((flags & FSYNC_VFS) != 0) {
                KASSERT(vp->v_specmountpoint != NULL);
                mp = vp->v_specmountpoint;
@@ -440,6 +441,7 @@ ffs_full_fsync(struct vnode *vp, int fla
 
                if ((flags & FSYNC_WAIT))
                        pflags |= PGO_SYNCIO;
+               KASSERT(mp != NULL);
                if (vp->v_type == VREG &&
                    fstrans_getstate(mp) == FSTRANS_SUSPENDING)
                        pflags |= PGO_FREE;
@@ -462,6 +464,7 @@ ffs_full_fsync(struct vnode *vp, int fla
                        error = UFS_WAPBL_BEGIN(mp);
                        if (error)
                                return error;
+                       KASSERT(vp->v_mount != NULL);
                        error = ffs_update(vp, NULL, NULL,
                                (flags & FSYNC_WAIT) ? UPDATE_WAIT : 0);
                        UFS_WAPBL_END(mp);
@@ -576,7 +579,7 @@ loop:
        else
                waitfor = (flags & FSYNC_WAIT) ? UPDATE_WAIT : 0;
 
-       if (ffsino)
+       if (ffsino && vp->v_mount != NULL)
                error = ffs_update(vp, NULL, NULL, waitfor);
 
        if (error == 0 && flags & FSYNC_CACHE) {