On 11/04/2024 14:18, Carsten Haitzler wrote:
[...]
@@ -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.
Still not sure this has anything to do with adding conversions, but I suppose it doesn't hurt either. Interestingly the function already has this ret variable and ends with return ret; despite never setting it... I guess it was asking to be used!
[...]
@@ -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.
Makes sense. Better leave the structs we don't handle unchanged :)
Kevin