Re: [PATCH net v2] stmmac: Clear variable when destroying workqueue

[Jiri Pirko <jiri@resnulli.us>]

Mon, Feb 26, 2024 at 04:42:17PM CET, j.raczynski@samsung.com wrote:
>Currently when suspending driver and stopping workqueue it is checked whether
>workqueue is not NULL and if so, it is destroyed.
>Function destroy_workqueue() does drain queue and does clear variable, but
>it does not set workqueue variable to NULL. This can cause kernel/module
>panic if code attempts to clear workqueue that was not initialized.
>
>This scenario is possible when resuming suspended driver in stmmac_resume(),
>because there is no handling for failed stmmac_hw_setup(),
>which can fail and return if DMA engine has failed to initialize,
>and workqueue is initialized after DMA engine.
>Should DMA engine fail to initialize, resume will proceed normally,
>but interface won't work and TX queue will eventually timeout,
>causing 'Reset adapter' error.
>This then does destroy workqueue during reset process.
>And since workqueue is initialized after DMA engine and can be skipped,
>it will cause kernel/module panic.

If you have a trace, it is good to inline it here so the future
reader/backporter can immediately match it.

>
>This commit sets workqueue variable to NULL when destroying workqueue,

Don't talk about "this commit" in the patch description, just tell the
codebase what to do using imperative mood:
https://www.kernel.org/doc/html/v6.6/process/submitting-patches.html#describe-your-changes


>which secures against that possible driver crash.
>
>Fixes: 5a5586112b929 ("net: stmmac: support FPE link partner hand-shaking procedure")
>Signed-off-by: Jakub Raczynski <j.raczynski@samsung.com>
>---
> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
>diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>index 75d029704503..0681029a2489 100644
>--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>@@ -4005,8 +4005,10 @@ static void stmmac_fpe_stop_wq(struct stmmac_priv *priv)
> {
> 	set_bit(__FPE_REMOVING, &priv->fpe_task_state);
> 
>-	if (priv->fpe_wq)
>+	if (priv->fpe_wq) {
> 		destroy_workqueue(priv->fpe_wq);
>+		priv->fpe_wq = NULL;
>+	}
> 
> 	netdev_info(priv->dev, "FPE workqueue stop");
> }
>-- 
>2.34.1
>
>