Re: [PATCH v6 15/15] drm/vkms: Add driver-specific plane property for testing

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm/vkms: NULL dereference in vkms_plane_create_state() if property alloc fails
- [Medium] drm/vkms: missing connector atomic_create_state breaks DRM_MODE_ATOMIC_RESET
--

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

drm/vkms: Add driver-specific plane property for testing

This commit adds a driver-specific "vkms-test-do-not-use-in-prod" range
property to vkms planes so userspace tests can verify that
DRM_MODE_ATOMIC_RESET resets plane state properties to their default values.

> diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
> index fd3d34dcf0fe5..0ebc131c2788f 100644
> --- a/drivers/gpu/drm/vkms/vkms_plane.c
> +++ b/drivers/gpu/drm/vkms/vkms_plane.c

[ ... ]

> @@ -85,7 +92,9 @@ static void vkms_plane_destroy_state(struct drm_plane *plane,
>  
>  static struct drm_plane_state *vkms_plane_create_state(struct drm_plane *plane)
>  {
> +	struct vkms_plane *vkms_plane = to_vkms_plane(plane);
>  	struct vkms_plane_state *vkms_state;
> +	u64 val;
>  
>  	vkms_state = kzalloc_obj(*vkms_state);
>  	if (!vkms_state) {
> @@ -95,14 +104,53 @@ static struct drm_plane_state *vkms_plane_create_state(struct drm_plane *plane)
>  
>  	__drm_gem_shadow_plane_state_init(plane, &vkms_state->base);
>  
> +	if (!drm_object_property_get_default_value(&plane->base,
> +						   vkms_plane->vkms_test_do_not_use_property,
> +						   &val))
> +		vkms_state->vkms_test_do_not_use_value = val;

[Severity: High]
Can this trigger a NULL pointer dereference in
drm_object_property_get_default_value() if drm_property_create_range()
fails in vkms_plane_init()?

In drivers/gpu/drm/vkms/vkms_plane.c, vkms_plane_init() treats a
drm_property_create_range() failure as non-fatal and leaves
plane->vkms_test_do_not_use_property as NULL:

	plane->vkms_test_do_not_use_property =
		drm_property_create_range(dev, 0,
					  "vkms-test-do-not-use-in-prod",
					  0, U32_MAX);
	if (plane->vkms_test_do_not_use_property)
		drm_object_attach_property(&plane->base.base,
					   plane->vkms_test_do_not_use_property,
					   0);

When vkms_plane_create_state() is subsequently called during device
initialization via vkms_output_init() -> drm_mode_config_reset() (or
during a DRM_MODE_ATOMIC_RESET commit via
drm_atomic_commit_fill_with_defaults()), it unconditionally passes
vkms_plane->vkms_test_do_not_use_property to
drm_object_property_get_default_value().

However, in drivers/gpu/drm/drm_mode_object.c,
drm_object_property_get_default_value() immediately dereferences
property->dev without checking whether property is NULL:

int drm_object_property_get_default_value(struct drm_mode_object *obj,
					  struct drm_property *property,
					  uint64_t *val)
{
	WARN_ON(!drm_drv_uses_atomic_modeset(property->dev));

	return __drm_object_property_get_prop_value(obj, property, val);
}

Should vkms_plane_create_state() check that
vkms_plane->vkms_test_do_not_use_property is non-NULL before calling
drm_object_property_get_default_value(), or should vkms_plane_init()
fail when drm_property_create_range() returns NULL?

[ ... ]

> @@ -283,6 +331,15 @@ struct vkms_plane *vkms_plane_init(struct vkms_device *vkmsdev,
>  					  DRM_COLOR_YCBCR_BT601,
>  					  DRM_COLOR_YCBCR_FULL_RANGE);
>  
> +	plane->vkms_test_do_not_use_property =
> +		drm_property_create_range(dev, 0,
> +					  "vkms-test-do-not-use-in-prod",
> +					  0, U32_MAX);
> +	if (plane->vkms_test_do_not_use_property)
> +		drm_object_attach_property(&plane->base.base,
> +					   plane->vkms_test_do_not_use_property,
> +					   0);

[Severity: Medium]
Will userspace be able to use DRM_MODE_ATOMIC_RESET to test resetting
this property on vkms while vkms connectors do not implement
atomic_create_state?

When userspace queries DRM_CAP_ATOMIC_RESET in drm_getcap() or submits
an atomic commit with DRM_MODE_ATOMIC_RESET in drm_mode_atomic_ioctl(),
drm_atomic_implements_create_state() and
drm_atomic_commit_fill_with_defaults() in drivers/gpu/drm/drm_atomic.c
require every connector on the device to provide an atomic_create_state
callback:

	drm_connector_list_iter_begin(dev, &conn_iter);
	drm_for_each_connector_iter(connector, &conn_iter) {
		if (!connector->funcs->atomic_create_state) {
			drm_connector_list_iter_end(&conn_iter);
			return false;
		}
	}
	drm_connector_list_iter_end(&conn_iter);

However, vkms_connector_funcs in drivers/gpu/drm/vkms/vkms_connector.c
and vkms_wb_connector_funcs in drivers/gpu/drm/vkms/vkms_writeback.c
still define .reset = drm_atomic_helper_connector_reset without setting
.atomic_create_state:

static const struct drm_connector_funcs vkms_connector_funcs = {
	.detect = vkms_connector_detect,
	.fill_modes = drm_helper_probe_single_connector_modes,
	.reset = drm_atomic_helper_connector_reset,
	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
};

Because every valid vkms device registers at least one connector,
drm_atomic_implements_create_state() always returns false on vkms, so
DRM_CAP_ATOMIC_RESET reports 0 and DRM_MODE_ATOMIC_RESET commits fail
with -EOPNOTSUPP before vkms_plane_create_state() can be reached.

Do vkms_connector_funcs and vkms_wb_connector_funcs also need to use
.atomic_create_state = drm_atomic_helper_connector_create_state so that
DRM_MODE_ATOMIC_RESET is supported on vkms?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022-drm-reset-state-flag-v6-0-f36d640b7e76@kernel.org?part=15