[PATCH 0/3] KVM: PPC: KVM module exit fixes

[PATCH 0/3] KVM: PPC: KVM module exit fixes

[Fabiano Rosas]

This is a resend the module cleanup fixes but this time without the
HV/PR merge.

Fabiano Rosas (1):
  KVM: PPC: Book3S HV: Check return value of kvmppc_radix_init
  KVM: PPC: Book3S HV: Delay setting of kvm ops
  KVM: PPC: Book3S HV: Free allocated memory if module init fails

 arch/powerpc/kvm/book3s_hv.c | 28 ++++++++++++++++++++--------
 1 file changed, 20 insertions(+), 8 deletions(-)

-- 
2.33.1

[PATCH 1/3] KVM: PPC: Book3S HV: Check return value of kvmppc_radix_init

[Fabiano Rosas]

The return of the function is being shadowed by the call to
kvmppc_uvmem_init.

Fixes: ca9f4942670c ("KVM: PPC: Book3S HV: Support for running secure guests")
Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
---
 arch/powerpc/kvm/book3s_hv.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
index 7b74fc0a986b..9f4765951733 100644
--- a/arch/powerpc/kvm/book3s_hv.c
+++ b/arch/powerpc/kvm/book3s_hv.c
@@ -6098,8 +6098,11 @@ static int kvmppc_book3s_init_hv(void)
 	if (r)
 		return r;
 
-	if (kvmppc_radix_possible())
+	if (kvmppc_radix_possible()) {
 		r = kvmppc_radix_init();
+		if (r)
+			return r;
+	}
 
 	r = kvmppc_uvmem_init();
 	if (r < 0)
-- 
2.33.1

Re: [PATCH 1/3] KVM: PPC: Book3S HV: Check return value of kvmppc_radix_init

[Nicholas Piggin]

Excerpts from Fabiano Rosas's message of December 24, 2021 7:19 am:
> The return of the function is being shadowed by the call to
> kvmppc_uvmem_init.
> 

Reviewed-by: Nicholas Piggin <npiggin@gmail.com>

> Fixes: ca9f4942670c ("KVM: PPC: Book3S HV: Support for running secure guests")
> Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
> ---
>  arch/powerpc/kvm/book3s_hv.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
> index 7b74fc0a986b..9f4765951733 100644
> --- a/arch/powerpc/kvm/book3s_hv.c
> +++ b/arch/powerpc/kvm/book3s_hv.c
> @@ -6098,8 +6098,11 @@ static int kvmppc_book3s_init_hv(void)
>  	if (r)
>  		return r;
>  
> -	if (kvmppc_radix_possible())
> +	if (kvmppc_radix_possible()) {
>  		r = kvmppc_radix_init();
> +		if (r)
> +			return r;
> +	}
>  
>  	r = kvmppc_uvmem_init();
>  	if (r < 0)
> -- 
> 2.33.1
> 
> 

[PATCH 2/3] KVM: PPC: Book3S HV: Delay setting of kvm ops

[Fabiano Rosas]

Delay the setting of kvm_hv_ops until after all init code has
completed. This avoids leaving the ops still accessible if the init
fails.

Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
---
 arch/powerpc/kvm/book3s_hv.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
index 9f4765951733..53400932f5d8 100644
--- a/arch/powerpc/kvm/book3s_hv.c
+++ b/arch/powerpc/kvm/book3s_hv.c
@@ -6087,9 +6087,6 @@ static int kvmppc_book3s_init_hv(void)
 	}
 #endif
 
-	kvm_ops_hv.owner = THIS_MODULE;
-	kvmppc_hv_ops = &kvm_ops_hv;
-
 	init_default_hcalls();
 
 	init_vcore_lists();
@@ -6105,10 +6102,15 @@ static int kvmppc_book3s_init_hv(void)
 	}
 
 	r = kvmppc_uvmem_init();
-	if (r < 0)
+	if (r < 0) {
 		pr_err("KVM-HV: kvmppc_uvmem_init failed %d\n", r);
+		return r;
+	}
 
-	return r;
+	kvm_ops_hv.owner = THIS_MODULE;
+	kvmppc_hv_ops = &kvm_ops_hv;
+
+	return 0;
 }
 
 static void kvmppc_book3s_exit_hv(void)
-- 
2.33.1

Re: [PATCH 2/3] KVM: PPC: Book3S HV: Delay setting of kvm ops

[Nicholas Piggin]

Excerpts from Fabiano Rosas's message of December 24, 2021 7:19 am:
> Delay the setting of kvm_hv_ops until after all init code has
> completed. This avoids leaving the ops still accessible if the init
> fails.
> 
> Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>

Also looks okay to me but KVM init has lots of details. IIRC Alexey may 
have run into a related issue with ops being set too early (or was it 
cleared too late?)

Thanks,
Nick

> ---
>  arch/powerpc/kvm/book3s_hv.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
> index 9f4765951733..53400932f5d8 100644
> --- a/arch/powerpc/kvm/book3s_hv.c
> +++ b/arch/powerpc/kvm/book3s_hv.c
> @@ -6087,9 +6087,6 @@ static int kvmppc_book3s_init_hv(void)
>  	}
>  #endif
>  
> -	kvm_ops_hv.owner = THIS_MODULE;
> -	kvmppc_hv_ops = &kvm_ops_hv;
> -
>  	init_default_hcalls();
>  
>  	init_vcore_lists();
> @@ -6105,10 +6102,15 @@ static int kvmppc_book3s_init_hv(void)
>  	}
>  
>  	r = kvmppc_uvmem_init();
> -	if (r < 0)
> +	if (r < 0) {
>  		pr_err("KVM-HV: kvmppc_uvmem_init failed %d\n", r);
> +		return r;
> +	}
>  
> -	return r;
> +	kvm_ops_hv.owner = THIS_MODULE;
> +	kvmppc_hv_ops = &kvm_ops_hv;
> +
> +	return 0;
>  }
>  
>  static void kvmppc_book3s_exit_hv(void)
> -- 
> 2.33.1
> 
> 

Re: [PATCH 2/3] KVM: PPC: Book3S HV: Delay setting of kvm ops

[Fabiano Rosas]

Nicholas Piggin <npiggin@gmail.com> writes:

> Excerpts from Fabiano Rosas's message of December 24, 2021 7:19 am:
>> Delay the setting of kvm_hv_ops until after all init code has
>> completed. This avoids leaving the ops still accessible if the init
>> fails.
>> 
>> Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
>> ---
>>  arch/powerpc/kvm/book3s_hv.c | 12 +++++++-----
>>  1 file changed, 7 insertions(+), 5 deletions(-)
>> 
>> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
>> index 9f4765951733..53400932f5d8 100644
>> --- a/arch/powerpc/kvm/book3s_hv.c
>> +++ b/arch/powerpc/kvm/book3s_hv.c
>> @@ -6087,9 +6087,6 @@ static int kvmppc_book3s_init_hv(void)
>>  	}
>>  #endif
>>  
>> -	kvm_ops_hv.owner = THIS_MODULE;
>> -	kvmppc_hv_ops = &kvm_ops_hv;
>> -
>>  	init_default_hcalls();
>>  
>>  	init_vcore_lists();
>> @@ -6105,10 +6102,15 @@ static int kvmppc_book3s_init_hv(void)
>>  	}
>>  
>>  	r = kvmppc_uvmem_init();
>> -	if (r < 0)
>> +	if (r < 0) {
>>  		pr_err("KVM-HV: kvmppc_uvmem_init failed %d\n", r);
>> +		return r;
>> +	}
>>  
>> -	return r;
>> +	kvm_ops_hv.owner = THIS_MODULE;
>> +	kvmppc_hv_ops = &kvm_ops_hv;
>> +
>> +	return 0;
>>  }
>>  
>>  static void kvmppc_book3s_exit_hv(void)
>> -- 
>> 2.33.1
>> 
>> 
>
> Also looks okay to me but KVM init has lots of details. IIRC Alexey may 
> have run into a related issue with ops being set too early (or was it 
> cleared too late?)
>
> Thanks,
> Nick
>

CC Alexey

[PATCH 3/3] KVM: PPC: Book3S HV: Free allocated memory if module init fails

[Fabiano Rosas]

The module's exit function is not called when the init fails, we need
to do cleanup before returning.

Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
---
 arch/powerpc/kvm/book3s_hv.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
index 53400932f5d8..2d79298e7ca4 100644
--- a/arch/powerpc/kvm/book3s_hv.c
+++ b/arch/powerpc/kvm/book3s_hv.c
@@ -6065,7 +6065,7 @@ static int kvmppc_book3s_init_hv(void)
 
 	r = kvm_init_subcore_bitmap();
 	if (r)
-		return r;
+		goto err;
 
 	/*
 	 * We need a way of accessing the XICS interrupt controller,
@@ -6080,7 +6080,8 @@ static int kvmppc_book3s_init_hv(void)
 		np = of_find_compatible_node(NULL, NULL, "ibm,opal-intc");
 		if (!np) {
 			pr_err("KVM-HV: Cannot determine method for accessing XICS\n");
-			return -ENODEV;
+			r = -ENODEV;
+			goto err;
 		}
 		/* presence of intc confirmed - node can be dropped again */
 		of_node_put(np);
@@ -6093,12 +6094,12 @@ static int kvmppc_book3s_init_hv(void)
 
 	r = kvmppc_mmu_hv_init();
 	if (r)
-		return r;
+		goto err;
 
 	if (kvmppc_radix_possible()) {
 		r = kvmppc_radix_init();
 		if (r)
-			return r;
+			goto err;
 	}
 
 	r = kvmppc_uvmem_init();
@@ -6111,6 +6112,12 @@ static int kvmppc_book3s_init_hv(void)
 	kvmppc_hv_ops = &kvm_ops_hv;
 
 	return 0;
+
+err:
+	kvmhv_nested_exit();
+	kvmppc_radix_exit();
+
+	return r;
 }
 
 static void kvmppc_book3s_exit_hv(void)
-- 
2.33.1

Re: [PATCH 3/3] KVM: PPC: Book3S HV: Free allocated memory if module init fails

[Nicholas Piggin]

Excerpts from Fabiano Rosas's message of December 24, 2021 7:19 am:
> The module's exit function is not called when the init fails, we need
> to do cleanup before returning.
> 
> Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
> ---
>  arch/powerpc/kvm/book3s_hv.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
> index 53400932f5d8..2d79298e7ca4 100644
> --- a/arch/powerpc/kvm/book3s_hv.c
> +++ b/arch/powerpc/kvm/book3s_hv.c
> @@ -6065,7 +6065,7 @@ static int kvmppc_book3s_init_hv(void)
>  
>  	r = kvm_init_subcore_bitmap();
>  	if (r)
> -		return r;
> +		goto err;
>  
>  	/*
>  	 * We need a way of accessing the XICS interrupt controller,
> @@ -6080,7 +6080,8 @@ static int kvmppc_book3s_init_hv(void)
>  		np = of_find_compatible_node(NULL, NULL, "ibm,opal-intc");
>  		if (!np) {
>  			pr_err("KVM-HV: Cannot determine method for accessing XICS\n");
> -			return -ENODEV;
> +			r = -ENODEV;
> +			goto err;
>  		}
>  		/* presence of intc confirmed - node can be dropped again */
>  		of_node_put(np);
> @@ -6093,12 +6094,12 @@ static int kvmppc_book3s_init_hv(void)
>  
>  	r = kvmppc_mmu_hv_init();
>  	if (r)
> -		return r;
> +		goto err;
>  
>  	if (kvmppc_radix_possible()) {
>  		r = kvmppc_radix_init();
>  		if (r)
> -			return r;
> +			goto err;
>  	}
>  
>  	r = kvmppc_uvmem_init();
> @@ -6111,6 +6112,12 @@ static int kvmppc_book3s_init_hv(void)
>  	kvmppc_hv_ops = &kvm_ops_hv;
>  
>  	return 0;
> +
> +err:
> +	kvmhv_nested_exit();
> +	kvmppc_radix_exit();

These should both be callable without init functions succeeding
so this looks right to me.

Reviewed-by: Nicholas Piggin <npiggin@gmail.com>

Thanks,
Nick

> +
> +	return r;
>  }
>  
>  static void kvmppc_book3s_exit_hv(void)
> -- 
> 2.33.1
> 
> 

Re: [PATCH 0/3] KVM: PPC: KVM module exit fixes

[Michael Ellerman]

On Thu, 23 Dec 2021 18:19:28 -0300, Fabiano Rosas wrote:
> This is a resend the module cleanup fixes but this time without the
> HV/PR merge.
> 
> Fabiano Rosas (1):
>   KVM: PPC: Book3S HV: Check return value of kvmppc_radix_init
>   KVM: PPC: Book3S HV: Delay setting of kvm ops
>   KVM: PPC: Book3S HV: Free allocated memory if module init fails
> 
> [...]

Applied to powerpc/topic/ppc-kvm.

[1/3] KVM: PPC: Book3S HV: Check return value of kvmppc_radix_init
      https://git.kernel.org/powerpc/c/69ab6ac380a00244575de02c406dcb9491bf3368
[2/3] KVM: PPC: Book3S HV: Delay setting of kvm ops
      https://git.kernel.org/powerpc/c/c5d0d77b45265905bba2ce6e63c9a02bbd11c43c
[3/3] KVM: PPC: Book3S HV: Free allocated memory if module init fails
      https://git.kernel.org/powerpc/c/175be7e5800e2782a7e38ee9e1b64633494c4b44

cheers

Re: [PATCH 0/3] KVM: PPC: KVM module exit fixes

[Michael Ellerman]

Michael Ellerman <patch-notifications@ellerman.id.au> writes:
> On Thu, 23 Dec 2021 18:19:28 -0300, Fabiano Rosas wrote:
>> This is a resend the module cleanup fixes but this time without the
>> HV/PR merge.
>> 
>> Fabiano Rosas (1):
>>   KVM: PPC: Book3S HV: Check return value of kvmppc_radix_init
>>   KVM: PPC: Book3S HV: Delay setting of kvm ops
>>   KVM: PPC: Book3S HV: Free allocated memory if module init fails
>> 
>> [...]
>
> Applied to powerpc/topic/ppc-kvm.
>
> [1/3] KVM: PPC: Book3S HV: Check return value of kvmppc_radix_init
>       https://git.kernel.org/powerpc/c/69ab6ac380a00244575de02c406dcb9491bf3368
> [2/3] KVM: PPC: Book3S HV: Delay setting of kvm ops
>       https://git.kernel.org/powerpc/c/c5d0d77b45265905bba2ce6e63c9a02bbd11c43c
> [3/3] KVM: PPC: Book3S HV: Free allocated memory if module init fails
>       https://git.kernel.org/powerpc/c/175be7e5800e2782a7e38ee9e1b64633494c4b44

These commits are actually of the v4 patches, the thanks email script
got confused.

See: https://lore.kernel.org/all/164708144610.832402.1913966629226492244.b4-ty@ellerman.id.au/

cheers