[PATCHv2] crypto: cesa: complete pending requests on device remove

[PATCHv2] crypto: cesa: complete pending requests on device remove

[Rosen Penev]

mv_cesa_remove() unregisters the algorithms but never drains the engine
queues.  Each engine can still hold outstanding requests in three
places: engine->req (currently in flight), engine->queue (queued but
not started), and engine->complete_queue (processed but not yet
reported).  After unregistration those waiters never receive their
completion callback and block indefinitely, leaking the request and its
scatterlist buffers.

In mv_cesa_remove() stop each engine (mask interrupts, clear the CMD and
TDMA control registers, then wait for the engine to report idle), quiesce
the threaded IRQ with disable_irq()/synchronize_irq(), and drain the
three queues, completing every outstanding request with -ENOENT.  The
completion callbacks unmap DMA buffers through the global cesa_dev
pointer, so keep it valid until the engines are drained, and mark the
engines aborted under their lock so a racing submitter cannot restart a
halted engine.

Assisted-by: LLM
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 v2: fix a bunch of sashiko errors
 drivers/crypto/marvell/cesa/cesa.c | 80 ++++++++++++++++++++++++++++++
 drivers/crypto/marvell/cesa/cesa.h |  4 ++
 2 files changed, 84 insertions(+)

diff --git a/drivers/crypto/marvell/cesa/cesa.c b/drivers/crypto/marvell/cesa/cesa.c
index 564b09773507..d91e1af0fe9c 100644
--- a/drivers/crypto/marvell/cesa/cesa.c
+++ b/drivers/crypto/marvell/cesa/cesa.c
@@ -52,6 +52,14 @@ static void mv_cesa_rearm_engine(struct mv_cesa_engine *engine)
 
 	spin_lock_bh(&engine->lock);
 	if (!engine->req) {
+		if (engine->aborted) {
+			/*
+			 * The device is being removed: do not restart the
+			 * engine nor fetch any new request.
+			 */
+			spin_unlock_bh(&engine->lock);
+			return;
+		}
 		req = mv_cesa_dequeue_req_locked(engine, &backlog);
 		engine->req = req;
 	}
@@ -542,9 +550,81 @@ static int mv_cesa_probe(struct platform_device *pdev)
 static void mv_cesa_remove(struct platform_device *pdev)
 {
 	struct mv_cesa_dev *cesa = platform_get_drvdata(pdev);
+	struct mv_cesa_engine *engine;
+	struct crypto_async_request *req;
+	unsigned int i;
+	unsigned int timeout;
 
 	mv_cesa_remove_algs(cesa);
 
+	for (i = 0; i < cesa->caps->nengines; i++) {
+		engine = &cesa->engines[i];
+
+		/*
+		 * Stop the engine so it no longer issues DMA to the SRAM
+		 * region or to request scatterlists that are about to be
+		 * unmapped.
+		 */
+		writel(0, engine->regs + CESA_SA_INT_MSK);
+		writel(0, engine->regs + CESA_SA_CMD);
+		writel(0, engine->regs + CESA_TDMA_CONTROL);
+
+		/*
+		 * Flush the posted writes above and wait for the engine to
+		 * report that it is stopped before any buffer is released.
+		 */
+		timeout = CESA_ENGINE_STOP_TIMEOUT_US;
+		while ((readl(engine->regs + CESA_SA_CMD) &
+			CESA_SA_CMD_EN_CESA_SA_ACCL0) && --timeout)
+			udelay(1);
+
+		/*
+		 * Synchronize with the threaded IRQ handler so that it cannot
+		 * run while the queues below are drained.
+		 */
+		disable_irq(engine->irq);
+		synchronize_irq(engine->irq);
+
+		spin_lock_bh(&engine->lock);
+		engine->aborted = true;
+
+		/*
+		 * Complete the request currently in flight and drain the
+		 * pending and already-processed queues with an error so that
+		 * waiters do not block indefinitely when the device is unbound
+		 * while requests are still outstanding.
+		 */
+		if (engine->req) {
+			req = engine->req;
+			engine->req = NULL;
+			spin_unlock_bh(&engine->lock);
+			mv_cesa_complete_req(crypto_tfm_ctx(req->tfm), req,
+					     -ENOENT);
+			spin_lock_bh(&engine->lock);
+		}
+
+		while ((req = crypto_dequeue_request(&engine->queue)) != NULL) {
+			spin_unlock_bh(&engine->lock);
+			mv_cesa_complete_req(crypto_tfm_ctx(req->tfm), req,
+					     -ENOENT);
+			spin_lock_bh(&engine->lock);
+		}
+
+		while ((req = mv_cesa_engine_dequeue_complete_request(engine))
+		       != NULL) {
+			spin_unlock_bh(&engine->lock);
+			mv_cesa_complete_req(crypto_tfm_ctx(req->tfm), req,
+					     -ENOENT);
+			spin_lock_bh(&engine->lock);
+		}
+		spin_unlock_bh(&engine->lock);
+	}
+
+	/*
+	 * The completion callbacks above (and any concurrent submitter) rely
+	 * on the global cesa_dev pointer: only clear it once the engines are
+	 * fully drained.
+	 */
 	cesa_dev = NULL;
 }
 
diff --git a/drivers/crypto/marvell/cesa/cesa.h b/drivers/crypto/marvell/cesa/cesa.h
index 44351b252861..56c0dd6f5772 100644
--- a/drivers/crypto/marvell/cesa/cesa.h
+++ b/drivers/crypto/marvell/cesa/cesa.h
@@ -10,6 +10,9 @@
 
 #define CESA_ENGINE_OFF(i)			(((i) * 0x2000))
 
+/* Max time in microseconds to wait for the engine to stop */
+#define CESA_ENGINE_STOP_TIMEOUT_US		1000
+
 #define CESA_TDMA_BYTE_CNT			0x800
 #define CESA_TDMA_SRC_ADDR			0x810
 #define CESA_TDMA_DST_ADDR			0x820
@@ -450,6 +453,7 @@ struct mv_cesa_engine {
 	struct mv_cesa_tdma_chain chain_sw;
 	struct list_head complete_queue;
 	int irq;
+	bool aborted;
 };
 
 /**
-- 
2.55.0

Re: [PATCHv2] crypto: cesa: complete pending requests on device remove

[Robert Lovrinovic]

There still appears to be a race between stopping the engine and
setting engine->aborted. An IRQ thread already in flight may call
mv_cesa_rearm_engine() after the CMD idle check but before aborted is
set, and ctx->ops->step() may restart the hardware. We can then
synchronize the IRQ and free request/DMA objects while the engine is
running.

I think engine->aborted needs to be set before the final engine stop,
so that once we have observed the engine idle, no software path can
restart it.

engine->aborted currently only prevents mv_cesa_rearm_engine() from
dequeuing a new request; it does not prevent mv_cesa_queue_req() from
enqueueing one. A submitter racing after the removal drain can
therefore leave a request permanently queued. Please check aborted
under engine->lock in mv_cesa_queue_req() before calling
crypto_enqueue_request() mv_cesa_tdma_chain(), and return an
appropriate teardown error.

The stop timeout also looks unsafe as written. If the timeout expires,
teardown still proceeds to the callbacks and cleanup, which may free
DMA-visible objects while the engine is still active. The timeout
needs an explicit failure path or hardware reset rather than simply
continuing teardown.