Re: [PATCH v3] drm/amd/display: Fix dangling pointers in state reset functions on allocation failure

[Fedor Pchelkin]

On Fri, 26. Jun 22:13, Evgenii Burenchev wrote:
> Fixes: 5d945cbcd4b1 ("drm/amd/display: Create a file dedicated to planes")
> Fixes: 473683a03495 ("drm/amd/display: Create a file dedicated for CRTC")
> Fixes: e7b07ceef2a6 ("drm/amd/display: Merge amdgpu_dm_types and amdgpu_dm")
> Signed-off-by: Evgenii Burenchev <evg28bur@xxxxxxxxx>

Having three different Fixes tags implies the big patch could be split up
into three separate patches which do one thing at a time.  They can be
combined in a series for ease of handling.

> @@ -8151,33 +8151,41 @@ static void amdgpu_dm_connector_destroy(struct drm_connector *connector)
>  
>  void amdgpu_dm_connector_funcs_reset(struct drm_connector *connector)
>  {
> -	struct dm_connector_state *state =
> +	/* Remember the old state */
> +	struct dm_connector_state *old_state =
>  		to_dm_connector_state(connector->state);
>  
> +	struct dm_connector_state *state;

No empty lines inside local variable declaration block, please.

> +
> +	/* Allocate new state */

Well, all the comments added with the patch - IMO they duplicate what the
code is doing - that doesn't add any real value and just bloats the
codebase.

> +	state = kzalloc_obj(*state);
> +	if (WARN_ON(!state))
> +		return;

It's not common to WARN on memory allocation errors.  If this code is ever
fuzzed with fault-injections enabled, that'd be one of the first issues to
pop up.

I think if the system is in a state when it can't allocate a bunch of
GFP_KERNEL memory, there'd definitely be some noticeable activity in
dmesg.  Some (random) assertion triggered inside amdgpu won't help much -
that will only halt those machines booted with panic_on_warn=1.