On 05/09/19(Thu) 23:54, Alexander Bluhm wrote:
> Hi,
>
> When doing a forced unmount on a stressed file system, it may crash
> here.
>
> uvm_fault(0xfffffd85827449a0, 0x50, 0, 1) -> e
> kernel: page fault trap, code=0
> Stopped at vfs_lookup+0x5d8: testb $0x1,0x50(%rcx)
> ddb{2}> bt
> vfs_lookup(ffff8000229b22a0) at vfs_lookup+0x5d8
> namei(ffff8000229b22a0) at namei+0x395
> dounlinkat(ffff8000fffeb1f8,ffffff9c,11339e48878,0) at dounlinkat+0x76
> syscall(ffff8000229b2490) at syscall+0x389
> Xsyscall(6,a,110cd90a000,a,0,113c729cb80) at Xsyscall+0x128
> end of kernel
> end trace frame: 0x7f7ffffded50, count: -5
>
> if (rdonly || (dp->v_mount->mnt_flag & MNT_RDONLY) ||
> (wantparent &&
> (ndp->ni_dvp->v_mount->mnt_flag & MNT_RDONLY))) {
> error = EROFS;
> goto bad2;
> }
>
> dp->v_mount can be NULL. I think this can happen at multiple places
> in vfs_lookup() when it is sleeping. Recently vn_lock() got even
> more calls to tsleep(9).
Is dereferencing `v_mount' the only problem here? If you copy the flags
earlier can you trigger a different crash?
> We can add checks like this. It this the right approach? Or should
> we make sure that all vnodes in vfs_lookup() are always locked and
> dounmount() has to wait until lock is released? The latter seems
> quite complicated.
How many sleep points we would have to fix like that? Can such check be
placed at a deeper place?
> Syzcaller creates a crash at the same location, but I think it is
> not using unmount. Don't know if it is related.
> https://syzkaller.appspot.com/bug?id=49b54eec7f3e2afc637cf49adf31d04dcca8b69e
>
> bluhm
>
> Index: kern/vfs_lookup.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/kern/vfs_lookup.c,v
> retrieving revision 1.82
> diff -u -p -r1.82 vfs_lookup.c
> --- kern/vfs_lookup.c 29 Jul 2019 12:35:19 -0000 1.82
> +++ kern/vfs_lookup.c 3 Sep 2019 19:16:15 -0000
> @@ -652,6 +652,12 @@ dirloop:
> ndp->ni_vp = dp = tdp;
> }
>
> + /* In case we were sleeping, file system may be unmounted. */
> + if (dp->v_type == VBAD) {
> + error = ENOENT;
> + goto bad2;
> + }
> +
> /*
> * Check for symbolic link. Back up over any slashes that we skipped,
> * as we will need them again.
>
