sendfile(2): rework compat implementation
AcceptedPublic
Actions

Authored by brooks on Thu, Aug 20, 9:16 AM.

Details

Reviewers

kib
jhb
glebius
markj

Group Reviewers

cheri

Summary

Rename the sendfile() function to kern_sendfile() and extend it with two
function pointer arguments to copy in the header/trailer structure and
the create uio's for the header and trailer as required. Use this
to allow the removal of freebsd32_do_sendfile() which was a nearly
idential duplicate of sendfile() with attendant synchronization cost.

In the process, restyle calls to kern_sendfile to use compound literals.

Effort: CHERI upstreaming
Sponsored by: Innovate UK

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 75779
Build 72662: arc lint + arc unit

Event Timeline

brooks created this revision.Thu, Aug 20, 9:16 AM

Herald added subscribers: olce, imp. · View Herald TranscriptThu, Aug 20, 9:16 AM

brooks requested review of this revision.Thu, Aug 20, 9:16 AM

Harbormaster completed remote builds in B75779: Diff 184445.Thu, Aug 20, 9:16 AM

Herald added a reviewer: cheri. · View Herald TranscriptThu, Aug 20, 9:16 AM

Herald added a reviewer: cheri. · View Herald Transcript

Merging the implementations will avoid future instances of oversights like the one fixed in 4b17776d9afd0009ac8547126c59c97eda0f3fc9 as well as avoiding a third copy of this code from freebsd64 when it lands.

markj added inline comments.Thu, Aug 20, 1:36 PM

sys/compat/freebsd32/freebsd32_misc.c
2201	Should the first parameter just be `const void *`? Otherwise it looks like we're calling this function via an incompatible function pointer. I'm not sure if that's UB.
2213
sys/kern/kern_sendfile.c
1250	Can we handle this in a COMPAT_FREEBSD4-specific copyinuio callback? That seems cleaner, and then you can drop the `compat` flag.

emaste added a subscriber: emaste.Thu, Aug 20, 1:38 PM

Why passing struct sendile_args to kern_sendfile()? This is not how all other kern_* wrappers are done, and IMO is worse than flattening the args. You have to explicitly initialize the literal anyway, which makes it equivalent to list the function's argument in the call.

freebsd32_copyin_hdtr: take a void *, fix trailer count
make copyinuio_t take a void * (but don't update the functions yet)

Harbormaster completed remote builds in B76139: Diff 185018.Tue, Aug 25, 9:56 AM

In D59034#1354663, @kib wrote:

Why passing struct sendile_args to kern_sendfile()? This is not how all other kern_* wrappers are done, and IMO is worse than flattening the args. You have to explicitly initialize the literal anyway, which makes it equivalent to list the function's argument in the call.

It's the lowest churn change since that's how sendfile() worked, but I could flatten the arguments (or use a different structure that would benefit slightly from default initialization.)

sys/compat/freebsd32/freebsd32_misc.c
2201	I can't imagine a compiler where it mattered (particularly since it's only used by copyin, which is assembly). A type-aware ABI is certainly possible, but would be a bit crazy. In this case I've followed the pattern of copyinuio.
sys/kern/kern_sendfile.c
1250	I don't see an easy way to do this as we're not modifying the uio, but instead updating `nbytes`. I guess I could do something like: if (update_nbytes != NULL) update_nbytes(&nbytes, uio); That would at least move it out of the main code path at the cost of having to expose the callback globally for the compat code. On a somewhat related note, I can't help but wonder if we should be returning EINVAL for `uap->nbytes < hdr_uio->uio_resid`.

markj added inline comments.Tue, Aug 25, 5:58 PM

sys/kern/kern_sendfile.c
1250	Couldn't you could provide a sendfile-specific copyinuio wrapper which takes the `uap` as a parameter and updates `nbytes`? On a somewhat related note, I can't help but wonder if we should be returning EINVAL for uap->nbytes < hdr_uio->uio_resid. In the compat case you mean? The `uap->nbytes == 0` case is special, so you'd have to exclude that at least, but it seems wrong to make the compat code stricter in any case.

brooks added inline comments.Wed, Aug 26, 12:39 PM

sys/kern/kern_sendfile.c
1250	Couldn't you could provide a sendfile-specific copyinuio wrapper which takes the `uap` as a parameter and updates `nbytes`? I've created D59198 which does roughly that. If it's what you're thinking of, I'll squash into this one.