Page MenuHomeFreeBSD
Differential D27222

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

Authored by kevans on Nov 15 2020, 4:13 AM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Nov 26, 12:42 PM
Unknown Object (File)
Thu, Nov 14, 8:56 AM
Unknown Object (File)
Oct 20 2024, 1:05 PM
Unknown Object (File)
Oct 20 2024, 1:05 PM
Unknown Object (File)
Oct 20 2024, 1:05 PM
Unknown Object (File)
Oct 20 2024, 1:04 PM
Unknown Object (File)
Oct 20 2024, 1:04 PM
Unknown Object (File)
Oct 20 2024, 1:04 PM
View All Files
Subscribers
andrew_tao173.riddles.org.uk
imp
mikael

Details

Reviewers
kib
brooks
Commits
rS367744: umtx_op: reduce redundancy required for compat32
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 34813

Event Timeline

kevans created this revision.Nov 15 2020, 4:13 AM
Herald added a subscriber: imp. · View Herald TranscriptNov 15 2020, 4:13 AM
kevans requested review of this revision.Nov 15 2020, 4:13 AM
Harbormaster completed remote builds in B34803: Diff 79559.Nov 15 2020, 4:13 AM
kevans added a child revision: D27223: [2/2] _umtx_op: introduce 32-bit/i386 flags for operations.Nov 15 2020, 4:13 AM
mikael added a subscriber: mikael.Nov 15 2020, 8:33 AM
andrew_tao173.riddles.org.uk added a subscriber: andrew_tao173.riddles.org.uk.Nov 15 2020, 9:10 AM
kib added a comment.Nov 15 2020, 11:31 AM

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 added inline comments.Nov 15 2020, 2:30 PM
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 updated this revision to Diff 79570.Nov 15 2020, 9:21 PM
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.

Harbormaster completed remote builds in B34813: Diff 79570.Nov 15 2020, 9:21 PM
kevans added inline comments.Nov 15 2020, 9:22 PM
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 updated this revision to Diff 79574.Nov 16 2020, 12:24 AM

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

Harbormaster completed remote builds in B34815: Diff 79574.Nov 16 2020, 12:24 AM
kib added inline comments.Nov 16 2020, 9:33 AM
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 updated this revision to Diff 79622.Nov 16 2020, 7:19 PM
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.

Harbormaster completed remote builds in B34834: Diff 79622.Nov 16 2020, 7:20 PM
kib added a comment.Nov 17 2020, 12:59 AM
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'.

kib accepted this revision.Nov 17 2020, 12:59 AM
This revision is now accepted and ready to land.Nov 17 2020, 12:59 AM
kevans added a comment.Nov 17 2020, 3:29 AM
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!

Closed by commit rS367744: umtx_op: reduce redundancy required for compat32 (authored by kevans). · Explain WhyNov 17 2020, 3:37 AM
This revision was automatically updated to reflect the committed changes.
kevans added a commit: rS367744: umtx_op: reduce redundancy required for compat32.
brooks added a comment.Nov 25 2020, 12:06 AM

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.

Revision Contents
Changeset List

PathSize
sys/
compat/
freebsd32/
21 lines
9 lines
kern/
612 lines
sys/
1 line
3 lines
17 lines

Diff 79570

View Options

sys/compat/freebsd32/freebsd32.h

Loading...
View Options

sys/compat/freebsd32/freebsd32_misc.c

Loading...
View Options

sys/kern/kern_umtx.c

Loading...
View Options

sys/sys/proc.h

Loading...
View Options

sys/sys/syscallsubr.h

Loading...
View Options

sys/sys/umtx.h

Loading...