Re: [PATCH v2 net 8/9] net: enetc: fix initialization order to prevent use of uninitialized resources

[Harshitha Ramamurthy]

On Sun, May 17, 2026 at 8:09 PM Wei Fang <wei.fang@xxxxxxx> wrote:
>
> Sashiko reported a potential issue in enetc_msg_psi_init() where the IRQ
> handler is registered before DMA resources are fully initialized [1].
>
> The current initialization sequence is:
>
>   1. request_irq(enetc_msg_psi_msix)    <- IRQ handler registered
>   2. INIT_WORK(&pf->msg_task, ...)      <- work_struct initialized
>   3. enetc_msg_alloc_mbx()              <- mailbox DMA allocated
>
> This ordering is unsafe because if a spurious interrupt or pending
> interrupt from a previous device state fires immediately after
> request_irq() returns, the registered ISR enetc_msg_psi_msix() will
> execute and unconditionally call:
>
>   schedule_work(&pf->msg_task)
>
> At this point, pf->msg_task has not been initialized by INIT_WORK(), so
> the work_struct contains garbage values in its internal linked list
> pointers (work_struct->entry). Passing an uninitialized work_struct to
> schedule_work() could corrupt the kernel's workqueue linked lists,
> potentially leading to:
>
>   - Kernel panic in __queue_work()
>   - Memory corruption in workqueue data structures
>   - System deadlock or undefined behavior
>
> Additionally, even if the work_struct were initialized, the mailbox DMA
> buffers (pf->rxmsg[]) may not yet be allocated when the work handler
> enetc_msg_task() runs, resulting in NULL pointer dereference.
>
> Fix by reordering the initialization sequence to ensure all resources are
> properly initialized before the interrupt handler can execute:
>
>   1. enetc_msg_alloc_mbx()              <- Allocate all mailboxes
>   2. INIT_WORK(&pf->msg_task, ...)      <- Initialize work first
>   3. request_irq(enetc_msg_psi_msix)    <- Register IRQ last
>   4. Configure hardware & enable MR interrupts
>
> This guarantees that when enetc_msg_psi_msix() runs:
>   - pf->msg_task is properly initialized (safe for schedule_work)
>   - pf->rxmsg[] buffers are allocated (safe for work handler access)
>   - Hardware is configured appropriately
>
> Link: https://sashiko.dev/#/patchset/20260511080805.2052495-1-wei.fang%40nxp.com #1
> Fixes: beb74ac878c8 ("enetc: Add vf to pf messaging support")
> Signed-off-by: Wei Fang <wei.fang@xxxxxxx>
> ---
>  .../net/ethernet/freescale/enetc/enetc_msg.c  | 35 ++++++++++---------
>  1 file changed, 18 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/net/ethernet/freescale/enetc/enetc_msg.c b/drivers/net/ethernet/freescale/enetc/enetc_msg.c
> index 39f057fe85c7..a999f1e7817f 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc_msg.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc_msg.c
> @@ -109,6 +109,15 @@ int enetc_msg_psi_init(struct enetc_pf *pf)
>         struct enetc_si *si = pf->si;
>         int vector, i, err;
>
> +       for (i = 0; i < pf->num_vfs; i++) {
> +               err = enetc_msg_alloc_mbx(si, i);
> +               if (err)
> +                       goto free_mbx;
> +       }
> +
> +       /* initialize PSI mailbox */
> +       INIT_WORK(&pf->msg_task, enetc_msg_task);
> +
>         /* register message passing interrupt handler */
>         snprintf(pf->msg_int_name, sizeof(pf->msg_int_name), "%s-vfmsg",
>                  si->ndev->name);
> @@ -117,32 +126,21 @@ int enetc_msg_psi_init(struct enetc_pf *pf)
>         if (err) {
>                 dev_err(&si->pdev->dev,
>                         "PSI messaging: request_irq() failed!\n");
> -               return err;
> +               goto free_mbx;
>         }
>
>         /* set one IRQ entry for PSI message receive notification (SI int) */
>         enetc_wr(&si->hw, ENETC_SIMSIVR, ENETC_SI_INT_IDX);
>
> -       /* initialize PSI mailbox */
> -       INIT_WORK(&pf->msg_task, enetc_msg_task);
> -
> -       for (i = 0; i < pf->num_vfs; i++) {
> -               err = enetc_msg_alloc_mbx(si, i);
> -               if (err)
> -                       goto err_init_mbx;
> -       }
> -
>         /* enable MR interrupts */
>         enetc_msg_enable_mr_int(&si->hw);
>
>         return 0;
>
> -err_init_mbx:
> +free_mbx:
>         for (i--; i >= 0; i--)
>                 enetc_msg_free_mbx(si, i);
>
> -       free_irq(vector, si);
> -
>         return err;
>  }
>
> @@ -151,14 +149,17 @@ void enetc_msg_psi_free(struct enetc_pf *pf)
>         struct enetc_si *si = pf->si;
>         int i;
>
> +       /* disable MR interrupts */
> +       enetc_msg_disable_mr_int(&si->hw);
> +
> +       /* de-register message passing interrupt handler */
> +       free_irq(pci_irq_vector(si->pdev, ENETC_SI_INT_IDX), si);
> +
>         cancel_work_sync(&pf->msg_task);
>
> -       /* disable MR interrupts */
> +       /* MR interrupts may be re-enabled by workqueue */
>         enetc_msg_disable_mr_int(&si->hw);
>
>         for (i = 0; i < pf->num_vfs; i++)
>                 enetc_msg_free_mbx(si, i);
> -
> -       /* de-register message passing interrupt handler */
> -       free_irq(pci_irq_vector(si->pdev, ENETC_SI_INT_IDX), si);

Code looks good to me but a nit: would be good if the change here in
enetc_msg_psi_free() is also called out in the commit message.

>  }
> --
> 2.34.1
>
>