Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] drm/amdgpu: memory leak and ERR_PTR dereference in dm_dp_add_mst_connector() - [High] drm/amdgpu: early return on OOM skips mandatory connector initialization --
commit 7f186f384d1ae8ab956532945397b878e65c8386 Author: Maxime Ripard <[email protected]> drm/amdgpu: dm: Convert to atomic_create_state The amdgpu_dm connector reset implementation creates a custom state subclass, but only initializes a pristine state without resetting any hardware. This is equivalent to what atomic_create_state expects. Convert to it. > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_connector.c > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_connector.c > index 3a4e98b08ac1f..2c22f00f521c6 100644 > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_connector.c > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_connector.c [ ... ] > @@ -3199,8 +3195,16 @@ void amdgpu_dm_connector_init_helper(struct > amdgpu_display_manager *dm, > * Some of the properties below require access to state, like bpc. > * Allocate some default initial connector state with our reset helper. > */ > - if (aconnector->base.funcs->reset) > - aconnector->base.funcs->reset(&aconnector->base); > + if (!aconnector->base.state && > + aconnector->base.funcs->atomic_create_state) { > + struct drm_connector_state *conn_state; > + > + conn_state = > aconnector->base.funcs->atomic_create_state(&aconnector->base); > + if (IS_ERR(conn_state)) > + return; [Severity: High] If memory allocation fails during atomic_create_state() and we return early here, does it skip mandatory connector initialization? Since amdgpu_dm_connector_init_helper() returns void, the caller amdgpu_dm_connector_init() is unaware of the failure and continues execution. It subsequently calls amdgpu_dm_initialize_dp_connector(), which unconditionally dereferences aconnector->dc_link->ddc. Since aconnector->dc_link assignment is skipped in the aborted helper, could this result in a NULL pointer dereference? [ ... ] > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c > index a5b15bef8aa0a..cb189c098ec3e 100644 > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c [ ... ] > @@ -738,7 +738,7 @@ dm_dp_add_mst_connector(struct drm_dp_mst_topology_mgr > *mgr, > * Initialize connector state before adding the connectror to drm and > * framebuffer lists > */ > - amdgpu_dm_connector_funcs_reset(connector); > + connector->state = amdgpu_dm_connector_atomic_create_state(connector); [Severity: High] Can this overwrite the connector->state pointer and leak the previously allocated state? Earlier in dm_dp_add_mst_connector(), amdgpu_dm_connector_init_helper() is called, which allocates and initializes a new connector state and assigns it to connector->state. Overwriting it here appears to drop custom properties attached to the state in the interim (such as max_requested_bpc set by drm_connector_attach_max_bpc_property). Additionally, amdgpu_dm_connector_atomic_create_state() can return ERR_PTR(-ENOMEM) on allocation failure. Because this is directly assigned to connector->state without an IS_ERR() check, will the DRM core subsequently dereference this error pointer? > > drm_dp_mst_get_port_malloc(port); -- Sashiko AI review · https://sashiko.dev/#/patchset/20261014-drm-no-more-connector-reset-v1-0-586c9c026...@kernel.org?part=66
