drm/amd/display: fix leak of probed modes

[PATCH] drm/amd/display: fix leak of probed modes

Posted by Fedor Pchelkin 1 month, 2 weeks ago

amdgpu_dm_connector_ddc_get_modes() reinitializes a connector's probed
modes list without cleaning it up. First time it is called during the
driver's initialization phase, then via drm_mode_getconnector() ioctl.
The leaks observed with Kmemleak are as following:

unreferenced object 0xffff88812f91b200 (size 128):
  comm "(udev-worker)", pid 388, jiffies 4294695475
  hex dump (first 32 bytes):
    ac dd 07 00 80 02 70 0b 90 0b e0 0b 00 00 e0 01  ......p.........
    0b 07 10 07 5c 07 00 00 0a 00 00 00 00 00 00 00  ....\...........
  backtrace (crc 89db554f):
    __kmalloc_cache_noprof+0x3a3/0x490
    drm_mode_duplicate+0x8e/0x2b0
    amdgpu_dm_create_common_mode+0x40/0x150 [amdgpu]
    amdgpu_dm_connector_add_common_modes+0x336/0x488 [amdgpu]
    amdgpu_dm_connector_get_modes+0x428/0x8a0 [amdgpu]
    amdgpu_dm_initialize_drm_device+0x1389/0x17b4 [amdgpu]
    amdgpu_dm_init.cold+0x157b/0x1a1e [amdgpu]
    dm_hw_init+0x3f/0x110 [amdgpu]
    amdgpu_device_ip_init+0xcf4/0x1180 [amdgpu]
    amdgpu_device_init.cold+0xb84/0x1863 [amdgpu]
    amdgpu_driver_load_kms+0x15/0x90 [amdgpu]
    amdgpu_pci_probe+0x391/0xce0 [amdgpu]
    local_pci_probe+0xd9/0x190
    pci_call_probe+0x183/0x540
    pci_device_probe+0x171/0x2c0
    really_probe+0x1e1/0x890

Found by Linux Verification Center (linuxtesting.org).

Fixes: acc96ae0d127 ("drm/amd/display: set panel orientation before drm_dev_register")
Cc: stable@vger.kernel.org
Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru>
---

I can't reproduce the issue before the commit in Fixes which placed that
extra amdgpu_dm_connector_get_modes() call. Though the reinitializing part

	/* empty probed_modes */
	INIT_LIST_HEAD(&connector->probed_modes);

was added years before and it looks OK since drm_connector_list_update()
should shake the list in between drm_mode_getconnector() ioctl calls.


For what the patch does there exists a drm_mode_remove() helper but it's
static at drivers/gpu/drm/drm_connector.c and requires to be exported
first. This probably looks like a subject for an independent for-next
patch, if needed.

 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index cd0e2976e268..4b84f944f066 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -8227,9 +8227,14 @@ static void amdgpu_dm_connector_ddc_get_modes(struct drm_connector *connector,
 {
 	struct amdgpu_dm_connector *amdgpu_dm_connector =
 			to_amdgpu_dm_connector(connector);
+	struct drm_display_mode *mode, *t;
 
 	if (drm_edid) {
 		/* empty probed_modes */
+		list_for_each_entry_safe(mode, t, &connector->probed_modes, head) {
+			list_del(&mode->head);
+			drm_mode_destroy(connector->dev, mode);
+		}
 		INIT_LIST_HEAD(&connector->probed_modes);
 		amdgpu_dm_connector->num_modes =
 				drm_edid_connector_add_modes(connector);
-- 
2.50.1

Re: [PATCH] drm/amd/display: fix leak of probed modes

Posted by Limonciello, Mario 1 month, 2 weeks ago

On 8/17/25 4:43 AM, Fedor Pchelkin wrote:
> amdgpu_dm_connector_ddc_get_modes() reinitializes a connector's probed
> modes list without cleaning it up. First time it is called during the
> driver's initialization phase, then via drm_mode_getconnector() ioctl.
> The leaks observed with Kmemleak are as following:
> 
> unreferenced object 0xffff88812f91b200 (size 128):
>    comm "(udev-worker)", pid 388, jiffies 4294695475
>    hex dump (first 32 bytes):
>      ac dd 07 00 80 02 70 0b 90 0b e0 0b 00 00 e0 01  ......p.........
>      0b 07 10 07 5c 07 00 00 0a 00 00 00 00 00 00 00  ....\...........
>    backtrace (crc 89db554f):
>      __kmalloc_cache_noprof+0x3a3/0x490
>      drm_mode_duplicate+0x8e/0x2b0
>      amdgpu_dm_create_common_mode+0x40/0x150 [amdgpu]
>      amdgpu_dm_connector_add_common_modes+0x336/0x488 [amdgpu]
>      amdgpu_dm_connector_get_modes+0x428/0x8a0 [amdgpu]
>      amdgpu_dm_initialize_drm_device+0x1389/0x17b4 [amdgpu]
>      amdgpu_dm_init.cold+0x157b/0x1a1e [amdgpu]
>      dm_hw_init+0x3f/0x110 [amdgpu]
>      amdgpu_device_ip_init+0xcf4/0x1180 [amdgpu]
>      amdgpu_device_init.cold+0xb84/0x1863 [amdgpu]
>      amdgpu_driver_load_kms+0x15/0x90 [amdgpu]
>      amdgpu_pci_probe+0x391/0xce0 [amdgpu]
>      local_pci_probe+0xd9/0x190
>      pci_call_probe+0x183/0x540
>      pci_device_probe+0x171/0x2c0
>      really_probe+0x1e1/0x890
> 
> Found by Linux Verification Center (linuxtesting.org).
> 
> Fixes: acc96ae0d127 ("drm/amd/display: set panel orientation before drm_dev_register")
> Cc: stable@vger.kernel.org
> Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru>
> ---
> 
> I can't reproduce the issue before the commit in Fixes which placed that
> extra amdgpu_dm_connector_get_modes() call. Though the reinitializing part
> 
> 	/* empty probed_modes */
> 	INIT_LIST_HEAD(&connector->probed_modes);
> 
> was added years before and it looks OK since drm_connector_list_update()
> should shake the list in between drm_mode_getconnector() ioctl calls.
> 
> 
> For what the patch does there exists a drm_mode_remove() helper but it's
> static at drivers/gpu/drm/drm_connector.c and requires to be exported
> first. This probably looks like a subject for an independent for-next
> patch, if needed.

So you're saying this change will take two iterations of patches to ping 
pong the code?

Why not just send this as a two patch series?

1) Export the symbol drm_mode_remove()
2) This patch, but use the symbol.

> 
>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index cd0e2976e268..4b84f944f066 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -8227,9 +8227,14 @@ static void amdgpu_dm_connector_ddc_get_modes(struct drm_connector *connector,
>   {
>   	struct amdgpu_dm_connector *amdgpu_dm_connector =
>   			to_amdgpu_dm_connector(connector);
> +	struct drm_display_mode *mode, *t;
>   
>   	if (drm_edid) {
>   		/* empty probed_modes */
> +		list_for_each_entry_safe(mode, t, &connector->probed_modes, head) {
> +			list_del(&mode->head);
> +			drm_mode_destroy(connector->dev, mode);
> +		}
>   		INIT_LIST_HEAD(&connector->probed_modes);
>   		amdgpu_dm_connector->num_modes =
>   				drm_edid_connector_add_modes(connector);

Re: [PATCH] drm/amd/display: fix leak of probed modes

Posted by Fedor Pchelkin 1 month, 2 weeks ago

On Mon, 18. Aug 21:17, Limonciello, Mario wrote:
> On 8/17/25 4:43 AM, Fedor Pchelkin wrote:
> > For what the patch does there exists a drm_mode_remove() helper but it's
> > static at drivers/gpu/drm/drm_connector.c and requires to be exported
> > first. This probably looks like a subject for an independent for-next
> > patch, if needed.
> 
> So you're saying this change will take two iterations of patches to ping 
> pong the code?
> 
> Why not just send this as a two patch series?
> 
> 1) Export the symbol drm_mode_remove()
> 2) This patch, but use the symbol.
> 

Initially I wasn't sure if the exporting patch was worth moving the code
around and in the end decided to make the current patch with a minimum of
prerequisites.

But giving this a second glance, I see exporting the symbol would be
technically better. I'll send out v2.

Thanks!