Address the fact that errors from `copyout(9)` are not properly percolated up on failure

[ngie]

Summary

Not properly reporting this error can result in values "returned" from
sbytes being misinterpreted by the caller.

MFC after: 2 weeks
Sponsored by: Netflix, Inc

Test Plan

I ran the sendfile testcases in https://github.com/freebsd/freebsd/pull/243 successfully
against this commit, e.g.,

$ kyua test -k /usr/tests/lib/libc/sys/Kyuafile sendfile_test

[ngie]

@emaste, @glebius, @imp, @jtl: ping?

[emaste]

I would like @imp or @glebius review

[imp]

sys/kern/kern_sendfile.c

1012

Why are EAGAIN and/or EINTR special?

[ngie]

sys/kern/kern_sendfile.c

1012

Apologies for not making this clearer in the review description.

I want to ensure that if sendfile fails with a non-fatal error (EAGAIN/EINTR), the error isn't squashed by the copyout operation failing.

This was a conclusion that I made based on our discussion on the GitHub PR I submitted a few months ago:

@markjdb: How can that be? An error from the copyout() will now be returned when it wasn't before.
I think @bsdimp was referring to the sizeof(off_t) -> sizeof(bytes) portion of the change. The reason why I proposed that change is that it feels really wrong doing a sizeof of a type, which can change for sbytes in the future, and the copyout will break.

@ngie-eign: In terms of the issue, here's a catch with this diff that I'm not comfortable with and I'm going back and forth with @glebius on: if the operation before the copyout fails, but the copyout succeeds, the copyout succeeding will mask the earlier failure.
@ngie-eign: I think it's better to expose the copyout error as long as it doesn't mask errors outside of EAGAIN/EINTR.

Sidenote: reading through the requirements again, it seems that EBUSY would need this treatment as well as it says "Partial data may have been sent":

[EBUSY]            A busy page was encountered and SF_NODISKIO had been
                   specified.  Partial data may have been sent.

[ngie]

sys/kern/kern_sendfile.c

1012

I typoed:

I want to ensure ... non-fatal error (EAGAIN/EINTR) ...

... fatal error (not EAGAIN/EINTR) ...

[ngie]

• Clarify goals/intentions with better comments.
• Handle EBUSY in addition to EAGAIN/EINTR, as sendfile failing with EBUSY should report the proper value of send bytes via sbytes per the sendfile(2) contract.
• Don't mask EBUSY, et al, if copyout(9) succeeds. This can mislead the caller.

[ngie]

@glebius, @imp, @markj: this change is ready for review again.

[ngie]

Clarify the first comment further

[ngie]

*sigh* ... reading the contract again: what about EIO, ENOBUFS, and EPIPE? I'm making this more complicated than it needs to be. I should just return the error (if there is one) or the copyout error if there was one (squashing the other error, if there was one, because EFAULT is a pretty rare error to run into with a user buffer).

[ngie]

Implement the less special case solution.

[markj]

sys/kern/kern_sendfile.c

1011

This should be declared at the beginning of the function per style(9).

1018

The comment is formatted oddly, and doesn't seem very useful to me. It's pretty clear what the code is doing, and whether or not copyout() fails depends entirely on whether the caller provided a valid address.

1020

Is there some justification for overwriting a non-zero error with copyout()'s error?

[ngie]

sys/kern/kern_sendfile.c

1018

Yeah... I shouldn't have been split across 3 lines. Fixed to be just 2 lines.

As to the comment itself, I'll remove it (you're right -- it's overkill).

1020

Some of the APIs state that they will return the number of bytes sent in sbytes, if interrupted, etc. What if these errors are triggered, but an EFAULT issue occurs because a bad address is passed in? It would make the value returned via sbytes invalid/misleading (hence, the need for the revision).

Per copyout(9), EFAULT is the only error that should be returned on failure:

RETURN VALUES
     The copy functions return 0 on success.  All but copystr() return EFAULT
     if a bad address is encountered.  The copyin_nofault() and
     copyout_nofault() functions return EFAULT if a page fault occurs.  The
     copystr() and copyinstr() functions return ENAMETOOLONG if the string is
     longer than len bytes.

[ngie]

Update per comments from @markj

[ngie]

@emaste, @jtl, @markj: ping?

[markj]

sys/kern/kern_sendfile.c

1020

But if a copyout() to sbytes returns EFAULT, then by definition the userspace program cannot access the memory at sbytes.

[ngie]

sys/kern/kern_sendfile.c

1020

Yes. That's the important point; that's why I chose to have the value from copyout overrides the value from the failed sendfile call.

Right now, if data is sent and sendfile(2) fails with EAGAIN, but the user provided a bogus pointer, it could result in programs calling sendfile(2) making decisions about sbytes being valid, whereas the data in sbytes is totally bogus.

[ngie]

After speaking with @markj on IRC, I think the best move forward is to restructure/change the sendfile(2) API to match Linux's a bit more, i.e.,

1. Return the number of sent bytes (ssize_t) or -1 if it failed. This would ensure that we don't lose information about how many bytes were sent if the value for sbytes was bogus, or have to choose between either reporting the error from copyout(9) or the underlying sendfile(2) call.
2. Remove sbytes from the API.
3. Add a compat API so older code can continue calling sendfile with the old [buggy] behavior.

[markj]

In D18623#404374, @ngie wrote:

After speaking with @markj on IRC, I think the best move forward is to restructure/change the sendfile(2) API to match Linux's a bit more, i.e.,

1. Return the number of sent bytes (ssize_t) or -1 if it failed. This would ensure that we don't lose information about how many bytes were sent if the value for sbytes was bogus, or have to choose between either reporting the error from copyout(9) or the underlying sendfile(2) call.
2. Remove sbytes from the API.
3. Add a compat API so older code can continue calling sendfile with the old [buggy] behavior.

To be clear, this approach induces a certain amount of pain, and it should not be undertaken unless the benefits are worth it. Some justification based on actual users of sendfile() is needed. If it's not worth it, then this problem is just a wart.

sys/kern/kern_sendfile.c

1020

If copyout() fails, an attempt by the program to access the data in sbytes will cause a segfault. Do you have a real-world program to demonstrate your concern?

[bdrewery]

sys/kern/kern_sendfile.c

1020

IMHO EFAULT on sbytes should not overwrite any other error. Anything that errored before this line is likely to be more significant in terms of why it failed. Trying to identify a blacklist of errors to not overwrite is risk prone; overwriting EAGAIN would be really bad. Less complexity is better so avoiding any kind of special list is good.

Consider the bigger picture here though.
Bad code that doesn't check the return value of sendfile will just crash if sbytes is invalid. Or it doesn't care about sbytes and works fine. Yes it's bad code but it works? Now forcing an error could break existing code for the sake of something that was going to crash anyway.

[ngie]

sys/kern/kern_sendfile.c

1020

The "real-world" program I have (so far) is the testcase that was just committed in rS343362.

1020

@bdrewery: Bad code that doesn't check the return value of sendfile will just crash if sbytes is invalid. Or it doesn't care about sbytes and works fine. Yes it's bad code but it works? Now forcing an error could break existing code for the sake of something that was going to crash anyway.

This is a good assumption to operate by, in most cases, but it gets marginally harder when dealing with programs that might concurrently modify memory in an malloc'ed arena, poorly (bad locking or pointer errors are what come to mind). I'm mentioning this because I saw similar issues at my most recent employer, just on Linux (and in this case, with libaio, et al). It was even more difficult because the top-layer implementation used C++, but the underlying code was written in C, and the developers/PEs weren't expected to understand or write C code :/...

I'm advocating/optimizing for better developer experience with sendfile(2) and to ensure that the sendfile(2) API contract doesn't become overly conditional, e.g., will return -1/EFAULT, unless you provided me with bogus storage for sbytes, then it may either return -1/EFAULT if there wasn't an error, or crash when you access the memory pointed to by sbytes :(.

[glebius]

To be clear, this approach induces a certain amount of pain, and it should not be undertaken unless the benefits are worth it. Some justification based on actual users of sendfile() is needed. If it's not worth it, then this problem is just a wart.

I agree with Mark. Usually if copyout fails it means that calling process is in such a bad state, that failed sendfile is a lesser problem.

[glebius]

2. Remove sbytes from the API.

No, this isn't going to work. Changing 20 year old API due to theoretical error. Let me remind that no one observed it in the wild.

I'd suggest the following. Add a note in the sendfile.2 manual page that this problem is potentially possible. Paranoid users of the API are advised to initialize sbytes to bogus value (e.g. -1) prior to syscall and after successful syscall (retval is 0 or EAGAIN) check if value changed at all.

[ngie]

In D18623#404659, @glebius wrote:

2. Remove sbytes from the API.

No, this isn't going to work. Changing 20 year old API due to theoretical error. Let me remind that no one observed it in the wild.

I'd suggest the following. Add a note in the sendfile.2 manual page that this problem is potentially possible. Paranoid users of the API are advised to initialize sbytes to bogus value (e.g. -1) prior to syscall and after successful syscall (retval is 0 or EAGAIN) check if value changed at all.

Ok. I'll document the issue in a BUGS section :/.

[ngie]

Abandoning this diff in favor of the documentation clarification in rS343444.