On 5/25/22 11:52, Alexey Kardashevskiy wrote:
On 5/25/22 06:01, Sean Christopherson wrote:
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().
After reading
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=2bde9b3ec8bdf60788e9e
and neighboring commits, it sounds that each create() should be paired
with either destroy() or release() but not necessarily both.
So I'm really not sure dummy handlers is a good idea. Thanks,
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().
Huh it used to be release() actually, nice:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=5422e95103cf9663bc
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.
I could probably borrow some docs from here:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=6f868405faf067e8cfb
Thanks for the review! At very least I'll add the dummies.
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.
--
Alexey
