Re: [PATCH v2 53/59] drm/amdgpu: dm: Convert to atomic_create_state

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm/amd/display: partial init and NULL deref when atomic_create_state() fails
- [Medium] drm/amd/display: ERR_PTR deref on cleanup in connector KUnit tests
--

commit a996d78c707775b6da542d46e144536aa2b44c5d
Author: Maxime Ripard <mripard@kernel.org>

drm/amdgpu: dm: Convert to atomic_create_state

This commit converts the amdgpu_dm connector state initialization from the
drm_connector_funcs.reset hook to drm_connector_funcs.atomic_create_state
across connectors, MST connectors, writeback connectors, and KUnit tests.

> 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..e59595b5e39b2 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;
> +
> +		aconnector->base.state = conn_state;
> +	}
>  
>  	aconnector->connector_id = link_index;
>  	aconnector->bl_idx = -1;

[Severity: High]
What happens in the callers of amdgpu_dm_connector_init_helper() in
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_connector.c when
atomic_create_state() fails with ERR_PTR(-ENOMEM) and returns early?

Returning early from this void function skips initializing
aconnector->dc_link, aconnector->bl_idx = -1, aconnector->audio_inst = -1,
aconnector->hpd_lock, aconnector->handle_mst_msg_ready, and connector
properties, while leaving aconnector->base.state as NULL without reporting
an error to the caller.

In drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_connector.c,
amdgpu_dm_connector_init() continues execution assuming the connector was
initialized:

    amdgpu_dm_connector_init_helper(
        dm,
        aconnector,
        connector_type,
        link,
        link_index);
    ...
    if (dc_is_dp_signal(link->connector_signal))
        amdgpu_dm_initialize_dp_connector(dm, aconnector, link->link_index);

For DP or eDP connectors, amdgpu_dm_connector_init() ->
amdgpu_dm_initialize_dp_connector() in
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c immediately
dereferences aconnector->dc_link while it is still NULL:

    aconnector->dm_dp_aux.ddc_service = aconnector->dc_link->ddc;

For other connectors, amdgpu_dm_mode_config_init() proceeds to call
amdgpu_dm_update_connector_after_detect(), which also dereferences
aconnector->dc_link->local_sink. In addition, if
amdgpu_dm_connector_destroy() runs on this partially initialized connector,
aconnector->bl_idx is still 0 rather than -1, so it will unregister
dm->backlight_dev[0].

Similarly, in drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c,
dm_dp_add_mst_connector() calls amdgpu_dm_connector_init_helper() and then
calls drm_connector_attach_max_bpc_property(connector, 8, 16), which
dereferences connector->state while it is NULL:

    amdgpu_dm_connector_init_helper(
        &adev->dm,
        aconnector,
        DRM_MODE_CONNECTOR_DisplayPort,
        master->dc_link,
        master->connector_id);
    ...
    connector->max_bpc_property = master->base.max_bpc_property;
    if (connector->max_bpc_property)
        drm_connector_attach_max_bpc_property(connector, 8, 16);

Along the same lines, in
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_wb.c,
amdgpu_dm_wb_connector_init() ignores an ERR_PTR return from
amdgpu_dm_connector_atomic_create_state() and returns 0 with
connector->state left as NULL:

    conn_state = amdgpu_dm_connector_atomic_create_state(connector);
    if (!IS_ERR(conn_state)) {
        if (connector->state)
            connector->funcs->atomic_destroy_state(connector, connector->state);

        connector->state = conn_state;
    }

Should amdgpu_dm_connector_init_helper() and amdgpu_dm_wb_connector_init()
propagate the allocation error to their callers so connector initialization
can fail cleanly?

[ ... ]

> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_connector_test.c b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_connector_test.c
> index 8117aaf41f220..ebf8155326cb1 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_connector_test.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_connector_test.c
> @@ -1468,19 +1468,19 @@ static void dm_test_decide_crtc_timing_no_crtc_clock(struct kunit *test)
>  	KUNIT_EXPECT_EQ(test, drm_mode.crtc_hdisplay, 0);
>  }
>  
> -/* Tests for amdgpu_dm_connector_funcs_reset() */
> +/* Tests for amdgpu_dm_connector_atomic_create_state() */
>  
>  static const struct drm_connector_funcs dm_test_connector_funcs = {
> -	.reset = amdgpu_dm_connector_funcs_reset,
> +	.atomic_create_state = amdgpu_dm_connector_atomic_create_state,
>  	.atomic_duplicate_state = amdgpu_dm_connector_atomic_duplicate_state,
>  	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
>  };
>  
>  /**
> - * dm_test_funcs_reset_sets_defaults - Test funcs_reset sets defaults
> + * dm_test_atomic_create_sets_defaults - Test funcs_reset sets defaults
>   * @test: The KUnit test context
>   */
> -static void dm_test_funcs_reset_sets_defaults(struct kunit *test)
> +static void dm_test_atomic_create_sets_defaults(struct kunit *test)
>  {
>  	struct device *dev;
>  	struct drm_device *drm;
> @@ -1501,9 +1501,9 @@ static void dm_test_funcs_reset_sets_defaults(struct kunit *test)
>  	drmm_connector_init(drm, connector, &dm_test_connector_funcs,
>  			    DRM_MODE_CONNECTOR_DisplayPort, NULL);
>  
> -	amdgpu_dm_connector_funcs_reset(connector);
> +	connector->state = amdgpu_dm_connector_atomic_create_state(connector);
> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, connector->state);
>  
> -	KUNIT_ASSERT_NOT_NULL(test, connector->state);
>  	dm_state = to_dm_connector_state(connector->state);
>  	KUNIT_EXPECT_EQ(test, (int)dm_state->scaling, (int)RMX_OFF);
>  	KUNIT_EXPECT_FALSE(test, dm_state->underscan_enable);
> @@ -1515,10 +1515,10 @@ static void dm_test_funcs_reset_sets_defaults(struct kunit *test)
>  }
>  
>  /**
> - * dm_test_funcs_reset_edp_abm_level - Test funcs_reset eDP sets ABM
> + * dm_test_atomic_create_edp_abm_level - Test funcs_reset eDP sets ABM
>   * @test: The KUnit test context
>   */
> -static void dm_test_funcs_reset_edp_abm_level(struct kunit *test)
> +static void dm_test_atomic_create_edp_abm_level(struct kunit *test)
>  {
>  	struct device *dev;
>  	struct drm_device *drm;
> @@ -1542,9 +1542,10 @@ static void dm_test_funcs_reset_edp_abm_level(struct kunit *test)
>  
>  	/* Test with abm_level > 0 */
>  	amdgpu_dm_set_abm_level_param(3);
> -	amdgpu_dm_connector_funcs_reset(connector);
>  
> -	KUNIT_ASSERT_NOT_NULL(test, connector->state);
> +	connector->state = amdgpu_dm_connector_atomic_create_state(connector);
> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, connector->state);
> +
>  	dm_state = to_dm_connector_state(connector->state);
>  	KUNIT_EXPECT_EQ(test, (int)dm_state->abm_level, 3);
>  
> @@ -1552,11 +1553,11 @@ static void dm_test_funcs_reset_edp_abm_level(struct kunit *test)
>  }
>  
>  /**
> - * dm_test_funcs_reset_edp_abm_disabled - Test funcs_reset eDP ABM
> + * dm_test_atomic_create_edp_abm_disabled - Test funcs_reset eDP ABM
>   * disabled
>   * @test: The KUnit test context
>   */
> -static void dm_test_funcs_reset_edp_abm_disabled(struct kunit *test)
> +static void dm_test_atomic_create_edp_abm_disabled(struct kunit *test)
>  {
>  	struct device *dev;
>  	struct drm_device *drm;
> @@ -1580,9 +1581,10 @@ static void dm_test_funcs_reset_edp_abm_disabled(struct kunit *test)
>  
>  	/* Test with abm_level <= 0 → immediate disable */
>  	amdgpu_dm_set_abm_level_param(-1);
> -	amdgpu_dm_connector_funcs_reset(connector);
>  
> -	KUNIT_ASSERT_NOT_NULL(test, connector->state);
> +	connector->state = amdgpu_dm_connector_atomic_create_state(connector);
> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, connector->state);
> +
>  	dm_state = to_dm_connector_state(connector->state);
>  	KUNIT_EXPECT_EQ(test, (int)dm_state->abm_level,
>  			(int)ABM_LEVEL_IMMEDIATE_DISABLE);
> @@ -1620,8 +1622,8 @@ static void dm_test_atomic_dup_state_copies_fields(struct kunit *test)
>  	drmm_connector_init(drm, connector, &dm_test_connector_funcs,
>  			    DRM_MODE_CONNECTOR_HDMIA, NULL);
>  
> -	amdgpu_dm_connector_funcs_reset(connector);
> -	KUNIT_ASSERT_NOT_NULL(test, connector->state);
> +	connector->state = amdgpu_dm_connector_atomic_create_state(connector);
> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, connector->state);
>  
>  	/* Modify original state fields */
>  	dm_state = to_dm_connector_state(connector->state);

[Severity: Medium]
In dm_test_atomic_create_sets_defaults(),
dm_test_atomic_create_edp_abm_level(),
dm_test_atomic_create_edp_abm_disabled(), and
dm_test_atomic_dup_state_copies_fields() in
drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_connector_test.c, can
assigning directly to connector->state before
KUNIT_ASSERT_NOT_ERR_OR_NULL() cause an error pointer dereference during
test teardown?

Each of these tests initializes the connector with drmm_connector_init(),
which registers drm_connector_cleanup() as a managed cleanup action. If
amdgpu_dm_connector_atomic_create_state() returns ERR_PTR(-ENOMEM),
connector->state holds that non-NULL error pointer when
KUNIT_ASSERT_NOT_ERR_OR_NULL() aborts the test, and drm_connector_cleanup()
in drivers/gpu/drm/drm_connector.c will then pass it to
atomic_destroy_state():

    if (connector->state && connector->funcs->atomic_destroy_state)
        connector->funcs->atomic_destroy_state(connector,
                               connector->state);

Would it be safer to store the return value in a local conn_state variable
and assert KUNIT_ASSERT_NOT_ERR_OR_NULL(test, conn_state) before assigning
connector->state, as done in the other tests in this file?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022-drm-no-more-connector-reset-v2-0-0b7e4e85b1e8@kernel.org?part=53