Re: [PATCH 1/6] KVM: PPC: Book3S HV: XIVE: initialize private pointer when VPs are allocated

[Greg Kurz]

On Tue, 24 Sep 2019 15:28:55 +1000
Paul Mackerras  wrote:

> On Mon, Sep 23, 2019 at 05:43:37PM +0200, Greg Kurz wrote:
> > From: Cédric Le Goater 
> > 
> > Do not assign the device private pointer before making sure the XIVE
> > VPs are allocated in OPAL and test pointer validity when releasing
> > the device.
> > 
> > Fixes: 5422e95103cf ("KVM: PPC: Book3S HV: XIVE: Replace the 'destroy' 
> > method by a 'release' method")
> > Signed-off-by: Cédric Le Goater 
> > Signed-off-by: Greg Kurz 
> 
> What happens in the case where the OPAL allocation fails?  Does the
> host crash, or hang, or leak resources?  I presume that users can
> trigger the allocation failure just by starting a suitably large
> number of guests - is that right?  Is there an easier way?  I'm trying
> to work out whether this is urgently needed in 5.4 and the stable
> trees or not.
> 

Wait... I don't quite remember how this patch landed in my tree but when
I look at it again I have the impression it tries to fix something that
cannot happen.

It is indeed easy to trigger the allocation failure, eg. start more than
127 guests on a Witherspoon system. But if this happens, the create
function returns an error and the device isn't created. I don't see how
the release function could hence get called with a "partially initialized"
device.

Please ignore this patch. Unfortunately the rest of the series doesn't
apply cleanly without it... I'll rebase and post a v2.

Sorry for the noise :-\

> Paul.

Re: [PATCH 1/6] KVM: PPC: Book3S HV: XIVE: initialize private pointer when VPs are allocated

[Paul Mackerras]

On Mon, Sep 23, 2019 at 05:43:37PM +0200, Greg Kurz wrote:
> From: Cédric Le Goater 
> 
> Do not assign the device private pointer before making sure the XIVE
> VPs are allocated in OPAL and test pointer validity when releasing
> the device.
> 
> Fixes: 5422e95103cf ("KVM: PPC: Book3S HV: XIVE: Replace the 'destroy' method 
> by a 'release' method")
> Signed-off-by: Cédric Le Goater 
> Signed-off-by: Greg Kurz 

What happens in the case where the OPAL allocation fails?  Does the
host crash, or hang, or leak resources?  I presume that users can
trigger the allocation failure just by starting a suitably large
number of guests - is that right?  Is there an easier way?  I'm trying
to work out whether this is urgently needed in 5.4 and the stable
trees or not.

Paul.

[PATCH 1/6] KVM: PPC: Book3S HV: XIVE: initialize private pointer when VPs are allocated

[Greg Kurz]

From: Cédric Le Goater 

Do not assign the device private pointer before making sure the XIVE
VPs are allocated in OPAL and test pointer validity when releasing
the device.

Fixes: 5422e95103cf ("KVM: PPC: Book3S HV: XIVE: Replace the 'destroy' method 
by a 'release' method")
Signed-off-by: Cédric Le Goater 
Signed-off-by: Greg Kurz 
---
 arch/powerpc/kvm/book3s_xive.c|   13 +++--
 arch/powerpc/kvm/book3s_xive_native.c |   13 +++--
 2 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/kvm/book3s_xive.c b/arch/powerpc/kvm/book3s_xive.c
index 591bfb4bfd0f..cd2006bfcd3e 100644
--- a/arch/powerpc/kvm/book3s_xive.c
+++ b/arch/powerpc/kvm/book3s_xive.c
@@ -1897,12 +1897,21 @@ void kvmppc_xive_free_sources(struct 
kvmppc_xive_src_block *sb)
 static void kvmppc_xive_release(struct kvm_device *dev)
 {
struct kvmppc_xive *xive = dev->private;
-   struct kvm *kvm = xive->kvm;
+   struct kvm *kvm;
struct kvm_vcpu *vcpu;
int i;
 
pr_devel("Releasing xive device\n");
 
+   /*
+* In case of failure (OPAL) at device creation, the device
+* can be partially initialized.
+*/
+   if (!xive)
+   return;
+
+   kvm = xive->kvm;
+
/*
 * Since this is the device release function, we know that
 * userspace does not have any open fd referring to the
@@ -2001,7 +2010,6 @@ static int kvmppc_xive_create(struct kvm_device *dev, u32 
type)
if (!xive)
return -ENOMEM;
 
-   dev->private = xive;
xive->dev = dev;
xive->kvm = kvm;
mutex_init(>lock);
@@ -2031,6 +2039,7 @@ static int kvmppc_xive_create(struct kvm_device *dev, u32 
type)
if (ret)
return ret;
 
+   dev->private = xive;
return 0;
 }
 
diff --git a/arch/powerpc/kvm/book3s_xive_native.c 
b/arch/powerpc/kvm/book3s_xive_native.c
index 248c1ea9e788..e9cbb42de424 100644
--- a/arch/powerpc/kvm/book3s_xive_native.c
+++ b/arch/powerpc/kvm/book3s_xive_native.c
@@ -987,12 +987,21 @@ static int kvmppc_xive_native_has_attr(struct kvm_device 
*dev,
 static void kvmppc_xive_native_release(struct kvm_device *dev)
 {
struct kvmppc_xive *xive = dev->private;
-   struct kvm *kvm = xive->kvm;
+   struct kvm *kvm;
struct kvm_vcpu *vcpu;
int i;
 
pr_devel("Releasing xive native device\n");
 
+   /*
+* In case of failure (OPAL) at device creation, the device
+* can be partially initialized.
+*/
+   if (!xive)
+   return;
+
+   kvm = xive->kvm;
+
/*
 * Clear the KVM device file address_space which is used to
 * unmap the ESB pages when a device is passed-through.
@@ -1076,7 +1085,6 @@ static int kvmppc_xive_native_create(struct kvm_device 
*dev, u32 type)
if (!xive)
return -ENOMEM;
 
-   dev->private = xive;
xive->dev = dev;
xive->kvm = kvm;
kvm->arch.xive = xive;
@@ -1100,6 +1108,7 @@ static int kvmppc_xive_native_create(struct kvm_device 
*dev, u32 type)
if (ret)
return ret;
 
+   dev->private = xive;
return 0;
 }