* [PATCH kernel] KVM: Don't null dereference ops->destroy
@ 2022-05-24 5:52 ` Alexey Kardashevskiy
0 siblings, 0 replies; 13+ messages in thread
From: Alexey Kardashevskiy @ 2022-05-24 5:52 UTC (permalink / raw)
To: kvm; +Cc: Alexey Kardashevskiy, Paolo Bonzini, kvm-ppc
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@ozlabs.ru>
---
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.
---
virt/kvm/kvm_main.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index f30bb8c16f26..17f698ccddd1 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -1205,7 +1205,8 @@ static void kvm_destroy_devices(struct kvm *kvm)
*/
list_for_each_entry_safe(dev, tmp, &kvm->devices, vm_node) {
list_del(&dev->vm_node);
- dev->ops->destroy(dev);
+ if (dev->ops->destroy)
+ dev->ops->destroy(dev);
}
}
@@ -4300,7 +4301,8 @@ static int kvm_ioctl_create_device(struct kvm *kvm,
mutex_lock(&kvm->lock);
list_del(&dev->vm_node);
mutex_unlock(&kvm->lock);
- ops->destroy(dev);
+ if (ops->destroy)
+ ops->destroy(dev);
return ret;
}
--
2.30.2
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH kernel] KVM: Don't null dereference ops->destroy
@ 2022-05-24 5:52 ` Alexey Kardashevskiy
0 siblings, 0 replies; 13+ messages in thread
From: Alexey Kardashevskiy @ 2022-05-24 5:52 UTC (permalink / raw)
To: kvm; +Cc: Alexey Kardashevskiy, Paolo Bonzini, kvm-ppc
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@ozlabs.ru>
---
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.
---
virt/kvm/kvm_main.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index f30bb8c16f26..17f698ccddd1 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -1205,7 +1205,8 @@ static void kvm_destroy_devices(struct kvm *kvm)
*/
list_for_each_entry_safe(dev, tmp, &kvm->devices, vm_node) {
list_del(&dev->vm_node);
- dev->ops->destroy(dev);
+ if (dev->ops->destroy)
+ dev->ops->destroy(dev);
}
}
@@ -4300,7 +4301,8 @@ static int kvm_ioctl_create_device(struct kvm *kvm,
mutex_lock(&kvm->lock);
list_del(&dev->vm_node);
mutex_unlock(&kvm->lock);
- ops->destroy(dev);
+ if (ops->destroy)
+ ops->destroy(dev);
return ret;
}
--
2.30.2
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH kernel] KVM: Don't null dereference ops->destroy
2022-05-24 5:52 ` Alexey Kardashevskiy
(?)
@ 2022-05-24 20:01 ` Sean Christopherson
2022-05-25 1:52 ` Alexey Kardashevskiy
-1 siblings, 1 reply; 13+ messages in thread
From: Sean Christopherson @ 2022-05-24 20:01 UTC (permalink / raw)
To: Alexey Kardashevskiy; +Cc: kvm, Paolo Bonzini, kvm-ppc
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@ozlabs.ru>
> ---
>
> 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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH kernel] KVM: Don't null dereference ops->destroy
2022-05-24 20:01 ` Sean Christopherson
@ 2022-05-25 1:52 ` Alexey Kardashevskiy
0 siblings, 0 replies; 13+ messages in thread
From: Alexey Kardashevskiy @ 2022-05-25 1:52 UTC (permalink / raw)
To: Sean Christopherson; +Cc: kvm, Paolo Bonzini, kvm-ppc
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@ozlabs.ru>
>> ---
>>
>> 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().
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH kernel] KVM: Don't null dereference ops->destroy
@ 2022-05-25 1:52 ` Alexey Kardashevskiy
0 siblings, 0 replies; 13+ messages in thread
From: Alexey Kardashevskiy @ 2022-05-25 1:52 UTC (permalink / raw)
To: Sean Christopherson; +Cc: kvm, Paolo Bonzini, kvm-ppc
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@ozlabs.ru>
>> ---
>>
>> 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().
Huh it used to be release() actually, nice:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?idT22e95103cf9663bc
> 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/?ido868405faf067e8cfb
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH kernel] KVM: Don't null dereference ops->destroy
2022-05-25 1:52 ` Alexey Kardashevskiy
@ 2022-05-25 2:58 ` Alexey Kardashevskiy
-1 siblings, 0 replies; 13+ messages in thread
From: Alexey Kardashevskiy @ 2022-05-25 2:58 UTC (permalink / raw)
To: Sean Christopherson; +Cc: kvm, Paolo Bonzini, kvm-ppc
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@ozlabs.ru>
>>> ---
>>>
>>> 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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH kernel] KVM: Don't null dereference ops->destroy
@ 2022-05-25 2:58 ` Alexey Kardashevskiy
0 siblings, 0 replies; 13+ messages in thread
From: Alexey Kardashevskiy @ 2022-05-25 2:58 UTC (permalink / raw)
To: Sean Christopherson; +Cc: kvm, Paolo Bonzini, kvm-ppc
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@ozlabs.ru>
>>> ---
>>>
>>> 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+de9b3ec8bdf60788e9e
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/?idT22e95103cf9663bc
>
>
>> 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/?ido868405faf067e8cfb
>
>
> 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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH kernel] KVM: Don't null dereference ops->destroy
2022-05-25 2:58 ` Alexey Kardashevskiy
@ 2022-05-25 7:47 ` Paolo Bonzini
-1 siblings, 0 replies; 13+ messages in thread
From: Paolo Bonzini @ 2022-05-25 7:47 UTC (permalink / raw)
To: Alexey Kardashevskiy, Sean Christopherson; +Cc: kvm, kvm-ppc
On 5/25/22 04:58, Alexey Kardashevskiy wrote:
>>>
>
>
> 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.
I agree, if release() is implemented then destroy() will never be called
(except in error situations).
kvm_destroy_devices() should not be touched, except to add a WARN_ON
perhaps.
> So I'm really not sure dummy handlers is a good idea. Thanks,
But in that case shouldn't kvm_ioctl_create_device also try
ops->release, i.e.
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 6d971fb1b08d..f265e2738d46 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -4299,8 +4299,11 @@ static int kvm_ioctl_create_device(struct kvm *kvm,
kvm_put_kvm_no_destroy(kvm);
mutex_lock(&kvm->lock);
list_del(&dev->vm_node);
+ if (ops->release)
+ ops->release(dev);
mutex_unlock(&kvm->lock);
- ops->destroy(dev);
+ if (ops->destroy)
+ ops->destroy(dev);
return ret;
}
?
Paolo
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH kernel] KVM: Don't null dereference ops->destroy
@ 2022-05-25 7:47 ` Paolo Bonzini
0 siblings, 0 replies; 13+ messages in thread
From: Paolo Bonzini @ 2022-05-25 7:47 UTC (permalink / raw)
To: Alexey Kardashevskiy, Sean Christopherson; +Cc: kvm, kvm-ppc
On 5/25/22 04:58, Alexey Kardashevskiy wrote:
>>>
>
>
> After reading
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id+de9b3ec8bdf60788e9e
> and neighboring commits, it sounds that each create() should be paired
> with either destroy() or release() but not necessarily both.
I agree, if release() is implemented then destroy() will never be called
(except in error situations).
kvm_destroy_devices() should not be touched, except to add a WARN_ON
perhaps.
> So I'm really not sure dummy handlers is a good idea. Thanks,
But in that case shouldn't kvm_ioctl_create_device also try
ops->release, i.e.
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 6d971fb1b08d..f265e2738d46 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -4299,8 +4299,11 @@ static int kvm_ioctl_create_device(struct kvm *kvm,
kvm_put_kvm_no_destroy(kvm);
mutex_lock(&kvm->lock);
list_del(&dev->vm_node);
+ if (ops->release)
+ ops->release(dev);
mutex_unlock(&kvm->lock);
- ops->destroy(dev);
+ if (ops->destroy)
+ ops->destroy(dev);
return ret;
}
?
Paolo
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH kernel] KVM: Don't null dereference ops->destroy
2022-05-25 7:47 ` Paolo Bonzini
@ 2022-05-31 4:32 ` Alexey Kardashevskiy
-1 siblings, 0 replies; 13+ messages in thread
From: Alexey Kardashevskiy @ 2022-05-31 4:32 UTC (permalink / raw)
To: Paolo Bonzini, Sean Christopherson; +Cc: kvm, kvm-ppc
On 5/25/22 17:47, Paolo Bonzini wrote:
> On 5/25/22 04:58, Alexey Kardashevskiy wrote:
>>>>
>>
>>
>> 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.
>
> I agree, if release() is implemented then destroy() will never be called
> (except in error situations).
>
> kvm_destroy_devices() should not be touched, except to add a WARN_ON
> perhaps.
I'll leave it as is.
>
>> So I'm really not sure dummy handlers is a good idea. Thanks,
>
> But in that case shouldn't kvm_ioctl_create_device also try
> ops->release, i.e.
>
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 6d971fb1b08d..f265e2738d46 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -4299,8 +4299,11 @@ static int kvm_ioctl_create_device(struct kvm *kvm,
> kvm_put_kvm_no_destroy(kvm);
> mutex_lock(&kvm->lock);
> list_del(&dev->vm_node);
> + if (ops->release)
> + ops->release(dev);
> mutex_unlock(&kvm->lock);
> - ops->destroy(dev);
> + if (ops->destroy)
> + ops->destroy(dev);
btw why is destroy() not under the kvm->lock here? The comment in
kvm_destroy_devices() suggests that it is an exception there but not
necessarily here. Thanks,
> return ret;
> }
>
> ?
>
> Paolo
>
--
Alexey
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH kernel] KVM: Don't null dereference ops->destroy
@ 2022-05-31 4:32 ` Alexey Kardashevskiy
0 siblings, 0 replies; 13+ messages in thread
From: Alexey Kardashevskiy @ 2022-05-31 4:32 UTC (permalink / raw)
To: Paolo Bonzini, Sean Christopherson; +Cc: kvm, kvm-ppc
On 5/25/22 17:47, Paolo Bonzini wrote:
> On 5/25/22 04:58, Alexey Kardashevskiy wrote:
>>>>
>>
>>
>> After reading
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id+de9b3ec8bdf60788e9e and neighboring commits, it sounds that each create() should be paired with either destroy() or release() but not necessarily both.
>
> I agree, if release() is implemented then destroy() will never be called
> (except in error situations).
>
> kvm_destroy_devices() should not be touched, except to add a WARN_ON
> perhaps.
I'll leave it as is.
>
>> So I'm really not sure dummy handlers is a good idea. Thanks,
>
> But in that case shouldn't kvm_ioctl_create_device also try
> ops->release, i.e.
>
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 6d971fb1b08d..f265e2738d46 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -4299,8 +4299,11 @@ static int kvm_ioctl_create_device(struct kvm *kvm,
> kvm_put_kvm_no_destroy(kvm);
> mutex_lock(&kvm->lock);
> list_del(&dev->vm_node);
> + if (ops->release)
> + ops->release(dev);
> mutex_unlock(&kvm->lock);
> - ops->destroy(dev);
> + if (ops->destroy)
> + ops->destroy(dev);
btw why is destroy() not under the kvm->lock here? The comment in
kvm_destroy_devices() suggests that it is an exception there but not
necessarily here. Thanks,
> return ret;
> }
>
> ?
>
> Paolo
>
--
Alexey
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH kernel] KVM: Don't null dereference ops->destroy
2022-05-31 4:32 ` Alexey Kardashevskiy
@ 2022-05-31 17:13 ` Paolo Bonzini
-1 siblings, 0 replies; 13+ messages in thread
From: Paolo Bonzini @ 2022-05-31 17:13 UTC (permalink / raw)
To: Alexey Kardashevskiy, Sean Christopherson; +Cc: kvm, kvm-ppc
On 5/31/22 06:32, Alexey Kardashevskiy wrote:
>> - ops->destroy(dev);
>> + if (ops->destroy)
>> + ops->destroy(dev);
>
>
> btw why is destroy() not under the kvm->lock here? The comment in
> kvm_destroy_devices() suggests that it is an exception there but not
> necessarily here. Thanks,
The comment refers to walking the list. The ops->destroy contract is
that it's called outside kvm->lock, and that's followed in both places
(both before and after the suggested patch).
Paolo
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH kernel] KVM: Don't null dereference ops->destroy
@ 2022-05-31 17:13 ` Paolo Bonzini
0 siblings, 0 replies; 13+ messages in thread
From: Paolo Bonzini @ 2022-05-31 17:13 UTC (permalink / raw)
To: Alexey Kardashevskiy, Sean Christopherson; +Cc: kvm, kvm-ppc
On 5/31/22 06:32, Alexey Kardashevskiy wrote:
>> - ops->destroy(dev);
>> + if (ops->destroy)
>> + ops->destroy(dev);
>
>
> btw why is destroy() not under the kvm->lock here? The comment in
> kvm_destroy_devices() suggests that it is an exception there but not
> necessarily here. Thanks,
The comment refers to walking the list. The ops->destroy contract is
that it's called outside kvm->lock, and that's followed in both places
(both before and after the suggested patch).
Paolo
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2022-05-31 17:14 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-24 5:52 [PATCH kernel] KVM: Don't null dereference ops->destroy Alexey Kardashevskiy
2022-05-24 5:52 ` Alexey Kardashevskiy
2022-05-24 20:01 ` Sean Christopherson
2022-05-25 1:52 ` Alexey Kardashevskiy
2022-05-25 1:52 ` Alexey Kardashevskiy
2022-05-25 2:58 ` Alexey Kardashevskiy
2022-05-25 2:58 ` Alexey Kardashevskiy
2022-05-25 7:47 ` Paolo Bonzini
2022-05-25 7:47 ` Paolo Bonzini
2022-05-31 4:32 ` Alexey Kardashevskiy
2022-05-31 4:32 ` Alexey Kardashevskiy
2022-05-31 17:13 ` Paolo Bonzini
2022-05-31 17:13 ` Paolo Bonzini
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.