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

[Anthony Krowiak]

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/ ?

I had to change that string at one time because the compiler
complained about the AP_RESET_TIMEOUT being a duplicate.
I'll fix it.

> > 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@xxxxxxxxxxxxxxx
> > Signed-off-by: Anthony Krowiak <akrowiak@xxxxxxxxxxxxx>
> > ---
> >   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.
>
> 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?
>
> 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.
>
> > +			}
> >   		} else {
> >   			if (q->reset_status.response_code == AP_RESPONSE_RESET_IN_PROGRESS ||
> >   			    q->reset_status.response_code == AP_RESPONSE_BUSY ||