Re: [PATCH 3/4] s390/vfio-ap: Fix unbounded loop in apq_reset_check()

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

On 8/24/26 1:04 PM, Matthew Rosato wrote:
> On 8/24/26 9:58 AM, Anthony Krowiak wrote:
>> The apq_reset_check() worker polls ap_tapq() in a while(true) loop
>> waiting for a queue reset to complete. When ap_tapq() returns
>> AP_RESPONSE_BUSY or AP_RESPONSE_RESET_IN_PROGRESS,
>> apq_status_check() returns -EBUSY and the loop continues after
>> sleeping AP_RESET_INTERVAL (20ms). There is no upper bound on how
>> many times the loop iterates, so if the hardware continuously
>> returns a busy response the worker runs indefinitely.
>>
>> This is particularly harmful because several callers of
>> vfio_ap_mdev_reset_queues() and vfio_ap_mdev_reset_qlist() call
>> flush_work() on each queue's reset_work while holding one or more
>> of the global matrix_dev locks (guests_lock, mdevs_lock) or the
>> KVM lock. An indefinitely spinning worker permanently blocks all
>> of those locks, hanging mdev removal, KVM guest teardown, and the
>> VFIO_DEVICE_RESET ioctl path.
>>
>> Fix this by introducing AP_RESET_TIMEOUT (2000ms) and breaking out
> s/AP_RESET_TIMEOUT/AP_RESET_MAX_WAIT/ ?
>
>> of the poll loop when elapsed time reaches that threshold. On
>> timeout the final busy status is written back to q->reset_status
>> so that callers inspecting reset_status.response_code after
>> flush_work() see a non-zero value and can return an appropriate
>> error. vfio_ap_free_aqic_resources() is called before returning
>> to release any KVM ISC registration and pinned NIB page,
>> consistent with all other early-exit paths in the function.
>>
>> Fixes: dd174833e44e ("s390/vfio-ap: remove upper limit on wait for queue reset to complete")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Anthony Krowiak <akrowiak@linux.ibm.com>
>> ---
>>   drivers/s390/crypto/vfio_ap_ops.c | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
>> index 6e4569d6b975..c7eebbd0ed40 100644
>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
>> @@ -31,6 +31,7 @@
>>   #define AP_QUEUE_IN_USE "in use"
>>   
>>   #define AP_RESET_INTERVAL		20	/* Reset sleep interval (20ms)		*/
>> +#define AP_RESET_MAX_WAIT		2000	/* Maximum wait for reset (2000ms)	*/
>>   
>>   static int vfio_ap_mdev_reset_queues(struct ap_matrix_mdev *matrix_mdev);
>>   static int vfio_ap_mdev_reset_qlist(struct list_head *qlist);
>> @@ -1973,6 +1974,12 @@ static void apq_reset_check(struct work_struct *reset_work)
>>   					      status.response_code,
>>   					      status.queue_empty,
>>   					      status.irq_enabled);
>> +			if (elapsed >= AP_RESET_MAX_WAIT) {
>> +				/* Timed out waiting for reset to complete */
>> +				memcpy(&q->reset_status, &status, sizeof(status));
>> +				vfio_ap_free_aqic_resources(q);
>> +				return;
> Sashiko points out a concern here and I tend to agree; if this timer
> elapses you are effectively freeing resources that could still be in-use.

Ironically, it was sashiko that precipitated this change given the
issue of hanging forever.

>
> This seems to go back to dd174833e44e 's390/vfio-ap: remove upper limit
> on wait for queue reset to complete' where it was decided to hang
> forever vs leak resources -- e.g. the hang seems intentional?

It may have been intentional, but I don't recall.

>
> If we don't have a way of forcing firmware to give up the resources I
> think we are stuck either waiting indefinitely or quarantining (leaking)
> the resources consciously.  And documenting the rationale in a comment
> block.

The AP architecture defines only a few instructions, none of which
provide a way to give up resources. I think it best to document this
in a comment block rather than waiting indefinitely. The
apq_reset_check() function is called under the matrix_dev->mdevs_lock
mutex which is a global lock that guards access to all active mdevs
in the system. Since the likelihood of this is happening is probably
extremely rare and the amount of storage leaked is not significant,
I think it makes more sense to allow things to proceed in this case.

>
>> +			}
>>   		} else {
>>   			if (q->reset_status.response_code == AP_RESPONSE_RESET_IN_PROGRESS ||
>>   			    q->reset_status.response_code == AP_RESPONSE_BUSY ||