Re: [PATCH net-next v12 5/5] net: wangxun: add pcie error handler

[Simon Horman <horms@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev/#/patchset/20260724101309.23472-2-jiawenwu@trustnetic.com?part=5
---
--- Patch [5]: [PATCH net-next v12 5/5] net: wangxun: add pcie error handler ---
commit bbdaff8533430e43d4fe6382b836808684487044
Author: Jiawen Wu <jiawenwu@trustnetic.com>

net: wangxun: add pcie error handler

Support AER driver to handle the PCIe errors. Sometimes netdev watchdog
Tx timeout happens before the AER error report when a PCIe error occurs,
CPU blocking would be caused by MMIO during the reset process. To
prevent it, check PCIe error status in .ndo_tx_timeout. The current
function of ngbe is not yet fully developed, it will be completed in the
future.

> diff --git a/drivers/net/ethernet/wangxun/libwx/wx_err.c b/drivers/net/ethernet/wangxun/libwx/wx_err.c
> index ee27f96735dc7..7936845c2894c 100644
> --- a/drivers/net/ethernet/wangxun/libwx/wx_err.c
> +++ b/drivers/net/ethernet/wangxun/libwx/wx_err.c

[ ... ]

> @@ -139,6 +253,33 @@ void wx_check_hang_subtask(struct wx *wx)
>  }
>  EXPORT_SYMBOL(wx_check_hang_subtask);
>  
> +static bool wx_check_pcie_error(struct wx *wx)
> +{
> +	u16 vid, pci_cmd;
> +
> +	pci_read_config_word(wx->pdev, PCI_VENDOR_ID, &vid);
> +	pci_read_config_word(wx->pdev, PCI_COMMAND, &pci_cmd);
> +
> +	/* PCIe link loss or memory space can't access */
> +	if (vid == U16_MAX || !(pci_cmd & PCI_COMMAND_MEMORY))
> +		return true;
> +
> +	return false;
> +}
> +
> +static void wx_tx_timeout_recovery(struct wx *wx)
> +{
> +	/*
> +	 * When a PCIe hardware error occurs, the driver should initiate a PCIe
> +	 * recovery mechanism. However, this recovery flow relies on the AER
> +	 * driver for current kernel policy. Therefore, a self-contained
> +	 * recovery mechanism is not implemented yet.
> +	 */
> +	set_bit(WX_FLAG_NEED_PCIE_RECOVERY, wx->flags);
> +	wx_err(wx, "PCIe error detected during tx timeout\n");
> +	queue_work(wx->reset_wq, &wx->reset_task);
> +}

[Severity: Medium]
Can a race condition between this timeout handler and concurrent AER recovery
permanently disable the device?

If the timeout handler detects an error but is delayed, AER recovery might
complete in the meantime, clearing the recovery flag and restoring the
device.

When this delayed handler resumes, it would blindly set
WX_FLAG_NEED_PCIE_RECOVERY and queue the reset task. This would cause
wx_reset_task() to unconditionally call wx_soft_quiesce() on the freshly
recovered device, leaving it permanently unresponsive.

> +
>  static void wx_tx_timeout_reset(struct wx *wx)
>  {

[ ... ]

> diff --git a/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c b/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
> index 92895f5035118..56d4b63387fd6 100644
> --- a/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
> +++ b/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c

[ ... ]

> @@ -529,12 +548,16 @@ static int ngbe_close(struct net_device *netdev)
>  {
>  	struct wx *wx = netdev_priv(netdev);
>  
> +	if (test_bit(WX_STATE_RES_FREED, wx->state))
> +		goto out;
> +

Simon says: I believe the issue below this was raised in the review of an
	    earlier version, and changes were made to address it. But could
	    you take another look?

[Severity: High]
Does bypassing wx_ptp_stop() here leak the PTP clock device and cause a
use-after-free?

When an AER error triggers a slot reset, wx_io_slot_reset() invokes
ngbe_down_suspend() which sets WX_STATE_RES_FREED but does not call
wx_ptp_stop().

If the device is subsequently closed, this check causes the driver to skip
wx_ptp_stop(). The driver memory is then freed while the kernel PTP subsystem
retains a dangling pointer to the destroyed clock device, which will panic
upon any subsequent userspace access.

>  	wx_ptp_stop(wx);
>  	ngbe_down(wx);

[ ... ]

> diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
> index 11ac4b04f5ee3..582ee567e6b9c 100644
> --- a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
> +++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c

[ ... ]

> @@ -538,12 +557,16 @@ static int txgbe_close(struct net_device *netdev)
>  {
>  	struct wx *wx = netdev_priv(netdev);
>  
> +	if (test_bit(WX_STATE_RES_FREED, wx->state))
> +		goto out;

Simon says: ditto

[Severity: High]
Similarly, does bypassing wx_ptp_stop() here leak the PTP clock device
and cause a use-after-free?

Just like in ngbe_close(), if an AER error triggers a slot reset,
wx_io_slot_reset() invokes txgbe_down_suspend() which sets
WX_STATE_RES_FREED without calling wx_ptp_stop().

If the device is then closed, this bypasses the cleanup, leaving the
kernel PTP subsystem with a dangling pointer to the destroyed clock device.

Simon says: I think the issue below can be looked at in the context
            of possible follow-up.

[Severity: High]
This is a pre-existing issue, but does failing to open the device in
txgbe_open() lead to a NULL pointer dereference on subsequent open attempts?

If txgbe_open() encounters an error, the error path calls
wx_reset_interrupt_capability(), which frees wx->msix_q_entries and sets
it to NULL.

If a user subsequently attempts to bring the interface up again,
txgbe_request_queue_irqs() is called, which dereferences the NULL
wx->msix_q_entries array.

>  	wx_ptp_stop(wx);
>  	txgbe_down(wx);