Re: [PATCH net-next v10 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
---
--- Patch [5]: [PATCH net-next v10 5/5] net: wangxun: add pcie error handler ---
commit 91baa4e77d07ab6772569a7d37ed86cfda625c00
Author: Jiawen Wu <jiawenwu@trustnetic.com>
Subject: net: wangxun: add pcie error handler

This commit adds PCIe error handling support (AER) to the wangxun drivers
(ngbe and txgbe). It detects hardware errors, attempts to soft quiesce,
and registers standard PCI error handler callbacks.

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

[ ... ]

> +static void wx_io_resume(struct pci_dev *pdev)
> +{
> +	struct wx *wx = pci_get_drvdata(pdev);
> +	struct net_device *netdev;
> +	int err;
> +
> +	netdev = wx->netdev;
> +	rtnl_lock();
> +	if (netif_running(netdev)) {
> +		err = netdev->netdev_ops->ndo_open(netdev);

[Severity: High]
Does this sequence risk corrupting the hw_tc_lock seqlock?

During AER recovery, wx_io_slot_reset() calls down_suspend(), which bypasses
wx_ptp_stop(). This leaves the PTP clock registered and potentially in use by
userspace.

When wx_io_resume() then calls ndo_open(), this triggers txgbe_up() or
ngbe_up(), which in turn calls wx_ptp_init(). Since wx_ptp_init()
unconditionally calls seqlock_init(&wx->hw_tc_lock), won't this corrupt the
lock if userspace is actively issuing PTP ioctls?

> +		if (err) {
> +			wx_err(wx, "Failed to open netdev after reset\n");

[ ... ]

> 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

[ ... ]

> @@ -413,6 +428,9 @@ static void ngbe_disable_device(struct wx *wx)
>  
>  static void ngbe_reset(struct wx *wx)
>  {
> +	if (test_bit(WX_FLAG_NEED_PCIE_RECOVERY, wx->flags))
> +		return;
> +
>  	wx_flush_sw_mac_table(wx);

[Severity: High]
Is the hardware properly reinitialized here after an AER slot reset?

In wx_io_slot_reset(), the hardware needs to be restarted from scratch.
While txgbe_reset() calls txgbe_reset_hw(), ngbe_reset() only flushes the
software MAC tables and skips calling ngbe_reset_hw(). 

Because this patch registers wx_err_handler for the ngbe driver, won't this
leave the hardware uninitialized and the interface broken after a PCIe slot
reset?

[ ... ]

> @@ -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;
> +
>  	wx_ptp_stop(wx);

[Severity: High]
Does this early return cause a use-after-free for the PTP clock?

If a PCI error occurs, down_suspend() sets WX_STATE_RES_FREED but
intentionally skips wx_ptp_stop(). If the interface is subsequently closed or
unregistered, ngbe_close() will return early here and never call
wx_ptp_stop().

Because the PTP clock device is never unregistered, it remains exposed to
userspace after the driver's memory is freed. Can this lead to a
use-after-free if userspace interacts with the clock later?

[ ... ]

> diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
> index a7bde03a98feb..d85ee83192e4f 100644
> --- a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
> +++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c

[ ... ]

> @@ -530,12 +549,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;
> +
>  	wx_ptp_stop(wx);

[Severity: High]
Similar to ngbe_close() above, does this early return skip wx_ptp_stop() and
lead to a use-after-free if the txgbe device is closed after an AER suspend?