Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <a7d2bc99-8224-6d8d-57ff-fe68f4a04c2e@microchip.com>
Date:   Mon, 22 May 2023 11:07:06 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.0
Subject: Re: [PATCH] ARM: at91: pm: fix imbalanced reference counter for
 ethernet devices
Content-Language: en-US
To:     Claudiu Beznea <claudiu.beznea@microchip.com>,
        <alexandre.belloni@bootlin.com>, <linux@armlinux.org.uk>
CC:     <linux-arm-kernel@lists.infradead.org>,
        <linux-kernel@vger.kernel.org>
References: <20230518062511.2988500-1-claudiu.beznea@microchip.com>
From:   Nicolas Ferre <nicolas.ferre@microchip.com>
Organization: microchip
In-Reply-To: <20230518062511.2988500-1-claudiu.beznea@microchip.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 18/05/2023 at 08:25, Claudiu Beznea wrote:
> The of_find_device_by_node() function is returning a struct platform_device
> object with the embedded struct device member's reference counter
> incremented. This needs to be dropped when done with the platform device
> returned by of_find_device_by_node().
> 
> at91_pm_eth_quirk_is_valid() calls of_find_device_by_node() on
> suspend and resume path. On suspend it calls of_find_device_by_node() and
> on resume and failure paths it drops the counter of
> struct platform_device::dev.
> 
> In case ethernet device may not wakeup there is a put_device() on
> at91_pm_eth_quirk_is_valid() which is wrong as it colides with
> put_device() on resume path leading to the reference counter of struct
> device embedded in struct platform_device to be messed, the following
> stack trace to be displayed (after 5 consecutive suspend/resume cycles)
> and the execution to hang:
> 
> WARNING: CPU: 0 PID: 378 at lib/refcount.c:25 0xc07ffc08
> refcount_t: addition on 0; use-after-free.
> Modules linked in:
> CPU: 0 PID: 378 Comm: sh Not tainted 6.1.22-linux4microchip-2023.04-rc3+ #7
> Hardware name: Microchip SAMA7
> Function entered at [<c010c134>] from [<c010993c>]
> Function entered at [<c010993c>] from [<c0823754>]
> Function entered at [<c0823754>] from [<c01162ac>]
> Function entered at [<c01162ac>] from [<c0116340>]
> Function entered at [<c0116340>] from [<c07ffc08>]
> Function entered at [<c07ffc08>] from [<c045fe88>]
> Function entered at [<c045fe88>] from [<c046004c>]
> Function entered at [<c046004c>] from [<c0141e94>]
> Function entered at [<c0141e94>] from [<c0142448>]
> Function entered at [<c0142448>] from [<c0140da8>]
> Function entered at [<c0140da8>] from [<c023dba0>]
> Function entered at [<c023dba0>] from [<c01d0700>]
> Function entered at [<c01d0700>] from [<c01d092c>]
> Function entered at [<c01d092c>] from [<c0100060>]
> Exception stack(0xe0e81fa8 to 0xe0e81ff0)
> 1fa0:                   00000004 0057c668 00000001 0057c668 00000004 00000000
> 1fc0: 00000004 0057c668 b6ecaba0 00000004 b6f4c0e0 b6ecb15c 00000000 00000000
> 1fe0: 005456f0 beb3a788 b6dcfac4 b6e3bab8
> ---[ end trace 0000000000000000 ]---
> ------------[ cut here ]------------
> WARNING: CPU: 0 PID: 378 at lib/refcount.c:28 0xc045fef4
> refcount_t: underflow; use-after-free.
> Modules linked in:
> CPU: 0 PID: 378 Comm: sh Tainted: G        W          6.1.22-linux4microchip-2023.04-rc3+ #7
> Hardware name: Microchip SAMA7
> Function entered at [<c010c134>] from [<c010993c>]
> Function entered at [<c010993c>] from [<c0823754>]
> Function entered at [<c0823754>] from [<c01162ac>]
> Function entered at [<c01162ac>] from [<c0116340>]
> Function entered at [<c0116340>] from [<c045fef4>]
> Function entered at [<c045fef4>] from [<c046004c>]
> Function entered at [<c046004c>] from [<c0141e94>]
> Function entered at [<c0141e94>] from [<c0142448>]
> Function entered at [<c0142448>] from [<c0140da8>]
> Function entered at [<c0140da8>] from [<c023dba0>]
> Function entered at [<c023dba0>] from [<c01d0700>]
> Function entered at [<c01d0700>] from [<c01d092c>]
> Function entered at [<c01d092c>] from [<c0100060>]
> Exception stack(0xe0e81fa8 to 0xe0e81ff0)
> 1fa0:                   00000004 0057c668 00000001 0057c668 00000004 00000000
> 1fc0: 00000004 0057c668 b6ecaba0 00000004 b6f4c0e0 b6ecb15c 00000000 00000000
> 1fe0: 005456f0 beb3a788 b6dcfac4 b6e3bab8
> ---[ end trace 0000000000000000 ]---
> ------------[ cut here ]------------
> WARNING: CPU: 0 PID: 378 at lib/refcount.c:22 0xc07ffbf4
> refcount_t: saturated; leaking memory.
> Modules linked in:
> CPU: 0 PID: 378 Comm: sh Tainted: G        W          6.1.22-linux4microchip-2023.04-rc3+ #7
> Hardware name: Microchip SAMA7
> Function entered at [<c010c134>] from [<c010993c>]
> Function entered at [<c010993c>] from [<c0823754>]
> Function entered at [<c0823754>] from [<c01162ac>]
> Function entered at [<c01162ac>] from [<c0116340>]
> Function entered at [<c0116340>] from [<c07ffbf4>]
> Function entered at [<c07ffbf4>] from [<c045eaa0>]
> Function entered at [<c045eaa0>] from [<c045fcc4>]
> Function entered at [<c045fcc4>] from [<c045fee4>]
> Function entered at [<c045fee4>] from [<c046004c>]
> Function entered at [<c046004c>] from [<c0141e94>]
> Function entered at [<c0141e94>] from [<c0142448>]
> Function entered at [<c0142448>] from [<c0140da8>]
> Function entered at [<c0140da8>] from [<c023dba0>]
> Function entered at [<c023dba0>] from [<c01d0700>]
> Function entered at [<c01d0700>] from [<c01d092c>]
> Function entered at [<c01d092c>] from [<c0100060>]
> Exception stack(0xe0e81fa8 to 0xe0e81ff0)
> 1fa0:                   00000004 0057c668 00000001 0057c668 00000004 00000000
> 1fc0: 00000004 0057c668 b6ecaba0 00000004 b6f4c0e0 b6ecb15c 00000000 00000000
> 1fe0: 005456f0 beb3a788 b6dcfac4 b6e3bab8
> ---[ end trace 0000000000000000 ]---
> ------------[ cut here ]------------
> WARNING: CPU: 0 PID: 378 at kernel/irq/chip.c:241 0xc014be2c
> Modules linked in:
> CPU: 0 PID: 378 Comm: sh Tainted: G        W          6.1.22-linux4microchip-2023.04-rc3+ #7
> Hardware name: Microchip SAMA7
> Function entered at [<c010c134>] from [<c010993c>]
> Function entered at [<c010993c>] from [<c0823754>]
> Function entered at [<c0823754>] from [<c01162ac>]
> Function entered at [<c01162ac>] from [<c011637c>]
> Function entered at [<c011637c>] from [<c014be2c>]
> Function entered at [<c014be2c>] from [<c014f808>]
> Function entered at [<c014f808>] from [<c0460050>]
> Function entered at [<c0460050>] from [<c0141e94>]
> Function entered at [<c0141e94>] from [<c0142448>]
> Function entered at [<c0142448>] from [<c0140da8>]
> Function entered at [<c0140da8>] from [<c023dba0>]
> Function entered at [<c023dba0>] from [<c01d0700>]
> Function entered at [<c01d0700>] from [<c01d092c>]
> Function entered at [<c01d092c>] from [<c0100060>]
> Exception stack(0xe0e81fa8 to 0xe0e81ff0)
> 1fa0:                   00000004 0057c668 00000001 0057c668 00000004 00000000
> 1fc0: 00000004 0057c668 b6ecaba0 00000004 b6f4c0e0 b6ecb15c 00000000 00000000
> 1fe0: 005456f0 beb3a788 b6dcfac4 b6e3bab8
> ---[ end trace 0000000000000000 ]---
> at_xdmac e1200000.dma-controller: controller in mem2mem mode.
> ------------[ cut here ]------------
> WARNING: CPU: 0 PID: 378 at lib/kobject.c:634 0xc07ffbe8
> kobject: '$���"����L��L��' (a3ba4c7d): is not initialized, yet kobject_get() is being called.
> Modules linked in:
> CPU: 0 PID: 378 Comm: sh Tainted: G        W          6.1.22-linux4microchip-2023.04-rc3+ #7
> Hardware name: Microchip SAMA7
> Function entered at [<c010c134>] from [<c010993c>]
> Function entered at [<c010993c>] from [<c0823754>]
> Function entered at [<c0823754>] from [<c01162ac>]
> Function entered at [<c01162ac>] from [<c0116340>]
> Function entered at [<c0116340>] from [<c07ffbe8>]
> Function entered at [<c07ffbe8>] from [<c0460300>]
> Function entered at [<c0460300>] from [<c0460634>]
> Function entered at [<c0460634>] from [<c0141ed4>]
> Function entered at [<c0141ed4>] from [<c0142448>]
> Function entered at [<c0142448>] from [<c0140da8>]
> Function entered at [<c0140da8>] from [<c023dba0>]
> Function entered at [<c023dba0>] from [<c01d0700>]
> Function entered at [<c01d0700>] from [<c01d092c>]
> Function entered at [<c01d092c>] from [<c0100060>]
> Exception stack(0xe0e81fa8 to 0xe0e81ff0)
> 1fa0:                   00000004 0057c668 00000001 0057c668 00000004 00000000
> 1fc0: 00000004 0057c668 b6ecaba0 00000004 b6f4c0e0 b6ecb15c 00000000 00000000
> 1fe0: 005456f0 beb3a788 b6dcfac4 b6e3bab8
> ---[ end trace 0000000000000000 ]---
> ------------[ cut here ]------------
> WARNING: CPU: 0 PID: 378 at lib/kobject.c:728 0xc07ffd7c
> kobject: '$���"����L��L��' (a3ba4c7d): is not initialized, yet kobject_put() is being called.
> Modules linked in:
> CPU: 0 PID: 378 Comm: sh Tainted: G        W          6.1.22-linux4microchip-2023.04-rc3+ #7
> Hardware name: Microchip SAMA7
> Function entered at [<c010c134>] from [<c010993c>]
> Function entered at [<c010993c>] from [<c0823754>]
> Function entered at [<c0823754>] from [<c01162ac>]
> Function entered at [<c01162ac>] from [<c0116340>]
> Function entered at [<c0116340>] from [<c07ffd7c>]
> Function entered at [<c07ffd7c>] from [<c0460384>]
> Function entered at [<c0460384>] from [<c0460634>]
> Function entered at [<c0460634>] from [<c0141ed4>]
> Function entered at [<c0141ed4>] from [<c0142448>]
> Function entered at [<c0142448>] from [<c0140da8>]
> Function entered at [<c0140da8>] from [<c023dba0>]
> Function entered at [<c023dba0>] from [<c01d0700>]
> Function entered at [<c01d0700>] from [<c01d092c>]
> Function entered at [<c01d092c>] from [<c0100060>]
> Exception stack(0xe0e81fa8 to 0xe0e81ff0)
> 1fa0:                   00000004 0057c668 00000001 0057c668 00000004 00000000
> 1fc0: 00000004 0057c668 b6ecaba0 00000004 b6f4c0e0 b6ecb15c 00000000 00000000
> 1fe0: 005456f0 beb3a788 b6dcfac4 b6e3bab8
> ---[ end trace 0000000000000000 ]---
> ------------[ cut here ]------------
> WARNING: CPU: 0 PID: 378 at lib/kobject.c:634 0xc07ffbe8
> kobject: '�Z����@Ą�?��8�H�Ĕ����UC�' (6407eb2a): is not initialized, yet kobject_get() is being called.
> Modules linked in:
> CPU: 0 PID: 378 Comm: sh Tainted: G        W          6.1.22-linux4microchip-2023.04-rc3+ #7
> Hardware name: Microchip SAMA7
> Function entered at [<c010c134>] from [<c010993c>]
> Function entered at [<c010993c>] from [<c0823754>]
> Function entered at [<c0823754>] from [<c01162ac>]
> Function entered at [<c01162ac>] from [<c0116340>]
> Function entered at [<c0116340>] from [<c07ffbe8>]
> Function entered at [<c07ffbe8>] from [<c0460300>]
> Function entered at [<c0460300>] from [<c0460634>]
> Function entered at [<c0460634>] from [<c0141ed4>]
> Function entered at [<c0141ed4>] from [<c0142448>]
> Function entered at [<c0142448>] from [<c0140da8>]
> Function entered at [<c0140da8>] from [<c023dba0>]
> Function entered at [<c023dba0>] from [<c01d0700>]
> Function entered at [<c01d0700>] from [<c01d092c>]
> Function entered at [<c01d092c>] from [<c0100060>]
> Exception stack(0xe0e81fa8 to 0xe0e81ff0)
> 1fa0:                   00000004 0057c668 00000001 0057c668 00000004 00000000
> 1fc0: 00000004 0057c668 b6ecaba0 00000004 b6f4c0e0 b6ecb15c 00000000 00000000
> 1fe0: 005456f0 beb3a788 b6dcfac4 b6e3bab8
> ---[ end trace 0000000000000000 ]---
> 
> Along with this the error path of at91_pm_config_quirks() had been also
> adapted to decrement propertly the reference counter of struct device
> embedded in struct platform_device.
> 
> Fixes: b7fc72c63399 ("ARM: at91: pm: add quirks for pm")
> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>

Acked-by: Nicolas Ferre <nicolas.ferre@microchip.com>
Thanks for this fix Claudiu. Best regards,
   Nicolas

> ---
>   arch/arm/mach-at91/pm.c | 20 +++++++++-----------
>   1 file changed, 9 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c
> index 60dc56d8acfb..437dd0352fd4 100644
> --- a/arch/arm/mach-at91/pm.c
> +++ b/arch/arm/mach-at91/pm.c
> @@ -334,16 +334,14 @@ static bool at91_pm_eth_quirk_is_valid(struct at91_pm_quirk_eth *eth)
>   		pdev = of_find_device_by_node(eth->np);
>   		if (!pdev)
>   			return false;
> +		/* put_device(eth->dev) is called at the end of suspend. */
>   		eth->dev = &pdev->dev;
>   	}
>   
>   	/* No quirks if device isn't a wakeup source. */
> -	if (!device_may_wakeup(eth->dev)) {
> -		put_device(eth->dev);
> +	if (!device_may_wakeup(eth->dev))
>   		return false;
> -	}
>   
> -	/* put_device(eth->dev) is called at the end of suspend. */
>   	return true;
>   }
>   
> @@ -439,14 +437,14 @@ static int at91_pm_config_quirks(bool suspend)
>   				pr_err("AT91: PM: failed to enable %s clocks\n",
>   				       j == AT91_PM_G_ETH ? "geth" : "eth");
>   			}
> -		} else {
> -			/*
> -			 * Release the reference to eth->dev taken in
> -			 * at91_pm_eth_quirk_is_valid().
> -			 */
> -			put_device(eth->dev);
> -			eth->dev = NULL;
>   		}
> +
> +		/*
> +		 * Release the reference to eth->dev taken in
> +		 * at91_pm_eth_quirk_is_valid().
> +		 */
> +		put_device(eth->dev);
> +		eth->dev = NULL;
>   	}
>   
>   	return ret;

-- 
Nicolas Ferre