[PATCH v4 0/2] s390/vfio-ap: fix memory leak in mdev remove callback

[PATCH v4 0/2] s390/vfio-ap: fix memory leak in mdev remove callback

[Tony Krowiak]

Fixes a memory leak in the mdev remove callback when invoked while the
mdev is in use by a KVM guest. Instead of returning -EBUSY from the
callback, a full cleanup of the resources allocated to the mdev is
performed because regardless of the value returned from the function, the
mdev is removed from sysfs.

The cleanup of resources allocated to the mdev may coincide with the 
interception of the PQAP(AQIC) instruction in which case data needed to
handle the interception may get removed. A patch is included in this series
to synchronize access to resources needed by the interception handler to
protect against invalid memory accesses.

The first pass (PATCH v3) at trying to synchronize access to the pqap
function pointer employed RCU. The problem is, the RCU read-side critical
section would have to include the execution of the pqap function which
sleeps; RCU disallows sleeping inside an RCU region. When I subsequently 
tried to encompass the pqap function within the 
rcu_read_lock/rcu_read_unlock, I ended up seeing lockdep warnings in the
syslog.

It was suggested that we use an rw_semaphore to synchronize access to 
the pqap hook, but I also ran into similar lockdep complaints something
like the following:

  Possible unsafe locking scenario:

        CPU0                            CPU1
        ----                            ----
   down_read(&rwsem)
   in handle_pqap (priv.c);   
   
                                  lock(&matrix_dev->lock);
                                  in vfio_ap_mdev_set_kvm (vfio_ap_ops.c)
                                
                                  down_write(&rwsem;
                                  in vfio_ap_mdev_set_kvm (vfio_ap_ops.c)
                                
   lock(&matrix_dev->lock);
   in handle_pqap(vfio_ap_ops.c)

Access to the mdev must be done under the matrix_dev->lock to ensure that
it doesn't get freed via the remove callback while in use. This appears
to be mutually exclusive with setting/unsetting the pqap_hook pointer
due to lockdep issues.

The solution:
------------
The lifetime of the handle_pqap function (vfio_ap_ops) is syncrhonous
with the lifetime of the vfio_ap module, so there really is not reason
to tie the setting/clearing of its function pointer with the lifetime
of a guest or even an mdev. If the function pointer is set when the
vfio_ap module is loaded and cleared when the vfio_ap module is unloaded,
then access to it can be protected independently from mdev creation or
removal as well as the starting or shutdown of a guest. As long as
access to the mdev is always controlled by the matrix_dev->lock, the 
mdev can not be freed without other functions being aware.

Change log:
v3 -> v4:
--------
* Created a registry for crypto hooks in priv.c with functions for
  registering/unregistering function pointers in kvm_host.h (for s390).

* Register the function pointer for handling the PQAP instruction when
  the vfio_ap module is loaded and unregister it when the module is 
  unloaded.

v2 -> v3:
--------
* Added a patch to control access to the PQAP(AQIC) hook using RCU

v1 -> v2:
--------
* Call vfio_ap_mdev_unset_kvm() function from the remove callback instead
  of merely clearing the guest's APCB.

Tony Krowiak (2):
  s390/vfio-ap: fix memory leak in mdev remove callback
  s390/vfio-ap: control access to PQAP(AQIC) interception handler

 arch/s390/include/asm/kvm_host.h      | 13 +++--
 arch/s390/kvm/priv.c                  | 70 +++++++++++++++++++++++++--
 drivers/s390/crypto/vfio_ap_ops.c     | 50 ++++++++++++-------
 drivers/s390/crypto/vfio_ap_private.h |  1 -
 4 files changed, 107 insertions(+), 27 deletions(-)

-- 
2.30.2

[PATCH v4 1/2] s390/vfio-ap: fix memory leak in mdev remove callback

[Tony Krowiak]

The mdev remove callback for the vfio_ap device driver bails out with
-EBUSY if the mdev is in use by a KVM guest. The intended purpose was
to prevent the mdev from being removed while in use; however, returning a
non-zero rc does not prevent removal. This could result in a memory leak
of the resources allocated when the mdev was created. In addition, the
KVM guest will still have access to the AP devices assigned to the mdev
even though the mdev no longer exists.

To prevent this scenario, cleanup will be done - including unplugging the
AP adapters, domains and control domains - regardless of whether the mdev
is in use by a KVM guest or not.

Fixes: 258287c994de ("s390: vfio-ap: implement mediated device open callback")
Cc: stable@vger.kernel.org
Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
---
 drivers/s390/crypto/vfio_ap_ops.c | 13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index b2c7e10dfdcd..f90c9103dac2 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -26,6 +26,7 @@
 
 static int vfio_ap_mdev_reset_queues(struct mdev_device *mdev);
 static struct vfio_ap_queue *vfio_ap_find_queue(int apqn);
+static void vfio_ap_mdev_unset_kvm(struct ap_matrix_mdev *matrix_mdev);
 
 static int match_apqn(struct device *dev, const void *data)
 {
@@ -366,17 +367,7 @@ static int vfio_ap_mdev_remove(struct mdev_device *mdev)
 	struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
 
 	mutex_lock(&matrix_dev->lock);
-
-	/*
-	 * If the KVM pointer is in flux or the guest is running, disallow
-	 * un-assignment of control domain.
-	 */
-	if (matrix_mdev->kvm_busy || matrix_mdev->kvm) {
-		mutex_unlock(&matrix_dev->lock);
-		return -EBUSY;
-	}
-
-	vfio_ap_mdev_reset_queues(mdev);
+	vfio_ap_mdev_unset_kvm(matrix_mdev);
 	list_del(&matrix_mdev->node);
 	kfree(matrix_mdev);
 	mdev_set_drvdata(mdev, NULL);
-- 
2.30.2

[PATCH v4 2/2] s390/vfio-ap: control access to PQAP(AQIC) interception handler

[Tony Krowiak]

The function pointer to the handler that processes interception of the
PQAP instruction is contained in the mdev. If the mdev is removed and
its storage de-allocated during the processing of the PQAP instruction,
the function pointer could get wiped out before the function is called
because there is currently nothing that controls access to it.

This patch introduces two new functions:
* The kvm_arch_crypto_register_hook() function registers a function pointer
  for processing intercepted crypto instructions.
* The kvm_arch_crypto_register_hook() function un-registers a function
  pointer that was previously registered.

Registration and unregistration of function pointers is protected by a
mutex lock. This lock is also employed when the hook is retrieved and the
hook function is called to protect against losing access to the function
while an intercepted crypto instruction is being processed.

The PQAP instruction handler function pointer is registered at the time
the vfio_ap module is loaded and unregistered when it is unloaded; so,
the lifespan of the function pointer is concurrent with the lifespan of
the vfio_ap module.

Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
---
 arch/s390/include/asm/kvm_host.h      | 13 +++--
 arch/s390/kvm/priv.c                  | 70 +++++++++++++++++++++++++--
 drivers/s390/crypto/vfio_ap_ops.c     | 37 +++++++++++---
 drivers/s390/crypto/vfio_ap_private.h |  1 -
 4 files changed, 105 insertions(+), 16 deletions(-)

diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
index 8925f3969478..d59b9309a6b8 100644
--- a/arch/s390/include/asm/kvm_host.h
+++ b/arch/s390/include/asm/kvm_host.h
@@ -803,14 +803,19 @@ struct kvm_s390_cpu_model {
 	unsigned short ibc;
 };
 
-struct kvm_s390_module_hook {
-	int (*hook)(struct kvm_vcpu *vcpu);
+enum kvm_s390_crypto_hook_type {
+	PQAP_HOOK
+};
+
+struct kvm_s390_crypto_hook {
+	enum kvm_s390_crypto_hook_type type;
 	struct module *owner;
+	int (*hook_fcn)(struct kvm_vcpu *vcpu);
+	struct list_head node;
 };
 
 struct kvm_s390_crypto {
 	struct kvm_s390_crypto_cb *crycb;
-	struct kvm_s390_module_hook *pqap_hook;
 	__u32 crycbd;
 	__u8 aes_kw;
 	__u8 dea_kw;
@@ -996,6 +1001,8 @@ static inline void kvm_arch_async_page_present_queued(struct kvm_vcpu *vcpu) {}
 void kvm_arch_crypto_clear_masks(struct kvm *kvm);
 void kvm_arch_crypto_set_masks(struct kvm *kvm, unsigned long *apm,
 			       unsigned long *aqm, unsigned long *adm);
+extern int kvm_arch_crypto_register_hook(struct kvm_s390_crypto_hook *hook);
+extern int kvm_arch_crypto_unregister_hook(struct kvm_s390_crypto_hook *hook);
 
 extern int sie64a(struct kvm_s390_sie_block *, u64 *);
 extern char sie_exit;
diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c
index 9928f785c677..1221c04f6f6a 100644
--- a/arch/s390/kvm/priv.c
+++ b/arch/s390/kvm/priv.c
@@ -12,6 +12,7 @@
 #include <linux/gfp.h>
 #include <linux/errno.h>
 #include <linux/compat.h>
+#include <linux/list.h>
 #include <linux/mm_types.h>
 #include <linux/pgtable.h>
 
@@ -31,6 +32,63 @@
 #include "kvm-s390.h"
 #include "trace.h"
 
+DEFINE_MUTEX(crypto_hooks_lock);
+static struct list_head crypto_hooks = { &(crypto_hooks), &(crypto_hooks) };
+
+static struct kvm_s390_crypto_hook
+*kvm_arch_crypto_find_hook(enum kvm_s390_crypto_hook_type type)
+{
+	struct kvm_s390_crypto_hook *crypto_hook;
+
+	list_for_each_entry(crypto_hook, &crypto_hooks, node) {
+		if (crypto_hook->type == type)
+			return crypto_hook;
+	}
+
+	return NULL;
+}
+
+int kvm_arch_crypto_register_hook(struct kvm_s390_crypto_hook *hook)
+{
+	struct kvm_s390_crypto_hook *crypto_hook;
+
+	mutex_lock(&crypto_hooks_lock);
+	crypto_hook = kvm_arch_crypto_find_hook(hook->type);
+	if (crypto_hook) {
+		if (crypto_hook->owner != hook->owner)
+			return -EACCES;
+		list_replace(&crypto_hook->node, &hook->node);
+	} else {
+		list_add(&hook->node, &crypto_hooks);
+	}
+	mutex_unlock(&crypto_hooks_lock);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(kvm_arch_crypto_register_hook);
+
+int kvm_arch_crypto_unregister_hook(struct kvm_s390_crypto_hook *hook)
+{
+	struct kvm_s390_crypto_hook *crypto_hook;
+
+	mutex_lock(&crypto_hooks_lock);
+	crypto_hook = kvm_arch_crypto_find_hook(hook->type);
+
+	if (crypto_hook) {
+		if (crypto_hook->owner != hook->owner)
+			return -EACCES;
+		if (crypto_hook->hook_fcn != hook->hook_fcn)
+			return -EFAULT;
+		list_del(&crypto_hook->node);
+	} else {
+		return -ENODEV;
+	}
+	mutex_unlock(&crypto_hooks_lock);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(kvm_arch_crypto_unregister_hook);
+
 static int handle_ri(struct kvm_vcpu *vcpu)
 {
 	vcpu->stat.instruction_ri++;
@@ -610,6 +668,7 @@ static int handle_io_inst(struct kvm_vcpu *vcpu)
 static int handle_pqap(struct kvm_vcpu *vcpu)
 {
 	struct ap_queue_status status = {};
+	struct kvm_s390_crypto_hook *pqap_hook;
 	unsigned long reg0;
 	int ret;
 	uint8_t fc;
@@ -657,15 +716,16 @@ static int handle_pqap(struct kvm_vcpu *vcpu)
 	 * Verify that the hook callback is registered, lock the owner
 	 * and call the hook.
 	 */
-	if (vcpu->kvm->arch.crypto.pqap_hook) {
-		if (!try_module_get(vcpu->kvm->arch.crypto.pqap_hook->owner))
-			return -EOPNOTSUPP;
-		ret = vcpu->kvm->arch.crypto.pqap_hook->hook(vcpu);
-		module_put(vcpu->kvm->arch.crypto.pqap_hook->owner);
+	mutex_lock(&crypto_hooks_lock);
+	pqap_hook = kvm_arch_crypto_find_hook(PQAP_HOOK);
+	if (pqap_hook) {
+		ret = pqap_hook->hook_fcn(vcpu);
 		if (!ret && vcpu->run->s.regs.gprs[1] & 0x00ff0000)
 			kvm_s390_set_psw_cc(vcpu, 3);
+		mutex_unlock(&crypto_hooks_lock);
 		return ret;
 	}
+	mutex_unlock(&crypto_hooks_lock);
 	/*
 	 * A vfio_driver must register a hook.
 	 * No hook means no driver to enable the SIE CRYCB and no queues.
diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index f90c9103dac2..3466ceffc46a 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -64,6 +64,21 @@ static struct vfio_ap_queue *vfio_ap_get_queue(
 	return q;
 }
 
+static struct ap_matrix_mdev *vfio_ap_find_mdev_for_apqn(int apqn)
+{
+	struct ap_matrix_mdev *matrix_mdev;
+	unsigned long apid = AP_QID_CARD(apqn);
+	unsigned long apqi = AP_QID_QUEUE(apqn);
+
+	list_for_each_entry(matrix_mdev, &matrix_dev->mdev_list, node) {
+		if (test_bit_inv(apid, matrix_mdev->matrix.apm) &&
+		    test_bit_inv(apqi, matrix_mdev->matrix.aqm))
+			return matrix_mdev;
+	}
+
+	return NULL;
+}
+
 /**
  * vfio_ap_wait_for_irqclear
  * @apqn: The AP Queue number
@@ -287,13 +302,13 @@ static int handle_pqap(struct kvm_vcpu *vcpu)
 	if (!(vcpu->arch.sie_block->eca & ECA_AIV))
 		return -EOPNOTSUPP;
 
+
 	apqn = vcpu->run->s.regs.gprs[0] & 0xffff;
 	mutex_lock(&matrix_dev->lock);
 
-	if (!vcpu->kvm->arch.crypto.pqap_hook)
+	matrix_mdev = vfio_ap_find_mdev_for_apqn(apqn);
+	if (!matrix_mdev)
 		goto out_unlock;
-	matrix_mdev = container_of(vcpu->kvm->arch.crypto.pqap_hook,
-				   struct ap_matrix_mdev, pqap_hook);
 
 	/*
 	 * If the KVM pointer is in the process of being set, wait until the
@@ -353,8 +368,6 @@ static int vfio_ap_mdev_create(struct mdev_device *mdev)
 	vfio_ap_matrix_init(&matrix_dev->info, &matrix_mdev->matrix);
 	init_waitqueue_head(&matrix_mdev->wait_for_kvm);
 	mdev_set_drvdata(mdev, matrix_mdev);
-	matrix_mdev->pqap_hook.hook = handle_pqap;
-	matrix_mdev->pqap_hook.owner = THIS_MODULE;
 	mutex_lock(&matrix_dev->lock);
 	list_add(&matrix_mdev->node, &matrix_dev->mdev_list);
 	mutex_unlock(&matrix_dev->lock);
@@ -1123,7 +1136,6 @@ static int vfio_ap_mdev_set_kvm(struct ap_matrix_mdev *matrix_mdev,
 					  matrix_mdev->matrix.aqm,
 					  matrix_mdev->matrix.adm);
 		mutex_lock(&matrix_dev->lock);
-		kvm->arch.crypto.pqap_hook = &matrix_mdev->pqap_hook;
 		matrix_mdev->kvm = kvm;
 		matrix_mdev->kvm_busy = false;
 		wake_up_all(&matrix_mdev->wait_for_kvm);
@@ -1193,7 +1205,6 @@ static void vfio_ap_mdev_unset_kvm(struct ap_matrix_mdev *matrix_mdev)
 		kvm_arch_crypto_clear_masks(matrix_mdev->kvm);
 		mutex_lock(&matrix_dev->lock);
 		vfio_ap_mdev_reset_queues(matrix_mdev->mdev);
-		matrix_mdev->kvm->arch.crypto.pqap_hook = NULL;
 		kvm_put_kvm(matrix_mdev->kvm);
 		matrix_mdev->kvm = NULL;
 		matrix_mdev->kvm_busy = false;
@@ -1433,14 +1444,26 @@ static const struct mdev_parent_ops vfio_ap_matrix_ops = {
 	.ioctl			= vfio_ap_mdev_ioctl,
 };
 
+static struct kvm_s390_crypto_hook pqap_hook = {
+	.type = PQAP_HOOK,
+	.owner = THIS_MODULE,
+	.hook_fcn = handle_pqap
+};
+
 int vfio_ap_mdev_register(void)
 {
+	int ret;
 	atomic_set(&matrix_dev->available_instances, MAX_ZDEV_ENTRIES_EXT);
 
+	ret = kvm_arch_crypto_register_hook(&pqap_hook);
+	if (ret)
+		return ret;
+
 	return mdev_register_device(&matrix_dev->device, &vfio_ap_matrix_ops);
 }
 
 void vfio_ap_mdev_unregister(void)
 {
+	WARN_ON(kvm_arch_crypto_unregister_hook(&pqap_hook));
 	mdev_unregister_device(&matrix_dev->device);
 }
diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
index f82a6396acae..542259b57972 100644
--- a/drivers/s390/crypto/vfio_ap_private.h
+++ b/drivers/s390/crypto/vfio_ap_private.h
@@ -86,7 +86,6 @@ struct ap_matrix_mdev {
 	bool kvm_busy;
 	wait_queue_head_t wait_for_kvm;
 	struct kvm *kvm;
-	struct kvm_s390_module_hook pqap_hook;
 	struct mdev_device *mdev;
 };
 
-- 
2.30.2

Re: [PATCH v4 2/2] s390/vfio-ap: control access to PQAP(AQIC) interception handler

[Jason Gunthorpe]

On Fri, May 21, 2021 at 03:36:48PM -0400, Tony Krowiak wrote:
> +static struct kvm_s390_crypto_hook
> +*kvm_arch_crypto_find_hook(enum kvm_s390_crypto_hook_type type)
> +{
> +	struct kvm_s390_crypto_hook *crypto_hook;
> +
> +	list_for_each_entry(crypto_hook, &crypto_hooks, node) {
> +		if (crypto_hook->type == type)
> +			return crypto_hook;
> +	}
> +
> +	return NULL;
> +}
> +
> +int kvm_arch_crypto_register_hook(struct kvm_s390_crypto_hook *hook)
> +{
> +	struct kvm_s390_crypto_hook *crypto_hook;
> +
> +	mutex_lock(&crypto_hooks_lock);
> +	crypto_hook = kvm_arch_crypto_find_hook(hook->type);
> +	if (crypto_hook) {
> +		if (crypto_hook->owner != hook->owner)
> +			return -EACCES;
> +		list_replace(&crypto_hook->node, &hook->node);

This is all dead code right? This is only called from a module init
function so it can't be called twice. Just always fail if the hook is
already used and delete the owner stuff.

But this is alot of complicated and unused code to solve a lock
ordering problem..

Jason

Re: [PATCH v4 2/2] s390/vfio-ap: control access to PQAP(AQIC) interception handler

[Jason J. Herne]

On 5/21/21 3:36 PM, Tony Krowiak wrote:
> The function pointer to the handler that processes interception of the
> PQAP instruction is contained in the mdev. If the mdev is removed and
> its storage de-allocated during the processing of the PQAP instruction,
> the function pointer could get wiped out before the function is called
> because there is currently nothing that controls access to it.
> 
> This patch introduces two new functions:
> * The kvm_arch_crypto_register_hook() function registers a function pointer
>    for processing intercepted crypto instructions.
> * The kvm_arch_crypto_register_hook() function un-registers a function
>    pointer that was previously registered.

Typo: You meant kvm_arch_crypto_UNregister_hook() in the second bullet.


Just one overall observation on this one. The whole hook system seems kind of 
over-engineered if this is our only use for it. It looks like a kvm_s390_crypto_hook is 
meant to link a specific module with a function pointer. Do we really need this concept?

I think a simpler design could be to just place a mutex and a function pointer in the 
kvm_s390_crypto struct. Then you can grab the mutex in vfio_ap_ops.c when 
registering/unregistering. You would also grab the mutex in priv.c when calling the 
function pointer. What I am suggesting is essentially the exact same scheme you have 
implemented here, but simpler and with less infrastructure.

With that said, I'll point out that I am relative new to this code (and this patch series) 
so maybe I've missed something and the extra complexity is needed for some reason. But if 
it is not, I'm all in favor of keeping things simple.

-- 
-- Jason J. Herne (jjherne@linux.ibm.com)

Re: [PATCH v4 1/2] s390/vfio-ap: fix memory leak in mdev remove callback

[Halil Pasic]

On Fri, 21 May 2021 15:36:47 -0400
Tony Krowiak <akrowiak@linux.ibm.com> wrote:

> The mdev remove callback for the vfio_ap device driver bails out with
> -EBUSY if the mdev is in use by a KVM guest. The intended purpose was
> to prevent the mdev from being removed while in use; however, returning a
> non-zero rc does not prevent removal. This could result in a memory leak
> of the resources allocated when the mdev was created. In addition, the
> KVM guest will still have access to the AP devices assigned to the mdev
> even though the mdev no longer exists.
> 
> To prevent this scenario, cleanup will be done - including unplugging the
> AP adapters, domains and control domains - regardless of whether the mdev
> is in use by a KVM guest or not.
> 
> Fixes: 258287c994de ("s390: vfio-ap: implement mediated device open callback")
> Cc: stable@vger.kernel.org
> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>

AFAIU we all agree that, after patch there is a possibility for an use
after free error. I'm a little confused by the fact that we want this
one for stable, but the patch that fixes the use after free as no
Cc stable (it can't have a proper fixes tag, because this one is not yet
merged). Actually I'm not a big fan of splitting up patches to the
extent that when not all patches of the series are applied we get bugous
behavior (e.g. patch n breaks something that is live at patch n level,
but it is supposed to be OK, because patch n+m is going to fix it (where
n,m \in \Z^{+}).

Do we want to squash these? Is the use after free possible prior to this
patch? 

Regards,
Halil

Re: [PATCH v4 2/2] s390/vfio-ap: control access to PQAP(AQIC) interception handler

[Tony Krowiak]

On 5/24/21 10:37 AM, Jason J. Herne wrote:
> On 5/21/21 3:36 PM, Tony Krowiak wrote:
>> The function pointer to the handler that processes interception of the
>> PQAP instruction is contained in the mdev. If the mdev is removed and
>> its storage de-allocated during the processing of the PQAP instruction,
>> the function pointer could get wiped out before the function is called
>> because there is currently nothing that controls access to it.
>>
>> This patch introduces two new functions:
>> * The kvm_arch_crypto_register_hook() function registers a function 
>> pointer
>>    for processing intercepted crypto instructions.
>> * The kvm_arch_crypto_register_hook() function un-registers a function
>>    pointer that was previously registered.
>
> Typo: You meant kvm_arch_crypto_UNregister_hook() in the second bullet.
>
>
> Just one overall observation on this one. The whole hook system seems 
> kind of over-engineered if this is our only use for it. It looks like 
> a kvm_s390_crypto_hook is meant to link a specific module with a 
> function pointer. Do we really need this concept?
>
> I think a simpler design could be to just place a mutex and a function 
> pointer in the kvm_s390_crypto struct. Then you can grab the mutex in 
> vfio_ap_ops.c when registering/unregistering. You would also grab the 
> mutex in priv.c when calling the function pointer. What I am 
> suggesting is essentially the exact same scheme you have implemented 
> here, but simpler and with less infrastructure.

That would be great, however; when I implemented something similar, it 
resulted in a
lockdep splat between the lock used to protect the hook and the 
matrix_dev->lock used to
protect updates to matrix_mdev (including the freeing thereof). After 
pulling what little hair
I have left out, this seemed like a reasonable solution, over-engineered 
though it may be.
If somebody has a simpler solution, I'm all ears.

>
> With that said, I'll point out that I am relative new to this code 
> (and this patch series) so maybe I've missed something and the extra 
> complexity is needed for some reason. But if it is not, I'm all in 
> favor of keeping things simple.
>

Re: [PATCH v4 2/2] s390/vfio-ap: control access to PQAP(AQIC) interception handler

[Jason Gunthorpe]

On Tue, May 25, 2021 at 09:16:30AM -0400, Tony Krowiak wrote:
> 
> 
> On 5/24/21 10:37 AM, Jason J. Herne wrote:
> > On 5/21/21 3:36 PM, Tony Krowiak wrote:
> > > The function pointer to the handler that processes interception of the
> > > PQAP instruction is contained in the mdev. If the mdev is removed and
> > > its storage de-allocated during the processing of the PQAP instruction,
> > > the function pointer could get wiped out before the function is called
> > > because there is currently nothing that controls access to it.
> > > 
> > > This patch introduces two new functions:
> > > * The kvm_arch_crypto_register_hook() function registers a function
> > > pointer
> > >    for processing intercepted crypto instructions.
> > > * The kvm_arch_crypto_register_hook() function un-registers a function
> > >    pointer that was previously registered.
> > 
> > Typo: You meant kvm_arch_crypto_UNregister_hook() in the second bullet.
> > 
> > 
> > Just one overall observation on this one. The whole hook system seems
> > kind of over-engineered if this is our only use for it. It looks like a
> > kvm_s390_crypto_hook is meant to link a specific module with a function
> > pointer. Do we really need this concept?
> > 
> > I think a simpler design could be to just place a mutex and a function
> > pointer in the kvm_s390_crypto struct. Then you can grab the mutex in
> > vfio_ap_ops.c when registering/unregistering. You would also grab the
> > mutex in priv.c when calling the function pointer. What I am suggesting
> > is essentially the exact same scheme you have implemented here, but
> > simpler and with less infrastructure.
> 
> That would be great, however; when I implemented something similar, it
> resulted in a
> lockdep splat between the lock used to protect the hook and the
> matrix_dev->lock used to
> protect updates to matrix_mdev (including the freeing thereof). After
> pulling what little hair
> I have left out, this seemed like a reasonable solution, over-engineered
> though it may be.
> If somebody has a simpler solution, I'm all ears.

Why can't you put the locks in the right order? It looked trivial, I'm confused.

Jason

Re: [PATCH v4 1/2] s390/vfio-ap: fix memory leak in mdev remove callback

[Tony Krowiak]

On 5/25/21 9:03 AM, Halil Pasic wrote:
> On Fri, 21 May 2021 15:36:47 -0400
> Tony Krowiak <akrowiak@linux.ibm.com> wrote:
>
>> The mdev remove callback for the vfio_ap device driver bails out with
>> -EBUSY if the mdev is in use by a KVM guest. The intended purpose was
>> to prevent the mdev from being removed while in use; however, returning a
>> non-zero rc does not prevent removal. This could result in a memory leak
>> of the resources allocated when the mdev was created. In addition, the
>> KVM guest will still have access to the AP devices assigned to the mdev
>> even though the mdev no longer exists.
>>
>> To prevent this scenario, cleanup will be done - including unplugging the
>> AP adapters, domains and control domains - regardless of whether the mdev
>> is in use by a KVM guest or not.
>>
>> Fixes: 258287c994de ("s390: vfio-ap: implement mediated device open callback")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
>> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> AFAIU we all agree that, after patch there is a possibility for an use
> after free error.

I am assuming here that you meant to say that after applying
patch 1/2, there is a possibility for a use after free error.

> I'm a little confused by the fact that we want this
> one for stable, but the patch that fixes the use after free as no
> Cc stable (it can't have a proper fixes tag, because this one is not yet
> merged). Actually I'm not a big fan of splitting up patches to the
> extent that when not all patches of the series are applied we get bugous
> behavior (e.g. patch n breaks something that is live at patch n level,
> but it is supposed to be OK, because patch n+m is going to fix it (where
> n,m \in \Z^{+}).
>
> Do we want to squash these? Is the use after free possible prior to this
> patch?

I am fine with squashing these if that is the consensus here. Prior
to this patch, the remove callback function returned -EBUSY
if a guest is still using the matrix_mdev (i.e., matrix_mdev->kvm
not NULL), so the matrix_mdev was not freed and hence the
memory leak for this this patch was designed to fix.

>
> Regards,
> Halil

Re: [PATCH v4 2/2] s390/vfio-ap: control access to PQAP(AQIC) interception handler

[Jason J. Herne]

On 5/24/21 10:37 AM, Jason J. Herne wrote:
> On 5/21/21 3:36 PM, Tony Krowiak wrote:
>> The function pointer to the handler that processes interception of the
>> PQAP instruction is contained in the mdev. If the mdev is removed and
>> its storage de-allocated during the processing of the PQAP instruction,
>> the function pointer could get wiped out before the function is called
>> because there is currently nothing that controls access to it.
>>
>> This patch introduces two new functions:
>> * The kvm_arch_crypto_register_hook() function registers a function pointer
>>    for processing intercepted crypto instructions.
>> * The kvm_arch_crypto_register_hook() function un-registers a function
>>    pointer that was previously registered.
> 
> Typo: You meant kvm_arch_crypto_UNregister_hook() in the second bullet.
> 
> 
> Just one overall observation on this one. The whole hook system seems kind of 
> over-engineered if this is our only use for it. It looks like a kvm_s390_crypto_hook is 
> meant to link a specific module with a function pointer. Do we really need this concept?
> 
> I think a simpler design could be to just place a mutex and a function pointer in the 
> kvm_s390_crypto struct. Then you can grab the mutex in vfio_ap_ops.c when 
> registering/unregistering. You would also grab the mutex in priv.c when calling the 
> function pointer. What I am suggesting is essentially the exact same scheme you have 
> implemented here, but simpler and with less infrastructure.
> 
> With that said, I'll point out that I am relative new to this code (and this patch series) 
> so maybe I've missed something and the extra complexity is needed for some reason. But if 
> it is not, I'm all in favor of keeping things simple.
> 

After thinking about this problem a bit more, I'm wondering if we can remove the lock 
entirely. How about we store a function pointer in kvm_s390_crypto? Initially that 
function pointer will point to a stub function that handles the error case, exactly like 
it is done in priv.c:handle_pqap() today when the function pointer would be NULL. When the 
ap module loads, we can simply change the function pointer to point to 
vfio_ap_ops:handle_pqap(). When we unload the module we change the function pointer back 
to the stub.  The updates should be atomic operations so no lock needed, right?

-- 
-- Jason J. Herne (jjherne@linux.ibm.com)

Re: [PATCH v4 2/2] s390/vfio-ap: control access to PQAP(AQIC) interception handler

[Jason Gunthorpe]

On Tue, May 25, 2021 at 09:24:59AM -0400, Jason J. Herne wrote:
> change the function pointer to point to vfio_ap_ops:handle_pqap(). When we
> unload the module we change the function pointer back to the stub.  The
> updates should be atomic operations so no lock needed, right?

No

Jason

Re: [PATCH v4 2/2] s390/vfio-ap: control access to PQAP(AQIC) interception handler

[Jason J. Herne]

On 5/25/21 9:26 AM, Jason Gunthorpe wrote:
> On Tue, May 25, 2021 at 09:24:59AM -0400, Jason J. Herne wrote:
>> change the function pointer to point to vfio_ap_ops:handle_pqap(). When we
>> unload the module we change the function pointer back to the stub.  The
>> updates should be atomic operations so no lock needed, right?
> 
> No
> 
> Jason
> 

Okay... Would you be willing to elaborate, please? A counter argument, or a simple 
explanation would be appreciated. A simple "no" does not really do much to advance the 
discussion :).

I'm fairly sure that a 64-bit pointer would be updated atomically. A reader of this value 
is either going to see value A or value B, not the high half of A and the low half of B. 
Maybe we also need a memory barrier to prevent stale values from being seen on another core?

-- 
-- Jason J. Herne (jjherne@linux.ibm.com)

Re: [PATCH v4 2/2] s390/vfio-ap: control access to PQAP(AQIC) interception handler

[Jason Gunthorpe]

On Tue, May 25, 2021 at 10:07:46AM -0400, Jason J. Herne wrote:
> On 5/25/21 9:26 AM, Jason Gunthorpe wrote:
> > On Tue, May 25, 2021 at 09:24:59AM -0400, Jason J. Herne wrote:
> > > change the function pointer to point to vfio_ap_ops:handle_pqap(). When we
> > > unload the module we change the function pointer back to the stub.  The
> > > updates should be atomic operations so no lock needed, right?
> > 
> > No
> > 
> > Jason
> > 
> 
> Okay... Would you be willing to elaborate, please? A counter argument, or a
> simple explanation would be appreciated. A simple "no" does not really do
> much to advance the discussion :).

Go back and review the earlier thread, the issue was never the
atomicity of the function pointer but the locking of the data that
function is accessing.

> I'm fairly sure that a 64-bit pointer would be updated atomically. A reader
> of this value is either going to see value A or value B, not the high half
> of A and the low half of B. Maybe we also need a memory barrier to prevent
> stale values from being seen on another core?

You need to use special macros in Linux to follow this memory model

Jason

Re: [PATCH v4 2/2] s390/vfio-ap: control access to PQAP(AQIC) interception handler

[Tony Krowiak]

On 5/23/21 6:57 PM, Jason Gunthorpe wrote:
> On Fri, May 21, 2021 at 03:36:48PM -0400, Tony Krowiak wrote:
>> +static struct kvm_s390_crypto_hook
>> +*kvm_arch_crypto_find_hook(enum kvm_s390_crypto_hook_type type)
>> +{
>> +	struct kvm_s390_crypto_hook *crypto_hook;
>> +
>> +	list_for_each_entry(crypto_hook, &crypto_hooks, node) {
>> +		if (crypto_hook->type == type)
>> +			return crypto_hook;
>> +	}
>> +
>> +	return NULL;
>> +}
>> +
>> +int kvm_arch_crypto_register_hook(struct kvm_s390_crypto_hook *hook)
>> +{
>> +	struct kvm_s390_crypto_hook *crypto_hook;
>> +
>> +	mutex_lock(&crypto_hooks_lock);
>> +	crypto_hook = kvm_arch_crypto_find_hook(hook->type);
>> +	if (crypto_hook) {
>> +		if (crypto_hook->owner != hook->owner)
>> +			return -EACCES;
>> +		list_replace(&crypto_hook->node, &hook->node);
> This is all dead code right? This is only called from a module init
> function so it can't be called twice.

That is true only if you are considering the current case.
Is it guaranteed that only the vfio_ap module
will call this function and is there a guarantee that it will
always and only be called from the vfio_ap module init
function? For example, suppose a hook is added to
intercept the AP NQAP or DQAP instruction? We don't
know how or when those handlers will be registered
or unregistered. We don't even know if the handlers
will be part of the vfio_ap module or not.

> Just always fail if the hook is
> already used and delete the owner stuff.

I suppose that is reasonable and simpler.

>
> But this is alot of complicated and unused code to solve a lock
> ordering problem..

If you have a better solution, I'm all ears. I've been down this
road a couple of times now and solving lock ordering for
multiple asynchronous processes is not trivial. This seems like
a reasonable solution and provides for flexibility for including
additional hooks to handle interception of other AP instructions.

>
> Jason

Re: [PATCH v4 2/2] s390/vfio-ap: control access to PQAP(AQIC) interception handler

[Jason Gunthorpe]

On Tue, May 25, 2021 at 10:59:25AM -0400, Tony Krowiak wrote:
> > But this is alot of complicated and unused code to solve a lock
> > ordering problem..
> 
> If you have a better solution, I'm all ears. I've been down this
> road a couple of times now and solving lock ordering for
> multiple asynchronous processes is not trivial. This seems like
> a reasonable solution and provides for flexibility for including
> additional hooks to handle interception of other AP instructions.

Lock ordering is very trivial. In this case you have to always hold
the hook lock before obtaining the matrix_dev lock. From what I
remember there was only one error on the set path where they were
ordered wrong

Jason

Re: [PATCH v4 2/2] s390/vfio-ap: control access to PQAP(AQIC) interception handler

[Tony Krowiak]

On 5/25/21 9:19 AM, Jason Gunthorpe wrote:
> On Tue, May 25, 2021 at 09:16:30AM -0400, Tony Krowiak wrote:
>>
>> On 5/24/21 10:37 AM, Jason J. Herne wrote:
>>> On 5/21/21 3:36 PM, Tony Krowiak wrote:
>>>> The function pointer to the handler that processes interception of the
>>>> PQAP instruction is contained in the mdev. If the mdev is removed and
>>>> its storage de-allocated during the processing of the PQAP instruction,
>>>> the function pointer could get wiped out before the function is called
>>>> because there is currently nothing that controls access to it.
>>>>
>>>> This patch introduces two new functions:
>>>> * The kvm_arch_crypto_register_hook() function registers a function
>>>> pointer
>>>>     for processing intercepted crypto instructions.
>>>> * The kvm_arch_crypto_register_hook() function un-registers a function
>>>>     pointer that was previously registered.
>>> Typo: You meant kvm_arch_crypto_UNregister_hook() in the second bullet.
>>>
>>>
>>> Just one overall observation on this one. The whole hook system seems
>>> kind of over-engineered if this is our only use for it. It looks like a
>>> kvm_s390_crypto_hook is meant to link a specific module with a function
>>> pointer. Do we really need this concept?
>>>
>>> I think a simpler design could be to just place a mutex and a function
>>> pointer in the kvm_s390_crypto struct. Then you can grab the mutex in
>>> vfio_ap_ops.c when registering/unregistering. You would also grab the
>>> mutex in priv.c when calling the function pointer. What I am suggesting
>>> is essentially the exact same scheme you have implemented here, but
>>> simpler and with less infrastructure.
>> That would be great, however; when I implemented something similar, it
>> resulted in a
>> lockdep splat between the lock used to protect the hook and the
>> matrix_dev->lock used to
>> protect updates to matrix_mdev (including the freeing thereof). After
>> pulling what little hair
>> I have left out, this seemed like a reasonable solution, over-engineered
>> though it may be.
>> If somebody has a simpler solution, I'm all ears.
> Why can't you put the locks in the right order? It looked trivial, I'm confused.

Because the handle_pqap() function in priv.c does not have access to the
matrix_dev lock.

>
> Jason

Re: [PATCH v4 2/2] s390/vfio-ap: control access to PQAP(AQIC) interception handler

[Jason Gunthorpe]

On Tue, May 25, 2021 at 11:08:22AM -0400, Tony Krowiak wrote:

> > Why can't you put the locks in the right order? It looked trivial, I'm confused.
> 
> Because the handle_pqap() function in priv.c does not have access to the
> matrix_dev lock.

Based on the sketch  made the handle_pqap() should only handle the
arch.crypto.rwsem.

When it calls the hook it gets the matrix dev

This sets the lock order as always: rwsem then matrix_dev

Of the other two places:

@@ -352,8 +352,7 @@ static int vfio_ap_mdev_create(struct mdev_device *mdev)
+       down_write(&&vcpu->kvm->arch.crypto.rwsem);
        mutex_lock(&matrix_dev->lock);

Obviously correct

@@ -1202,7 +1203,9 @@ static void vfio_ap_mdev_unset_kvm(struct ap_matrix_mdev *matrix_mdev)
                mutex_lock(&matrix_dev->lock);
                vfio_ap_mdev_reset_queues(matrix_mdev->mdev);
+               down_write(&matrix_mdev->kvm->arch.crypto.rwsem);
                matrix_mdev->kvm->arch.crypto.pqap_hook = NULL;
+               up_write(&matrix_mdev->kvm->arch.crypto.rwsem);

This is inverted

Just move the down_write up two lines

What is missing?

Jason

Re: [PATCH v4 2/2] s390/vfio-ap: control access to PQAP(AQIC) interception handler

[Tony Krowiak]

> Why can't you put the locks in the right order? It looked trivial, I'm confused.
>
> Jason

I explained this in one of my responses to the previous series. Maybe I 
didn't do
a good job of it, so let me see if I can provide a more thorough 
explanation.

The handle_pqap() function in priv.c does not have any access to the
matrix_dev->lock which lives in the vfio_ap module, so there is no
way for this function to control the order of locking. The only lock
it can access is the lock provided for the hook function pointer which
must be held for the duration of the execution of the hook. When the
hook function (handle_pqap() in vfio_ap_ops.c) is called, it has to lock
the matrix_dev->lock mutex to do its thing. The interception of the
PQAP instruction that sets off the above scenario can happen simultaneously
with both the vfio_ap_mdev_set_kvm() and vfio_ap_mdev_unset_kvm()
instructions in vfio_ap_ops.c.

The vfio_ap_mdev_set_kvm() function is called only by the group notifier
callback when the vfio_ap driver is notified that the KVM pointer has 
been set.
In this case, we could set the lock for the hook function before setting
the matrix_dev->lock and calling the vfio_ap_mdev_set_kvm() function and
all would be well.

The vfio_ap_mdev_unset_kvm() function, however, is called both by the group
notifier when the KVM pointer has been cleared or when the mdev is
being removed. In both cases, the only way to get the KVM pointer - which
is needed to unplug the AP resources from the guest - is from the 
matrix_mdev
which contains it. This, of course, needs to be done while holding the
matrix_dev->lock mutex. The vfio_ap_mdev_unset_kvm() function also
clears the hook function pointer, but can only get the lock used to 
control access
to it from the matrix_mdev; therein lies the rub. So we can have the 
following
scenario which is flagged by lockdep:

CPU x:                                            CPU y:
--------                                             --------
                                                       lock the 
matrix_dev->lock:
vfio_ap_mdev_set_kvm in vfio_ap_ops.c

lock the hook pointer:
handle_pqap in priv.c

lock the matrix_dev->lock
handle_pqap in vfio_ap_ops.c

                                                       lock the hook 
pointer:
vfio_ap_mdev_set_kvm in vfio_ap_ops.c


Maybe I'm missing something, but I was unable to find a way around this when
the hook function pointer and its locking mechanism is stored in a field 
of a satellite
structure of struct kvm.

Re: [PATCH v4 2/2] s390/vfio-ap: control access to PQAP(AQIC) interception handler

[Jason Gunthorpe]

On Tue, May 25, 2021 at 11:56:50AM -0400, Tony Krowiak wrote:

> The vfio_ap_mdev_unset_kvm() function, however, is called both by
> the group notifier when the KVM pointer has been cleared or when the
> mdev is being removed. In both cases, the only way to get the KVM
> pointer - which is needed to unplug the AP resources from the guest
> - is from the matrix_mdev which contains it.

Okay, but that isn't a problem, the matrix dev holds a ref on the kvm
pointer so we can just copy it outside the lock after we prevent it
from changing by unregistering the notifier:

@@ -1362,14 +1365,19 @@ static void vfio_ap_mdev_release(struct mdev_device *mdev)
 {
        struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
 
-       mutex_lock(&matrix_dev->lock);
-       vfio_ap_mdev_unset_kvm(matrix_mdev);
-       mutex_unlock(&matrix_dev->lock);
-
        vfio_unregister_notifier(mdev_dev(mdev), VFIO_IOMMU_NOTIFY,
                                 &matrix_mdev->iommu_notifier);
        vfio_unregister_notifier(mdev_dev(mdev), VFIO_GROUP_NOTIFY,
                                 &matrix_mdev->group_notifier);
+
+       mutex_lock(&matrix_dev->lock);
+       /* matrix_dev->kvm cannot be changed now since we removed the notifiers */
+       kvm = matrix_mdev->kvm;
+       matrix_mdev->kvm = NULL;
+       mutex_unlock(&matrix_dev->lock);
+
+       vfio_ap_mdev_unset_kvm(matrix_mdev, kvm);
+
        module_put(THIS_MODULE);

Note the above misordering is an existing bug too

And reoganize unset_kvm so it uses internal locking and gets the kvm
from the argument.

Also the kvm_busy should be replaced by a proper rwsem, don't try to
open code locks like that - it just defeats lockdep analysis.

Finally, since the only way the ->kvm can be become non-NULL is if the
notifier is registered, release above removes the notifier, and remove
can't be called unless release has been completed, it looks to me like
this the remove check is just dead code, delete it, or leave it as a
WARN_ON:

@@ -366,16 +366,6 @@ static int vfio_ap_mdev_remove(struct mdev_device *mdev)
        struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);

        mutex_lock(&matrix_dev->lock);
-
-       /*
-        * If the KVM pointer is in flux or the guest is running, disallow
-        * un-assignment of control domain.
-        */
-       if (matrix_mdev->kvm_busy || matrix_mdev->kvm) {
-               mutex_unlock(&matrix_dev->lock);
-               return -EBUSY;
-       }

Jason

Re: [PATCH v4 1/2] s390/vfio-ap: fix memory leak in mdev remove callback

[Tony Krowiak]

On 5/25/21 9:03 AM, Halil Pasic wrote:
> On Fri, 21 May 2021 15:36:47 -0400
> Tony Krowiak <akrowiak@linux.ibm.com> wrote:
>
>> The mdev remove callback for the vfio_ap device driver bails out with
>> -EBUSY if the mdev is in use by a KVM guest. The intended purpose was
>> to prevent the mdev from being removed while in use; however, returning a
>> non-zero rc does not prevent removal. This could result in a memory leak
>> of the resources allocated when the mdev was created. In addition, the
>> KVM guest will still have access to the AP devices assigned to the mdev
>> even though the mdev no longer exists.
>>
>> To prevent this scenario, cleanup will be done - including unplugging the
>> AP adapters, domains and control domains - regardless of whether the mdev
>> is in use by a KVM guest or not.
>>
>> Fixes: 258287c994de ("s390: vfio-ap: implement mediated device open callback")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
>> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> AFAIU we all agree that, after patch there is a possibility for an use
> after free error. I'm a little confused by the fact that we want this
> one for stable, but the patch that fixes the use after free as no
> Cc stable (it can't have a proper fixes tag, because this one is not yet
> merged). Actually I'm not a big fan of splitting up patches to the
> extent that when not all patches of the series are applied we get bugous
> behavior (e.g. patch n breaks something that is live at patch n level,
> but it is supposed to be OK, because patch n+m is going to fix it (where
> n,m \in \Z^{+}).

After thinking about this some more, this patch does not really
fix a memory leak and should probably not be flagged as a fix
for 258287c994. Memory is not leaked
because the remove callback returns -EBUSY without freeing
mdev storage or resetting the queues.

Under normal circumstances, if the mdev is removed before
the mdev fd is closed (i.e., the guest is shut down), the process
will wait until the fd is closed, at which time the
release callback will get invoked. Since the release callback
clears the KVM pointer from the matrix_mdev, the remove
callback will not return -EBUSY and will in fact free the mdev
storage when it is subsequently invoked.

I am going to change the subject and remove the 'Fixes'
tag as well as the 'Cc' of stable. I'll change the subject to
something like:

"s390/vfio-ap: always free storage for mdev in remove callback"

>
> Do we want to squash these? Is the use after free possible prior to this
> patch?
>
> Regards,
> Halil

Re: [PATCH v4 2/2] s390/vfio-ap: control access to PQAP(AQIC) interception handler

[Tony Krowiak]

On 5/25/21 12:29 PM, Jason Gunthorpe wrote:
> On Tue, May 25, 2021 at 11:56:50AM -0400, Tony Krowiak wrote:
>
>> The vfio_ap_mdev_unset_kvm() function, however, is called both by
>> the group notifier when the KVM pointer has been cleared or when the
>> mdev is being removed. In both cases, the only way to get the KVM
>> pointer - which is needed to unplug the AP resources from the guest
>> - is from the matrix_mdev which contains it.
> Okay, but that isn't a problem, the matrix dev holds a ref on the kvm
> pointer so we can just copy it outside the lock after we prevent it
> from changing by unregistering the notifier:
>
> @@ -1362,14 +1365,19 @@ static void vfio_ap_mdev_release(struct mdev_device *mdev)
>   {
>          struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
>   
> -       mutex_lock(&matrix_dev->lock);
> -       vfio_ap_mdev_unset_kvm(matrix_mdev);
> -       mutex_unlock(&matrix_dev->lock);
> -
>          vfio_unregister_notifier(mdev_dev(mdev), VFIO_IOMMU_NOTIFY,
>                                   &matrix_mdev->iommu_notifier);
>          vfio_unregister_notifier(mdev_dev(mdev), VFIO_GROUP_NOTIFY,
>                                   &matrix_mdev->group_notifier);
> +
> +       mutex_lock(&matrix_dev->lock);
> +       /* matrix_dev->kvm cannot be changed now since we removed the notifiers */
> +       kvm = matrix_mdev->kvm;
> +       matrix_mdev->kvm = NULL;
> +       mutex_unlock(&matrix_dev->lock);
> +
> +       vfio_ap_mdev_unset_kvm(matrix_mdev, kvm);
> +
>          module_put(THIS_MODULE);
>
> Note the above misordering is an existing bug too
>
> And reoganize unset_kvm so it uses internal locking and gets the kvm
> from the argument.

As I told you in a previous email, this is not a trivial exercise.
If you take a look at the vfio_ap_mdev_unset_kvm()
function, you will notice that it invokes the vfio_ap_mdev_reset_queues()
function to reset the queues after taking them away from the
guest. As each queue is reset, the resources required for
processing interrupts for the queue are freed in the
vfio_ap_free_aqic_resources() function. In order to unregister the
the guest's ISC, the matrix_mdev->kvm pointer must still
be set, however, you cleared it above.

Another thing you're overlooking is the fact that all of the
assignment/unassignment functions associated with the
corresponding syfs attributes of the mdev change the
content of the matrix_mdev->matrix and
matrix_mdev->shadow_apcb structures. In particular,
the matrix_mdev->matrix contains the APQNs of the
queues that must be reset. These sysfs attributes can
be accessed at any time including when the
vfio_ap_mdev_unset_kvm() function is executing,
so that is something that must also be taken into
consideration.


>
> Also the kvm_busy should be replaced by a proper rwsem, don't try to
> open code locks like that - it just defeats lockdep analysis.

I've had no luck trying to refactor this using rwsem. I always
run into lockdep problems between the matrix_dev->lock
and matrix_mdev->rwsem, even if the locking order is maintained.
Clearly, I am lacking in understanding of how these locks
interact. Any clues here?

> Finally, since the only way the ->kvm can be become non-NULL is if the
> notifier is registered, release above removes the notifier, and remove
> can't be called unless release has been completed, it looks to me like
> this the remove check is just dead code, delete it, or leave it as a
> WARN_ON:
>
> @@ -366,16 +366,6 @@ static int vfio_ap_mdev_remove(struct mdev_device *mdev)
>          struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
>
>          mutex_lock(&matrix_dev->lock);
> -
> -       /*
> -        * If the KVM pointer is in flux or the guest is running, disallow
> -        * un-assignment of control domain.
> -        */
> -       if (matrix_mdev->kvm_busy || matrix_mdev->kvm) {
> -               mutex_unlock(&matrix_dev->lock);
> -               return -EBUSY;
> -       }
>
> Jason

Re: [PATCH v4 2/2] s390/vfio-ap: control access to PQAP(AQIC) interception handler

[Jason Gunthorpe]

On Wed, May 26, 2021 at 10:28:29PM -0400, Tony Krowiak wrote:
> 
> 
> On 5/25/21 12:29 PM, Jason Gunthorpe wrote:
> > On Tue, May 25, 2021 at 11:56:50AM -0400, Tony Krowiak wrote:
> > 
> > > The vfio_ap_mdev_unset_kvm() function, however, is called both by
> > > the group notifier when the KVM pointer has been cleared or when the
> > > mdev is being removed. In both cases, the only way to get the KVM
> > > pointer - which is needed to unplug the AP resources from the guest
> > > - is from the matrix_mdev which contains it.
> > Okay, but that isn't a problem, the matrix dev holds a ref on the kvm
> > pointer so we can just copy it outside the lock after we prevent it
> > from changing by unregistering the notifier:
> > 
> > @@ -1362,14 +1365,19 @@ static void vfio_ap_mdev_release(struct mdev_device *mdev)
> >   {
> >          struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
> > -       mutex_lock(&matrix_dev->lock);
> > -       vfio_ap_mdev_unset_kvm(matrix_mdev);
> > -       mutex_unlock(&matrix_dev->lock);
> > -
> >          vfio_unregister_notifier(mdev_dev(mdev), VFIO_IOMMU_NOTIFY,
> >                                   &matrix_mdev->iommu_notifier);
> >          vfio_unregister_notifier(mdev_dev(mdev), VFIO_GROUP_NOTIFY,
> >                                   &matrix_mdev->group_notifier);
> > +
> > +       mutex_lock(&matrix_dev->lock);
> > +       /* matrix_dev->kvm cannot be changed now since we removed the notifiers */
> > +       kvm = matrix_mdev->kvm;
> > +       matrix_mdev->kvm = NULL;
> > +       mutex_unlock(&matrix_dev->lock);
> > +
> > +       vfio_ap_mdev_unset_kvm(matrix_mdev, kvm);
> > +
> >          module_put(THIS_MODULE);
> > 
> > Note the above misordering is an existing bug too
> > 
> > And reoganize unset_kvm so it uses internal locking and gets the kvm
> > from the argument.
> 
> As I told you in a previous email, this is not a trivial exercise.

Well, it is not a 5 line patch, but it looks like 10 mins work and
some testing to me, tracking down all the uses of matrx_mdev->kvm
under the vfio_ap_mdev_unset_kvm() call does not seem difficult nor do
there seem to be so many.

> vfio_ap_free_aqic_resources() function. In order to unregister the
> the guest's ISC, the matrix_mdev->kvm pointer must still
> be set, however, you cleared it above.

Which is why I said unset_kvm needs to be reorganized to use the kvm
argument, not the matrixt_mdev->kvm

> Another thing you're overlooking is the fact that all of the
> assignment/unassignment functions associated with the
> corresponding syfs attributes of the mdev change the
> content of the matrix_mdev->matrix and
> matrix_mdev->shadow_apcb structures. In particular,
> the matrix_mdev->matrix contains the APQNs of the
> queues that must be reset. These sysfs attributes can
> be accessed at any time including when the
> vfio_ap_mdev_unset_kvm() function is executing,
> so that is something that must also be taken into
> consideration.

I checked and thought they already had a lock?
 
> > Also the kvm_busy should be replaced by a proper rwsem, don't try to
> > open code locks like that - it just defeats lockdep analysis.
> 
> I've had no luck trying to refactor this using rwsem. I always
> run into lockdep problems between the matrix_dev->lock
> and matrix_mdev->rwsem, even if the locking order is maintained.

Usually when people start open coding locks it is often because
lockdep complained.

Open coding a lock makes lockdep stop because the lockdep code is
removed, but it doesn't fix anything.

> Clearly, I am lacking in understanding of how these locks
> interact. Any clues here?

I'd have to see the lockdep reports and look at it quite a lot
more. 

Jason

Re: [PATCH v4 0/2] s390/vfio-ap: fix memory leak in mdev remove callback

[Christian Borntraeger]

On 21.05.21 21:36, Tony Krowiak wrote:
> Fixes a memory leak in the mdev remove callback when invoked while the
> mdev is in use by a KVM guest. Instead of returning -EBUSY from the
> callback, a full cleanup of the resources allocated to the mdev is
> performed because regardless of the value returned from the function, the
> mdev is removed from sysfs.
> 
> The cleanup of resources allocated to the mdev may coincide with the
> interception of the PQAP(AQIC) instruction in which case data needed to
> handle the interception may get removed. A patch is included in this series
> to synchronize access to resources needed by the interception handler to
> protect against invalid memory accesses.
> 
> The first pass (PATCH v3) at trying to synchronize access to the pqap
> function pointer employed RCU. The problem is, the RCU read-side critical
> section would have to include the execution of the pqap function which
> sleeps; RCU disallows sleeping inside an RCU region. When I subsequently
> tried to encompass the pqap function within the
> rcu_read_lock/rcu_read_unlock, I ended up seeing lockdep warnings in the
> syslog.
> 
> It was suggested that we use an rw_semaphore to synchronize access to
> the pqap hook, but I also ran into similar lockdep complaints something
> like the following:
> 
>    Possible unsafe locking scenario:
> 
>          CPU0                            CPU1
>          ----                            ----
>     down_read(&rwsem)
>     in handle_pqap (priv.c);
>     
>                                    lock(&matrix_dev->lock);
>                                    in vfio_ap_mdev_set_kvm (vfio_ap_ops.c)
>                                  
>                                    down_write(&rwsem;
>                                    in vfio_ap_mdev_set_kvm (vfio_ap_ops.c)
>                                  
>     lock(&matrix_dev->lock);
>     in handle_pqap(vfio_ap_ops.c)
> 
> Access to the mdev must be done under the matrix_dev->lock to ensure that
> it doesn't get freed via the remove callback while in use. This appears
> to be mutually exclusive with setting/unsetting the pqap_hook pointer
> due to lockdep issues.
> 
> The solution:
> ------------
> The lifetime of the handle_pqap function (vfio_ap_ops) is syncrhonous
> with the lifetime of the vfio_ap module, so there really is not reason
> to tie the setting/clearing of its function pointer with the lifetime
> of a guest or even an mdev. If the function pointer is set when the
> vfio_ap module is loaded and cleared when the vfio_ap module is unloaded,
> then access to it can be protected independently from mdev creation or
> removal as well as the starting or shutdown of a guest. As long as
> access to the mdev is always controlled by the matrix_dev->lock, the
> mdev can not be freed without other functions being aware.
> 
> Change log:
> v3 -> v4:
> --------
> * Created a registry for crypto hooks in priv.c with functions for
>    registering/unregistering function pointers in kvm_host.h (for s390).
> 
> * Register the function pointer for handling the PQAP instruction when
>    the vfio_ap module is loaded and unregister it when the module is
>    unloaded.

Was there a v5? I cannot find it.

Re: [PATCH v4 0/2] s390/vfio-ap: fix memory leak in mdev remove callback

[Tony Krowiak]

On 6/14/21 3:51 AM, Christian Borntraeger wrote:
>
> On 21.05.21 21:36, Tony Krowiak wrote:
>> Fixes a memory leak in the mdev remove callback when invoked while the
>> mdev is in use by a KVM guest. Instead of returning -EBUSY from the
>> callback, a full cleanup of the resources allocated to the mdev is
>> performed because regardless of the value returned from the function, 
>> the
>> mdev is removed from sysfs.
>>
>> The cleanup of resources allocated to the mdev may coincide with the
>> interception of the PQAP(AQIC) instruction in which case data needed to
>> handle the interception may get removed. A patch is included in this 
>> series
>> to synchronize access to resources needed by the interception handler to
>> protect against invalid memory accesses.
>>
>> The first pass (PATCH v3) at trying to synchronize access to the pqap
>> function pointer employed RCU. The problem is, the RCU read-side 
>> critical
>> section would have to include the execution of the pqap function which
>> sleeps; RCU disallows sleeping inside an RCU region. When I subsequently
>> tried to encompass the pqap function within the
>> rcu_read_lock/rcu_read_unlock, I ended up seeing lockdep warnings in the
>> syslog.
>>
>> It was suggested that we use an rw_semaphore to synchronize access to
>> the pqap hook, but I also ran into similar lockdep complaints something
>> like the following:
>>
>>    Possible unsafe locking scenario:
>>
>>          CPU0                            CPU1
>>          ----                            ----
>>     down_read(&rwsem)
>>     in handle_pqap (priv.c);
>> lock(&matrix_dev->lock);
>>                                    in vfio_ap_mdev_set_kvm 
>> (vfio_ap_ops.c)
>>                                    down_write(&rwsem;
>>                                    in vfio_ap_mdev_set_kvm 
>> (vfio_ap_ops.c)
>> lock(&matrix_dev->lock);
>>     in handle_pqap(vfio_ap_ops.c)
>>
>> Access to the mdev must be done under the matrix_dev->lock to ensure 
>> that
>> it doesn't get freed via the remove callback while in use. This appears
>> to be mutually exclusive with setting/unsetting the pqap_hook pointer
>> due to lockdep issues.
>>
>> The solution:
>> ------------
>> The lifetime of the handle_pqap function (vfio_ap_ops) is syncrhonous
>> with the lifetime of the vfio_ap module, so there really is not reason
>> to tie the setting/clearing of its function pointer with the lifetime
>> of a guest or even an mdev. If the function pointer is set when the
>> vfio_ap module is loaded and cleared when the vfio_ap module is 
>> unloaded,
>> then access to it can be protected independently from mdev creation or
>> removal as well as the starting or shutdown of a guest. As long as
>> access to the mdev is always controlled by the matrix_dev->lock, the
>> mdev can not be freed without other functions being aware.
>>
>> Change log:
>> v3 -> v4:
>> --------
>> * Created a registry for crypto hooks in priv.c with functions for
>>    registering/unregistering function pointers in kvm_host.h (for s390).
>>
>> * Register the function pointer for handling the PQAP instruction when
>>    the vfio_ap module is loaded and unregister it when the module is
>>    unloaded.
>
> Was there a v5? I cannot find it.

I'm sorry, it morphed into a different set of patches due to the 
addition of a
patch precipitated by review comments of an unrelated issue. I pushed a
v5 today that contains only the relevant patches. I believe that set can be
integrated.