diff mbox series

[net] dpaa2-ptp: fix double free of the ptp_qoriq IRQ

Message ID 1576231462-23886-1-git-send-email-ioana.ciornei@nxp.com
State Changes Requested
Delegated to: David Miller
Headers show
Series [net] dpaa2-ptp: fix double free of the ptp_qoriq IRQ | expand

Commit Message

Ioana Ciornei Dec. 13, 2019, 10:04 a.m. UTC 
Upon reusing the ptp_qoriq driver, the ptp_qoriq_free() function was
used on the remove path to free any allocated resources.
The ptp_qoriq IRQ is among these resources that are freed in
ptp_qoriq_free() even though it is also a managed one (allocated using
devm_request_threaded_irq).

Drop the resource managed version of requesting the IRQ in order to not
trigger a double free of the interrupt as below:

[  226.731005] Trying to free already-free IRQ 126
[  226.735533] WARNING: CPU: 6 PID: 749 at kernel/irq/manage.c:1707
__free_irq+0x9c/0x2b8
[  226.743435] Modules linked in:
[  226.746480] CPU: 6 PID: 749 Comm: bash Tainted: G        W
5.4.0-03629-gfd7102c32b2c-dirty #912
[  226.755857] Hardware name: NXP Layerscape LX2160ARDB (DT)
[  226.761244] pstate: 40000085 (nZcv daIf -PAN -UAO)
[  226.766022] pc : __free_irq+0x9c/0x2b8
[  226.769758] lr : __free_irq+0x9c/0x2b8
[  226.773493] sp : ffff8000125039f0
(...)
[  226.856275] Call trace:
[  226.858710]  __free_irq+0x9c/0x2b8
[  226.862098]  free_irq+0x30/0x70
[  226.865229]  devm_irq_release+0x14/0x20
[  226.869054]  release_nodes+0x1b0/0x220
[  226.872790]  devres_release_all+0x34/0x50
[  226.876790]  device_release_driver_internal+0x100/0x1c0

Fixes: d346c9e86d86 ("dpaa2-ptp: reuse ptp_qoriq driver")
Cc: Yangbo Lu <yangbo.lu@nxp.com>
Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
---
 drivers/net/ethernet/freescale/dpaa2/dpaa2-ptp.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

Comments

Jakub Kicinski Dec. 15, 2019, 5:30 a.m. UTC | #1 
On Fri, 13 Dec 2019 12:04:22 +0200, Ioana Ciornei wrote:
> Upon reusing the ptp_qoriq driver, the ptp_qoriq_free() function was
> used on the remove path to free any allocated resources.
> The ptp_qoriq IRQ is among these resources that are freed in
> ptp_qoriq_free() even though it is also a managed one (allocated using
> devm_request_threaded_irq).
> 
> Drop the resource managed version of requesting the IRQ in order to not
> trigger a double free of the interrupt as below:
> 
> [  226.731005] Trying to free already-free IRQ 126
> [  226.735533] WARNING: CPU: 6 PID: 749 at kernel/irq/manage.c:1707
> __free_irq+0x9c/0x2b8
> [  226.743435] Modules linked in:
> [  226.746480] CPU: 6 PID: 749 Comm: bash Tainted: G        W
> 5.4.0-03629-gfd7102c32b2c-dirty #912
> [  226.755857] Hardware name: NXP Layerscape LX2160ARDB (DT)
> [  226.761244] pstate: 40000085 (nZcv daIf -PAN -UAO)
> [  226.766022] pc : __free_irq+0x9c/0x2b8
> [  226.769758] lr : __free_irq+0x9c/0x2b8
> [  226.773493] sp : ffff8000125039f0
> (...)
> [  226.856275] Call trace:
> [  226.858710]  __free_irq+0x9c/0x2b8
> [  226.862098]  free_irq+0x30/0x70
> [  226.865229]  devm_irq_release+0x14/0x20
> [  226.869054]  release_nodes+0x1b0/0x220
> [  226.872790]  devres_release_all+0x34/0x50
> [  226.876790]  device_release_driver_internal+0x100/0x1c0
> 
> Fixes: d346c9e86d86 ("dpaa2-ptp: reuse ptp_qoriq driver")
> Cc: Yangbo Lu <yangbo.lu@nxp.com>
> Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
> ---
>  drivers/net/ethernet/freescale/dpaa2/dpaa2-ptp.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-ptp.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-ptp.c
> index a9503aea527f..04a4b316f1dc 100644
> --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-ptp.c
> +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-ptp.c
> @@ -160,10 +160,10 @@ static int dpaa2_ptp_probe(struct fsl_mc_device *mc_dev)
>  	irq = mc_dev->irqs[0];
>  	ptp_qoriq->irq = irq->msi_desc->irq;
>  
> -	err = devm_request_threaded_irq(dev, ptp_qoriq->irq, NULL,
> -					dpaa2_ptp_irq_handler_thread,
> -					IRQF_NO_SUSPEND | IRQF_ONESHOT,
> -					dev_name(dev), ptp_qoriq);
> +	err = request_threaded_irq(ptp_qoriq->irq, NULL,
> +				   dpaa2_ptp_irq_handler_thread,
> +				   IRQF_NO_SUSPEND | IRQF_ONESHOT,
> +				   dev_name(dev), ptp_qoriq);
>  	if (err < 0) {
>  		dev_err(dev, "devm_request_threaded_irq(): %d\n", err);
>  		goto err_free_mc_irq;
> @@ -173,7 +173,7 @@ static int dpaa2_ptp_probe(struct fsl_mc_device *mc_dev)
>  				   DPRTC_IRQ_INDEX, 1);
>  	if (err < 0) {
>  		dev_err(dev, "dprtc_set_irq_enable(): %d\n", err);
> -		goto err_free_mc_irq;
> +		goto err_free_threaded_irq;
>  	}
>  
>  	err = ptp_qoriq_init(ptp_qoriq, base, &dpaa2_ptp_caps);

There is another goto right here which still jumps to err_free_mc_irq
rather than err_free_threaded_irq. Is that intentional?

> @@ -185,6 +185,8 @@ static int dpaa2_ptp_probe(struct fsl_mc_device *mc_dev)
>  
>  	return 0;
>  
> +err_free_threaded_irq:
> +	free_irq(ptp_qoriq->irq, ptp_qoriq);
>  err_free_mc_irq:
>  	fsl_mc_free_irqs(mc_dev);
>  err_unmap:
Ioana Ciornei Dec. 16, 2019, 2:55 p.m. UTC | #2 
> Subject: Re: [PATCH net] dpaa2-ptp: fix double free of the ptp_qoriq IRQ
> 
> On Fri, 13 Dec 2019 12:04:22 +0200, Ioana Ciornei wrote:
> > Upon reusing the ptp_qoriq driver, the ptp_qoriq_free() function was
> > used on the remove path to free any allocated resources.
> > The ptp_qoriq IRQ is among these resources that are freed in
> > ptp_qoriq_free() even though it is also a managed one (allocated using
> > devm_request_threaded_irq).
> >
> > Drop the resource managed version of requesting the IRQ in order to
> > not trigger a double free of the interrupt as below:
> >
> > [  226.731005] Trying to free already-free IRQ 126 [  226.735533]
> > WARNING: CPU: 6 PID: 749 at kernel/irq/manage.c:1707
> > __free_irq+0x9c/0x2b8
> > [  226.743435] Modules linked in:
> > [  226.746480] CPU: 6 PID: 749 Comm: bash Tainted: G        W
> > 5.4.0-03629-gfd7102c32b2c-dirty #912
> > [  226.755857] Hardware name: NXP Layerscape LX2160ARDB (DT) [
> > 226.761244] pstate: 40000085 (nZcv daIf -PAN -UAO) [  226.766022] pc :
> > __free_irq+0x9c/0x2b8 [  226.769758] lr : __free_irq+0x9c/0x2b8 [
> > 226.773493] sp : ffff8000125039f0
> > (...)
> > [  226.856275] Call trace:
> > [  226.858710]  __free_irq+0x9c/0x2b8
> > [  226.862098]  free_irq+0x30/0x70
> > [  226.865229]  devm_irq_release+0x14/0x20 [  226.869054]
> > release_nodes+0x1b0/0x220 [  226.872790]  devres_release_all+0x34/0x50
> > [  226.876790]  device_release_driver_internal+0x100/0x1c0
> >
> > Fixes: d346c9e86d86 ("dpaa2-ptp: reuse ptp_qoriq driver")
> > Cc: Yangbo Lu <yangbo.lu@nxp.com>
> > Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
> > ---
> >  drivers/net/ethernet/freescale/dpaa2/dpaa2-ptp.c | 12 +++++++-----
> >  1 file changed, 7 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-ptp.c
> > b/drivers/net/ethernet/freescale/dpaa2/dpaa2-ptp.c
> > index a9503aea527f..04a4b316f1dc 100644
> > --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-ptp.c
> > +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-ptp.c
> > @@ -160,10 +160,10 @@ static int dpaa2_ptp_probe(struct fsl_mc_device
> *mc_dev)
> >  	irq = mc_dev->irqs[0];
> >  	ptp_qoriq->irq = irq->msi_desc->irq;
> >
> > -	err = devm_request_threaded_irq(dev, ptp_qoriq->irq, NULL,
> > -					dpaa2_ptp_irq_handler_thread,
> > -					IRQF_NO_SUSPEND | IRQF_ONESHOT,
> > -					dev_name(dev), ptp_qoriq);
> > +	err = request_threaded_irq(ptp_qoriq->irq, NULL,
> > +				   dpaa2_ptp_irq_handler_thread,
> > +				   IRQF_NO_SUSPEND | IRQF_ONESHOT,
> > +				   dev_name(dev), ptp_qoriq);
> >  	if (err < 0) {
> >  		dev_err(dev, "devm_request_threaded_irq(): %d\n", err);
> >  		goto err_free_mc_irq;
> > @@ -173,7 +173,7 @@ static int dpaa2_ptp_probe(struct fsl_mc_device
> *mc_dev)
> >  				   DPRTC_IRQ_INDEX, 1);
> >  	if (err < 0) {
> >  		dev_err(dev, "dprtc_set_irq_enable(): %d\n", err);
> > -		goto err_free_mc_irq;
> > +		goto err_free_threaded_irq;
> >  	}
> >
> >  	err = ptp_qoriq_init(ptp_qoriq, base, &dpaa2_ptp_caps);
> 
> There is another goto right here which still jumps to err_free_mc_irq rather than
> err_free_threaded_irq. Is that intentional?

No, that is an oversight from my part. Will fix in v2.

Thanks,
Ioana


> 
> > @@ -185,6 +185,8 @@ static int dpaa2_ptp_probe(struct fsl_mc_device
> > *mc_dev)
> >
> >  	return 0;
> >
> > +err_free_threaded_irq:
> > +	free_irq(ptp_qoriq->irq, ptp_qoriq);
> >  err_free_mc_irq:
> >  	fsl_mc_free_irqs(mc_dev);
> >  err_unmap:
diff mbox series

Patch

diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-ptp.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-ptp.c
index a9503aea527f..04a4b316f1dc 100644
--- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-ptp.c
+++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-ptp.c
@@ -160,10 +160,10 @@  static int dpaa2_ptp_probe(struct fsl_mc_device *mc_dev)
 	irq = mc_dev->irqs[0];
 	ptp_qoriq->irq = irq->msi_desc->irq;
 
-	err = devm_request_threaded_irq(dev, ptp_qoriq->irq, NULL,
-					dpaa2_ptp_irq_handler_thread,
-					IRQF_NO_SUSPEND | IRQF_ONESHOT,
-					dev_name(dev), ptp_qoriq);
+	err = request_threaded_irq(ptp_qoriq->irq, NULL,
+				   dpaa2_ptp_irq_handler_thread,
+				   IRQF_NO_SUSPEND | IRQF_ONESHOT,
+				   dev_name(dev), ptp_qoriq);
 	if (err < 0) {
 		dev_err(dev, "devm_request_threaded_irq(): %d\n", err);
 		goto err_free_mc_irq;
@@ -173,7 +173,7 @@  static int dpaa2_ptp_probe(struct fsl_mc_device *mc_dev)
 				   DPRTC_IRQ_INDEX, 1);
 	if (err < 0) {
 		dev_err(dev, "dprtc_set_irq_enable(): %d\n", err);
-		goto err_free_mc_irq;
+		goto err_free_threaded_irq;
 	}
 
 	err = ptp_qoriq_init(ptp_qoriq, base, &dpaa2_ptp_caps);
@@ -185,6 +185,8 @@  static int dpaa2_ptp_probe(struct fsl_mc_device *mc_dev)
 
 	return 0;
 
+err_free_threaded_irq:
+	free_irq(ptp_qoriq->irq, ptp_qoriq);
 err_free_mc_irq:
 	fsl_mc_free_irqs(mc_dev);
 err_unmap: