Re: [PATCH net-next 14/15] net: enetc: add PSI-to-VSI link status notification support for VF

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Incorrect teardown sequence in device removal and probe error paths leads to Use-After-Free and hardware interrupt storms.
- [High] VF initialization fails completely on older host PF drivers due to unsupported link status registration command.
- [Medium] Missing workqueue flush in `enetc_close()` allows asynchronous carrier state modification on a down interface.
--

commit 24125d222c23303bc6d2d8db3f12047a510f7b92
Author: Wei Fang <wei.fang@nxp.com>

    net: enetc: add PSI-to-VSI link status notification support for VF
    
    Add infrastructure for ENETC v4 VFs to track PF link status changes via
    the PSI-to-VSI messaging channel. Two new ops,
    vf_reg_link_status_notifier and vf_unreg_link_status_notifier, are added
    to enetc_si_ops and wired into enetc_phylink_connect() and enetc_close()
    for the phy-less path.

> diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
> index fdceaf36daa72..a1a8a1551cd8e 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc.c
> @@ -2892,11 +2892,15 @@ static void enetc_clear_interrupts(struct enetc_ndev_priv *priv)
>  static int enetc_phylink_connect(struct net_device *ndev)
>  {
>  	struct enetc_ndev_priv *priv = netdev_priv(ndev);
> +	struct enetc_si *si = priv->si;
>  	struct ethtool_keee edata;
>  	int err;
>  
>  	if (!priv->phylink) {
>  		/* phy-less mode */
> +		if (si->ops->vf_reg_link_status_notifier)
> +			return si->ops->vf_reg_link_status_notifier(si);
> +

[Severity: High]
If an older PF driver rejects the ENETC_MSG_REGISTER_LINK_CHANGE_NOTIFIER
command and returns -EOPNOTSUPP, will propagating this error cause
enetc_open() to abort and completely break VF initialization on older host
kernels?

Would it be safer to intercept -EOPNOTSUPP and fall back to the legacy
netif_carrier_on() behavior?

>  		netif_carrier_on(ndev);
>  		return 0;
>  	}

[ ... ]

> @@ -3050,6 +3057,7 @@ EXPORT_SYMBOL_GPL(enetc_stop);
>  int enetc_close(struct net_device *ndev)
>  {
>  	struct enetc_ndev_priv *priv = netdev_priv(ndev);
> +	struct enetc_si *si = priv->si;
>  
>  	enetc_stop(ndev);
>  
> @@ -3057,6 +3065,9 @@ int enetc_close(struct net_device *ndev)
>  		phylink_stop(priv->phylink);
>  		phylink_disconnect_phy(priv->phylink);
>  	} else {
> +		if (si->ops->vf_unreg_link_status_notifier)
> +			si->ops->vf_unreg_link_status_notifier(si);
> +
>  		netif_carrier_off(ndev);
>  	}

[Severity: Medium]
Since si->msg_task isn't flushed or canceled here, could a pending link status
interrupt execute asynchronously after the device is closed?

If enetc_vf_msg_task() runs after netif_carrier_off(), it might incorrectly
invoke netif_carrier_on() and turn the carrier state back on for a closed
interface. Does it make sense to add flush_work(&si->msg_task) here to ensure
proper synchronization?

> diff --git a/drivers/net/ethernet/freescale/enetc/enetc_vf.c b/drivers/net/ethernet/freescale/enetc/enetc_vf.c
> index 418ee98da17d6..9cace77110b8a 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc_vf.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc_vf.c

[ ... ]

> @@ -524,7 +706,8 @@ static void enetc_vf_remove(struct pci_dev *pdev)
>  
>  	priv = netdev_priv(si->ndev);
>  	unregister_netdev(si->ndev);
> -
> +	enetc_vf_free_msg_msix(si);
> +	enetc_vf_wq_task_destroy(si);
>  	enetc_free_msix(priv);
>  
>  	enetc_free_si_resources(priv);

[Severity: High]
Could this teardown sequence result in an interrupt storm and a
use-after-free? This also appears to apply to the enetc_vf_probe() error
path.

During removal, enetc_vf_free_msg_msix() frees the OS-level IRQ handler. Then
enetc_vf_wq_task_destroy() waits for the workqueue to drain. If
enetc_vf_msg_task() executes during this time, it unconditionally calls
enetc_vf_enable_mr_int() at the end, re-enabling the interrupt in the
hardware. Since the OS handler is gone, this leaves the device generating
unhandled MSI-X interrupts, which might lead to an IRQ storm or IOMMU fault.

Additionally, unregister_netdev() has already completed before the workqueue
is destroyed. If the work item executes and calls netif_carrier_on() for the
unregistered device, it adds the device to the global linkwatch list. When the
linkwatch worker later processes the list, it accesses the freed net_device
memory.

Should the teardown order be adjusted so the workqueue is destroyed before the
interrupt is freed, and should both happen before the netdev is unregistered?

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