svn commit: r324405 - head/sys/kern

[Gleb Smirnoff]

  Sean & Jason,

On Sat, Oct 07, 2017 at 11:30:57PM +0000, Sean Bruno wrote:
S> Author: sbruno
S> Date: Sat Oct  7 23:30:57 2017
S> New Revision: 324405
S> URL: https://svnweb.freebsd.org/changeset/base/324405
S> 
S> Log:
S>   Check so_error early in sendfile() call.  Prior to this patch, if a
S>   connection was reset by the remote end, sendfile() would just report
S>   ENOTCONN instead of ECONNRESET.
S>   
S>   Submitted by:	Jason Eggleston <jason at eggnet.com>
S>   Reviewed by:	glebius
S>   Sponsored by:	Limelight Networks
S>   Differential Revision:	https://reviews.freebsd.org/D12575
S> 
S> Modified:
S>   head/sys/kern/kern_sendfile.c
S> 
S> Modified: head/sys/kern/kern_sendfile.c
S> ==============================================================================
S> --- head/sys/kern/kern_sendfile.c	Sat Oct  7 23:10:16 2017	(r324404)
S> +++ head/sys/kern/kern_sendfile.c	Sat Oct  7 23:30:57 2017	(r324405)
S> @@ -514,6 +514,11 @@ sendfile_getsock(struct thread *td, int s, struct file
S>  	*so = (*sock_fp)->f_data;
S>  	if ((*so)->so_type != SOCK_STREAM)
S>  		return (EINVAL);
S> +	if ((*so)->so_error) {
S> +		error = (*so)->so_error;
S> +		(*so)->so_error = 0;
S> +		return (error);
S> +	}
S>  	if (((*so)->so_state & SS_ISCONNECTED) == 0)
S>  		return (ENOTCONN);
S>  	return (0);

Despite my initial positive review, now I am quite unsure on that.

Problem is that sendfile(2) isn't defined by SUS, so there is no
distinctive final answer on that. Should we match other OSes?
Should we match our historic behaviour? Or should we match
write(2)/send(2) to socket, which are closest analogy. I probably
believe in the latter: sendfile(2) belongs to write(2)/send(2)
family.

SUS specifies that write may return ECONNRESET. It also documents
EPIPE. However, our manual page documents only EPIPE for both
send(2) and write(2). For write we have:

        SOCKBUF_LOCK(&so->so_snd);
        if (so->so_snd.sb_state & SBS_CANTSENDMORE) {
                SOCKBUF_UNLOCK(&so->so_snd);
                error = EPIPE;
                goto out;
        }
        if (so->so_error) {
                error = so->so_error;
                so->so_error = 0;
                SOCKBUF_UNLOCK(&so->so_snd);
                goto out;
        }

Indeed, EPIPE will be returned prior to return/clear of so_error,
which supposedly is ECONNRESET.

In the sendfile(2) implementation we see exactly same code:

                if (so->so_snd.sb_state & SBS_CANTSENDMORE) {
                        error = EPIPE;
                        SOCKBUF_UNLOCK(&so->so_snd);
                        goto done;
                } else if (so->so_error) {
                        error = so->so_error;
                        so->so_error = 0;
                        SOCKBUF_UNLOCK(&so->so_snd);
                        goto done;
                }

But it isn't reached. Before due to SS_ISCONNECTED check, now
due to your change. Now we got two spots where so_error is
returned/cleared.

For the receive family (read(2) and recv(2)), SUS specifies
ECONNRESET and our code does that. It also clears so_error.

So, after your change at least one standard case is broken: an
application that polls/selects for both readability and
writeability of a socket. It discovers that socket is available
for writing, does sendfile(2), receives ECONNRESET. Then it does
read on the socket, and blocks(?) or returns a bogus error(?),
not sure. But definitely not ECONNRESET, since it is now cleared.

Now, where does this ENOTCONN come from? These two lines:

	if (((*so)->so_state & SS_ISCONNECTED) == 0)
		return (ENOTCONN);

can be traced back all the way to original 1998 commit of sendfile.

Here comes difference between sendfile(2) and send(2). In send(2),
this check is done _after_ so_error check, so your change is correct
in placing of so_error check wrt so_state check, but is incorrect
in placing wrt socket buffer state check.

After all this considerations, I would suggest to revert your change,
and apply this patch. It would make behavior of sendfile(2) to match
send(2). I would appreciate more reviews around it.

-- 
Gleb Smirnoff
-------------- next part --------------
A non-text attachment was scrubbed...
Name: sendfile-error.diff
Type: text/x-diff
Size: 796 bytes
Desc: not available
URL: <http://lists.freebsd.org/pipermail/svn-src-head/attachments/20171009/2ca7ca30/attachment.diff>