vmm: take exclusive mem_segs_lock in vm_cleanup()

[rew]

Summary

Perhaps vm_cleanup() should always be called with mem_segs_lock locked
but, if the vm is being destroyed...vm_cleanup() will also destroy the
mem_segs_lock.

Unread portion of the kernel message buffer:
panic: Lock (sx) vm mem_segs not locked @ /cobra/src/sys/amd64/vmm/vmm.c:1188.
cpuid = 6
time = 1673288410
KDB: stack backtrace:
db_trace_self_wrapper() at db_trace_self_wrapper+0x2b/frame 0xfffffe01116d1890
vpanic() at vpanic+0x151/frame 0xfffffe01116d18e0
panic() at panic+0x43/frame 0xfffffe01116d1940
witness_assert() at witness_assert+0x21e/frame 0xfffffe01116d1970
vm_iommu_modify() at vm_iommu_modify+0x135/frame 0xfffffe01116d19d0
vm_unassign_pptdev() at vm_unassign_pptdev+0x2b/frame 0xfffffe01116d19f0
ppt_unassign_all() at ppt_unassign_all+0x176/frame 0xfffffe01116d1a40
vm_cleanup() at vm_cleanup+0x16/frame 0xfffffe01116d1a70
vm_destroy() at vm_destroy+0x13/frame 0xfffffe01116d1a90
vmmdev_destroy() at vmmdev_destroy+0x90/frame 0xfffffe01116d1ab0
sysctl_vmm_destroy() at sysctl_vmm_destroy+0x1de/frame 0xfffffe01116d1af0
sysctl_root_handler_locked() at sysctl_root_handler_locked+0x9c/frame 0xfffffe01116d1b40
sysctl_root() at sysctl_root+0x21c/frame 0xfffffe01116d1bc0
userland_sysctl() at userland_sysctl+0x187/frame 0xfffffe01116d1c70
kern___sysctlbyname() at kern___sysctlbyname+0x21d/frame 0xfffffe01116d1dc0
sys___sysctlbyname() at sys___sysctlbyname+0x2d/frame 0xfffffe01116d1e00
amd64_syscall() at amd64_syscall+0x12e/frame 0xfffffe01116d1f30
fast_syscall_common() at fast_syscall_common+0xf8/frame 0xfffffe01116d1f30

• syscall (570, FreeBSD ELF64, __sysctlbyname), rip = 0x26e59d2a480a, rsp = 0x26e59a291f78, rbp = 0x26e59a291f90 ---

KDB: enter: panic

[markj]

sys/amd64/vmm/vmm.c

655

Why isn't the lock needed in the !destroy case? ppt_unassign_all() is called unconditionally.

696

Formally, the lock isn't needed here. Access to the memseg array should be serialized since we're destroying the VM, so this looks a bit strange.

[rew]

sys/amd64/vmm/vmm.c

655

The lock is already taken when entering vm_cleanup() for the !destroy case. The lock is taken in vmmdev_ioctl().

vm_cleanup() is called from two places, vm_reinit() and vm_destroy().

vmmdev_ioctl() takes the lock and calls vm_reinit() -> vm_cleanup(destroy=false).

vm_destroy() is called from the sysctl call path which looks like:

sysctl_vmm_destroy() -> vmmdev_destroy() -> vm_destroy() -> vm_cleanup(destroy=true)

I considered taking the lock in vm_destroy() or vmmdev_destroy() so that vm_cleanup() would consistently be called with memsegs locked - but I didn't see a strong reason to push the lock up higher.

I also thought about factoring the destroy argument out of vm_cleanup() but, that was gonna be a bigger change.

696

In other parts of the code (see VM_ALLOC_MEMSEG) the lock is held across vm_free_memseg().

So I ended up holding the lock across vm_free_memseg() to be consistent with how it was used elsewhere. It also seemed logical to hold the lock while freeing mem_maps/mem_segs but, maybe not?

Where do you think the unlock should take place?

[markj]

sys/amd64/vmm/vmm.c

655

I see, thanks. I agree that there isn't really a cleaner way to handle this, at least not without some churn. I was going to suggest acquiring the lock in vm_destroy() and letting the sx_destroy() call "unlock" the mutex, but sx_destroy() asserts that the lock is unowned, unlike mtx_destroy().

696

It looks a bit strange only because the lock is destroyed a few lines down. Therefore, the lock must be uncontended, otherwise we'd get a kernel crash. But I don't see any harm in it.

You could instead unlock right before destroying the lock but it doesn't really matter.