Skip to contentSkip to conversation

vfio/ccw: Flush notoper_work before returning from reset

PATCH Superseded Yuho Choi 2 participants 2 messages Sep 22

Subsystems (4)

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Yuho Choi

author
#0 lore raw

PATCH 1/1 vfio/ccw: Flush notoper_work before returning from reset

vfio_ccw_dma_unmap() is the DMA invalidation callback, which must unpin the pages covering the range before it returns. It calls vfio_ccw_mdev_reset(), which raises a CLOSE event.

On the normal path fsm_close() calls cp_free() itself, so the pages are unpinned synchronously. But if the subchannel cannot be disabled or quiesced, fsm_close() falls through to a NOT_OPER event, and fsm_notoper() only queues notoper_work, which is where cp_free() then runs. Nothing waits for that work, so the callback returns with the pages still pinned.

Because notoper_work runs asynchronously, vfio_dma_do_unmap() can exhaust its 10 retries before cp_free() completes, hitting BUG_ON(++retries > 10) and taking the host down. A further CLOSE cannot help either, as both CLOSE and OPEN are fsm_nop in the NOT_OPER state.

Flush notoper_work after the CLOSE event, as vfio_ccw_mdev_close_device() already does for the same reason. The flush returns immediately when the work was never queued, so the normal path is unaffected.

Fixes ce4b4657ff18 ("vfio: Replace the DMA unmapping notifier with a callback")

---
Found by code review. Cross-compiled for s390 with gcc 14.3.0, W=1 clean, but not tested on s390 hardware and I have no reproducer.

VFIO_DEVICE_RESET also reaches vfio_ccw_mdev_reset() and benefits from this flush, ensuring channel program memory is cleanly released before the reset ioctl returns.

On locking: the flush is not called with io_mutex held. cp_iova_pinned() takes and drops it before vfio_ccw_mdev_reset() runs, vfio_ccw_fsm_event() takes no lock, and the VFIO_DEVICE_RESET path holds nothing. This matches vfio_ccw_mdev_close_device(), which already flushes the same work.

 drivers/s390/cio/vfio_ccw_ops.c | 8 ++++++++
 1 file changed, 8 insertions(+)
drivers/s390/cio/vfio_ccw_ops.c +8 -0
index 5ce91285c7d5..8e0a2edbd34a 100644
@@ -25,6 +25,14 @@ static int vfio_ccw_mdev_reset(struct vfio_ccw_private *private)
 	 * and re-opening the mdev, return an error.
 	 */
 	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_CLOSE);
+
+	/*
+	 * A failed CLOSE leaves the FSM Not Operational and defers cp_free()
+	 * to notoper_work. Wait for it, so the channel program pages are
+	 * unpinned before this returns.
+	 */
+	flush_work(&private->notoper_work);
+
 	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_OPEN);
 	if (private->state == VFIO_CCW_STATE_NOT_OPER)
 		return -EINVAL;

base-commit: f0100363d8c374bd8e9ea7c9ba02744f0b802ca4

Signature · 1 line
2.43.0
#1 lore raw
Sashiko has reviewed this patch and found no issues. It looks great!
Quoted 15 lines

vfio_ccw_dma_unmap() is the DMA invalidation callback, which must unpin the pages covering the range before it returns. It calls vfio_ccw_mdev_reset(), which raises a CLOSE event.

On the normal path fsm_close() calls cp_free() itself, so the pages are unpinned synchronously. But if the subchannel cannot be disabled or quiesced, fsm_close() falls through to a NOT_OPER event, and fsm_notoper() only queues notoper_work, which is where cp_free() then runs. Nothing waits for that work, so the callback returns with the pages still pinned.

Because notoper_work runs asynchronously, vfio_dma_do_unmap() can exhaust its 10 retries before cp_free() completes, hitting BUG_ON(++retries > 10) and taking the host down. A further CLOSE cannot help either, as both CLOSE and OPEN are fsm_nop in the NOT_OPER state.

Quoted 6 lines

Flush notoper_work after the CLOSE event, as vfio_ccw_mdev_close_device() already does for the same reason. The flush returns immediately when the work was never queued, so the normal path is unaffected.

Fixes ce4b4657ff18 ("vfio: Replace the DMA unmapping notifier with a callback")

Sashiko has reviewed this patch and found no issues. It looks great!

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1