> -----Original Message-----
> From: Kohei Enju <[email protected]>
> Sent: Monday, September 1, 2025 10:38 AM
> To: Loktionov, Aleksandr <[email protected]>
> Cc: [email protected]; Nguyen, Anthony L
> <[email protected]>; [email protected]; [email protected];
> [email protected]; [email protected]; intel-wired-
> [email protected]; Jagielski, Jedrzej
> <[email protected]>; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; Kitszel, Przemyslaw <[email protected]>;
> Wegrzyn, Stefan <[email protected]>
> Subject: Re: [Intel-wired-lan] [PATCH iwl-net v1] ixgbe: fix memory
> leak and use-after-free in ixgbe_recovery_probe()
>
> On Mon, 1 Sep 2025 07:11:26 +0000, Loktionov, Aleksandr wrote:
>
>
>
> >> [...]
>
> >>
>
> >> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>
> >> b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>
> >> index ff6e8ebda5ba..08368e2717c2 100644
>
> >> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>
> >> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>
> >> @@ -11510,10 +11510,10 @@ static int ixgbe_recovery_probe(struct
>
> >> ixgbe_adapter *adapter)
>
> >> shutdown_aci:
>
> >> mutex_destroy(&adapter->hw.aci.lock);
>
> >> ixgbe_release_hw_control(adapter);
>
> >> - devlink_free(adapter->devlink);
>
> >> clean_up_probe:
>
> >> disable_dev = !test_and_set_bit(__IXGBE_DISABLED, &adapter-
>
> >> >state);
>
> >> free_netdev(netdev);
>
> >I'd add a guard here: if (adapter->devlink)
>
> >What do you think?
>
>
>
> Thank you for the review.
>
>
>
> Currently ixgbe_recovery_probe() is only called from one location,
>
> ixgbe_probe(), and also always adapter->devlink is not NULL in this
> path
>
> since this is called after ixgbe_allocate_devlink(). In other words,
> if
>
> ixgbe_allocate_devlink() fails, ixgbe_recovery_probe() never be
> called.
>
>
>
> So I've thought that the guard might not be necessary like error
>
> handling in ixgbe_probe(), but could you let me know your concern if
> I'm
>
> overlooking something?
Good day, Kohei
Thank you for the explanation.
You are technically right (today), and not having the guard doesn't make the
patch incorrect!
That said, error-paths tend to evolve. A tiny if (adapter->devlink) {
devlink_free(...); adapter->devlink = NULL; }
makes this code resilient to future refactors or partial init/unwind changes
and prevents potential double-free if another label ever frees it earlier.
I'm fine either way for this fix, but I'd prefer the guard+NULL for robustness
and consistency with typical probe unwind patterns.
Reviewed-by: Aleksandr Loktionov <[email protected]>
>
>
>
> Thanks,
>
> Kohei.
>
>
>
> >> + devlink_free(adapter->devlink);
>
> >> pci_release_mem_regions(pdev);
>
> >> if (disable_dev)
>
> >> pci_disable_device(pdev);
>
> >> --
>
> >> 2.51.0
