(Resend from my properly subscribed email)
On 11/1/23 15:28, Kevin Brodsky wrote:
On 31/10/2023 17:20, Akram Ahmad wrote:
- __u64 data_ptr;
- __kernel_uintptr_t data_ptr;
Yup. I've hit this too - battled with it. I was a bit bemused for a while with uintptr_t not working... the above was the solution. It moved me forward. I am unhappy with it but it's one of those "come back to it at a later stage". I have quite a few of these in my drm work by now.
rebuilt against the updated kernel headers anyway, but it is a major issue in compat64, as it is essential for userspace to keep working
Yup. That's been a lot of my fiddling in keeping drm compat64 working and making purecap work at the same time. I've made purecap work several times only to find I broke compat subtly and had to spend a lot of printk()ing and rebooting to figure out why (or ... I broke libdrm in userspace which is a separate thing I have to also adapt to a new purecap ABI AND keep it working with the original 64 and 32bit APIs). libdrm has it's own copy of drm kernel header files too btw... but I've managed to keep my compat binaries working so far. It's a slow and exacting process. Though for now size of ioctl structs will change and at least the drm infra seems to copy in the right sized struct based on compat vs purecap (thanks to much printk debugging in the ioctl core handling in drm)
_IOC(_IOC_READ|_IOC_WRITE, MMC_BLOCK_MAJOR, 0, 64)) - compat structs are not defined in uapi headers, so we cannot use them. Opinions very
Well my structs are beginning to look like:
struct drm_mode_obj_get_properties { #ifdef __CHERI__ __kernel_uintptr_t props_ptr; __kernel_uintptr_t prop_values_ptr; #else __u64 props_ptr; __u64 prop_values_ptr; #endif __u32 count_props; __u32 obj_id; __u32 obj_type; };
So same struct - keep the previous compat behavior but CHERI gets the new type sizes. The drm folk seems to have oscillated between unsigned longs and __u64's being "good enough for pointers from userspace". It's not consistent, but it is what it is. This above is the kernel header. My copy of the same structs in libdrm:
struct drm_mode_obj_get_properties { #ifdef __CHERI__ uintcap_t props_ptr; uintcap_t prop_values_ptr; #else __u64 props_ptr; __u64 prop_values_ptr; #endif __u32 count_props; __u32 obj_id; __u32 obj_type; };
So same pattern but using the cleaner type. i could typdef this and use those instead. I am actually wanting to be a bit more explicit here due to the "sometimes long, sometimes __u64 is good enough". I actually am ignoring the doubling up of kernel headers for now (include/uapi/drm/ and tools/include/uapi/drm/ often duplicate the same drm struct types etc. ... hooray - I'm ignoring tools/... for now and will worry about that at the end).
So for vblank I'm hybrid re-using the compat32 infra that for us is actually compat64 and the shared common infra. For other things I've morphed into doing things like:
int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv) { #ifdef CONFIG_COMPAT struct drm_mode_obj_get_properties32 { __u64 props_ptr; __u64 prop_values_ptr; __u32 count_props; __u32 obj_id; __u32 obj_type; }; struct drm_mode_obj_get_properties32 *arg32 = data; #endif struct drm_mode_obj_get_properties *arg = data;
...
#ifdef CONFIG_COMPAT if (test_thread_flag(TIF_64BIT_COMPAT)) { count_props = arg32->count_props; obj_id = arg32->obj_id; obj_type = arg32->obj_type; props_ptr = uaddr_to_user_ptr(arg32->props_ptr); prop_values_ptr = uaddr_to_user_ptr(arg32->prop_values_ptr); } else { #endif count_props = arg->count_props; obj_id = arg->obj_id; obj_type = arg->obj_type; props_ptr = (uint32_t __user *)(arg->props_ptr); prop_values_ptr = (uint64_t __user *)(arg->prop_values_ptr); #ifdef CONFIG_COMPAT } #endif
...
Yes - I know it's "32". It lies. drm infra has already accepted that "compat" == named 32bit by convention within its tree but for us compat == 64bit, so I'm just following the convention. count_props, obj_id, obj_type etc. are local vars and then to write them back out on successful ioctl call before return:
#ifdef CONFIG_COMPAT if (test_thread_flag(TIF_64BIT_COMPAT)) { arg32->count_props = count_props; } else { #endif arg->count_props = count_props; #ifdef CONFIG_COMPAT } #endif DRM_MODESET_LOCK_ALL_END(dev, ctx, ret); return ret; }
I have to restructure the code to basically have these import+export blocks. It's not pretty but it works. I could do some weird macros like
#ifdef CONFIG_COMPAT # define VALMIRROR(structname, member) if (test_thread_flag(TIF_64BIT_COMPAT)) { member = structname ## 32->member; } else { member = structname->member; } #else # define VALMIRROR(structname, member) member = structname->member #endif
But... that'd be a lot of expanded if's and it makes it a less obvious to me what's going on so i chose not to get tricky. Also the way members were used from the ioctls was often passing them in to funcs so I still need to create my local mirror types and i need to be nice with some casting stuff too so I've kept it more obvious.
All in the fun of defining a new ABI and still keeping compat working. 🙂