Re: [PATCH 17/17] dmaengine: idxd: run the interrupt-handle revoke under the queue lock

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] dmaengine: idxd: deadlock on wq_lock in idxd_abort_invalid_int_handle_descs()
- [High] dmaengine: idxd: drain and handle update on killed wq in idxd_int_handle_revoke()
- [Medium] dmaengine: idxd: handle leak and rebind race in idxd_int_handle_revoke()
--

commit 2920ff390e78f9271a288774e6a5796ea738663d
Author: Daniel Byrne <daniel1.byrne@intel.com>

dmaengine: idxd: run the interrupt-handle revoke under the queue lock

This commit acquires wq_lock in idxd_int_handle_revoke() after requesting a
new interrupt handle so that handle updates, drains, or failure quiesce and
descriptor aborts serialize against workqueue unbind and halt.

> diff --git a/drivers/dma/idxd/irq.c b/drivers/dma/idxd/irq.c
> index 4517a09fcf1b9..8217698056bb4 100644
> --- a/drivers/dma/idxd/irq.c
> +++ b/drivers/dma/idxd/irq.c
> @@ -115,6 +115,11 @@ static void idxd_abort_invalid_int_handle_descs(struct idxd_irq_entry *ie)
>  	struct idxd_desc *d, *t;
>  	struct llist_node *head;
>  
> +	/*
> +	 * An unbind frees the descriptors and the allocator under
> +	 * wq_lock, so they are completed under it as well.
> +	 */
> +	lockdep_assert_held(&ie_to_wq(ie)->wq_lock);
>  	spin_lock(&ie->list_lock);
>  	head = llist_del_all(&ie->pending_llist);
>  	if (head) {
> @@ -142,6 +147,7 @@ static void idxd_int_handle_revoke(struct work_struct *work)
>  	struct pci_dev *pdev = idxd->pdev;
>  	struct device *dev = &pdev->dev;
>  	int i, new_handle, rc;
> +	bool killed;
>  
>  	if (!idxd->request_int_handles) {
>  		kfree(revoke);
> @@ -166,15 +172,31 @@ static void idxd_int_handle_revoke(struct work_struct *work)
>  			continue;
>  
>  		rc = idxd_device_request_int_handle(idxd, i, &new_handle, IDXD_IRQ_MSIX);
> -		if (rc < 0) {
> +		if (rc < 0)
>  			dev_warn(dev, "get int handle %d failed: %d\n", i, rc);
> +
> +		/*
> +		 * The request runs without wq_lock, so that a reset can end
> +		 * it, and the queue may have been unbound meanwhile. An
> +		 * unbind invalidates the handle in the same wq_lock hold
> +		 * that unmaps the portal and frees the descriptors, so a
> +		 * handle still valid under the lock means that the queue
> +		 * still has its portal, its descriptors and its completion
> +		 * handler. Everything below, the drain included, stays in
> +		 * this hold.
> +		 */
> +		guard(mutex)(&wq->wq_lock);
> +		if (ie->int_handle == INVALID_INT_HANDLE)
> +			continue;

[Severity: Medium]
Can this leak new_handle or race with a concurrent unbind and rebind while
wq->wq_lock is not held during idxd_device_request_int_handle()?

First, if idxd_device_request_int_handle() succeeds (rc == 0) while a
concurrent unbind runs idxd_drv_disable_wq() -> idxd_wq_free_irq():

drivers/dma/idxd/device.c:idxd_wq_free_irq() {
	if (idxd->request_int_handles)
		idxd_device_release_int_handle(idxd, ie->int_handle, IDXD_IRQ_MSIX);
	idxd_device_clear_perm_entry(idxd, ie);
	ie->vector = -1;
	ie->int_handle = INVALID_INT_HANDLE;
	ie->pasid = IOMMU_PASID_INVALID;
}

idxd_wq_free_irq() only releases the old ie->int_handle and sets
ie->int_handle = INVALID_INT_HANDLE. When idxd_int_handle_revoke() then
acquires wq->wq_lock and sees ie->int_handle == INVALID_INT_HANDLE, it hits
continue without calling idxd_device_release_int_handle() on new_handle.

Second, if the queue is unbound and then rebound via idxd_drv_enable_wq() ->
idxd_wq_request_irq() before idxd_int_handle_revoke() acquires wq->wq_lock:

drivers/dma/idxd/device.c:idxd_wq_request_irq() {
	if (idxd->request_int_handles) {
		rc = idxd_device_request_int_handle(idxd, ie->id, &ie->int_handle,
						    IDXD_IRQ_MSIX);
		if (rc < 0)
			goto err_int_handle;
	} else {
		ie->int_handle = ie->id;
	}
}

ie->int_handle is populated with a new valid handle for the rebound queue.
When idxd_int_handle_revoke() acquires wq->wq_lock, ie->int_handle is no
longer INVALID_INT_HANDLE, so it treats the newly rebound queue as the old
binding: if the earlier idxd_device_request_int_handle() failed (for example,
while the permission entry was cleared during the unbind), it overwrites the
new ie->int_handle with INVALID_INT_HANDLE and quiesces the rebound queue; if
it succeeded before the rebind, it overwrites the rebound queue's
ie->int_handle with the stale new_handle.

> +
> +		if (rc < 0) {
>  			/*
>  			 * Failed to acquire new interrupt handle. Kill the WQ
>  			 * and release all the pending submitters. The submitters will
>  			 * get error return code and handle appropriately.
>  			 */
>  			ie->int_handle = INVALID_INT_HANDLE;
> -			idxd_wq_quiesce(wq);
> +			__idxd_wq_quiesce(wq);
>  			idxd_abort_invalid_int_handle_descs(ie);
>  			continue;
>  		}

[Severity: High]
Can calling idxd_abort_invalid_int_handle_descs() while holding wq->wq_lock
deadlock on wq->wq_lock?

idxd_abort_invalid_int_handle_descs() completes descriptors synchronously:

idxd_int_handle_revoke()
  guard(mutex)(&wq->wq_lock)
  idxd_abort_invalid_int_handle_descs(ie)
    idxd_desc_complete(d, IDXD_COMPLETE_ABORT, true)
      idxd_dma_complete_txd(d, IDXD_COMPLETE_ABORT, true, NULL, NULL)
        dmaengine_desc_get_callback_invoke(tx, &res)

For a dmaengine workqueue, the client completion callback is invoked
synchronously under wq->wq_lock. As noted in idxd_flush_pending_descs() in
drivers/dma/idxd/device.c:

	/*
	 * A halt flush that delivers the callbacks of a dmaengine queue
	 * leaves its descriptors to the interrupt thread, which runs them
	 * without wq_lock, as it runs every completion. A callback may
	 * terminate the channel, which takes that lock, and the halt flush
	 * holds it. The halted device writes no record any more, so one it
	 * never finished is marked aborted for the thread to complete.
	 */

If the client callback terminates the channel on error via
dmaengine_terminate_async() or dmaengine_terminate_all(), it enters
idxd_dma_terminate_all():

drivers/dma/idxd/dma.c:idxd_dma_terminate_all() {
	struct idxd_wq *wq = to_idxd_wq(c);

	guard(mutex)(&wq->wq_lock);
	...
}

Wouldn't that immediately self-deadlock on wq->wq_lock?

> @@ -193,21 +215,20 @@ static void idxd_int_handle_revoke(struct work_struct *work)
>  			continue;
>  		}
>  
> -		mutex_lock(&wq->wq_lock);
> -		reinit_completion(&wq->wq_resurrect);
> -
> -		/* Kill percpu_ref to pause additional descriptor submissions */
> -		percpu_ref_kill(&wq->wq_active);
> -
> -		/* Wait for all submitters quiesce before we change interrupt handle */
> -		wait_for_completion(&wq->wq_dead);
> +		/*
> +		 * Kill percpu_ref to pause additional descriptor submissions
> +		 * and wait for all submitters to quiesce before we change
> +		 * the interrupt handle. A queue a halt already killed and did
> +		 * not revive is left dead.
> +		 */
> +		killed = idxd_wq_kill(wq);
>  
>  		ie->int_handle = new_handle;
>  
>  		/* Revive percpu ref and wake up all the waiting submitters */
> -		percpu_ref_reinit(&wq->wq_active);
> +		if (killed)
> +			percpu_ref_reinit(&wq->wq_active);
>  		complete_all(&wq->wq_resurrect);
> -		mutex_unlock(&wq->wq_lock);
>  
>  		/*
>  		 * The delay here is to wait for all possible MOVDIR64B that

[Severity: High]
When idxd_wq_kill(wq) returns false because a halt or teardown quiesce
already killed wq->wq_active, is it safe to still update ie->int_handle and
call idxd_int_handle_revoke_drain(ie)?

In idxd_halt() in drivers/dma/idxd/irq.c:

		if (halt_flr(err)) {
			idxd->state = IDXD_DEV_HALTED;
			...
			idxd_wqs_halt_flush(idxd, false);
			for (i = 0; i < idxd->max_wqs; i++) {
				struct idxd_wq *wq = idxd->wqs[i];

				if (wq->type == IDXD_WQT_KERNEL)
					WRITE_ONCE(wq->ie.int_handle, INVALID_INT_HANDLE);
			}
			...
		} else {
			idxd->state = IDXD_DEV_HALTED;
			/* Same as above: nothing in flight will ever complete. */
			idxd_wqs_halt_flush(idxd, false);
			idxd_wqs_unmap_portal(idxd);
			idxd_device_clear_state(idxd);

In the system-reset branch, idxd_wqs_halt_flush() kills wq->wq_active under
wq->wq_lock and unlocks, while wq->state is still IDXD_WQ_ENABLED and
ie->int_handle is not invalidated. Next, idxd_wqs_unmap_portal() unmaps
wq->portal and sets wq->portal = NULL without holding wq->wq_lock before
idxd_device_clear_state() clears wq->state under wq->wq_lock (and on
non-configurable devices without IDXD_FLAG_CONFIGURABLE,
idxd_device_clear_state() does not clear wq->state at all).

If idxd_int_handle_revoke() acquires wq->wq_lock after idxd_wqs_halt_flush(),
idxd_wq_kill(wq) returns false, but idxd_int_handle_revoke() proceeds to
idxd_int_handle_revoke_drain(ie):

drivers/dma/idxd/irq.c:idxd_int_handle_revoke_drain() {
	...
	portal = idxd_wq_portal_addr(wq);
	...
	wmb();
	if (wq_dedicated(wq)) {
		iosubmit_cmds512(portal, &desc, 1);
	} else {
		rc = idxd_enqcmds(wq, portal, &desc);

which dereferences wq->portal while it is NULL or being unmapped.

Similarly, in the FLR branch of idxd_halt(), wq->ie.int_handle is set to
INVALID_INT_HANDLE outside wq->wq_lock after idxd_wqs_halt_flush(). If
idxd_int_handle_revoke() acquires wq->wq_lock after idxd_wqs_halt_flush()
before that write, or if iaa_crypto_remove() has called idxd_wq_quiesce(wq)
prior to disabling the queue, idxd_wq_kill(wq) returns false, yet
idxd_int_handle_revoke() still writes ie->int_handle = new_handle and submits
a hardware drain descriptor to the halted or quiesced queue.

Should idxd_int_handle_revoke() avoid updating ie->int_handle and issuing the
drain when killed is false (or when the device is halted)?

титься

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019150630.4026341-1-daniel1.byrne@intel.com?part=17