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

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

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

Signed-off-by: Fabiano Rosas <redacted>
---
 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

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 <redacted>
---
 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

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 <redacted>
---
 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

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>

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 <redacted>

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

Excerpts from Fabiano Rosas's message of December 24, 2021 7:19 am:

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

Nicholas Piggin [off-list ref] writes:

Excerpts from Fabiano Rosas's message of December 24, 2021 7:19 am:

quoted

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 <redacted>
---
 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

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

Michael Ellerman [off-list ref] writes:

On Thu, 23 Dec 2021 18:19:28 -0300, Fabiano Rosas wrote:

quoted

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