[1/2] _umtx_op: reduce redundancy required for compat32

[kevans]

Summary

All of the compat32 variants are substantially the same, save for copyin/copyout. Apply the same kind of technique used with kevent here by having the syscall routines supply a umtx_copyops describing the operations needed.

umtx_copyops carries the bare minimum needed- size of timespec and _umtx_time are used for determining if copyout is needed in the sem2_wait case.

This will be used in an upcoming patch to supply a 32-bit/i386 interface to help qemu-bsd-user out.

[kib]

This looks nice.

sys/kern/kern_umtx.c

225

Why is NEED_UMTX32 needed ?

Can't you continue to use COMPAT_FREEBSD32 ? Or even drop this conditional at all.

227

I assume all addr are user addresses ? Might be call parameters uaddr ?

4145

Isn't sz user-controlled ? Or I am missing something.

4152

Might be use the opportunity and move both umtx_time32 and umtx_robust*32 to compat/freebsd32/freebsd32.h ?

4204

sys/abi_compat.h has a lot of macros to both make this more compact and obscure.

[kevans]

sys/kern/kern_umtx.c

225

This one ended up looking weird because of how the patch evolved and I had to split this part back up, but the next patch (D27223) will add LP64 here as it wants the compat32 definitions as well as the equivalents for "other" compat32 (i.e. non-i386 vs. i386). I think the end result ended up using it in few enough places that there's no problem with just changing those to || defined(__LP64__)

[kevans]

Comments addressed; additionally discovered that UMTX_OP_WAIT and UMTX_OP_NWAKE_PRIVATE were both wrong, as they deal with host longs.

An additional fix included for robust lists... it's maybe a little hacky and it's definitely overkill for this patch, but it will make sense in the follow-up. Other users of the "compat32" interface (UMTX_OP__32BIT in follow-up) will not actually be 32-bit binaries, so we can't key off of that going forward.. This version sets a thread p2 flag to indicate that we've seen a 32-bit robust list call, and will reject any future native _umtx_op calls for robust lists.

The future patch will drop #ifdef COMPAT_FREEBSD32 from umtx_read_rb_list().

freebsd32_* stuff has been more completely moved into freebsd32/; the copyops still remain in kern_mutex since they'll be used for non-compat32 32-bit calls.

[kevans]

sys/kern/kern_umtx.c

4145

I might remove this altogether or at least add a coment- the caller passes in uasize - ops->umtx_time_sz after checking that uasize >= ops->umtx_time_sz + ops->timespec_sz, so the result will always be greater than ops->timespec_sz -- the intention of this was to catch where ops->timespec_sz, which is filled out directly in the copyops and actually const now locally, was populated incorrectly.

[kevans]

Fix sneaky bug in nwake_private_compat32 and use uintptr_t when we widen the uint32 for umtx_key while we're here.

[kib]

sys/kern/kern_umtx.c

3554

Could you please add {} around loop body ? It is multi-line.

4137

Where is this flag cleared ?

4263

Make this const ?

4273

And this ?

4286

const for ops ?

4298

return ((*op_table[uap.op])(td, &uap, ops));

4304

umtx_ops does not make much sense, why not pass &umtx_native_ops directly ?

sys/sys/umtx.h

202

I do not see a need to use underscore as a prefix.

212

use bool ?

[kevans]

Address comments; TDP2_COMPAT32RB is cleared in umtx_thread_cleanup. It must persist at least through umtx_cleanup_rb_list -> umtx_handle_rb -> umtx_read_rb_list, so current approach is to just set a compat32 local and clear it(*), then pass that through to the other small handful of rb-related functions.

(*) It wasn't clear to me that it's impossible to reset the robust lists to 0 in the application, so out of an abundance of caution I wanted to clear it before the first return. I suppose __umtx_op_robust_list could do it instead in the unlikely event that userland's set them all to 0 to simplify the later bits, but at a cursory glance it didn't look like this would be useful for the one in-tree user.

[kib]

In D27222#608274, @kevans wrote:

Address comments; TDP2_COMPAT32RB is cleared in umtx_thread_cleanup. It must persist at least through umtx_cleanup_rb_list -> umtx_handle_rb -> umtx_read_rb_list, so current approach is to just set a compat32 local and clear it(*), then pass that through to the other small handful of rb-related functions.

(*) It wasn't clear to me that it's impossible to reset the robust lists to 0 in the application, so out of an abundance of caution I wanted to clear it before the first return. I suppose __umtx_op_robust_list could do it instead in the unlikely event that userland's set them all to 0 to simplify the later bits, but at a cursory glance it didn't look like this would be useful for the one in-tree user.

My concern was with the flag persist through exec.

sys/sys/proc.h

522

I would say 'compat32 ABI for robust list'.

[kevans]

In D27222#608375, @kib wrote:

In D27222#608274, @kevans wrote:

Address comments; TDP2_COMPAT32RB is cleared in umtx_thread_cleanup. It must persist at least through umtx_cleanup_rb_list -> umtx_handle_rb -> umtx_read_rb_list, so current approach is to just set a compat32 local and clear it(*), then pass that through to the other small handful of rb-related functions.

(*) It wasn't clear to me that it's impossible to reset the robust lists to 0 in the application, so out of an abundance of caution I wanted to clear it before the first return. I suppose __umtx_op_robust_list could do it instead in the unlikely event that userland's set them all to 0 to simplify the later bits, but at a cursory glance it didn't look like this would be useful for the one in-tree user.

My concern was with the flag persist through exec.

Ah, fair enough- thanks!

[brooks]

Unfortunately, much of the redundancy removed here isn't redundant for CHERI. On CHERI systems freebsd32__umtx_op_args can not be cast to _umtx_op_args. I'll need to ponder how best to proceed. For now I think I'll merge as-is and make compat32 more broken in CheriBSD.