On Tue, May 24, 2022, Alexey Kardashevskiy wrote: > There are 2 places calling kvm_device_ops::destroy(): > 1) when creating a KVM device failed; > 2) when a VM is destroyed: kvm_destroy_devices() destroys all devices > from &kvm->devices. > > All 3 Book3s's interrupt controller KVM devices (XICS, XIVE, XIVE-native) > do not define kvm_device_ops::destroy() and only define release() which > is normally fine as device fds are closed before KVM gets to 2) but > by then the &kvm->devices list is empty. > > However Syzkaller manages to trigger 1). > > This adds checks in 1) and 2). > > Signed-off-by: Alexey Kardashevskiy <aik@xxxxxxxxx> > --- > > I could define empty handlers for XICS/XIVE guys but > kvm_ioctl_create_device() already checks for ops->init() so I guess > kvm_device_ops are expected to not have certain handlers. Oof. IMO, ->destroy() should be mandatory in order to pair with ->create(). kvmppc_xive_create(), kvmppc_xics_create(), and kvmppc_core_destroy_vm() are doing some truly funky stuff to avoid leaking the device that's allocate in ->create(). A nop/dummy ->destroy() would be a good place to further document those shenanigans. There's a comment at the end of the ->release() hooks, but that's still not very obvious. The comment above kvmppc_xive_get_device() strongly suggests that keeping the allocation is a hack to avoid having to audit all relevant code paths, i.e. isn't done for performance reasons.
