Re: [PATCH kernel] KVM: Don't null dereference ops->destroy

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