Received-SPF: pass (google.com: domain of linux-kernel+bounces-193934-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <8d95757c-fd05-4a48-ae9d-24d78d04d663@samsung.com>
Date: Wed, 29 May 2024 12:12:44 +0200
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH][next] of: property: Remove calls to of_node_put
To: Shresth Prasad <shresthprasad7@gmail.com>, robh@kernel.org,
	saravanak@google.com, DRI mailing list <dri-devel@lists.freedesktop.org>
Cc: devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
	skhan@linuxfoundation.org, javier.carrasco.cruz@gmail.com,
	julia.lawall@inria.fr
Content-Language: en-US
From: Marek Szyprowski <m.szyprowski@samsung.com>
In-Reply-To: <20240515202915.16214-3-shresthprasad7@gmail.com>
Content-Transfer-Encoding: 8bit
Content-Type: text/plain; charset="utf-8"
CMS-TYPE: 201P
References: <20240515202915.16214-3-shresthprasad7@gmail.com>
	<CGME20240529101246eucas1p1266853c07f5178c7e3e4b8a264eb436e@eucas1p1.samsung.com>

On 15.05.2024 22:29, Shresth Prasad wrote:
> Add __free cleanup handler to some variable initialisations, which
> ensures that the resource is freed as soon as the variable goes out of
> scope. Thus removing the need to manually free up the resource using
> of_node_put.
>
> Suggested-by: Julia Lawall <julia.lawall@inria.fr>
> Signed-off-by: Shresth Prasad <shresthprasad7@gmail.com>
> ---

This patch landed in today's linux-next as commit b94d24c08ee1 ("of: 
property: Remove calls to of_node_put"). I found that it triggers the 
following warning while booting of the Samsung Exynos5800 based Pi 
Chromebook (arch/arm/boot/dts/samsung/exynos5800-peach-pi.dts):

OF: ERROR: of_node_release() detected bad of_node_put() on /panel
CPU: 2 PID: 68 Comm: kworker/u36:1 Not tainted 
6.10.0-rc1-00001-gb94d24c08ee1 #8619
Hardware name: Samsung Exynos (Flattened Device Tree)
Workqueue: events_unbound deferred_probe_work_func
tps65090 20-0048: No cache defaults, reading back from HW
Call trace:
  unwind_backtrace from show_stack+0x10/0x14
  show_stack from dump_stack_lvl+0x50/0x64
  dump_stack_lvl from of_node_release+0x110/0x138
  of_node_release from kobject_put+0x98/0x108
  kobject_put from drm_of_find_panel_or_bridge+0x94/0xd8
  drm_of_find_panel_or_bridge from exynos_dp_probe+0xf0/0x158 [exynosdrm]
  exynos_dp_probe [exynosdrm] from platform_probe+0x80/0xc0
  platform_probe from really_probe+0xc8/0x288
  really_probe from __driver_probe_device+0x8c/0x190
  __driver_probe_device from driver_probe_device+0x30/0xd0
  driver_probe_device from __device_attach_driver+0x8c/0xbc
  __device_attach_driver from bus_for_each_drv+0x74/0xc0
  bus_for_each_drv from __device_attach+0xe8/0x184
  __device_attach from bus_probe_device+0x88/0x8c
  bus_probe_device from deferred_probe_work_func+0x7c/0xa8
  deferred_probe_work_func from process_scheduled_works+0xe8/0x41c
  process_scheduled_works from worker_thread+0x14c/0x35c
  worker_thread from kthread+0xd0/0x104
  kthread from ret_from_fork+0x14/0x28
Exception stack(0xf0a81fb0 to 0xf0a81ff8)

OF: ERROR: next of_node_put() on this node will result in a kobject 
warning 'refcount_t: underflow; use-after-free.'
------------[ cut here ]------------
WARNING: CPU: 3 PID: 68 at lib/refcount.c:25 kobject_get+0xa0/0xa4
refcount_t: addition on 0; use-after-free.
Modules linked in: i2c_cros_ec_tunnel(+) cros_ec_keyb cros_ec_dev 
cros_ec_spi cros_ec snd_soc_i2s snd_soc_idma snd_soc_max98090 
snd_soc_snow snd_soc_s3c_dma snd_soc_core tpm_i2c_infineon ac97_bus 
snd_pcm_dmaengine tpm exynosdrm libsha256 libaescfb snd_pcm analogix_dp 
ecdh_generic samsung_dsim ecc snd_timer atmel_mxt_ts snd libaes 
soundcore exynos_gsc s5p_jpeg s5p_mfc v4l2_mem2mem spi_s3c64xx 
videobuf2_dma_contig exynos_adc pwm_samsung videobuf2_memops 
videobuf2_v4l2 videodev phy_exynos_usb2 videobuf2_common ohci_exynos 
ehci_exynos mc exynos_ppmu rtc_s3c exynos_rng s3c2410_wdt s5p_sss 
phy_exynos_mipi_video phy_exynos_dp_video
CPU: 3 PID: 68 Comm: kworker/u36:1 Not tainted 
6.10.0-rc1-00001-gb94d24c08ee1 #8619
Hardware name: Samsung Exynos (Flattened Device Tree)
Workqueue: events_unbound deferred_probe_work_func
Call trace:
  unwind_backtrace from show_stack+0x10/0x14
  show_stack from dump_stack_lvl+0x50/0x64
  dump_stack_lvl from __warn+0x108/0x12c
  __warn from warn_slowpath_fmt+0x118/0x17c
  warn_slowpath_fmt from kobject_get+0xa0/0xa4
  kobject_get from of_node_get+0x14/0x1c
  of_node_get from of_get_next_parent+0x24/0x50
  of_get_next_parent from of_graph_get_port_parent.part.1+0x58/0xa4
  of_graph_get_port_parent.part.1 from 
of_graph_get_remote_port_parent+0x1c/0x38
  of_graph_get_remote_port_parent from of_graph_get_remote_node+0x10/0x6c
  of_graph_get_remote_node from drm_of_find_panel_or_bridge+0x50/0xd8
  drm_of_find_panel_or_bridge from exynos_dp_probe+0xf0/0x158 [exynosdrm]
  exynos_dp_probe [exynosdrm] from platform_probe+0x80/0xc0
  platform_probe from really_probe+0xc8/0x288
  really_probe from __driver_probe_device+0x8c/0x190
  __driver_probe_device from driver_probe_device+0x30/0xd0
  driver_probe_device from __device_attach_driver+0x8c/0xbc
  __device_attach_driver from bus_for_each_drv+0x74/0xc0
  bus_for_each_drv from __device_attach+0xe8/0x184
  __device_attach from bus_probe_device+0x88/0x8c
  bus_probe_device from deferred_probe_work_func+0x7c/0xa8
  deferred_probe_work_func from process_scheduled_works+0xe8/0x41c
  process_scheduled_works from worker_thread+0x14c/0x35c
  worker_thread from kthread+0xd0/0x104
  kthread from ret_from_fork+0x14/0x28
Exception stack(0xf0a81fb0 to 0xf0a81ff8)

---[ end trace 0000000000000000 ]---
------------[ cut here ]------------

If I got this right, this points to the drm_of_find_panel_or_bridge() 
function. I briefly scanned it, but I don't see any obvious 
of_node_put() related issue there.


> I had submitted a similar patch a couple weeks ago addressing the same
> issue, but as it turns out I wasn't thorough enough and had left a couple
> instances.
>
> I hope this isn't too big an issue.
> ---
>   drivers/of/property.c | 27 +++++++++++----------------
>   1 file changed, 11 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/of/property.c b/drivers/of/property.c
> index 17b294e16c56..96a74f6a8d64 100644
> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c
> @@ -773,15 +773,14 @@ EXPORT_SYMBOL(of_graph_get_port_parent);
>   struct device_node *of_graph_get_remote_port_parent(
>   			       const struct device_node *node)
>   {
> -	struct device_node *np, *pp;
> +	struct device_node *pp;
>   
>   	/* Get remote endpoint node. */
> -	np = of_graph_get_remote_endpoint(node);
> +	struct device_node *np __free(device_node) =
> +			    of_graph_get_remote_endpoint(node);
>   
>   	pp = of_graph_get_port_parent(np);
>   
> -	of_node_put(np);
> -
>   	return pp;
>   }
>   EXPORT_SYMBOL(of_graph_get_remote_port_parent);
> @@ -835,17 +834,18 @@ EXPORT_SYMBOL(of_graph_get_endpoint_count);
>   struct device_node *of_graph_get_remote_node(const struct device_node *node,
>   					     u32 port, u32 endpoint)
>   {
> -	struct device_node *endpoint_node, *remote;
> +	struct device_node *endpoint_node __free(device_node) =
> +			    of_graph_get_endpoint_by_regs(node, port, endpoint);
> +
> +	struct device_node *remote __free(device_node) =
> +			    of_graph_get_remote_port_parent(endpoint_node);
>   
> -	endpoint_node = of_graph_get_endpoint_by_regs(node, port, endpoint);
>   	if (!endpoint_node) {
>   		pr_debug("no valid endpoint (%d, %d) for node %pOF\n",
>   			 port, endpoint, node);
>   		return NULL;
>   	}
>   
> -	remote = of_graph_get_remote_port_parent(endpoint_node);
> -	of_node_put(endpoint_node);
>   	if (!remote) {
>   		pr_debug("no valid remote node\n");
>   		return NULL;
> @@ -853,7 +853,6 @@ struct device_node *of_graph_get_remote_node(const struct device_node *node,
>   
>   	if (!of_device_is_available(remote)) {
>   		pr_debug("not available for remote node\n");
> -		of_node_put(remote);
>   		return NULL;
>   	}
>   
> @@ -1064,19 +1063,15 @@ static void of_link_to_phandle(struct device_node *con_np,
>   			      struct device_node *sup_np,
>   			      u8 flags)
>   {
> -	struct device_node *tmp_np = of_node_get(sup_np);
> +	struct device_node *tmp_np __free(device_node) = of_node_get(sup_np);
>   
>   	/* Check that sup_np and its ancestors are available. */
>   	while (tmp_np) {
> -		if (of_fwnode_handle(tmp_np)->dev) {
> -			of_node_put(tmp_np);
> +		if (of_fwnode_handle(tmp_np)->dev)
>   			break;
> -		}
>   
> -		if (!of_device_is_available(tmp_np)) {
> -			of_node_put(tmp_np);
> +		if (!of_device_is_available(tmp_np))
>   			return;
> -		}
>   
>   		tmp_np = of_get_next_parent(tmp_np);
>   	}

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland