Re: [PATCH] s390/vfio-ap: Fix leak of KVM GISC resources

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Use-After-Free of `q->matrix_mdev` due to dangling pointer left after unassigning an adapter or domain for a queue not present in shadow_apcb.
- [High] The patch fixes KVM GISC and NIB page leaks when a queue is synchronously reset by handling AP_RESPONSE_Q_NOT_AVAIL, but it misses the asynchronous reset polling path, leading to the same resource leaks.
- [High] The apq_reset_check workqueue function loops indefinitely without a timeout while waiting for an AP queue reset to complete, causing a KVM process deadlock if hardware fails.
--

--- Patch [1]: [PATCH] s390/vfio-ap: Fix leak of KVM GISC resources ---
commit c19d4565d7b440cb8eddb06c5a8a375c867d24ea
Author: Anthony Krowiak <[email protected]>

s390/vfio-ap: Fix leak of KVM GISC resources

Two related problems exist in the handling of KVM interrupt and page
resources when a queue is removed from the host's AP configuration
while assigned to a mediated device (mdev). This patch addresses the
AP_RESPONSE_Q_NOT_AVAIL response from ap_zapq() and fixes the
vfio_ap_free_aqic_resources() cleanup.

> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index 44b3a1dcc1b36..e277062e92bea 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
[ ... ]
> @@ -2004,6 +2004,7 @@ static void vfio_ap_mdev_reset_queue(struct vfio_ap_queue *q)
>  		break;
>  	case AP_RESPONSE_DECONFIGURED:
>  	case AP_RESPONSE_CHECKSTOPPED:
> +	case AP_RESPONSE_Q_NOT_AVAIL:
>  		vfio_ap_free_aqic_resources(q);
>  		break;
>  	default:

[Severity: High]
This isn't a bug introduced by this patch, but does the same resource leak
happen when the asynchronous polling path in apq_reset_check() encounters an
AP_RESPONSE_Q_NOT_AVAIL response?

When an adapter or domain is removed from the host config while a reset is
asynchronously polling, ap_tapq() can return AP_RESPONSE_Q_NOT_AVAIL. The
helper apq_status_check() falls through to its default case and returns -EIO:

drivers/s390/crypto/vfio_ap_ops.c:apq_status_check() {
    ...
    default:
        WARN(true,
             "failed to verify reset of queue %02x.%04x: TAPQ rc=%u\n",
             AP_QID_CARD(apqn), AP_QID_QUEUE(apqn),
             status->response_code);
        return -EIO;
}

This causes apq_reset_check() to exit immediately, skipping the cleanup:

drivers/s390/crypto/vfio_ap_ops.c:apq_reset_check() {
    ...
        ret = apq_status_check(q->apqn, &status);
        if (ret == -EIO)
            return;
    ...
}

Does the default case in vfio_ap_mdev_reset_queue() also need to call
vfio_ap_free_aqic_resources() to prevent a leak on an unhandled ZAPQ
response?

>  		WARN(true,
>  		     "PQAP/ZAPQ for %02x.%04x failed with invalid rc=%u\n",
>  		     AP_QID_CARD(q->apqn), AP_QID_QUEUE(q->apqn),
>  		     status.response_code);
>  	}
>  }
[ ... ]

[Severity: Critical]
This is a pre-existing issue, but is there a use-after-free of q->matrix_mdev
if an adapter is unassigned that is not passed through to a guest?

If an adapter is unassigned, its bit is cleared from matrix.apm and
vfio_ap_mdev_unlink_adapter() removes the queue from the mdev's hash table.
However, if the queue's bit is not set in shadow_apcb, it is skipped from
qlist:

drivers/s390/crypto/vfio_ap_ops.c:vfio_ap_mdev_unlink_adapter() {
    ...
        if (q && qlist) {
            if (test_bit_inv(apid, matrix_mdev->shadow_apcb.apm) &&
                test_bit_inv(apqi, matrix_mdev->shadow_apcb.aqm))
                list_add_tail(&q->reset_qnode, qlist);
        }
    ...
}

Because it is missing from qlist, vfio_ap_mdev_hot_unplug_adapters() skips
calling vfio_ap_unlink_mdev_fr_queue(q):

drivers/s390/crypto/vfio_ap_ops.c:vfio_ap_mdev_hot_unplug_adapters() {
    ...
    list_for_each_entry_safe(q, tmpq, &qlist, reset_qnode) {
        vfio_ap_unlink_mdev_fr_queue(q);
        list_del(&q->reset_qnode);
    }
}

This leaves q->matrix_mdev dangling. Later, when the AP bus queue device is
removed, get_update_locks_for_queue() dereferences the freed pointer:

drivers/s390/crypto/vfio_ap_ops.c:get_update_locks_for_queue() {
    ...
    if (q->matrix_mdev && q->matrix_mdev->kvm)
        mutex_lock(&q->matrix_mdev->kvm->lock);
    ...
}

Can this lead to a kernel panic during normal operational reconfiguration?


[Severity: High]
This is a pre-existing issue, but will apq_reset_check() poll indefinitely
without a timeout if the hardware wedges?

drivers/s390/crypto/vfio_ap_ops.c:apq_reset_check() {
    ...
    while (true) {
        msleep(AP_RESET_INTERVAL);
        elapsed += AP_RESET_INTERVAL;
        status = ap_tapq(q->apqn, NULL);
        ret = apq_status_check(q->apqn, &status);
        if (ret == -EIO)
            return;
        if (ret == -EBUSY) {
            pr_notice_ratelimited(...);
        } else {
            ...
            if (q->reset_status.response_code == AP_RESPONSE_RESET_IN_PROGRESS ||
                ...
                ret == -EAGAIN) {
                status = ap_zapq(q->apqn, 0);
                ...
                continue;
            }
            ...
            break;
        }
    }
}

Operations like vfio_ap_mdev_reset_queues() schedule this work and synchronously
wait for it while holding global locks:

drivers/s390/crypto/vfio_ap_ops.c:vfio_ap_mdev_reset_queues() {
    ...
    hash_for_each(matrix_mdev->qtable.queues, loop_cursor, q, mdev_qnode) {
        flush_work(&q->reset_work);

        if (q->reset_status.response_code)
            ret = -EIO;
    }
    ...
}

If the queue is stuck in hardware and continuously returns AP_RESPONSE_BUSY or
AP_RESPONSE_RESET_IN_PROGRESS, will this permanently block the locks and
deadlock the host driver and KVM process?

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