The drm_atomic_state structure is freed through the
drm_atomic_state_put() function, that eventually calls
drm_atomic_state_default_clear() by default when there's no active
users of that state.

It then iterates over all entities with a state, and will call the
atomic_destroy_state callback on the state pointer. The state pointer is
mostly used these days to point to which of the old or new state needs
to be freed, depending on whether the state was committed or not.

So it all makes sense.

However, with the hardware state readout support approaching, we might
have a state, with multiple entities in it, but no state to free because
we want them to persist. In such a case, state is going to be NULL, and
thus we'll end up with NULL pointer dereference.

In order to make it work, let's first test if the state pointer isn't
NULL before calling atomic_destroy_state on it.

Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 drivers/gpu/drm/drm_atomic.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 38f2b2633fa992b3543e8c425c7faeab1ce69765..f26678835a94f40da56a8c1297d92f226d7ff2e2 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -249,12 +249,14 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state)
 		struct drm_connector *connector = state->connectors[i].ptr;
 
 		if (!connector)
 			continue;
 
-		connector->funcs->atomic_destroy_state(connector,
-						       state->connectors[i].state);
+		if (state->connectors[i].state)
+			connector->funcs->atomic_destroy_state(connector,
+							       state->connectors[i].state);
+
 		state->connectors[i].ptr = NULL;
 		state->connectors[i].state = NULL;
 		state->connectors[i].old_state = NULL;
 		state->connectors[i].new_state = NULL;
 		drm_connector_put(connector);
@@ -264,12 +266,13 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state)
 		struct drm_crtc *crtc = state->crtcs[i].ptr;
 
 		if (!crtc)
 			continue;
 
-		crtc->funcs->atomic_destroy_state(crtc,
-						  state->crtcs[i].state);
+		if (state->crtcs[i].state)
+			crtc->funcs->atomic_destroy_state(crtc,
+							  state->crtcs[i].state);
 
 		state->crtcs[i].ptr = NULL;
 		state->crtcs[i].state = NULL;
 		state->crtcs[i].old_state = NULL;
 		state->crtcs[i].new_state = NULL;
@@ -284,12 +287,14 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state)
 		struct drm_plane *plane = state->planes[i].ptr;
 
 		if (!plane)
 			continue;
 
-		plane->funcs->atomic_destroy_state(plane,
-						   state->planes[i].state);
+		if (state->planes[i].state)
+			plane->funcs->atomic_destroy_state(plane,
+							       state->planes[i].state);
+
 		state->planes[i].ptr = NULL;
 		state->planes[i].state = NULL;
 		state->planes[i].old_state = NULL;
 		state->planes[i].new_state = NULL;
 	}
@@ -298,12 +303,14 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state)
 		struct drm_private_obj *obj = state->private_objs[i].ptr;
 
 		if (!obj)
 			continue;
 
-		obj->funcs->atomic_destroy_state(obj,
-						 state->private_objs[i].state);
+		if (state->private_objs[i].state)
+			obj->funcs->atomic_destroy_state(obj,
+							       state->private_objs[i].state);
+
 		state->private_objs[i].ptr = NULL;
 		state->private_objs[i].state = NULL;
 		state->private_objs[i].old_state = NULL;
 		state->private_objs[i].new_state = NULL;
 	}

-- 
2.50.1

On Tue, Sep 02, 2025 at 10:32:36AM +0200, Maxime Ripard wrote:
> The drm_atomic_state structure is freed through the
> drm_atomic_state_put() function, that eventually calls
> drm_atomic_state_default_clear() by default when there's no active
> users of that state.
> 
> It then iterates over all entities with a state, and will call the

Did you mean s/with a state/within the state/ ?

I'd also replace "entity" with "object" as mentioned in the review of a
previous patch.

> atomic_destroy_state callback on the state pointer. The state pointer is
> mostly used these days to point to which of the old or new state needs
> to be freed, depending on whether the state was committed or not.
> 
> So it all makes sense.
> 
> However, with the hardware state readout support approaching, we might
> have a state, with multiple entities in it, but no state to free because
> we want them to persist. In such a case, state is going to be NULL, and
> thus we'll end up with NULL pointer dereference.

I'm not sure to follow you here. Are we talking with objects whose old
and new states will both need to be preserved ? Or objects that have no
old state ? I assume it's the latter, a clarification would be useful
here. With that,

Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

> In order to make it work, let's first test if the state pointer isn't
> NULL before calling atomic_destroy_state on it.
> 
> Signed-off-by: Maxime Ripard <mripard@kernel.org>
> ---
>  drivers/gpu/drm/drm_atomic.c | 23 +++++++++++++++--------
>  1 file changed, 15 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 38f2b2633fa992b3543e8c425c7faeab1ce69765..f26678835a94f40da56a8c1297d92f226d7ff2e2 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -249,12 +249,14 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state)
>  		struct drm_connector *connector = state->connectors[i].ptr;
>  
>  		if (!connector)
>  			continue;
>  
> -		connector->funcs->atomic_destroy_state(connector,
> -						       state->connectors[i].state);
> +		if (state->connectors[i].state)
> +			connector->funcs->atomic_destroy_state(connector,
> +							       state->connectors[i].state);
> +
>  		state->connectors[i].ptr = NULL;
>  		state->connectors[i].state = NULL;
>  		state->connectors[i].old_state = NULL;
>  		state->connectors[i].new_state = NULL;
>  		drm_connector_put(connector);
> @@ -264,12 +266,13 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state)
>  		struct drm_crtc *crtc = state->crtcs[i].ptr;
>  
>  		if (!crtc)
>  			continue;
>  
> -		crtc->funcs->atomic_destroy_state(crtc,
> -						  state->crtcs[i].state);
> +		if (state->crtcs[i].state)
> +			crtc->funcs->atomic_destroy_state(crtc,
> +							  state->crtcs[i].state);
>  
>  		state->crtcs[i].ptr = NULL;
>  		state->crtcs[i].state = NULL;
>  		state->crtcs[i].old_state = NULL;
>  		state->crtcs[i].new_state = NULL;
> @@ -284,12 +287,14 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state)
>  		struct drm_plane *plane = state->planes[i].ptr;
>  
>  		if (!plane)
>  			continue;
>  
> -		plane->funcs->atomic_destroy_state(plane,
> -						   state->planes[i].state);
> +		if (state->planes[i].state)
> +			plane->funcs->atomic_destroy_state(plane,
> +							       state->planes[i].state);
> +
>  		state->planes[i].ptr = NULL;
>  		state->planes[i].state = NULL;
>  		state->planes[i].old_state = NULL;
>  		state->planes[i].new_state = NULL;
>  	}
> @@ -298,12 +303,14 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state)
>  		struct drm_private_obj *obj = state->private_objs[i].ptr;
>  
>  		if (!obj)
>  			continue;
>  
> -		obj->funcs->atomic_destroy_state(obj,
> -						 state->private_objs[i].state);
> +		if (state->private_objs[i].state)
> +			obj->funcs->atomic_destroy_state(obj,
> +							       state->private_objs[i].state);
> +
>  		state->private_objs[i].ptr = NULL;
>  		state->private_objs[i].state = NULL;
>  		state->private_objs[i].old_state = NULL;
>  		state->private_objs[i].new_state = NULL;
>  	}

-- 
Regards,

Laurent Pinchart

On Tue, Sep 02, 2025 at 10:52:47PM +0200, Laurent Pinchart wrote:
> On Tue, Sep 02, 2025 at 10:32:36AM +0200, Maxime Ripard wrote:
> > The drm_atomic_state structure is freed through the
> > drm_atomic_state_put() function, that eventually calls
> > drm_atomic_state_default_clear() by default when there's no active
> > users of that state.
> > 
> > It then iterates over all entities with a state, and will call the
> 
> Did you mean s/with a state/within the state/ ?

I'm not sure how to phrase it, but I meant "entities" that have a
matching state structure. Encoders for example don't.

> I'd also replace "entity" with "object" as mentioned in the review of a
> previous patch.

And bridges aren't objects :/

> > atomic_destroy_state callback on the state pointer. The state pointer is
> > mostly used these days to point to which of the old or new state needs
> > to be freed, depending on whether the state was committed or not.
> > 
> > So it all makes sense.
> > 
> > However, with the hardware state readout support approaching, we might
> > have a state, with multiple entities in it, but no state to free because
> > we want them to persist. In such a case, state is going to be NULL, and
> > thus we'll end up with NULL pointer dereference.
> 
> I'm not sure to follow you here. Are we talking with objects whose old
> and new states will both need to be preserved ? Or objects that have no
> old state ?

It's more of the latter, but not really. Objects, at this point of the
series, will always have an old state.

However, due to how the initial state is being built with hardware
readout, we would move the old state of an entity/object/whatever to
$object->state, and clear it from drm_atomic_state so it's not freed.

so drm_atomic_state ends up with a whole bunch of objects that don't
have an old state anymore, and drm_atomic_state_default_clear() chokes
on that.

Maxime

Am 02.09.25 um 10:32 schrieb Maxime Ripard:
> The drm_atomic_state structure is freed through the
> drm_atomic_state_put() function, that eventually calls
> drm_atomic_state_default_clear() by default when there's no active
> users of that state.
>
> It then iterates over all entities with a state, and will call the
> atomic_destroy_state callback on the state pointer. The state pointer is
> mostly used these days to point to which of the old or new state needs
> to be freed, depending on whether the state was committed or not.
>
> So it all makes sense.
>
> However, with the hardware state readout support approaching, we might
> have a state, with multiple entities in it, but no state to free because
> we want them to persist. In such a case, state is going to be NULL, and
> thus we'll end up with NULL pointer dereference.
>
> In order to make it work, let's first test if the state pointer isn't
> NULL before calling atomic_destroy_state on it.
>
> Signed-off-by: Maxime Ripard <mripard@kernel.org>

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

> ---
>   drivers/gpu/drm/drm_atomic.c | 23 +++++++++++++++--------
>   1 file changed, 15 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 38f2b2633fa992b3543e8c425c7faeab1ce69765..f26678835a94f40da56a8c1297d92f226d7ff2e2 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -249,12 +249,14 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state)
>   		struct drm_connector *connector = state->connectors[i].ptr;
>   
>   		if (!connector)
>   			continue;
>   
> -		connector->funcs->atomic_destroy_state(connector,
> -						       state->connectors[i].state);
> +		if (state->connectors[i].state)
> +			connector->funcs->atomic_destroy_state(connector,
> +							       state->connectors[i].state);
> +
>   		state->connectors[i].ptr = NULL;
>   		state->connectors[i].state = NULL;
>   		state->connectors[i].old_state = NULL;
>   		state->connectors[i].new_state = NULL;
>   		drm_connector_put(connector);
> @@ -264,12 +266,13 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state)
>   		struct drm_crtc *crtc = state->crtcs[i].ptr;
>   
>   		if (!crtc)
>   			continue;
>   
> -		crtc->funcs->atomic_destroy_state(crtc,
> -						  state->crtcs[i].state);
> +		if (state->crtcs[i].state)
> +			crtc->funcs->atomic_destroy_state(crtc,
> +							  state->crtcs[i].state);
>   
>   		state->crtcs[i].ptr = NULL;
>   		state->crtcs[i].state = NULL;
>   		state->crtcs[i].old_state = NULL;
>   		state->crtcs[i].new_state = NULL;
> @@ -284,12 +287,14 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state)
>   		struct drm_plane *plane = state->planes[i].ptr;
>   
>   		if (!plane)
>   			continue;
>   
> -		plane->funcs->atomic_destroy_state(plane,
> -						   state->planes[i].state);
> +		if (state->planes[i].state)
> +			plane->funcs->atomic_destroy_state(plane,
> +							       state->planes[i].state);
> +
>   		state->planes[i].ptr = NULL;
>   		state->planes[i].state = NULL;
>   		state->planes[i].old_state = NULL;
>   		state->planes[i].new_state = NULL;
>   	}
> @@ -298,12 +303,14 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state)
>   		struct drm_private_obj *obj = state->private_objs[i].ptr;
>   
>   		if (!obj)
>   			continue;
>   
> -		obj->funcs->atomic_destroy_state(obj,
> -						 state->private_objs[i].state);
> +		if (state->private_objs[i].state)
> +			obj->funcs->atomic_destroy_state(obj,
> +							       state->private_objs[i].state);
> +
>   		state->private_objs[i].ptr = NULL;
>   		state->private_objs[i].state = NULL;
>   		state->private_objs[i].old_state = NULL;
>   		state->private_objs[i].new_state = NULL;
>   	}
>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)