[linux-morello] Re: [PATCH 3/3] drm: fix up purecap handling of all iotcls in libdrm test tools

On 4/9/24 3:58 PM, Kevin Brodsky wrote:

On 05/04/2024 12:20, carsten.haitzler@foss.arm.com wrote:

...

[...]

@@ -2919,16 +2962,9 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, copied++; } }

• out_resp->count_encoders = encoders_count;
• out_resp->connector_id = connector->base.id;
• out_resp->connector_type = connector->connector_type;
• out_resp->connector_type_id = connector->connector_type_id;
• is_current_master = drm_is_current_master(file_priv);

Nit: probably want to keep the empty lines around that line.

will add them back - just as al of it was going away there wasnt aby logical blocking worth talking about left.

...

mutex_lock(&dev->mode_config.mutex);

• if (out_resp->count_modes == 0) {

• if (count_modes == 0) { if (is_current_master) connector->funcs->fill_modes(connector, dev->mode_config.max_width,

@@ -2938,11 +2974,6 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, connector->base.id, connector->name); }

• out_resp->mm_width = connector->display_info.width_mm;
• out_resp->mm_height = connector->display_info.height_mm;
• out_resp->subpixel = connector->display_info.subpixel_order;
• out_resp->connection = connector->status;
• /* delayed so we get modes regardless of pre-fill_modes state */ list_for_each_entry(mode, &connector->modes, head) { WARN_ON(mode->expose_to_userspace);

@@ -2958,9 +2989,8 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, * This ioctl is called twice, once to determine how much space is * needed, and the 2nd time to fill it. */

• if ((out_resp->count_modes >= mode_count) && mode_count) {

• if ((count_modes >= mode_count) && mode_count) { copied = 0;

• mode_ptr = uaddr_to_user_ptr(out_resp->modes_ptr);
  

  list_for_each_entry(mode, &connector->modes, head) { if (!mode->expose_to_userspace) continue;

@@ -2998,25 +3028,50 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, mode->expose_to_userspace = false; }

• out_resp->count_modes = mode_count; mutex_unlock(&dev->mode_config.mutex);

drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); encoder = drm_connector_get_encoder(connector);

• if (encoder)

• out_resp->encoder_id = encoder->base.id;
  

• else

• out_resp->encoder_id = 0;
  

/* Only grab properties after probing, to make sure EDID and other * properties reflect the latest status. */ ret = drm_mode_object_get_properties(&connector->base, file_priv->atomic,

• 	uaddr_to_user_ptr(out_resp->props_ptr),
  

• 	uaddr_to_user_ptr(out_resp->prop_values_ptr),
  

• 	&out_resp->count_props);
  

• 	prop_ptr, prop_values,
  

• 	&count_props);
  

I think this will result in UB, because drm_mode_object_get_properties() reads from arg_count_props (last argument) *before* writing to it, and count_props is uninitialised at this point. Maybe we should initialise it like the other properties at the beginning of this function.

Oh yeah totally missed the count_props init from userspace ioctl data along with the others! and yet... it still worked in tests. :)

...

drm_modeset_unlock(&dev->mode_config.connection_mutex);

• if (drm_test_compat64()) {

• out_resp32->count_encoders = encoders_count;
  

• out_resp32->count_modes = mode_count;
  

• out_resp32->count_props = count_props;
  

• out_resp32->connector_id = connector->base.id;
  

• out_resp32->connector_type = connector->connector_type;
  

• out_resp32->connector_type_id = connector->connector_type_id;
  

• if (encoder)
  

• 	out_resp32->encoder_id = encoder->base.id;
  

• else
  

• 	out_resp32->encoder_id = 0;
  

• out_resp32->mm_width = connector->display_info.width_mm;
  

• out_resp32->mm_height = connector->display_info.height_mm;
  

• out_resp32->subpixel = connector->display_info.subpixel_order;
  

• out_resp32->connection = connector->status;
  

• } else {

• out_resp->count_encoders = encoders_count;
  

• out_resp->count_modes = mode_count;
  

• out_resp->count_props = count_props;
  

• out_resp->connector_id = connector->base.id;
  

• out_resp->connector_type = connector->connector_type;
  

• out_resp->connector_type_id = connector->connector_type_id;
  

• if (encoder)
  

• 	out_resp->encoder_id = encoder->base.id;
  

• else
  

• 	out_resp->encoder_id = 0;
  

• out_resp->mm_width = connector->display_info.width_mm;
  

• out_resp->mm_height = connector->display_info.height_mm;
  

• out_resp->subpixel = connector->display_info.subpixel_order;
  

• out_resp->connection = connector->status;
  

• } out: drm_connector_put(connector);

diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c index 8525ef851540..eab94e74b56a 100644 --- a/drivers/gpu/drm/drm_mode_config.c +++ b/drivers/gpu/drm/drm_mode_config.c @@ -92,11 +92,30 @@ int drm_mode_getresources(struct drm_device *dev, void *data, struct drm_file *file_priv) { struct drm_mode_card_res *card_res = data;

• struct drm_mode_card_res32 {

• __u64 fb_id_ptr;
  

• __u64 crtc_id_ptr;
  

• __u64 connector_id_ptr;
  

• __u64 encoder_id_ptr;
  

• __u32 count_fbs;
  

• __u32 count_crtcs;
  

• __u32 count_connectors;
  

• __u32 count_encoders;
  

• __u32 min_width;
  

• __u32 max_width;
  

• __u32 min_height;
  

• __u32 max_height;
  

• };
• struct drm_mode_card_res32 *card_res32 = data; struct drm_framebuffer *fb; struct drm_connector *connector; struct drm_crtc *crtc; struct drm_encoder *encoder;

• int count, ret = 0;

• int count_fb = 0, count_fbs;
• int count_crtc = 0, count_crtcs;
• int count_encoder = 0, count_encoders;
• int count_connector = 0, count_connectors;
• int ret = 0; uint32_t __user *fb_id; uint32_t __user *crtc_id; uint32_t __user *connector_id;

@@ -107,49 +126,54 @@ int drm_mode_getresources(struct drm_device *dev, void *data, return -EOPNOTSUPP; mutex_lock(&file_priv->fbs_lock);

• count = 0;
• fb_id = u64_to_user_ptr(card_res->fb_id_ptr);

• if (drm_test_compat64()) {

• fb_id        = compat_ptr(card_res32->fb_id_ptr);
  

• crtc_id      = compat_ptr(card_res32->crtc_id_ptr);
  

• encoder_id   = compat_ptr(card_res32->encoder_id_ptr);
  

• connector_id = compat_ptr(card_res32->connector_id_ptr);
  

• count_fbs        = card_res32->count_fbs;
  

• count_crtcs      = card_res32->count_crtcs;
  

• count_encoders   = card_res32->count_encoders;
  

• count_connectors = card_res32->count_connectors;
  

• } else {

• fb_id        = (uint32_t __user *)(card_res->fb_id_ptr);
  

• crtc_id      = (uint32_t __user *)(card_res->crtc_id_ptr);
  

• encoder_id   = (uint32_t __user *)(card_res->encoder_id_ptr);
  

• connector_id = (uint32_t __user *)(card_res->connector_id_ptr);
  

• count_fbs        = card_res->count_fbs;
  

• count_crtcs      = card_res->count_crtcs;
  

• count_encoders   = card_res->count_encoders;
  

• count_connectors = card_res->count_connectors;
  

• } list_for_each_entry(fb, &file_priv->fbs, filp_head) {

• if (count < card_res->count_fbs &&
  

•     put_user(fb->base.id, fb_id + count)) {
  

• 	mutex_unlock(&file_priv->fbs_lock);
  

• 	return -EFAULT;
  

• if (count_fb < count_fbs &&
  

•     put_user(fb->base.id, fb_id + count_fb)) {
  

• 	ret = -EFAULT;
  

• 	goto error;
  

  }

• count++;
  

• count_fb++;
  

  }

• card_res->count_fbs = count;
• mutex_unlock(&file_priv->fbs_lock);

I can't quite convince myself that postponing the mutex_unlock() until the end of the function is a good idea or even correct. Either way it seems completely unrelated to the rest of the patch.

as the return is only at the end now, error handling is done with the goto wich then immediately unlocks mutex and returns in one spot instead of N spots. as i needed an encode+decode pass basically i kept the code consistent and easy to manage this way - the mutex is held a bit longer but the way i see it... what kind of real contention do you ever get with drm apps doing this... at the same time? :) you pretty much don't. you have some process that owns the display now. X. a wl compositor etc. and it's going to query the device - it and only it. no one else is because ... you only really have one vt owning process at a time doing this. it's not going to do this from multiple threads. it'll have some serial logic and then make a decision and be done. :)

so it's less hairy code with less to go wrong when done this way with a mutex held a bit longer for ... no real life loss. :) that's my take.

...

[...]

int drm_mode_getplane(struct drm_device *dev, void *data, struct drm_file *file_priv) {

• struct drm_mode_get_plane32 {

• __u32 plane_id;
  

• __u32 crtc_id;
  

• __u32 fb_id;
  

• __u32 possible_crtcs;
  

• __u32 gamma_size;
  

• __u32 count_format_types;
  

• __u64 format_type_ptr;
  

• };
• struct drm_mode_get_plane32 *plane_resp32 = data; struct drm_mode_get_plane *plane_resp = data; struct drm_plane *plane; uint32_t __user *format_ptr;
• __u32 plane_id, count_format_types, crtc_id, fb_id, possible_crtcs, gamma_size;

if (!drm_core_check_feature(dev, DRIVER_MODESET)) return -EOPNOTSUPP;

• plane = drm_plane_find(dev, file_priv, plane_resp->plane_id);

• if (drm_test_compat64()) {

• format_ptr = uaddr_to_user_ptr(plane_resp32->format_type_ptr);
  

• plane_id = plane_resp32->plane_id;
  

• count_format_types = plane_resp32->count_format_types;
  

• } else {

• format_ptr = uaddr_to_user_ptr(plane_resp->format_type_ptr);
  

The pointer conversions need to be updated here (compat_ptr / simple cast).

...

• plane_id = plane_resp->plane_id;
  

• count_format_types = plane_resp->count_format_types;
  

• }
• plane = drm_plane_find(dev, file_priv, plane_id); if (!plane) return -ENOENT;

drm_modeset_lock(&plane->mutex, NULL); if (plane->state && plane->state->crtc && drm_lease_held(file_priv, plane->state->crtc->base.id))

• plane_resp->crtc_id = plane->state->crtc->base.id;
  

• crtc_id = plane->state->crtc->base.id;
  

  else if (!plane->state && plane->crtc && drm_lease_held(file_priv, plane->crtc->base.id))

• plane_resp->crtc_id = plane->crtc->base.id;
  

• crtc_id = plane->crtc->base.id;
  

  else

• plane_resp->crtc_id = 0;
  

• crtc_id = 0;
  

if (plane->state && plane->state->fb)

• plane_resp->fb_id = plane->state->fb->base.id;
  

• fb_id = plane->state->fb->base.id;
  

  else if (!plane->state && plane->fb)

• plane_resp->fb_id = plane->fb->base.id;
  

• fb_id = plane->fb->base.id;
  

  else

• plane_resp->fb_id = 0;
  

• fb_id = 0;
  

  drm_modeset_unlock(&plane->mutex);

• plane_resp->plane_id = plane->base.id;
• plane_resp->possible_crtcs = drm_lease_filter_crtcs(file_priv,

• 					    plane->possible_crtcs);
  

• plane_id = plane->base.id;
• possible_crtcs = drm_lease_filter_crtcs(file_priv,

• 				plane->possible_crtcs);
  

• plane_resp->gamma_size = 0;

• gamma_size = 0;

/* * This ioctl is called twice, once to determine how much space is * needed, and the 2nd time to fill it. */ if (plane->format_count &&

•    (plane_resp->count_format_types >= plane->format_count)) {
  

• format_ptr = uaddr_to_user_ptr(plane_resp->format_type_ptr);
  

•    (count_format_types >= plane->format_count)) {
  

  if (copy_to_user(format_ptr, plane->format_types, sizeof(uint32_t) * plane->format_count)) {

@@ -741,6 +781,22 @@ int drm_mode_getplane(struct drm_device *dev, void *data, } plane_resp->count_format_types = plane->format_count;

Should be deleted (it's now in the if/else below).

gah - missed that in my conversion. yup. it was then overwritten later anyway.

...

• if (drm_test_compat64()) {

• plane_resp32->crtc_id = crtc_id;
  

• plane_resp32->fb_id = fb_id;
  

• plane_resp32->plane_id = plane_id;
  

• plane_resp32->possible_crtcs = possible_crtcs;
  

• plane_resp32->gamma_size = gamma_size;
  

• plane_resp32->count_format_types = count_format_types;
  

• } else {

• plane_resp->crtc_id = crtc_id;
  

• plane_resp->fb_id = fb_id;
  

• plane_resp->plane_id = plane_id;
  

• plane_resp->possible_crtcs = possible_crtcs;
  

• plane_resp->gamma_size = gamma_size;
  

• plane_resp->count_format_types = count_format_types;
  

• }
• return 0; }

diff --git a/drivers/gpu/drm/drm_property.c b/drivers/gpu/drm/drm_property.c index dfec479830e4..6ae86d4510a8 100644 --- a/drivers/gpu/drm/drm_property.c +++ b/drivers/gpu/drm/drm_property.c @@ -457,6 +457,16 @@ EXPORT_SYMBOL(drm_property_destroy); int drm_mode_getproperty_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv) {

• struct drm_mode_get_property32 {

• __u64 values_ptr;
  

• __u64 enum_blob_ptr;
  

• __u32 prop_id;
  

• __u32 flags;
  

• char name[DRM_PROP_NAME_LEN];
  

• __u32 count_values;
  

• __u32 count_enum_blobs;
  

• };
• struct drm_mode_get_property32 *out_resp32 = data; struct drm_mode_get_property *out_resp = data; struct drm_property *property; int enum_count = 0;

@@ -465,22 +475,39 @@ int drm_mode_getproperty_ioctl(struct drm_device *dev, struct drm_property_enum *prop_enum; struct drm_mode_property_enum __user *enum_ptr; uint64_t __user *values_ptr;

• __u32 prop_id, flags, count_values, count_enum_blobs;
• char *name;

if (!drm_core_check_feature(dev, DRIVER_MODESET)) return -EOPNOTSUPP;

• property = drm_property_find(dev, file_priv, out_resp->prop_id);

• if (drm_test_compat64()) {

• values_ptr = compat_ptr(out_resp32->values_ptr);
  

• enum_ptr = compat_ptr(out_resp32->enum_blob_ptr);
  

• prop_id = out_resp32->prop_id;
  

• name = out_resp32->name;
  

• count_values = out_resp32->count_values;
  

• count_enum_blobs = out_resp32->count_enum_blobs;
  

• } else {

• values_ptr = (uint64_t __user *)out_resp->values_ptr;
  

• enum_ptr = (struct drm_mode_property_enum __user *)out_resp->enum_blob_ptr;
  

• prop_id = out_resp->prop_id;
  

• name = out_resp->name;
  

• count_values = out_resp->count_values;
  

• count_enum_blobs = out_resp->count_enum_blobs;
  

• }
• property = drm_property_find(dev, file_priv, prop_id); if (!property) return -ENOENT;

• strscpy_pad(out_resp->name, property->name, DRM_PROP_NAME_LEN);
• out_resp->flags = property->flags;

• strscpy_pad(name, property->name, DRM_PROP_NAME_LEN);
• flags = property->flags;

value_count = property->num_values;

• values_ptr = u64_to_user_ptr(out_resp->values_ptr);

for (i = 0; i < value_count; i++) {

• if (i < out_resp->count_values &&
  

• if (i < count_values &&
  

  put_user(property->values[i], values_ptr + i)) { return -EFAULT; }

@@ -488,13 +515,12 @@ int drm_mode_getproperty_ioctl(struct drm_device *dev, out_resp->count_values = value_count;

Should be deleted too.

same as above - yup.

...

copied = 0;

• enum_ptr = u64_to_user_ptr(out_resp->enum_blob_ptr);

if (drm_property_type_is(property, DRM_MODE_PROP_ENUM) || drm_property_type_is(property, DRM_MODE_PROP_BITMASK)) { list_for_each_entry(prop_enum, &property->enum_list, head) { enum_count++;

• 	if (out_resp->count_enum_blobs < enum_count)
  

• 	if (count_enum_blobs < enum_count)
  	continue;
  

if (copy_to_user(&enum_ptr[copied].value, @@ -506,7 +532,7 @@ int drm_mode_getproperty_ioctl(struct drm_device *dev, return -EFAULT; copied++; }

• out_resp->count_enum_blobs = enum_count;
  

• count_enum_blobs = enum_count;
  

  }

/* @@ -518,7 +544,18 @@ int drm_mode_getproperty_ioctl(struct drm_device *dev, * the property itself. */ if (drm_property_type_is(property, DRM_MODE_PROP_BLOB))

• out_resp->count_enum_blobs = 0;
  

• count_enum_blobs = 0;
  

Nit: empty line here.

okay. :)

...

• if (drm_test_compat64()) {

• out_resp32->prop_id = prop_id;
  

• out_resp32->flags = flags;
  

• out_resp32->count_values = count_values;
  

• out_resp32->count_enum_blobs = count_enum_blobs;
  

• } else {

• out_resp->prop_id = prop_id;
  

• out_resp->flags = flags;
  

• out_resp->count_values = count_values;
  

• out_resp->count_enum_blobs = count_enum_blobs;
  

• }

return 0; } @@ -754,29 +791,53 @@ EXPORT_SYMBOL(drm_property_replace_blob); int drm_mode_getblob_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv) {

• struct drm_mode_get_blob32 {

• __u32 blob_id;
  

• __u32 length;
  

• __u64 data;
  

• };
• struct drm_mode_get_blob32 *out_resp32 = data; struct drm_mode_get_blob *out_resp = data; struct drm_property_blob *blob; int ret = 0;
• __u32 blob_id;
• __u32 length;
• uint8_t __user *blob_data;
• if (drm_test_compat64()) {

• blob_id = out_resp32->blob_id;
  

• length = out_resp32->length;
  

• blob_data = compat_ptr(out_resp32->data);
  

• } else {

• blob_id = out_resp->blob_id;
  

• length = out_resp->length;
  

• blob_data = (uint8_t __user *)(out_resp->data);
  

• }

if (!drm_core_check_feature(dev, DRIVER_MODESET)) return -EOPNOTSUPP;

• blob = drm_property_lookup_blob(dev, out_resp->blob_id);

• blob = drm_property_lookup_blob(dev, blob_id); if (!blob) return -ENOENT;

• if (out_resp->length == blob->length) {

• if (copy_to_user(u64_to_user_ptr(out_resp->data),
  

• if (length == blob->length) {

• if (copy_to_user(blob_data,
  	 blob->data,
  	 blob->length)) {
  ret = -EFAULT;
  goto unref;
  

  } }

• out_resp->length = blob->length;

• length = blob->length; unref: drm_property_blob_put(blob);

• if (drm_test_compat64()) {

• out_resp32->length = length;
  

• } else {

• out_resp->length = length;
  

• } return ret; }

diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h index e2640dc64e08..05e048c16c10 100644 --- a/include/drm/drm_drv.h +++ b/include/drm/drm_drv.h @@ -34,6 +34,15 @@ #include <drm/drm_device.h> +static inline bool drm_test_compat64(void)

We now have in_compat64_syscall() that does exactly the same thing (but isn't arm64-specific).

ahh yeah - i built this a bit earlier when we didn't have that. i'll move it over.

...

+{ +#ifdef CONFIG_COMPAT

• return test_thread_flag(TIF_64BIT_COMPAT);

+#else

• return false;

+#endif +}

[...] struct drm_mode_fb_cmd { @@ -1029,7 +1029,7 @@ struct drm_mode_crtc_page_flip_target { __u32 fb_id; __u32 flags; __u32 sequence;

• __u64 user_data;

• __kernel_uintptr_t user_data;

Aren't we missing the corresponding compat handling for this change?

oh yeah - this may have been me flagging "stuff to do" that i neeed more stack up to go drive it and i switcvhed over to getting userspace up more... so it was the first steps but not the rest as i had no test case yet.

...

}; /** @@ -1135,7 +1135,7 @@ struct drm_mode_atomic { __u64 props_ptr; __u64 prop_values_ptr; __u64 reserved;

• __u64 user_data;

• __kernel_uintptr_t user_data;

Ditto here.

same as above. will remove as this is a "need more of userspace" problem.

Kevin

...

}; struct drm_format_modifier_blob { diff --git a/tools/include/uapi/drm/drm.h b/tools/include/uapi/drm/drm.h index de723566c5ae..7a0524d7d9f5 100644 --- a/tools/include/uapi/drm/drm.h +++ b/tools/include/uapi/drm/drm.h @@ -971,7 +971,7 @@ struct drm_crtc_queue_sequence { __u32 crtc_id; __u32 flags; __u64 sequence; /* on input, target sequence. on output, actual sequence */

• __u64 user_data; /* user data passed to event */

• uintptr_t user_data; /* user data passed to event */ };

#if defined(__cplusplus) @@ -1289,7 +1289,7 @@ struct drm_event_vblank { */ struct drm_event_crtc_sequence { struct drm_event base;

• __u64 user_data;

• uintptr_t user_data; __s64 time_ns; __u64 sequence; };