CCP_CMD_MAY_BACKLOG commands are removed from ccp->backlog by a
queue kthread and advanced asynchronously by ccp_do_cmd_backlog().
The promoted command is no longer on either command list.

During device removal, ccp*_destroy() stops the queue kthreads and
flushes the lists, but does not wait for a promoted work on system_wq.
That work can subsequently access the devm-allocated ccp_device after
it has been released, and wake a queue kthread whose task_struct
kthread_stop() has already released.

Use a per-device workqueue for backlog advancement and destroy it after
stopping all queue kthreads, so every promoted work has completed before
the command lists are flushed. The queue kthreads are the only source
of backlog works, so no new work can be queued once they are stopped.
Individual cancellation is not possible because a promoted command is
no longer reachable from either list.

Mark the device as halting before teardown. A draining backlog work
then completes its command with -ENODEV instead of re-queuing it.
Keep the halting check and the kthread wake under cmd_lock so teardown
cannot stop the selected kthread between them.

This issue was found by an in-house static analysis tool.

Fixes: 63b945091a07 ("crypto: ccp - CCP device driver and interface support")
Cc: stable@vger.kernel.org
Assisted-by: Codex:gpt-5.6
Co-developed-by: Song Li <songl@zju.edu.cn>
Signed-off-by: Song Li <songl@zju.edu.cn>
Signed-off-by: Fan Wu <fanwu01@zju.edu.cn>
---
 drivers/crypto/ccp/ccp-dev-v3.c | 19 +++++++++++++++++++
 drivers/crypto/ccp/ccp-dev-v5.c | 18 ++++++++++++++++++
 drivers/crypto/ccp/ccp-dev.c    | 33 ++++++++++++++++++++++++++++-----
 drivers/crypto/ccp/ccp-dev.h    |  4 ++++
 4 files changed, 69 insertions(+), 5 deletions(-)

diff --git a/drivers/crypto/ccp/ccp-dev-v3.c b/drivers/crypto/ccp/ccp-dev-v3.c
index fe69053b2394..b84ac117ea25 100644
--- a/drivers/crypto/ccp/ccp-dev-v3.c
+++ b/drivers/crypto/ccp/ccp-dev-v3.c
@@ -460,6 +460,14 @@ static int ccp_init(struct ccp_device *ccp)
 		tasklet_init(&ccp->irq_tasklet, ccp_irq_bh,
 			     (unsigned long)ccp);
 
+	ccp->backlog_wq = alloc_workqueue("%s-backlog", WQ_MEM_RECLAIM, 0,
+					  ccp->name);
+	if (!ccp->backlog_wq) {
+		dev_err(dev, "unable to allocate backlog workqueue\n");
+		ret = -ENOMEM;
+		goto e_irq;
+	}
+
 	dev_dbg(dev, "Starting threads...\n");
 	/* Create a kthread for each queue */
 	for (i = 0; i < ccp->cmd_q_count; i++) {
@@ -501,10 +509,15 @@ static int ccp_init(struct ccp_device *ccp)
 	ccp_unregister_rng(ccp);
 
 e_kthread:
+	ccp_halt_cmds(ccp);
+
 	for (i = 0; i < ccp->cmd_q_count; i++)
 		if (ccp->cmd_q[i].kthread)
 			kthread_stop(ccp->cmd_q[i].kthread);
 
+	destroy_workqueue(ccp->backlog_wq);
+
+e_irq:
 	sp_free_ccp_irq(ccp->sp, ccp);
 
 e_pool:
@@ -520,6 +533,9 @@ static void ccp_destroy(struct ccp_device *ccp)
 	struct ccp_cmd *cmd;
 	unsigned int i;
 
+	/* Complete pending backlog cmds instead of executing them */
+	ccp_halt_cmds(ccp);
+
 	/* Unregister the DMA engine */
 	ccp_dmaengine_unregister(ccp);
 
@@ -546,6 +562,9 @@ static void ccp_destroy(struct ccp_device *ccp)
 
 	sp_free_ccp_irq(ccp->sp, ccp);
 
+	/* Drain promoted backlog commands before flushing the command lists. */
+	destroy_workqueue(ccp->backlog_wq);
+
 	for (i = 0; i < ccp->cmd_q_count; i++)
 		dma_pool_destroy(ccp->cmd_q[i].dma_pool);
 
diff --git a/drivers/crypto/ccp/ccp-dev-v5.c b/drivers/crypto/ccp/ccp-dev-v5.c
index 7b73332d6aa1..307a408eff89 100644
--- a/drivers/crypto/ccp/ccp-dev-v5.c
+++ b/drivers/crypto/ccp/ccp-dev-v5.c
@@ -943,6 +943,14 @@ static int ccp5_init(struct ccp_device *ccp)
 		ccp->cmd_q[i].sb_ctx = ccp_lsb_alloc(&ccp->cmd_q[i], 2);
 	}
 
+	ccp->backlog_wq = alloc_workqueue("%s-backlog", WQ_MEM_RECLAIM, 0,
+					  ccp->name);
+	if (!ccp->backlog_wq) {
+		dev_err(dev, "unable to allocate backlog workqueue\n");
+		ret = -ENOMEM;
+		goto e_irq;
+	}
+
 	dev_dbg(dev, "Starting threads...\n");
 	/* Create a kthread for each queue */
 	for (i = 0; i < ccp->cmd_q_count; i++) {
@@ -989,10 +997,14 @@ static int ccp5_init(struct ccp_device *ccp)
 	ccp_unregister_rng(ccp);
 
 e_kthread:
+	ccp_halt_cmds(ccp);
+
 	for (i = 0; i < ccp->cmd_q_count; i++)
 		if (ccp->cmd_q[i].kthread)
 			kthread_stop(ccp->cmd_q[i].kthread);
 
+	destroy_workqueue(ccp->backlog_wq);
+
 e_irq:
 	sp_free_ccp_irq(ccp->sp, ccp);
 
@@ -1009,6 +1021,9 @@ static void ccp5_destroy(struct ccp_device *ccp)
 	struct ccp_cmd *cmd;
 	unsigned int i;
 
+	/* Complete pending backlog cmds instead of executing them */
+	ccp_halt_cmds(ccp);
+
 	/* Unregister the DMA engine */
 	ccp_dmaengine_unregister(ccp);
 
@@ -1047,6 +1062,9 @@ static void ccp5_destroy(struct ccp_device *ccp)
 
 	sp_free_ccp_irq(ccp->sp, ccp);
 
+	/* Drain promoted backlog commands before flushing the command lists. */
+	destroy_workqueue(ccp->backlog_wq);
+
 	/* Flush the cmd and backlog queue */
 	while (!list_empty(&ccp->cmd)) {
 		/* Invoke the callback directly with an error code */
diff --git a/drivers/crypto/ccp/ccp-dev.c b/drivers/crypto/ccp/ccp-dev.c
index 246801912e1a..b8eeaf0c3c95 100644
--- a/drivers/crypto/ccp/ccp-dev.c
+++ b/drivers/crypto/ccp/ccp-dev.c
@@ -177,6 +177,15 @@ void ccp_del_device(struct ccp_device *ccp)
 	write_unlock_irqrestore(&ccp_unit_lock, flags);
 }
 
+/* Mark the device halting so draining backlog works complete with -ENODEV. */
+void ccp_halt_cmds(struct ccp_device *ccp)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&ccp->cmd_lock, flags);
+	ccp->halting = true;
+	spin_unlock_irqrestore(&ccp->cmd_lock, flags);
+}
 
 
 int ccp_register_rng(struct ccp_device *ccp)
@@ -349,6 +358,20 @@ static void ccp_do_cmd_backlog(struct work_struct *work)
 	unsigned long flags;
 	unsigned int i;
 
+	spin_lock_irqsave(&ccp->cmd_lock, flags);
+	if (ccp->halting) {
+		spin_unlock_irqrestore(&ccp->cmd_lock, flags);
+
+		/* The device is being removed; the cmd can no longer be
+		 * executed, so complete it with an error like the cmds
+		 * still queued on the cmd and backlog lists
+		 */
+		cmd->callback(cmd->data, -ENODEV);
+
+		return;
+	}
+	spin_unlock_irqrestore(&ccp->cmd_lock, flags);
+
 	cmd->callback(cmd->data, -EINPROGRESS);
 
 	spin_lock_irqsave(&ccp->cmd_lock, flags);
@@ -364,11 +387,11 @@ static void ccp_do_cmd_backlog(struct work_struct *work)
 		break;
 	}
 
-	spin_unlock_irqrestore(&ccp->cmd_lock, flags);
-
-	/* If we found an idle queue, wake it up */
-	if (i < ccp->cmd_q_count)
+	/* Keep this under cmd_lock so teardown cannot stop this kthread first. */
+	if (!ccp->halting && i < ccp->cmd_q_count)
 		wake_up_process(ccp->cmd_q[i].kthread);
+
+	spin_unlock_irqrestore(&ccp->cmd_lock, flags);
 }
 
 static struct ccp_cmd *ccp_dequeue_cmd(struct ccp_cmd_queue *cmd_q)
@@ -410,7 +433,7 @@ static struct ccp_cmd *ccp_dequeue_cmd(struct ccp_cmd_queue *cmd_q)
 
 	if (backlog) {
 		INIT_WORK(&backlog->work, ccp_do_cmd_backlog);
-		schedule_work(&backlog->work);
+		queue_work(ccp->backlog_wq, &backlog->work);
 	}
 
 	return cmd;
diff --git a/drivers/crypto/ccp/ccp-dev.h b/drivers/crypto/ccp/ccp-dev.h
index 83350e2d9821..91b303ec46a9 100644
--- a/drivers/crypto/ccp/ccp-dev.h
+++ b/drivers/crypto/ccp/ccp-dev.h
@@ -374,6 +374,9 @@ struct ccp_device {
 	struct list_head cmd;
 	struct list_head backlog;
 
+	struct workqueue_struct *backlog_wq;
+	bool halting;
+
 	/* The command queues. These represent the queues available on the
 	 * CCP that are available for processing cmds
 	 */
@@ -630,6 +633,7 @@ struct ccp5_desc {
 
 void ccp_add_device(struct ccp_device *ccp);
 void ccp_del_device(struct ccp_device *ccp);
+void ccp_halt_cmds(struct ccp_device *ccp);
 
 extern void ccp_log_error(struct ccp_device *, unsigned int);

Fan Wu <fanwu01@zju.edu.cn> wrote:
> CCP_CMD_MAY_BACKLOG commands are removed from ccp->backlog by a
> queue kthread and advanced asynchronously by ccp_do_cmd_backlog().
> The promoted command is no longer on either command list.
> 
> During device removal, ccp*_destroy() stops the queue kthreads and
> flushes the lists, but does not wait for a promoted work on system_wq.
> That work can subsequently access the devm-allocated ccp_device after
> it has been released, and wake a queue kthread whose task_struct
> kthread_stop() has already released.
> 
> Use a per-device workqueue for backlog advancement and destroy it after
> stopping all queue kthreads, so every promoted work has completed before
> the command lists are flushed. The queue kthreads are the only source
> of backlog works, so no new work can be queued once they are stopped.
> Individual cancellation is not possible because a promoted command is
> no longer reachable from either list.
> 
> Mark the device as halting before teardown. A draining backlog work
> then completes its command with -ENODEV instead of re-queuing it.
> Keep the halting check and the kthread wake under cmd_lock so teardown
> cannot stop the selected kthread between them.
> 
> This issue was found by an in-house static analysis tool.
> 
> Fixes: 63b945091a07 ("crypto: ccp - CCP device driver and interface support")
> Cc: stable@vger.kernel.org
> Assisted-by: Codex:gpt-5.6
> Co-developed-by: Song Li <songl@zju.edu.cn>
> Signed-off-by: Song Li <songl@zju.edu.cn>
> Signed-off-by: Fan Wu <fanwu01@zju.edu.cn>
> ---
> drivers/crypto/ccp/ccp-dev-v3.c | 19 +++++++++++++++++++
> drivers/crypto/ccp/ccp-dev-v5.c | 18 ++++++++++++++++++
> drivers/crypto/ccp/ccp-dev.c    | 33 ++++++++++++++++++++++++++++-----
> drivers/crypto/ccp/ccp-dev.h    |  4 ++++
> 4 files changed, 69 insertions(+), 5 deletions(-)

Thanks for catching this!

The backlog handling still looks a bit wrong though.  When you detach
a backlogged request after processing a normal request, there is
a gap before the backlogged request is added to the normal queue.

In that gap new requests could have come in and taken up the space
meant for the backlogged request.

I think this should be done atomically so that there is no gap
between the two operations, to ensure that the space is actually
reserved for the backlogged entry.

Once this is done, there should be no need to have a per-backlog
work struct, which means that you don't need to add a separate
work queue.  In fact, there may not be a need for a work struct
at all since this is all running out of process context anyway.

On a higher level, this driver should probably switch over to
using crypto_engine rather than doing backlog handling manually.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt