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

Anthony Krowiak <[email protected]>
Newsgroups org.kernel.vger.kvm,org.kernel.vger.linux-kernel,org.kernel.vger.linux-s390,org.kernel.vger.stable
Message-ID <[email protected]>
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_MAX_WAIT (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_MAX_WAIT (2000ms) and breaking out
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: [email protected]
Signed-off-by: Anthony Krowiak <[email protected]>
---
 drivers/s390/crypto/vfio_ap_ops.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index 3f99b239fa95..7a9b5448e90a 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);
@@ -1971,6 +1972,27 @@ 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.
+				 *
+				 * The AQIC resources associated with this queue - the pinned page
+				 * containing the NIB and the registered guest ISC - cannot be freed
+				 * here. The NIB is the active DMA target for AP interrupt delivery
+				 * until the reset completes; freeing the pinned page while the
+				 * hardware may still write to it would result in a use-after-free
+				 * kernel crash.
+				 *
+				 * If the reset eventually completes, interrupts will be terminated
+				 * and the pinned NIB page and ISC registration will be leaked. This
+				 * is preferable to either a use-after-free or waiting indefinitely:
+				 * apq_reset_check() holds the matrix_dev->mdevs_lock mutex, which
+				 * serializes access to all mdev objects system-wide, so blocking
+				 * here would stall all other guests using AP queues.
+				 */
+				memcpy(&q->reset_status, &status, sizeof(status));
+				return;
+			}
 		} else {
 			if (q->reset_status.response_code == AP_RESPONSE_RESET_IN_PROGRESS ||
 			    q->reset_status.response_code == AP_RESPONSE_BUSY ||
-- 
2.53.0
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.