Re: [PATCH] s390/vfio-ap: fix unregister GISC when KVM is already gone results in OOPS

[Tony Krowiak <akrowiak@linux.ibm.com>]

On 9/21/20 11:45 AM, Halil Pasic wrote:
> On Fri, 18 Sep 2020 13:02:34 -0400
> Tony Krowiak <akrowiak@linux.ibm.com> wrote:
>
>> Attempting to unregister Guest Interruption Subclass (GISC) when the
>> link between the matrix mdev and KVM has been removed results in the
>> following:
>>
>>     "Kernel panic -not syncing: Fatal exception: panic_on_oops"
>>
>> This patch fixes this bug by verifying the matrix mdev and KVM are still
>> linked prior to unregistering the GISC.
>
> I read from your commit message that this happens when the link between
> the KVM and the matrix mdev was established and then got severed.
>
> I assume the interrupts were previously enabled, and were not been
> disabled or cleaned up because q->saved_isc != VFIO_AP_ISC_INVALID.
>
> That means the guest enabled  interrupts and then for whatever
> reason got destroyed, and this happens on mdev cleanup.
>
> Does it happen all the time or is it some sort of a race?

This is a race condition that happens when a guest is terminated and the 
mdev is
removed in rapid succession. I came across it with one of my hades test 
cases
on cleanup of the resources after the test case completes. There is a 
bug in the problem appears
the vfio_ap_mdev_release function because it tries to reset the APQNs 
after the bits are
cleared from the matrix_mdev.matrix, so the resets never happen.

Fixing that, however, does not resolve the issue, so I'm in the process 
of doing a bunch of
tracing to see the flow of the resets etc. during the lifecycle of the 
mdev during this
hades test. I should have a better answer next week.

>
>> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
>> ---
>>   drivers/s390/crypto/vfio_ap_ops.c | 14 +++++++++-----
>>   1 file changed, 9 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
>> index e0bde8518745..847a88642644 100644
>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
>> @@ -119,11 +119,15 @@ static void vfio_ap_wait_for_irqclear(int apqn)
>>    */
>>   static void vfio_ap_free_aqic_resources(struct vfio_ap_queue *q)
>>   {
>> -	if (q->saved_isc != VFIO_AP_ISC_INVALID && q->matrix_mdev)
>> -		kvm_s390_gisc_unregister(q->matrix_mdev->kvm, q->saved_isc);
>> -	if (q->saved_pfn && q->matrix_mdev)
>> -		vfio_unpin_pages(mdev_dev(q->matrix_mdev->mdev),
>> -				 &q->saved_pfn, 1);
>> +	if (q->matrix_mdev) {
>> +		if (q->saved_isc != VFIO_AP_ISC_INVALID && q->matrix_mdev->kvm)
>> +			kvm_s390_gisc_unregister(q->matrix_mdev->kvm,
>> +						 q->saved_isc);
> I don't quite understand the logic here. I suppose we need to ensure
> that the struct kvm is 'alive' at least until kvm_s390_gisc_unregister()
> is done. That is supposed be ensured by kvm_get_kvm() in
> vfio_ap_mdev_set_kvm() and kvm_put_kvm() in vfio_ap_mdev_release().
>
> If the critical section in vfio_ap_mdev_release() is done and
> matrix_mdev->kvm was set to NULL there then I would expect that the
> queues are already reset and q->saved_isc == VFIO_AP_ISC_INVALID. So
> this should not blow up.
>
> Now if this happens before the critical section in
> vfio_ap_mdev_release() is done, I ask myself how are we going to do the
> kvm_put_kvm()?
>
> Another question. Do we hold the matrix_dev->lock here?
>
>> +		if (q->saved_pfn)
>> +			vfio_unpin_pages(mdev_dev(q->matrix_mdev->mdev),
>> +					 &q->saved_pfn, 1);
>> +	}
>> +
>>   	q->saved_pfn = 0;
>>   	q->saved_isc = VFIO_AP_ISC_INVALID;
>>   }