[PATCH] EDAC/fsl_ddr: use non devm for request_irq()

[PATCH] EDAC/fsl_ddr: use non devm for request_irq()

[Rosen Penev]

We need to tear down the irq before edac_mc_free() to avoid a use after
free on teardown as request_irq uses mci as dev_id.

Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/edac/fsl_ddr_edac.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/edac/fsl_ddr_edac.c b/drivers/edac/fsl_ddr_edac.c
index f4cc89b26954..f1ddd1f47f2a 100644
--- a/drivers/edac/fsl_ddr_edac.c
+++ b/drivers/edac/fsl_ddr_edac.c
@@ -601,7 +601,7 @@ int fsl_mc_err_probe(struct platform_device *op)
 
 		/* register interrupts */
 		pdata->irq = platform_get_irq(op, 0);
-		res = devm_request_irq(&op->dev, pdata->irq,
+		res = request_irq(pdata->irq,
 				       fsl_mc_isr,
 				       IRQF_SHARED,
 				       "[EDAC] MC err", mci);
@@ -639,6 +639,7 @@ void fsl_mc_err_remove(struct platform_device *op)
 
 	if (edac_op_state == EDAC_OPSTATE_INT) {
 		ddr_out32(pdata, FSL_MC_ERR_INT_EN, 0);
+		free_irq(pdata->irq, mci);
 	}
 
 	ddr_out32(pdata, FSL_MC_ERR_DISABLE,
-- 
2.55.0

Re: [PATCH] EDAC/fsl_ddr: use non devm for request_irq()

[Frank Li]

On Tue, Jul 28, 2026 at 06:23:20PM -0700, Rosen Penev wrote:
> We need to tear down the irq before edac_mc_free() to avoid a use after
> free on teardown as request_irq uses mci as dev_id.
>
> Signed-off-by: Rosen Penev <rosenp@gmail.com>
> ---
>  drivers/edac/fsl_ddr_edac.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/edac/fsl_ddr_edac.c b/drivers/edac/fsl_ddr_edac.c
> index f4cc89b26954..f1ddd1f47f2a 100644
> --- a/drivers/edac/fsl_ddr_edac.c
> +++ b/drivers/edac/fsl_ddr_edac.c
> @@ -601,7 +601,7 @@ int fsl_mc_err_probe(struct platform_device *op)
>
>  		/* register interrupts */
>  		pdata->irq = platform_get_irq(op, 0);
> -		res = devm_request_irq(&op->dev, pdata->irq,
> +		res = request_irq(pdata->irq,
>  				       fsl_mc_isr,
>  				       IRQF_SHARED,
>  				       "[EDAC] MC err", mci);
> @@ -639,6 +639,7 @@ void fsl_mc_err_remove(struct platform_device *op)
>
>  	if (edac_op_state == EDAC_OPSTATE_INT) {
>  		ddr_out32(pdata, FSL_MC_ERR_INT_EN, 0);
> +		free_irq(pdata->irq, mci);

here already disable hardware irq. Just call
synchronize_irq() to make sure pending irq handled.

Frank

>  	}
>
>  	ddr_out32(pdata, FSL_MC_ERR_DISABLE,
> --
> 2.55.0
>
>

Re: [PATCH] EDAC/fsl_ddr: use non devm for request_irq()

[Rosen Penev]

On Thu, Jul 30, 2026 at 11:49 AM Frank Li <Frank.li@oss.nxp.com> wrote:
>
> On Tue, Jul 28, 2026 at 06:23:20PM -0700, Rosen Penev wrote:
> > We need to tear down the irq before edac_mc_free() to avoid a use after
> > free on teardown as request_irq uses mci as dev_id.
> >
> > Signed-off-by: Rosen Penev <rosenp@gmail.com>
> > ---
> >  drivers/edac/fsl_ddr_edac.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/edac/fsl_ddr_edac.c b/drivers/edac/fsl_ddr_edac.c
> > index f4cc89b26954..f1ddd1f47f2a 100644
> > --- a/drivers/edac/fsl_ddr_edac.c
> > +++ b/drivers/edac/fsl_ddr_edac.c
> > @@ -601,7 +601,7 @@ int fsl_mc_err_probe(struct platform_device *op)
> >
> >               /* register interrupts */
> >               pdata->irq = platform_get_irq(op, 0);
> > -             res = devm_request_irq(&op->dev, pdata->irq,
> > +             res = request_irq(pdata->irq,
> >                                      fsl_mc_isr,
> >                                      IRQF_SHARED,
> >                                      "[EDAC] MC err", mci);
> > @@ -639,6 +639,7 @@ void fsl_mc_err_remove(struct platform_device *op)
> >
> >       if (edac_op_state == EDAC_OPSTATE_INT) {
> >               ddr_out32(pdata, FSL_MC_ERR_INT_EN, 0);
> > +             free_irq(pdata->irq, mci);
>
> here already disable hardware irq. Just call
> synchronize_irq() to make sure pending irq handled.
AI disagrees with this suggestion:

The reviewer's suggestion to just call synchronize_irq() instead of
free_irq() is problematic for two reasons:
1. Shared IRQ: The handler is registered with IRQF_SHARED. Disabling
the DDR controller's local IRQ output (ddr_out32(..., 0)) prevents
this device from asserting the IRQ line, but other devices sharing the
same IRQ line can still trigger it. Without free_irq, the handler
remains in the shared action list and can be called with a dangling
mci pointer after edac_mc_free() — the very UAF this patch aims to
fix.
2. free_irq already synchronizes: free_irq() internally calls
synchronize_irq() after removing the action from the descriptor. An
explicit synchronize_irq() before free_irq() would be redundant.
>
> Frank
>
> >       }
> >
> >       ddr_out32(pdata, FSL_MC_ERR_DISABLE,
> > --
> > 2.55.0
> >
> >

Re: [PATCH] EDAC/fsl_ddr: use non devm for request_irq()

[Borislav Petkov]

On Thu, Jul 30, 2026 at 05:16:11PM -0700, Rosen Penev wrote:
> > here already disable hardware irq. Just call
> > synchronize_irq() to make sure pending irq handled.
> AI disagrees with this suggestion:

How about you try to parse what AI says and verify it yourself?

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] EDAC/fsl_ddr: use non devm for request_irq()

[Rosen Penev]

On Thu, Jul 30, 2026 at 5:37 PM Borislav Petkov <bp@alien8.de> wrote:
>
> On Thu, Jul 30, 2026 at 05:16:11PM -0700, Rosen Penev wrote:
> > > here already disable hardware irq. Just call
> > > synchronize_irq() to make sure pending irq handled.
> > AI disagrees with this suggestion:
>
> How about you try to parse what AI says and verify it yourself?
Makes sense to me.

The alternative is to use devm_add_action_or_reset for edac_mc_free.
I'd rather not do that.
>
> --
> Regards/Gruss,
>     Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] EDAC/fsl_ddr: use non devm for request_irq()

[Frank Li]

On Thu, Jul 30, 2026 at 05:44:34PM -0700, Rosen Penev wrote:
> On Thu, Jul 30, 2026 at 5:37 PM Borislav Petkov <bp@alien8.de> wrote:
> >
> > On Thu, Jul 30, 2026 at 05:16:11PM -0700, Rosen Penev wrote:
> > > > here already disable hardware irq. Just call
> > > > synchronize_irq() to make sure pending irq handled.
> > > AI disagrees with this suggestion:
> >
> > How about you try to parse what AI says and verify it yourself?
> Makes sense to me.
>
> The alternative is to use devm_add_action_or_reset for edac_mc_free.
> I'd rather not do that.

This one should work, of add devm_edac_add*() version to simplufy probe and
beanfit other EDAC driver.

Frank

> >
> > --
> > Regards/Gruss,
> >     Boris.
> >
> > https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] EDAC/fsl_ddr: use non devm for request_irq()

[Rosen Penev]

On Fri, Jul 31, 2026 at 8:12 AM Frank Li <Frank.li@oss.nxp.com> wrote:
>
> On Thu, Jul 30, 2026 at 05:44:34PM -0700, Rosen Penev wrote:
> > On Thu, Jul 30, 2026 at 5:37 PM Borislav Petkov <bp@alien8.de> wrote:
> > >
> > > On Thu, Jul 30, 2026 at 05:16:11PM -0700, Rosen Penev wrote:
> > > > > here already disable hardware irq. Just call
> > > > > synchronize_irq() to make sure pending irq handled.
> > > > AI disagrees with this suggestion:
> > >
> > > How about you try to parse what AI says and verify it yourself?
> > Makes sense to me.
> >
> > The alternative is to use devm_add_action_or_reset for edac_mc_free.
> > I'd rather not do that.
>
> This one should work, of add devm_edac_add*() version to simplufy probe and
> beanfit other EDAC driver.
That's probably appropriate.
>
> Frank
>
> > >
> > > --
> > > Regards/Gruss,
> > >     Boris.
> > >
> > > https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] EDAC/fsl_ddr: use non devm for request_irq()

[Frank Li]

On Thu, Jul 30, 2026 at 05:16:11PM -0700, Rosen Penev wrote:
> On Thu, Jul 30, 2026 at 11:49 AM Frank Li <Frank.li@oss.nxp.com> wrote:
> >
> > On Tue, Jul 28, 2026 at 06:23:20PM -0700, Rosen Penev wrote:
> > > We need to tear down the irq before edac_mc_free() to avoid a use after
> > > free on teardown as request_irq uses mci as dev_id.
> > >
> > > Signed-off-by: Rosen Penev <rosenp@gmail.com>
> > > ---
> > >  drivers/edac/fsl_ddr_edac.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/edac/fsl_ddr_edac.c b/drivers/edac/fsl_ddr_edac.c
> > > index f4cc89b26954..f1ddd1f47f2a 100644
> > > --- a/drivers/edac/fsl_ddr_edac.c
> > > +++ b/drivers/edac/fsl_ddr_edac.c
> > > @@ -601,7 +601,7 @@ int fsl_mc_err_probe(struct platform_device *op)
> > >
> > >               /* register interrupts */
> > >               pdata->irq = platform_get_irq(op, 0);
> > > -             res = devm_request_irq(&op->dev, pdata->irq,
> > > +             res = request_irq(pdata->irq,
> > >                                      fsl_mc_isr,
> > >                                      IRQF_SHARED,
> > >                                      "[EDAC] MC err", mci);
> > > @@ -639,6 +639,7 @@ void fsl_mc_err_remove(struct platform_device *op)
> > >
> > >       if (edac_op_state == EDAC_OPSTATE_INT) {
> > >               ddr_out32(pdata, FSL_MC_ERR_INT_EN, 0);
> > > +             free_irq(pdata->irq, mci);
> >
> > here already disable hardware irq. Just call
> > synchronize_irq() to make sure pending irq handled.
> AI disagrees with this suggestion:
>
> The reviewer's suggestion to just call synchronize_irq() instead of
> free_irq() is problematic for two reasons:
> 1. Shared IRQ: The handler is registered with IRQF_SHARED. Disabling
> the DDR controller's local IRQ output (ddr_out32(..., 0)) prevents
> this device from asserting the IRQ line, but other devices sharing the
> same IRQ line can still trigger it. Without free_irq, the handler
> remains in the shared action list and can be called with a dangling
> mci pointer after edac_mc_free() — the very UAF this patch aims to
> fix.
> 2. free_irq already synchronizes: free_irq() internally calls
> synchronize_irq() after removing the action from the descriptor. An
> explicit synchronize_irq() before free_irq() would be redundant.

You post too quick, I have not seen this mail yet.

devm_free_irq() here. and add comment why need explicit call it.

Frank

> >
> > Frank
> >
> > >       }
> > >
> > >       ddr_out32(pdata, FSL_MC_ERR_DISABLE,
> > > --
> > > 2.55.0
> > >
> > >

Re: [PATCH] EDAC/fsl_ddr: use non devm for request_irq()

[Rosen Penev]

On Fri, Jul 31, 2026 at 7:53 AM Frank Li <Frank.li@oss.nxp.com> wrote:
>
> On Thu, Jul 30, 2026 at 05:16:11PM -0700, Rosen Penev wrote:
> > On Thu, Jul 30, 2026 at 11:49 AM Frank Li <Frank.li@oss.nxp.com> wrote:
> > >
> > > On Tue, Jul 28, 2026 at 06:23:20PM -0700, Rosen Penev wrote:
> > > > We need to tear down the irq before edac_mc_free() to avoid a use after
> > > > free on teardown as request_irq uses mci as dev_id.
> > > >
> > > > Signed-off-by: Rosen Penev <rosenp@gmail.com>
> > > > ---
> > > >  drivers/edac/fsl_ddr_edac.c | 3 ++-
> > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/edac/fsl_ddr_edac.c b/drivers/edac/fsl_ddr_edac.c
> > > > index f4cc89b26954..f1ddd1f47f2a 100644
> > > > --- a/drivers/edac/fsl_ddr_edac.c
> > > > +++ b/drivers/edac/fsl_ddr_edac.c
> > > > @@ -601,7 +601,7 @@ int fsl_mc_err_probe(struct platform_device *op)
> > > >
> > > >               /* register interrupts */
> > > >               pdata->irq = platform_get_irq(op, 0);
> > > > -             res = devm_request_irq(&op->dev, pdata->irq,
> > > > +             res = request_irq(pdata->irq,
> > > >                                      fsl_mc_isr,
> > > >                                      IRQF_SHARED,
> > > >                                      "[EDAC] MC err", mci);
> > > > @@ -639,6 +639,7 @@ void fsl_mc_err_remove(struct platform_device *op)
> > > >
> > > >       if (edac_op_state == EDAC_OPSTATE_INT) {
> > > >               ddr_out32(pdata, FSL_MC_ERR_INT_EN, 0);
> > > > +             free_irq(pdata->irq, mci);
> > >
> > > here already disable hardware irq. Just call
> > > synchronize_irq() to make sure pending irq handled.
> > AI disagrees with this suggestion:
> >
> > The reviewer's suggestion to just call synchronize_irq() instead of
> > free_irq() is problematic for two reasons:
> > 1. Shared IRQ: The handler is registered with IRQF_SHARED. Disabling
> > the DDR controller's local IRQ output (ddr_out32(..., 0)) prevents
> > this device from asserting the IRQ line, but other devices sharing the
> > same IRQ line can still trigger it. Without free_irq, the handler
> > remains in the shared action list and can be called with a dangling
> > mci pointer after edac_mc_free() — the very UAF this patch aims to
> > fix.
> > 2. free_irq already synchronizes: free_irq() internally calls
> > synchronize_irq() after removing the action from the descriptor. An
> > explicit synchronize_irq() before free_irq() would be redundant.
>
> You post too quick, I have not seen this mail yet.
>
> devm_free_irq() here. and add comment why need explicit call it.
Needing to explicitly call devm_free_irq kind of defeats the purpose of devm.

Explicit ordering in this case makes more sense to me, especially with
the ddr_out32 calls.

There's actually a second benefit to the non devm version. The devm
version prints an error message on failure. The normal one does not.
The significance is that the driver prints its own message.
>
> Frank
>
> > >
> > > Frank
> > >
> > > >       }
> > > >
> > > >       ddr_out32(pdata, FSL_MC_ERR_DISABLE,
> > > > --
> > > > 2.55.0
> > > >
> > > >

Re: [PATCH] EDAC/fsl_ddr: use non devm for request_irq()

[Frank Li]

On Fri, Jul 31, 2026 at 10:38:53AM -0700, Rosen Penev wrote:
> On Fri, Jul 31, 2026 at 7:53 AM Frank Li <Frank.li@oss.nxp.com> wrote:
> >
> > On Thu, Jul 30, 2026 at 05:16:11PM -0700, Rosen Penev wrote:
> > > On Thu, Jul 30, 2026 at 11:49 AM Frank Li <Frank.li@oss.nxp.com> wrote:
> > > >
> > > > On Tue, Jul 28, 2026 at 06:23:20PM -0700, Rosen Penev wrote:
> > > > > We need to tear down the irq before edac_mc_free() to avoid a use after
> > > > > free on teardown as request_irq uses mci as dev_id.
> > > > >
> > > > > Signed-off-by: Rosen Penev <rosenp@gmail.com>
> > > > > ---
> > > > >  drivers/edac/fsl_ddr_edac.c | 3 ++-
> > > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/edac/fsl_ddr_edac.c b/drivers/edac/fsl_ddr_edac.c
> > > > > index f4cc89b26954..f1ddd1f47f2a 100644
> > > > > --- a/drivers/edac/fsl_ddr_edac.c
> > > > > +++ b/drivers/edac/fsl_ddr_edac.c
> > > > > @@ -601,7 +601,7 @@ int fsl_mc_err_probe(struct platform_device *op)
> > > > >
> > > > >               /* register interrupts */
> > > > >               pdata->irq = platform_get_irq(op, 0);
> > > > > -             res = devm_request_irq(&op->dev, pdata->irq,
> > > > > +             res = request_irq(pdata->irq,
> > > > >                                      fsl_mc_isr,
> > > > >                                      IRQF_SHARED,
> > > > >                                      "[EDAC] MC err", mci);
> > > > > @@ -639,6 +639,7 @@ void fsl_mc_err_remove(struct platform_device *op)
> > > > >
> > > > >       if (edac_op_state == EDAC_OPSTATE_INT) {
> > > > >               ddr_out32(pdata, FSL_MC_ERR_INT_EN, 0);
> > > > > +             free_irq(pdata->irq, mci);
> > > >
> > > > here already disable hardware irq. Just call
> > > > synchronize_irq() to make sure pending irq handled.
> > > AI disagrees with this suggestion:
> > >
> > > The reviewer's suggestion to just call synchronize_irq() instead of
> > > free_irq() is problematic for two reasons:
> > > 1. Shared IRQ: The handler is registered with IRQF_SHARED. Disabling
> > > the DDR controller's local IRQ output (ddr_out32(..., 0)) prevents
> > > this device from asserting the IRQ line, but other devices sharing the
> > > same IRQ line can still trigger it. Without free_irq, the handler
> > > remains in the shared action list and can be called with a dangling
> > > mci pointer after edac_mc_free() — the very UAF this patch aims to
> > > fix.
> > > 2. free_irq already synchronizes: free_irq() internally calls
> > > synchronize_irq() after removing the action from the descriptor. An
> > > explicit synchronize_irq() before free_irq() would be redundant.
> >
> > You post too quick, I have not seen this mail yet.
> >
> > devm_free_irq() here. and add comment why need explicit call it.
> Needing to explicitly call devm_free_irq kind of defeats the purpose of devm.
>
> Explicit ordering in this case makes more sense to me, especially with
> the ddr_out32 calls.
>
> There's actually a second benefit to the non devm version. The devm
> version prints an error message on failure. The normal one does not.
> The significance is that the driver prints its own message.

It may need goto if add logic after request_irq(), I want to make probe
code clean enough.

Error message doesn't mattter, we can remove it.

I suggest use other method, let use devm add action to make free irq before
free mci.

Frank

> >
> > Frank
> >
> > > >
> > > > Frank
> > > >
> > > > >       }
> > > > >
> > > > >       ddr_out32(pdata, FSL_MC_ERR_DISABLE,
> > > > > --
> > > > > 2.55.0
> > > > >
> > > > >

Re: [PATCH] EDAC/fsl_ddr: use non devm for request_irq()

[Rosen Penev]

On Fri, Jul 31, 2026 at 11:47 AM Frank Li <Frank.li@oss.nxp.com> wrote:
>
> On Fri, Jul 31, 2026 at 10:38:53AM -0700, Rosen Penev wrote:
> > On Fri, Jul 31, 2026 at 7:53 AM Frank Li <Frank.li@oss.nxp.com> wrote:
> > >
> > > On Thu, Jul 30, 2026 at 05:16:11PM -0700, Rosen Penev wrote:
> > > > On Thu, Jul 30, 2026 at 11:49 AM Frank Li <Frank.li@oss.nxp.com> wrote:
> > > > >
> > > > > On Tue, Jul 28, 2026 at 06:23:20PM -0700, Rosen Penev wrote:
> > > > > > We need to tear down the irq before edac_mc_free() to avoid a use after
> > > > > > free on teardown as request_irq uses mci as dev_id.
> > > > > >
> > > > > > Signed-off-by: Rosen Penev <rosenp@gmail.com>
> > > > > > ---
> > > > > >  drivers/edac/fsl_ddr_edac.c | 3 ++-
> > > > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/drivers/edac/fsl_ddr_edac.c b/drivers/edac/fsl_ddr_edac.c
> > > > > > index f4cc89b26954..f1ddd1f47f2a 100644
> > > > > > --- a/drivers/edac/fsl_ddr_edac.c
> > > > > > +++ b/drivers/edac/fsl_ddr_edac.c
> > > > > > @@ -601,7 +601,7 @@ int fsl_mc_err_probe(struct platform_device *op)
> > > > > >
> > > > > >               /* register interrupts */
> > > > > >               pdata->irq = platform_get_irq(op, 0);
> > > > > > -             res = devm_request_irq(&op->dev, pdata->irq,
> > > > > > +             res = request_irq(pdata->irq,
> > > > > >                                      fsl_mc_isr,
> > > > > >                                      IRQF_SHARED,
> > > > > >                                      "[EDAC] MC err", mci);
> > > > > > @@ -639,6 +639,7 @@ void fsl_mc_err_remove(struct platform_device *op)
> > > > > >
> > > > > >       if (edac_op_state == EDAC_OPSTATE_INT) {
> > > > > >               ddr_out32(pdata, FSL_MC_ERR_INT_EN, 0);
> > > > > > +             free_irq(pdata->irq, mci);
> > > > >
> > > > > here already disable hardware irq. Just call
> > > > > synchronize_irq() to make sure pending irq handled.
> > > > AI disagrees with this suggestion:
> > > >
> > > > The reviewer's suggestion to just call synchronize_irq() instead of
> > > > free_irq() is problematic for two reasons:
> > > > 1. Shared IRQ: The handler is registered with IRQF_SHARED. Disabling
> > > > the DDR controller's local IRQ output (ddr_out32(..., 0)) prevents
> > > > this device from asserting the IRQ line, but other devices sharing the
> > > > same IRQ line can still trigger it. Without free_irq, the handler
> > > > remains in the shared action list and can be called with a dangling
> > > > mci pointer after edac_mc_free() — the very UAF this patch aims to
> > > > fix.
> > > > 2. free_irq already synchronizes: free_irq() internally calls
> > > > synchronize_irq() after removing the action from the descriptor. An
> > > > explicit synchronize_irq() before free_irq() would be redundant.
> > >
> > > You post too quick, I have not seen this mail yet.
> > >
> > > devm_free_irq() here. and add comment why need explicit call it.
> > Needing to explicitly call devm_free_irq kind of defeats the purpose of devm.
> >
> > Explicit ordering in this case makes more sense to me, especially with
> > the ddr_out32 calls.
> >
> > There's actually a second benefit to the non devm version. The devm
> > version prints an error message on failure. The normal one does not.
> > The significance is that the driver prints its own message.
>
> It may need goto if add logic after request_irq(), I want to make probe
> code clean enough.
>
> Error message doesn't mattter, we can remove it.
>
> I suggest use other method, let use devm add action to make free irq before
> free mci.
ACK. A couple of other drivers suffer from the same issue.
>
> Frank
>
> > >
> > > Frank
> > >
> > > > >
> > > > > Frank
> > > > >
> > > > > >       }
> > > > > >
> > > > > >       ddr_out32(pdata, FSL_MC_ERR_DISABLE,
> > > > > > --
> > > > > > 2.55.0
> > > > > >
> > > > > >