netdev - Re: [PATCH net resend] ixgbe: fix pci device refcount leak

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Mon, 28 Nov 2022 13:28:25 -0800
From:   Tony Nguyen <anthony.l.nguyen@...el.com>
To:     Yang Yingliang <yangyingliang@...wei.com>,
        <netdev@...r.kernel.org>, <intel-wired-lan@...ts.osuosl.org>
CC:     <jesse.brandeburg@...el.com>, <davem@...emloft.net>,
        <edumazet@...gle.com>, <kuba@...nel.org>, <pabeni@...hat.com>,
        <stephend@...icom-usa.com>, <jeffrey.t.kirsher@...el.com>,
        <andrew@...n.ch>, <f.fainelli@...il.com>
Subject: Re: [PATCH net resend] ixgbe: fix pci device refcount leak

On 11/21/2022 10:08 PM, Yang Yingliang wrote:
> As comment of pci_get_domain_bus_and_slot() says, it returns
> a pci device with refcount increment, when finish using it,
> the caller must decrement the reference count by calling
> pci_dev_put().
> 
> In ixgbe_get_first_secondary_devfn() and ixgbe_x550em_a_has_mii(),
> pci_dev_put() is called to avoid leak.
> 
> Fixes: 8fa10ef01260 ("ixgbe: register a mdiobus")
> Signed-off-by: Yang Yingliang <yangyingliang@...wei.com>
> ---

<snip>

> @@ -882,6 +884,7 @@ static bool ixgbe_x550em_a_has_mii(struct ixgbe_hw *hw)
>   	 * of those two root ports
>   	 */
>   	func0_pdev = ixgbe_get_first_secondary_devfn(PCI_DEVFN(0x16, 0));
> +	pci_dev_put(func0_pdev);
>   	if (func0_pdev) {
>   		if (func0_pdev == pdev)
>   			return true;
> @@ -889,6 +892,7 @@ static bool ixgbe_x550em_a_has_mii(struct ixgbe_hw *hw)
>   			return false;
>   	}
>   	func0_pdev = ixgbe_get_first_secondary_devfn(PCI_DEVFN(0x17, 0));
> +	pci_dev_put(func0_pdev);
>   	if (func0_pdev == pdev)
>   		return true;

It would probably be better to defer these puts until after the checks 
and values are set. I'd think some local vars and gotos may be cleaner 
than placing puts before all the returns.

Thanks,
Tony