On 21/02/2019 17:26, Michael Ellerman wrote: > Alexey Kardashevskiy <aik@xxxxxxxxx> writes: > >> The anon fd's ops releases the KVM reference in the release hook. >> However we reference the KVM object after we create the fd so there is >> small window when the release function can be called and >> dereferenced the KVM object which potentially may free it. > dereference >> >> It is not a problem at the moment as the file is created and KVM is >> referenced under the KVM lock and the release function obtains the same >> lock before dereferencing the KVM (although the lock is not held when >> calling kvm_put_kvm()) but it is a fragile against future changes. >> >> This references the KVM object before creating a file. >> >> Signed-off-by: Alexey Kardashevskiy <aik@xxxxxxxxx> >> --- >> >> The original bug is described here: >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=cfa393811 >> But in this case kvm_put_kvm() is called straight away with no locks before/after/around. >> >> --- >> arch/powerpc/kvm/book3s_64_vio.c | 7 ++++--- >> 1 file changed, 4 insertions(+), 3 deletions(-) >> >> diff --git a/arch/powerpc/kvm/book3s_64_vio.c b/arch/powerpc/kvm/book3s_64_vio.c >> index 532ab797..d68c969 100644 >> --- a/arch/powerpc/kvm/book3s_64_vio.c >> +++ b/arch/powerpc/kvm/book3s_64_vio.c >> @@ -338,14 +338,15 @@ long kvm_vm_ioctl_create_spapr_tce(struct kvm *kvm, >> } >> } >> >> + kvm_get_kvm(kvm); >> if (!ret) >> ret = anon_inode_getfd("kvm-spapr-tce", &kvm_spapr_tce_fops, >> stt, O_RDWR | O_CLOEXEC); >> >> - if (ret >= 0) { >> + if (ret >= 0) >> list_add_rcu(&stt->list, &kvm->arch.spapr_tce_tables); >> - kvm_get_kvm(kvm); >> - } >> + else >> + kvm_put_kvm(kvm); >> >> mutex_unlock(&kvm->lock); > > This looks correct to me. But I feel like the logic could be cleaner, > perhaps like this (patch below): And I feel that 1) your patch tries to hide what it actually does 2) having 2 unlocks for one lock is an invite for future bugs imho, I'd think the whole point of adding new gotos is exactly to have 1 unlock. > > mutex_lock(&kvm->lock); > > /* Check this LIOBN hasn't been previously allocated */ > list_for_each_entry(siter, &kvm->arch.spapr_tce_tables, list) { > if (siter->liobn == args->liobn) { > ret = -EBUSY; > goto fail_unlock; > } > } > > kvm_get_kvm(kvm); > ret = anon_inode_getfd("kvm-spapr-tce", &kvm_spapr_tce_fops, > stt, O_RDWR | O_CLOEXEC); > > if (ret < 0) { > kvm_put_kvm(kvm); > goto fail_unlock; > } > > list_add_rcu(&stt->list, &kvm->arch.spapr_tce_tables); > > mutex_unlock(&kvm->lock); > > return ret; > > fail_unlock: > mutex_unlock(&kvm->lock); > fail: > > > cheers > > diff --git a/arch/powerpc/kvm/book3s_64_vio.c b/arch/powerpc/kvm/book3s_64_vio.c > index 532ab79734c7..5d74602db0d0 100644 > --- a/arch/powerpc/kvm/book3s_64_vio.c > +++ b/arch/powerpc/kvm/book3s_64_vio.c > @@ -296,7 +296,7 @@ long kvm_vm_ioctl_create_spapr_tce(struct kvm *kvm, > struct kvmppc_spapr_tce_table *stt = NULL; > struct kvmppc_spapr_tce_table *siter; > unsigned long npages, size = args->size; > - int ret = -ENOMEM; > + int ret; > int i; > > if (!args->size || args->page_shift < 12 || args->page_shift > 34 || > @@ -330,28 +330,30 @@ long kvm_vm_ioctl_create_spapr_tce(struct kvm *kvm, > mutex_lock(&kvm->lock); > > /* Check this LIOBN hasn't been previously allocated */ > - ret = 0; > list_for_each_entry(siter, &kvm->arch.spapr_tce_tables, list) { > if (siter->liobn == args->liobn) { > ret = -EBUSY; > - break; > + goto fail_unlock; > } > } > > - if (!ret) > - ret = anon_inode_getfd("kvm-spapr-tce", &kvm_spapr_tce_fops, > - stt, O_RDWR | O_CLOEXEC); > + kvm_get_kvm(kvm); > + ret = anon_inode_getfd("kvm-spapr-tce", &kvm_spapr_tce_fops, > + stt, O_RDWR | O_CLOEXEC); > > - if (ret >= 0) { > - list_add_rcu(&stt->list, &kvm->arch.spapr_tce_tables); > - kvm_get_kvm(kvm); > + if (ret < 0) { > + kvm_put_kvm(kvm); > + goto fail_unlock; > } > > + list_add_rcu(&stt->list, &kvm->arch.spapr_tce_tables); > + > mutex_unlock(&kvm->lock); > > - if (ret >= 0) > - return ret; > + return ret; > > + fail_unlock: > + mutex_unlock(&kvm->lock); > fail: > for (i = 0; i < npages; i++) > if (stt->pages[i]) > -- Alexey
