Re: [PATCH] net: fman: guard IRQ handlers against pre-init interrupt

[Paolo Abeni <pabeni@redhat.com>]

On 6/29/26 10:45 AM, ZhaoJinming wrote:
> read_dts_node() registers shared interrupt handlers via
> devm_request_irq() with fman as the dev_id, passing it to fman_irq()
> and fman_err_irq(). At registration time, fman is only partially
> initialized -- kzalloc_obj() zero-initializes all fields, so
> fman->cfg and fman->fpm_regs are NULL.
> 
> The handlers guard against incomplete initialization via:
> 
>     if (!is_init_done(fman->cfg))
>         return IRQ_NONE;
> 
> However, is_init_done(NULL) returns true (intended to indicate that
> fman_init() has completed and cfg has been freed). This means when
> cfg is NULL before fman_config() allocates it, the guard does not
> take effect:
> 
>     is_init_done(NULL)  -> returns true
>     !true            -> false
>     guard skipped     -> proceeds to dereference NULL fpm_regs
> 
> If another device on the same shared IRQ line fires during the window
> between devm_request_irq() and fman_init(), the handler accesses
> NULL fman->fpm_regs via ioread32be(), causing a crash. The window
> includes of_platform_populate() which can be slow.
> 
> Add an irq_ready flag to struct fman that is set to true only after
> fman_init() completes in fman_probe(). Check this flag at the start
> of fman_irq() and fman_err_irq() before any register access.
> 
> Use READ_ONCE()/WRITE_ONCE() for the flag accesses since it is a
> cross-context shared variable written in process context and read
> in interrupt context.
> 
> This issue was identified in code review during the discussion of a
> separate IRQF_SHARED UAF fix patch:
> https://lore.kernel.org/netdev/20260626162323.GE1310988@horms.kernel.org/
> 
> Signed-off-by: ZhaoJinming <zhaojinming@uniontech.com>
> ---
>  drivers/net/ethernet/freescale/fman/fman.c | 5 +++++
>  drivers/net/ethernet/freescale/fman/fman.h | 1 +
>  2 files changed, 6 insertions(+)
> 
> diff --git a/drivers/net/ethernet/freescale/fman/fman.c b/drivers/net/ethernet/freescale/fman/fman.c
> index 013273a2de32..f14cb02d85a4 100644
> --- a/drivers/net/ethernet/freescale/fman/fman.c
> +++ b/drivers/net/ethernet/freescale/fman/fman.c
> @@ -2510,6 +2510,8 @@ static irqreturn_t fman_err_irq(int irq, void *handle)
>  	struct fman_fpm_regs __iomem *fpm_rg;
>  	irqreturn_t single_ret, ret = IRQ_NONE;
>  
> +	if (!READ_ONCE(fman->irq_ready))
> +		return IRQ_NONE;

Sashiko noted that on weakly ordered arch this could not be enough:

https://sashiko.dev/#/patchset/20260629084529.3709393-1-zhaojinming%40uniontech.com

Also I have the feeling this is papering over the real issue. Why
initializing the irq when the driver is not yet ready? Why don't move
IRQ initialization later?

/P