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

On 8/18/26 8:15 AM, [email protected] wrote:
> 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:

This is a legitimate concern that needs to be fixed. While this bug was not
introduced by this patch, the fix is related to the purpose of this
patch, so a v2 will be posted.

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

This covers truly invalid/unexpected response codes — it would not be
appropriate to unconditionally free resources in that case without
knowing the queue state. I might also add that this should never
happen as all response codes expected by the architecture are
covered by the cases in the switch statement - except for response
code 03 which will be captured in v2 of this patch - so the default
case is handled correctly.

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

This may be a legitimate concern, I'll look into it and post a patch if 
determined
to be necessary.

>