sendfile(2): rework compat implementation

[brooks]

Summary

Rename the sendfile() function to kern_sendfile(), expand the arguments in sendfile_args, and extend 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 identical duplicate of sendfile() with attendant maintenance cost.

Effort: CHERI upstreaming
Sponsored by: Innovate UK

[brooks]

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]

sys/compat/freebsd32/freebsd32_misc.c

2225

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.

2236

sys/kern/kern_sendfile.c

1251

Can we handle this in a COMPAT_FREEBSD4-specific copyinuio callback? That seems cleaner, and then you can drop the compat flag.

[kib]

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.

[brooks]

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

[brooks]

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

2225

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

1251

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]

sys/kern/kern_sendfile.c

1251

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]

sys/kern/kern_sendfile.c

1251

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.

[markj]

sys/kern/kern_sendfile.c

1251

I left a comment on that review. I'm okay with this change as it is.

[kib]

In D59034#1357245, @brooks wrote:

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

I still think it is worth doing.

[brooks]

Expand struct sendfile_args in kern_sendfile signature