Page MenuHomeFreeBSD

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

Authored by kevans on Sun, Nov 15, 4:13 AM.

Details

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.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

This looks nice.

sys/kern/kern_umtx.c
226 ↗(On Diff #79559)

Why is NEED_UMTX32 needed ?

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

242 ↗(On Diff #79559)

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

4138 ↗(On Diff #79559)

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

4155 ↗(On Diff #79559)

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

4214 ↗(On Diff #79559)

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

sys/kern/kern_umtx.c
226 ↗(On Diff #79559)

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 marked 4 inline comments as done.

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.

sys/kern/kern_umtx.c
4138 ↗(On Diff #79559)

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.

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

sys/kern/kern_umtx.c
3554 ↗(On Diff #79574)

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

4137 ↗(On Diff #79574)

Where is this flag cleared ?

4263 ↗(On Diff #79574)

Make this const ?

4273 ↗(On Diff #79574)

And this ?

4286 ↗(On Diff #79574)

const for ops ?

4298 ↗(On Diff #79574)

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

4304 ↗(On Diff #79574)

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

sys/sys/umtx.h
202 ↗(On Diff #79574)

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

212 ↗(On Diff #79574)

use bool ?

kevans marked 9 inline comments as done.

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.

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 ↗(On Diff #79622)

I would say 'compat32 ABI for robust list'.

This revision is now accepted and ready to land.Tue, Nov 17, 12:59 AM
In D27222#608375, @kib 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!

This revision was automatically updated to reflect the committed changes.

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.