From: Alexey Kardashevskiy <aik@ozlabs.ru> To: Paolo Bonzini <pbonzini@redhat.com>, Sean Christopherson <seanjc@google.com> Cc: kvm@vger.kernel.org, kvm-ppc@vger.kernel.org Subject: Re: [PATCH kernel] KVM: Don't null dereference ops->destroy Date: Tue, 31 May 2022 14:32:18 +1000 [thread overview] Message-ID: <24c22c5c-2656-d590-2ae2-adfe0d3fd113@ozlabs.ru> (raw) In-Reply-To: <4fdbe38d-0e7d-764f-beab-034a9f172137@redhat.com> 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
WARNING: multiple messages have this Message-ID (diff)
From: Alexey Kardashevskiy <aik@ozlabs.ru> To: Paolo Bonzini <pbonzini@redhat.com>, Sean Christopherson <seanjc@google.com> Cc: kvm@vger.kernel.org, kvm-ppc@vger.kernel.org Subject: Re: [PATCH kernel] KVM: Don't null dereference ops->destroy Date: Tue, 31 May 2022 04:32:18 +0000 [thread overview] Message-ID: <24c22c5c-2656-d590-2ae2-adfe0d3fd113@ozlabs.ru> (raw) In-Reply-To: <4fdbe38d-0e7d-764f-beab-034a9f172137@redhat.com> 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
next prev parent reply other threads:[~2022-05-31 4:32 UTC|newest] Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top 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 [this message] 2022-05-31 4:32 ` Alexey Kardashevskiy 2022-05-31 17:13 ` Paolo Bonzini 2022-05-31 17:13 ` Paolo Bonzini
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=24c22c5c-2656-d590-2ae2-adfe0d3fd113@ozlabs.ru \ --to=aik@ozlabs.ru \ --cc=kvm-ppc@vger.kernel.org \ --cc=kvm@vger.kernel.org \ --cc=pbonzini@redhat.com \ --cc=seanjc@google.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
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.