Fix use-after-free of zfsvfs after unmount

[EchterAgo]

During unmount zfsvfs is destroyed and the pointer is zeroed in the VCB. There is however still a copy of the pointer in the DCB.

Windows can still call into zfs_AcquireForLazyWrite, through CcMgr after unmount and this would use the already freed zfsvfs pointer.

To fix this we set the pointer to zero in the DCB and add a zero check in zfs_AcquireForLazyWrite, zfs_AcquireForReadAhead and fastio_acquire_for_mod_write.

fixes #282

[EchterAgo]

With this the tests.py with no delay finally passes consistently.

[EchterAgo]

Should we include removing the delays from the tests in this PR or in #286?

[EchterAgo]

This test can be used to easily reproduce the error:

fsutil file createnew C:\testfolder\poolfile.bin 200000000

:hans
zpool create tank \\?\C:\testfolder\poolfile.bin
zpool destroy tank
goto hans

del C:\testfolder\poolfile.bin

[andrewc12]

I don't think it matters as long as they get merged with or after this one gets merged

[EchterAgo]

Yay, test5_py passed!

[EchterAgo]

Hmmm, while unlikely to occur, I think there is an issue with this, zfs_AcquireForLazyWrite could still be called between freeing the VCB zfsvfs and setting it to zero in DCB. The mutex does not guard against this because it is in zfsvfs. Maybe we should set a flag in mount?

[EchterAgo]

We also can't just vfs_setfsprivate(zmo_dcb, NULL); before zfs_vfs_unmount because it could fail. It also looks like if zfs_vfs_unmount fails things are already in a bad state. We could check zmo_dcb->vpb->Flags & VPB_MOUNTED in zfs_AcquireForLazyWrite

[EchterAgo]

Updated to add VPB_MOUNTED check, I ran tests.py in a loop for a while and didn't see any problems.

[EchterAgo]

Sorry, converting to draft because I think my last change is wrong, checking the VPB_MOUNTED flag will likely prevent data from flushing in zfs_vfs_unmount.

[EchterAgo]

We should also have a test for flushing on unmount, I'll try to add one.

[EchterAgo]

Ideally we'd set both zmo and zmo_dcbs fsprivate to zero before freeing it, but at the same location, at the end of zfs_vfs_unmount. I'll push such a change soon.

[EchterAgo]

Updated, I think that is the best I can come up with :\

[lundman]

04 ffffd086dde6e8e0 fffff80021f1e6e8 nt!ObReferenceObjectByPointer+0x21bec3
05 ffffd086dde6e920 fffff80022220a3e OpenZFS!vflush+0x168 [H:\dev\openzfs\module\os\windows\spl\spl-vnode.c @ 1604]

This is unrelated, and has been a thorn for quite some time.

[EchterAgo]

This is unrelated, and has been a thorn for quite some time.

Yea, I found #95 which seems to be the same. I added my info there.

[EchterAgo]

I think there are a bunch more locations that might need a NULL check, I have a change for that too, but haven't tested it enough yet.

Better to have a NULL dereference than having to chase a use-after-free though.

[lundman]

Generally, all requests to ZFS come in via dispatcher(). There are 3 exceptions, guess which ones they are :)

[lundman]

./module/os/windows/zfs/zfs_vnops_windows.c: 7965: continuation should be indented 4 spaces

[EchterAgo]

Fix for checkstyle