Re: [PATCH kernel] KVM: PPC: Improve KVM reference counting

[Michael Ellerman <mpe@xxxxxxxxxxxxxx>]

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):

	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])