RE: [PATCH net-next v12 5/5] net: wangxun: add pcie error handler
From: Jiawen Wu <jiawenwu@trustnetic.com>
Date: 2026-07-31 02:29:49
On Thu, Jul 30, 2026 9:19 PM, Simon Horman wrote:
quoted hunk ↗ jump to hunk
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 [off-list ref] 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.quoted
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[ ... ]quoted
@@ -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.
I believe the window for this race condition is extremely small and the probability of hitting it is very low. But I'll add a double-check mechanism in wx_reset_task() to verify the PCIe error status before calling wx_soft_quiesce().
quoted
+ static void wx_tx_timeout_reset(struct wx *wx) {[ ... ]quoted
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[ ... ]quoted
@@ -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().
wx_ptp_quiesce() was invoked in wx_io_error_detected(), which destroys clock device. I think wx_io_error_detected() is called first when an AER error occurs.
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.quoted
wx_ptp_stop(wx); ngbe_down(wx);[ ... ]quoted
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[ ... ]quoted
@@ -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.
I'll consider handling this in another patch.
[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.quoted
wx_ptp_stop(wx); txgbe_down(wx);