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

["Jiawen Wu" <jiawenwu@trustnetic.com>]

On Thu, Jul 30, 2026 9:19 PM, Simon Horman wrote:
> 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.

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().

> 
> > +
> >  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().

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.
> 
> >  	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.

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.
> 
> >  	wx_ptp_stop(wx);
> >  	txgbe_down(wx);
>