Re: [PATCH net-next 14/15] net: enetc: refactor MR interrupt enable/disable helpers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Teardown race in enetc_msg_psi_free() leading to Use-After-Free.
- [High] Uninitialized work_struct access in enetc_msg_psi_init() leading to kernel panic.
- [High] Resource leak and Use-After-Free on error path in enetc_msg_psi_init().
- [High] DMA Memory Use-After-Free in enetc_msg_free_mbx().
- [High] Race condition in enetc_msg_task() resulting in missed VF messages.
--

commit 776ed9b5141bfafabe9032ba504482f8555f1985
Author: Wei Fang <wei.fang@nxp.com>

net: enetc: refactor MR interrupt enable/disable helpers

The patch consolidates enetc_msg_enable_mr_int() and
enetc_msg_disable_mr_int() into a single enetc_msg_set_mr_int() helper
that takes a boolean enable parameter. This prepares the code for
dynamic interrupt mask generation based on the number of enabled VFs.

> diff --git a/drivers/net/ethernet/freescale/enetc/enetc_msg.c b/drivers/net/ethernet/freescale/enetc/enetc_msg.c
> index f3e78865617e6..73e32c9b65a89 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc_msg.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc_msg.c

[ ... ]

> @@ -171,7 +171,7 @@ static void enetc_msg_task(struct work_struct *work)
>  		if (!mr_mask) {
>  			/* re-arm MR interrupts, w1c the IDR reg */
>  			enetc_wr(hw, ENETC_PSIIDR, ENETC_PSIIER_MR_MASK);
> -			enetc_msg_enable_mr_int(hw);
> +			enetc_msg_set_mr_int(pf, true);
>  			return;
>  		}

This isn't a bug in your patch, but is there a race condition here that
could result in missed VF messages?

The code reads ENETC_PSIMSGRR earlier in the loop. If a VF sends a message
after the read but before the write to ENETC_PSIIDR, the hardware will set
the interrupt status bit, which is then immediately cleared by the write
to ENETC_PSIIDR. When the interrupt is enabled shortly after, it will not
fire.

Should the driver clear the interrupt status before checking for messages?

[ ... ]

> @@ -264,7 +264,7 @@ static int enetc_msg_psi_init(struct enetc_pf *pf)
>  	}
>  
>  	/* enable MR interrupts */
> -	enetc_msg_enable_mr_int(&si->hw);
> +	enetc_msg_set_mr_int(pf, true);
>  
>  	return 0;

This isn't a bug in your patch, but looking at the surrounding code in
enetc_msg_psi_init(), is there a risk of accessing an uninitialized
work_struct?

	vector = pci_irq_vector(si->pdev, ENETC_SI_INT_IDX);
	err = request_irq(vector, enetc_msg_psi_msix, 0, pf->msg_int_name, si);
	...
	/* initialize PSI mailbox */
	INIT_WORK(&pf->msg_task, enetc_msg_task);

Since request_irq() enables the interrupt immediately, if a pending
interrupt fires before INIT_WORK() is called, the handler will schedule
the uninitialized work_struct. Should INIT_WORK() be called before
request_irq()?

Also, looking at the error path just below this:

err_init_mbx:
	for (i--; i >= 0; i--)
		enetc_msg_free_mbx(si, i);

	free_irq(vector, si);

	return err;

Does this code leak the scheduled work? If an interrupt fired during the
allocation loop and scheduled pf->msg_task, free_irq() will wait for the
handler to finish but will not cancel the scheduled work. After returning,
the work could execute and access the freed mailbox memory. Should
cancel_work_sync() be called in this error path?

>  
> @@ -285,7 +285,7 @@ static void enetc_msg_psi_free(struct enetc_pf *pf)
>  	cancel_work_sync(&pf->msg_task);
>  
>  	/* disable MR interrupts */
> -	enetc_msg_disable_mr_int(&si->hw);
> +	enetc_msg_set_mr_int(pf, false);

This isn't a bug in your patch, but is there a teardown race here?

If a hardware interrupt fires right after cancel_work_sync() completes,
the handler will execute and call schedule_work(&pf->msg_task). The work
will then run after the mailboxes are freed and the IRQ is unregistered.

Should enetc_msg_set_mr_int() and free_irq() be called before
cancel_work_sync()?

>  
>  	for (i = 0; i < pf->num_vfs; i++)
>  		enetc_msg_free_mbx(si, i);

While looking at this teardown path, I noticed enetc_msg_free_mbx() calls
dma_free_coherent() before clearing the hardware base address registers
(ENETC_PSIVMSGRCVAR0 and ENETC_PSIVMSGRCVAR1).

If a VF sends a message before the registers are zeroed, could the hardware
perform a DMA write to memory that has already been returned to the allocator?

Should the hardware registers be cleared before the backing DMA memory
is freed?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260511080805.2052495-1-wei.fang@nxp.com?part=14