Re: [Intel-wired-lan] [PATCH iwl-next v1 1/2] ixgbe: Refactor device operations to check whether netdev is available

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 [1]: [PATCH iwl-next v1 1/2] ixgbe: Refactor device operations to check whether netdev is available ---
commit f7037ec91fe4185b79d89bd64ec8042f9f9b1904
Author: Sergey Temerkhanov <[email protected]>

ixgbe: Refactor device operations to check whether netdev is available

Refactor several ixgbe driver operations to check whether the
netdev they operate on is enabled. This will allow the system
to get synchronized, for example, during the PCI resets.

> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_fcoe.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_fcoe.c
> index 011fda9c61939..e4cd4673f21c2 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_fcoe.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_fcoe.c
> @@ -836,6 +836,10 @@ int ixgbe_fcoe_enable(struct net_device *netdev)
>  {
>  	struct ixgbe_adapter *adapter = ixgbe_from_netdev(netdev);
>  	struct ixgbe_fcoe *fcoe = &adapter->fcoe;
> +	bool running;
> +
> +	if (!netif_device_present(netdev))
> +		return -ENETDOWN;
>  
>  	atomic_inc(&fcoe->refcnt);
>  
> @@ -845,12 +849,14 @@ int ixgbe_fcoe_enable(struct net_device *netdev)
>  	if (adapter->flags & IXGBE_FLAG_FCOE_ENABLED)
>  		return -EINVAL;

[Severity: High]
This is a pre-existing issue, but does this leak the FCoE reference count?

If ixgbe_fcoe_enable() is called when FCoE is already enabled or the adapter
is not FCoE capable, the function returns early without decrementing the
freshly incremented fcoe->refcnt.

Later, when ixgbe_fcoe_disable() is called, atomic_dec_and_test() will not
reach zero, causing it to return -EINVAL without freeing the FCoE DDP pool or
clearing the FCoE flags.

[ ... ]
> @@ -882,6 +888,10 @@ int ixgbe_fcoe_enable(struct net_device *netdev)
>  int ixgbe_fcoe_disable(struct net_device *netdev)
>  {
>  	struct ixgbe_adapter *adapter = ixgbe_from_netdev(netdev);
> +	bool running;
> +
> +	if (!netif_device_present(netdev))
> +		return -ENETDOWN;

[Severity: High]
Does this early return cause a memory leak of the FCoE DDP pool?

During a hardware error or device hot-unplug, ixgbe_io_error_detected() calls
netif_device_detach(), which clears the device present state. When the
driver is subsequently removed, the FCoE stack catches the unregister event
and calls ndo_fcoe_disable().

Because the device is already detached, this new early return triggers,
causing the function to skip calling ixgbe_fcoe_ddp_disable(). Will this
abandon the per-CPU DDP pool so it is never freed?

>  
>  	if (!atomic_dec_and_test(&adapter->fcoe.refcnt))
>  		return -EINVAL;
[ ... ]
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> index 8873a8cc4a185..12361ed58a0ff 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
[ ... ]
> @@ -10954,8 +10958,10 @@ static int ixgbe_xdp_setup(struct net_device *dev, struct bpf_prog *prog)
>  			synchronize_rcu();
>  		err = ixgbe_setup_tc(dev, adapter->hw_tcs);
>  
> -		if (err)
> +		if (err) {
> +			xchg(&adapter->xdp_prog, old_prog);
>  			return -EINVAL;
> +		}

[Severity: Critical]
This is a pre-existing issue, but does this error path leave a freed BPF
program installed in the active Rx rings?

If DCB is enabled on the interface, ixgbe_setup_tc() returns -EINVAL.
However, before returning, it calls ixgbe_open() to restart the interface:

ixgbe_setup_tc() {
        ...
        if (running)
                return ixgbe_open(dev);
}

During this restart, the Rx rings are initialized and assigned the new BPF
program currently stored in adapter->xdp_prog.

When ixgbe_xdp_setup() sees the error, it restores old_prog and returns
-EINVAL to the network stack, which then frees the new program. The
rx_ring[i]->xdp_prog pointers still reference the freed program. Can this
lead to a use-after-free?