net: airoha: npu: cancel wdt_work after releasing the WDT IRQ

[PATCH net] net: airoha: npu: cancel wdt_work after releasing the WDT IRQ

Posted by Myeonghun Pak 2 days, 16 hours ago

airoha_npu_remove() calls cancel_work_sync() on each core's wdt_work,
but the watchdog IRQ that queues it is requested with devm_request_irq()
and is freed only after .remove() returns.  airoha_npu_wdt_handler() can
therefore schedule_work() again once the cancel has returned.  struct
airoha_npu, which contains the work, is devm_kzalloc()'d and is freed in
that same unwind, so the late work dereferences freed memory.

Register the work with devm_work_autocancel() before devm_request_irq()
and drop .remove().  Devres runs in reverse order, so the IRQ is freed
before cancel_work_sync(), including when probe fails.  A cancel left in
.remove() cannot get that order.  Initializing the work first also stops
a pending watchdog interrupt from queuing an uninitialized work item.
Probe currently calls INIT_WORK() only after devm_request_irq().

This issue was identified during our ongoing static-analysis research
while reviewing kernel code.

Fixes: 23290c7bc190 ("net: airoha: Introduce Airoha NPU support")
Cc: stable@vger.kernel.org # 6.15+
Assisted-by: LLM
Co-developed-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Myeonghun Pak <mhun512@gmail.com>
---
 drivers/net/ethernet/airoha/airoha_npu.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/airoha/airoha_npu.c b/drivers/net/ethernet/airoha/airoha_npu.c
index 5bb4817a898d..4d3195eb00f7 100644
--- a/drivers/net/ethernet/airoha/airoha_npu.c
+++ b/drivers/net/ethernet/airoha/airoha_npu.c
@@ -5,6 +5,7 @@
  */
 
 #include <linux/devcoredump.h>
+#include <linux/devm-helpers.h>
 #include <linux/firmware.h>
 #include <linux/platform_device.h>
 #include <linux/of_net.h>
@@ -751,12 +752,15 @@ static int airoha_npu_probe(struct platform_device *pdev)
 		if (irq < 0)
 			return irq;
 
+		err = devm_work_autocancel(dev, &core->wdt_work,
+					   airoha_npu_wdt_work);
+		if (err)
+			return err;
+
 		err = devm_request_irq(dev, irq, airoha_npu_wdt_handler,
 				       IRQF_SHARED, "airoha-npu-wdt", core);
 		if (err)
 			return err;
-
-		INIT_WORK(&core->wdt_work, airoha_npu_wdt_work);
 	}
 
 	/* wlan IRQ lines */
@@ -803,18 +807,8 @@ static int airoha_npu_probe(struct platform_device *pdev)
 	return 0;
 }
 
-static void airoha_npu_remove(struct platform_device *pdev)
-{
-	struct airoha_npu *npu = platform_get_drvdata(pdev);
-	int i;
-
-	for (i = 0; i < ARRAY_SIZE(npu->cores); i++)
-		cancel_work_sync(&npu->cores[i].wdt_work);
-}
-
 static struct platform_driver airoha_npu_driver = {
 	.probe = airoha_npu_probe,
-	.remove = airoha_npu_remove,
 	.driver = {
 		.name = "airoha-npu",
 		.of_match_table = of_airoha_npu_match,

base-commit: 238650ef6c7c7cca08e032527329424c9fbd70e5
-- 
2.53.0

Re: [PATCH net] net: airoha: npu: cancel wdt_work after releasing the WDT IRQ

Posted by Lorenzo Bianconi 2 days, 8 hours ago

> airoha_npu_remove() calls cancel_work_sync() on each core's wdt_work,
> but the watchdog IRQ that queues it is requested with devm_request_irq()
> and is freed only after .remove() returns.  airoha_npu_wdt_handler() can
> therefore schedule_work() again once the cancel has returned.  struct
> airoha_npu, which contains the work, is devm_kzalloc()'d and is freed in
> that same unwind, so the late work dereferences freed memory.
> 
> Register the work with devm_work_autocancel() before devm_request_irq()
> and drop .remove().  Devres runs in reverse order, so the IRQ is freed
> before cancel_work_sync(), including when probe fails.  A cancel left in
> .remove() cannot get that order.  Initializing the work first also stops
> a pending watchdog interrupt from queuing an uninitialized work item.
> Probe currently calls INIT_WORK() only after devm_request_irq().
> 
> This issue was identified during our ongoing static-analysis research
> while reviewing kernel code.
> 
> Fixes: 23290c7bc190 ("net: airoha: Introduce Airoha NPU support")
> Cc: stable@vger.kernel.org # 6.15+
> Assisted-by: LLM
> Co-developed-by: Ijae Kim <ae878000@gmail.com>
> Signed-off-by: Ijae Kim <ae878000@gmail.com>
> Signed-off-by: Myeonghun Pak <mhun512@gmail.com>

I guess this is net-next material, it is just an optimization, not a real fix.
Anyway:

Acked-by: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>

> ---
>  drivers/net/ethernet/airoha/airoha_npu.c | 18 ++++++------------
>  1 file changed, 6 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/ethernet/airoha/airoha_npu.c b/drivers/net/ethernet/airoha/airoha_npu.c
> index 5bb4817a898d..4d3195eb00f7 100644
> --- a/drivers/net/ethernet/airoha/airoha_npu.c
> +++ b/drivers/net/ethernet/airoha/airoha_npu.c
> @@ -5,6 +5,7 @@
>   */
>  
>  #include <linux/devcoredump.h>
> +#include <linux/devm-helpers.h>
>  #include <linux/firmware.h>
>  #include <linux/platform_device.h>
>  #include <linux/of_net.h>
> @@ -751,12 +752,15 @@ static int airoha_npu_probe(struct platform_device *pdev)
>  		if (irq < 0)
>  			return irq;
>  
> +		err = devm_work_autocancel(dev, &core->wdt_work,
> +					   airoha_npu_wdt_work);
> +		if (err)
> +			return err;
> +
>  		err = devm_request_irq(dev, irq, airoha_npu_wdt_handler,
>  				       IRQF_SHARED, "airoha-npu-wdt", core);
>  		if (err)
>  			return err;
> -
> -		INIT_WORK(&core->wdt_work, airoha_npu_wdt_work);
>  	}
>  
>  	/* wlan IRQ lines */
> @@ -803,18 +807,8 @@ static int airoha_npu_probe(struct platform_device *pdev)
>  	return 0;
>  }
>  
> -static void airoha_npu_remove(struct platform_device *pdev)
> -{
> -	struct airoha_npu *npu = platform_get_drvdata(pdev);
> -	int i;
> -
> -	for (i = 0; i < ARRAY_SIZE(npu->cores); i++)
> -		cancel_work_sync(&npu->cores[i].wdt_work);
> -}
> -
>  static struct platform_driver airoha_npu_driver = {
>  	.probe = airoha_npu_probe,
> -	.remove = airoha_npu_remove,
>  	.driver = {
>  		.name = "airoha-npu",
>  		.of_match_table = of_airoha_npu_match,
> 
> base-commit: 238650ef6c7c7cca08e032527329424c9fbd70e5
> -- 
> 2.53.0
>

Re: [PATCH net] net: airoha: npu: cancel wdt_work after releasing the WDT IRQ

Posted by Lorenzo Bianconi 2 days, 8 hours ago

> > airoha_npu_remove() calls cancel_work_sync() on each core's wdt_work,
> > but the watchdog IRQ that queues it is requested with devm_request_irq()
> > and is freed only after .remove() returns.  airoha_npu_wdt_handler() can
> > therefore schedule_work() again once the cancel has returned.  struct
> > airoha_npu, which contains the work, is devm_kzalloc()'d and is freed in
> > that same unwind, so the late work dereferences freed memory.
> > 
> > Register the work with devm_work_autocancel() before devm_request_irq()
> > and drop .remove().  Devres runs in reverse order, so the IRQ is freed
> > before cancel_work_sync(), including when probe fails.  A cancel left in
> > .remove() cannot get that order.  Initializing the work first also stops
> > a pending watchdog interrupt from queuing an uninitialized work item.
> > Probe currently calls INIT_WORK() only after devm_request_irq().
> > 
> > This issue was identified during our ongoing static-analysis research
> > while reviewing kernel code.
> > 
> > Fixes: 23290c7bc190 ("net: airoha: Introduce Airoha NPU support")
> > Cc: stable@vger.kernel.org # 6.15+
> > Assisted-by: LLM
> > Co-developed-by: Ijae Kim <ae878000@gmail.com>
> > Signed-off-by: Ijae Kim <ae878000@gmail.com>
> > Signed-off-by: Myeonghun Pak <mhun512@gmail.com>
> 
> I guess this is net-next material, it is just an optimization, not a real fix.
> Anyway:

Sorry I misread the commit message, this is actually for net, thx for fixing it.

Regards,
Lorenzo

> 
> Acked-by: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>
> 
> > ---
> >  drivers/net/ethernet/airoha/airoha_npu.c | 18 ++++++------------
> >  1 file changed, 6 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/airoha/airoha_npu.c b/drivers/net/ethernet/airoha/airoha_npu.c
> > index 5bb4817a898d..4d3195eb00f7 100644
> > --- a/drivers/net/ethernet/airoha/airoha_npu.c
> > +++ b/drivers/net/ethernet/airoha/airoha_npu.c
> > @@ -5,6 +5,7 @@
> >   */
> >  
> >  #include <linux/devcoredump.h>
> > +#include <linux/devm-helpers.h>
> >  #include <linux/firmware.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/of_net.h>
> > @@ -751,12 +752,15 @@ static int airoha_npu_probe(struct platform_device *pdev)
> >  		if (irq < 0)
> >  			return irq;
> >  
> > +		err = devm_work_autocancel(dev, &core->wdt_work,
> > +					   airoha_npu_wdt_work);
> > +		if (err)
> > +			return err;
> > +
> >  		err = devm_request_irq(dev, irq, airoha_npu_wdt_handler,
> >  				       IRQF_SHARED, "airoha-npu-wdt", core);
> >  		if (err)
> >  			return err;
> > -
> > -		INIT_WORK(&core->wdt_work, airoha_npu_wdt_work);
> >  	}
> >  
> >  	/* wlan IRQ lines */
> > @@ -803,18 +807,8 @@ static int airoha_npu_probe(struct platform_device *pdev)
> >  	return 0;
> >  }
> >  
> > -static void airoha_npu_remove(struct platform_device *pdev)
> > -{
> > -	struct airoha_npu *npu = platform_get_drvdata(pdev);
> > -	int i;
> > -
> > -	for (i = 0; i < ARRAY_SIZE(npu->cores); i++)
> > -		cancel_work_sync(&npu->cores[i].wdt_work);
> > -}
> > -
> >  static struct platform_driver airoha_npu_driver = {
> >  	.probe = airoha_npu_probe,
> > -	.remove = airoha_npu_remove,
> >  	.driver = {
> >  		.name = "airoha-npu",
> >  		.of_match_table = of_airoha_npu_match,
> > 
> > base-commit: 238650ef6c7c7cca08e032527329424c9fbd70e5
> > -- 
> > 2.53.0
> >