Re: [PATCH 2/3] dmaengine: idxd: generation-number transaction status

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [Critical] dmaengine: idxd: pre-halt epoch in idxd_dma_tx_submit() frees active desc
- [Critical] dmaengine: idxd: cookie 0 in idxd_dma_tx_status() double-frees wq->descs[0]
- [High] dmaengine: idxd: unordered state and reset_epoch in idxd_dma_tx_status()
- [High] dmaengine: idxd: untracked polled descriptors leak on terminate or abandon
- [High] dmaengine: idxd: unretired desc on disabled state in idxd_dma_tx_status()
- [High] dmaengine: idxd: stale txd state on submit failure in idxd_dma_tx_submit()
- [Medium] dmaengine: idxd: late epoch read in idxd_dma_synchronize() wedges workqueue
--

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

dmaengine: idxd: generation-number transaction status

This commit implements per-descriptor transaction status polling in
idxd_dma_tx_status() by encoding the descriptor slot ID, a 14-bit generation
counter, and an interrupt flag in the DMA cookie, and uses a device
reset_epoch counter to retire polled descriptors caught by a device halt.

> diff --git a/drivers/dma/idxd/dma.c b/drivers/dma/idxd/dma.c
> index 0a7517cb51f29..dd5a9d9274ce6 100644
> --- a/drivers/dma/idxd/dma.c
> +++ b/drivers/dma/idxd/dma.c

[ ... ]

> @@ -154,11 +168,128 @@ static void idxd_dma_free_chan_resources(struct dma_chan *chan)
>  		idxd_wq_refcount(wq));
>  }
>  
> +/*
> + * A cookie has exactly one polling owner, the client that submitted the
> + * transaction. Observing a terminal completion record here retires and
> + * recycles the descriptor, since a polled descriptor has no interrupt
> + * and nobody else will. Two contexts polling the same live cookie
> + * would race that recycle, which every in-tree dmaengine polling
> + * client already avoids by construction.
> + */
>  static enum dma_status idxd_dma_tx_status(struct dma_chan *dma_chan,
>  					  dma_cookie_t cookie,
>  					  struct dma_tx_state *txstate)
>  {
> -	return DMA_OUT_OF_ORDER;
> +	struct idxd_wq *wq;
> +	struct idxd_desc *desc;
> +	u32 idx;
> +	u8 status;
> +
> +	if (txstate)
> +		memset(txstate, 0, sizeof(*txstate));
> +
> +	if (dma_submit_error(cookie))
> +		return DMA_ERROR;
> +
> +	wq = to_idxd_wq(dma_chan);
> +
> +	idx = cookie & DESC_ID_MASK;
> +	if (idx >= wq->num_descs)
> +		return DMA_ERROR;
> +
> +	desc = wq->descs[idx];
> +
> +	/*
> +	 * Descriptors with a completion interrupt armed (RCI) are
> +	 * completed and recycled by the IRQ handler. Reading their
> +	 * completion record here would race with the handler, and so
> +	 * would reading the recycled descriptor's flags, which is why
> +	 * the property travels in the cookie the caller owns. The handler
> +	 * invalidates the cookie on completion, so a matching cookie means
> +	 * the command is still in progress.
> +	 */
> +	if (cookie & IDXD_COOKIE_RCI)
> +		return READ_ONCE(desc->txd.cookie) == cookie ?
> +			DMA_IN_PROGRESS : DMA_COMPLETE;
> +
> +	if (desc->txd.cookie != cookie) {
> +		/*
> +		 * The cookie belongs to an old transaction that has
> +		 * already completed and been recycled.
> +		 */
> +		return DMA_COMPLETE;
> +	}

[Severity: Critical]
What happens here when a caller passes cookie == 0 (or any cookie whose
generation field is 0) to idxd_dma_tx_status()?

For example, ntb_transport_free_queue() in drivers/ntb/ntb_transport.c calls
dma_sync_wait(chan, qp->last_cookie) when tearing down a queue, where
qp->last_cookie is 0 if no transfer was submitted.

Because dma_submit_error(0) returns 0, cookie == 0 selects wq->descs[0].
When wq->descs[0] is free (either newly allocated or previously completed via
idxd_dma_complete_txd() -> dma_cookie_complete()), desc->txd.cookie is 0 and
desc->cpu is -1.

As a result, desc->txd.cookie != cookie evaluates 0 != 0 (false). If
wq->descs[0] completed a previous transaction, desc->completion->status is
still non-zero (or if a halt occurred, desc->epoch != reset_epoch), so
idxd_dma_tx_status() calls idxd_dma_complete_txd() -> idxd_free_desc() on
the already-free wq->descs[0]:

drivers/dma/idxd/submit.c:idxd_free_desc() {
	int cpu = desc->cpu;

	desc->cpu = -1;
	sbitmap_queue_clear(&wq->sbq, desc->id, cpu);
}

Passing cpu = -1 into sbitmap_queue_clear() -> sbitmap_update_cpu_hint() on a
non-round-robin sbitmap_queue executes *per_cpu_ptr(sb->alloc_hint, -1) = 0,
causing an out-of-bounds per-CPU write, or double-freeing slot 0 if another
thread just allocated it in idxd_alloc_desc() before reaching
idxd_dma_tx_submit().

Should idxd_dma_tx_status() reject cookies with cookie < DMA_MIN_COOKIE or a
zero generation field before indexing wq->descs[]?

> +
> +	status = desc->completion->status & DSA_COMP_STATUS_MASK;
> +
> +	if (status) {
> +		/* Capture completion record fields before desc is freed below. */
> +		u8 fault_info = desc->completion->fault_info;
> +		u64 fault_addr = desc->completion->fault_addr;
> +
> +		/*
> +		 * Check against the original status because ABORT is
> +		 * software defined as 0xff, which DSA_COMP_STATUS_MASK
> +		 * can mask out.
> +		 */
> +		if (unlikely(desc->completion->status == IDXD_COMP_DESC_ABORT))
> +			idxd_dma_complete_txd(desc, IDXD_COMPLETE_ABORT, true, NULL, NULL);
> +		else
> +			idxd_dma_complete_txd(desc, IDXD_COMPLETE_NORMAL, true, NULL, NULL);
> +
> +		dev_dbg(&wq->idxd->pdev->dev,
> +			"dsa completion: status=0x%02x fault_info=0x%02x fault_addr=0x%llx\n",
> +			status, fault_info, fault_addr);
> +
> +		if (status == DSA_COMP_SUCCESS)
> +			return DMA_COMPLETE;
> +
> +		return DMA_ERROR;
> +	}
> +
> +	/*
> +	 * The descriptor is genuinely outstanding. A polled descriptor
> +	 * carries no interrupt, so nothing but this function will ever
> +	 * retire it, and a work queue or device that is no longer enabled
> +	 * will not complete it either. Report the failure instead of
> +	 * leaving the caller polling forever.

[Severity: High]
Since a polled descriptor is not submitted with IDXD_OP_FLAG_RCI,
__idxd_submit_desc() does not add it to wq->ie.pending_llist:

drivers/dma/idxd/submit.c:__idxd_submit_desc() {
	if (desc_flags & IDXD_OP_FLAG_RCI) {
		ie = &wq->ie;
		desc->hw->int_handle = ie->int_handle;
		if (submitter_cleans)
			desc->terminate_gen = READ_ONCE(wq->terminate_gen);
		llist_add(&desc->llnode, &ie->pending_llist);
	}
}

If a client calls dmaengine_terminate_all() / dmaengine_terminate_sync() (for
example, dmatest_func() in drivers/dma/dmatest.c or nvmet_pci_epf_dma_transfer()
after a timeout) or abandons a cookie without polling it to a terminal result,
idxd_dma_terminate_all() -> __idxd_wq_flush_descs() only flushes
ie->pending_llist and ie->work_list.

Does this leak the descriptor slot in wq->sbq and its desc->txd.unmap buffer
while also bypassing the quarantine mechanism in idxd_dma_synchronize() for
in-flight polled descriptors?

> +	 *
> +	 * A descriptor submitted before a device halt is dead for good:
> +	 * the halt ended the engines and the reset that follows starts
> +	 * them empty, so its record will never be written even once the
> +	 * device is enabled again. Only in that case can the descriptor
> +	 * be retired here safely; otherwise the pool would shrink by one
> +	 * for every descriptor caught by a halt.
> +	 */
> +	if (desc->epoch != READ_ONCE(wq->idxd->reset_epoch)) {
> +		idxd_dma_complete_txd(desc, IDXD_COMPLETE_ABORT, true, NULL, NULL);
> +		return DMA_ERROR;
> +	}
> +	/*
> +	 * A device that is no longer enabled runs nothing it holds, so an
> +	 * outstanding descriptor on it has failed. A halt stores its state
> +	 * before it bumps the epoch, and this poll may have read the epoch
> +	 * before the halt and the state after it, so the epoch is read
> +	 * again: only the retire frees the slot of a descriptor the halt
> +	 * caught. A halted device whose epoch still matches is between the
> +	 * two stores, and the next poll retires the descriptor. The submit
> +	 * reads the state after the stamp, so it refuses a descriptor
> +	 * stamped with the bumped epoch.
> +	 */
> +	if (wq->idxd->state != IDXD_DEV_ENABLED) {
> +		if (desc->epoch != READ_ONCE(wq->idxd->reset_epoch)) {
> +			idxd_dma_complete_txd(desc, IDXD_COMPLETE_ABORT, true,
> +					      NULL, NULL);
> +			return DMA_ERROR;
> +		}
> +		if (wq->idxd->state == IDXD_DEV_HALTED)
> +			return DMA_IN_PROGRESS;
> +		return DMA_ERROR;
> +	}
> +	if (wq->state != IDXD_WQ_ENABLED)
> +		return DMA_ERROR;
> +
> +	return DMA_IN_PROGRESS;
>  }

[Severity: High]
There appear to be two issues in the non-enabled state handling at the end of
idxd_dma_tx_status():

First, wq->idxd->state is read twice without READ_ONCE() and without memory
barriers ordering it against READ_ONCE(wq->idxd->reset_epoch), while
idxd_halt() in drivers/dma/idxd/irq.c also writes idxd->state = IDXD_DEV_HALTED
without WRITE_ONCE() or a write barrier before bumping idxd->reset_epoch:

drivers/dma/idxd/irq.c:idxd_halt() {
	if (gensts.state == IDXD_DEVICE_STATE_HALT) {
		idxd->state = IDXD_DEV_HALTED;
		WRITE_ONCE(idxd->reset_epoch, idxd->reset_epoch + 1);
}

If a poller observes wq->idxd->state != IDXD_DEV_ENABLED (because it is
IDXD_DEV_HALTED) and desc->epoch == READ_ONCE(wq->idxd->reset_epoch) before
idxd_halt() increments reset_epoch, and idxd_halt() then bumps reset_epoch and
transitions idxd->state to IDXD_DEV_DISABLED or IDXD_DEV_ENABLED before the
second check:

	if (wq->idxd->state == IDXD_DEV_HALTED)
		return DMA_IN_PROGRESS;
	return DMA_ERROR;

won't the second read of wq->idxd->state see a state other than
IDXD_DEV_HALTED and return terminal DMA_ERROR without retiring desc?
Similarly, without barriers between idxd->state and idxd->reset_epoch in
idxd_halt() and the submit path, can a submitter observe the bumped
reset_epoch before observing idxd->state == IDXD_DEV_HALTED, stamping a dead
descriptor with the post-halt epoch so polling returns DMA_IN_PROGRESS
forever after software reset?

Second, when idxd_dma_tx_status() returns terminal DMA_ERROR because
wq->idxd->state == IDXD_DEV_DISABLED (for example after idxd_device_disable(),
which drains/disables the device without changing reset_epoch) or because
wq->state != IDXD_WQ_ENABLED (after idxd_wq_disable() or idxd_wq_reset()),
it does not call idxd_dma_complete_txd().

Since idxd_wq_disable(), idxd_wq_reset(), and idxd_device_disable() only set
the disabled state after hardware has stopped, doesn't returning DMA_ERROR
here without calling idxd_dma_complete_txd(desc, IDXD_COMPLETE_ABORT, true,
NULL, NULL) leak desc->txd.unmap and the descriptor slot in wq->sbq, while
leaving desc->txd.cookie intact?

> @@ -177,7 +308,17 @@ static dma_cookie_t idxd_dma_tx_submit(struct dma_async_tx_descriptor *tx)
>  	int rc;
>  	struct idxd_desc *desc = container_of(tx, struct idxd_desc, txd);
>  
> -	cookie = dma_cookie_assign(tx);
> +	cookie = (desc->gen << DESC_ID_BITS) | (desc->id & DESC_ID_MASK);
> +	if (desc->hw->flags & IDXD_OP_FLAG_RCI)
> +		cookie |= IDXD_COOKIE_RCI;
> +
> +	if (desc->gen == DESC_GEN_MAX)
> +		desc->gen = 1;
> +	else
> +		desc->gen++;
> +
> +	tx->cookie = cookie;
> +	desc->epoch = READ_ONCE(wq->idxd->reset_epoch);
>  
>  	rc = idxd_submit_desc(wq, desc);
>  	if (rc < 0) {

[Severity: Critical]
Can stamping desc->epoch in idxd_dma_tx_submit() before calling
idxd_submit_desc() cause an active post-reset descriptor to be prematurely
unmapped and freed by idxd_dma_tx_status()?

In idxd_dma_tx_submit(), desc->epoch is read before idxd_submit_desc() ->
__idxd_submit_desc() acquires wq->wq_active:

drivers/dma/idxd/submit.c:__idxd_submit_desc() {
	if (!percpu_ref_tryget_live(&wq->wq_active)) {
		wait_for_completion(&wq->wq_resurrect);
		if (!percpu_ref_tryget_live(&wq->wq_active))
			return -ENXIO;
	}

	smp_rmb();
	if (idxd->state != IDXD_DEV_ENABLED) {
		percpu_ref_put(&wq->wq_active);
		return -EIO;
	}
}

If a thread reads desc->epoch in idxd_dma_tx_submit() (say epoch E), and then
a software-reset halt occurs in idxd_halt() (bumping idxd->reset_epoch to
E + 1, killing and reviving wq->wq_active in idxd_wqs_halt_flush(idxd, true),
and re-enabling the device in idxd_device_reinit()), the submitter can wait on
wq->wq_resurrect (or be preempted before percpu_ref_tryget_live()), wake up
after idxd_device_reinit() sets idxd->state = IDXD_DEV_ENABLED, and submit
desc to the newly enabled hardware while desc->epoch is still E.

When the client then polls idxd_dma_tx_status() while hardware is still
executing the descriptor, desc->epoch != READ_ONCE(wq->idxd->reset_epoch)
(E != E + 1) matches and calls idxd_dma_complete_txd(desc,
IDXD_COMPLETE_ABORT, true, NULL, NULL), unmapping the DMA buffers and freeing
the descriptor slot while the DMA engine is still writing to them.

Should desc->epoch instead be stamped inside __idxd_submit_desc() after
acquiring wq->wq_active and before checking idxd->state != IDXD_DEV_ENABLED
(similar to how desc->terminate_gen is stamped while holding wq->wq_active)?

[Severity: High]
Also in idxd_dma_tx_submit(), when idxd_submit_desc(wq, desc) returns an error
(rc < 0), idxd_free_desc(wq, desc) is called directly:

	tx->cookie = cookie;
	desc->epoch = READ_ONCE(wq->idxd->reset_epoch);

	rc = idxd_submit_desc(wq, desc);
	if (rc < 0) {
		idxd_free_desc(wq, desc);
		return rc;
	}

In __idxd_submit_desc(), llist_abort_desc() is only called if submission fails
after pending_llist insertion and only when ie is non-NULL (which is only true
for IDXD_OP_FLAG_RCI descriptors). If __idxd_submit_desc() fails early (e.g.
idxd->state != IDXD_DEV_ENABLED or wq_active is dead) or if idxd_enqcmds()
fails for a polled descriptor where ie is NULL, desc is freed back to wq->sbq
with tx->cookie, tx->callback, tx->callback_result, and tx->unmap still set.

When that slot is later reallocated by a polling client that does not set a
callback and is completed by idxd_dma_tx_status() -> idxd_dma_complete_txd(),
won't dmaengine_desc_get_callback_invoke() invoke the stale callback from the
earlier failed submission?

> @@ -219,6 +360,7 @@ static void idxd_dma_synchronize(struct dma_chan *c)
>  {
>  	struct idxd_wq *wq = to_idxd_wq(c);
>  	LIST_HEAD(quarantined);
> +	u32 epoch;
>  
>  	/*
>  	 * dmaengine_synchronize() promises the client that its memory may
> @@ -240,7 +382,13 @@ static void idxd_dma_synchronize(struct dma_chan *c)
>  	 * Only a device reset ends the exposure of the client's memory.
>  	 */
>  	idxd_wq_quarantine_take(wq, &quarantined);
> -	if (!idxd_wq_drain(wq)) {
> +	/*
> +	 * A halt during the drain kills every descriptor submitted before
> +	 * it, which is all of them, and a reset that followed may have
> +	 * ended the drain itself. Such a drain is as good as complete.
> +	 */
> +	epoch = READ_ONCE(wq->idxd->reset_epoch);
> +	if (!idxd_wq_drain(wq) || READ_ONCE(wq->idxd->reset_epoch) != epoch) {
>  		idxd_wq_quarantine_free(wq, &quarantined);
>  		WRITE_ONCE(wq->stuck, false);
>  		return;

[Severity: Medium]
What happens if idxd_halt() runs after idxd_wq_quarantine_take(wq,
&quarantined) but before epoch = READ_ONCE(wq->idxd->reset_epoch)?

In that window:
1. idxd_wq_quarantine_take() splices the entries out of wq->quarantine.
2. idxd_halt() increments idxd->reset_epoch to E + 1 and calls
   idxd_wqs_halt_flush() -> idxd_wq_halt_release(wq), which sees an empty
   wq->quarantine and sets wq->stuck = false.
3. idxd_dma_synchronize() reads epoch = E + 1 and calls idxd_wq_drain(wq).
4. If idxd_device_reinit() resets the device while idxd_wq_drain() is running
   so that idxd_cmd_exec() fails with IDXD_CMDSTS_HW_ERR and
   idxd_device_is_halted() is already false when idxd_cmd_drain_result() checks
   it, idxd_wq_drain() returns -ETIMEDOUT while reset_epoch is still E + 1.

Wouldn't idxd_dma_synchronize() then put the quarantined descriptors back and
set wq->stuck = true after idxd_wq_halt_release() has already finished,
wedging the newly reset workqueue? Would reading reset_epoch before calling
idxd_wq_quarantine_take() avoid this window?

> diff --git a/drivers/dma/idxd/idxd.h b/drivers/dma/idxd/idxd.h
> index b0cfd71011742..7fe5ec6c3bf36 100644
> --- a/drivers/dma/idxd/idxd.h
> +++ b/drivers/dma/idxd/idxd.h

[ ... ]

> diff --git a/drivers/dma/idxd/irq.c b/drivers/dma/idxd/irq.c
> index 8217698056bb4..5e34790359ee9 100644
> --- a/drivers/dma/idxd/irq.c
> +++ b/drivers/dma/idxd/irq.c
> @@ -508,6 +508,14 @@ static irqreturn_t idxd_halt(struct idxd_device *idxd)
>  	gensts.bits = ioread32(idxd->reg_base + IDXD_GENSTATS_OFFSET);
>  	if (gensts.state == IDXD_DEVICE_STATE_HALT) {
>  		idxd->state = IDXD_DEV_HALTED;
> +		/*
> +		 * Every descriptor submitted before this point is dead: the
> +		 * device will never write its completion record. Pollers
> +		 * compare their descriptor's epoch against this to retire
> +		 * such descriptors instead of waiting for a record that never
> +		 * comes.
> +		 */
> +		WRITE_ONCE(idxd->reset_epoch, idxd->reset_epoch + 1);
>  		if (gensts.reset_type == IDXD_DEVICE_RESET_SOFTWARE) {
>  			/* The reinit below re-enables the queues, so revive them. */
>  			idxd_wqs_halt_flush(idxd, true);

[Severity: High]
As noted above in idxd_dma_tx_status(), should idxd->state = IDXD_DEV_HALTED
use WRITE_ONCE() followed by smp_wmb() before updating idxd->reset_epoch so
that readers in idxd_dma_tx_status() and __idxd_submit_desc() cannot observe
the incremented reset_epoch before observing IDXD_DEV_HALTED?

Also, if idxd_halt() is entered again while a reset is in progress or via a
subsequent halt, could bumping idxd->reset_epoch while idxd->state is already
IDXD_DEV_HALTED race with a poller in idxd_dma_tx_status() that relies on
reset_epoch only changing after idxd->state transitions to IDXD_DEV_HALTED?

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