Page MenuHomeFreeBSD

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

Authored by kevans on Nov 15 2020, 4:13 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Mar 20, 12:15 AM
Unknown Object (File)
Wed, Mar 20, 12:15 AM
Unknown Object (File)
Wed, Mar 20, 12:11 AM
Unknown Object (File)
Dec 20 2023, 6:38 AM
Unknown Object (File)
Dec 15 2023, 12:52 AM
Unknown Object (File)
Dec 11 2023, 1:30 AM
Unknown Object (File)
Nov 16 2023, 9:23 AM
Unknown Object (File)
Nov 10 2023, 2:06 PM

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 - subversion
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 34834

Event Timeline

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 ?

4152

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

4159

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

4211

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

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 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
4152

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
3583

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

4144

Where is this flag cleared ?

4275

Make this const ?

4285

And this ?

4298

const for ops ?

4310

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

4316

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 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

I would say 'compat32 ABI for robust list'.

This revision is now accepted and ready to land.Nov 17 2020, 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.