Re: svn commit: r324405 - head/sys/kern
Sendfile does return EPIPE today. All that has to happen is receiving a RST
after sendfile starts looping.
There is a race condition, which still exists after this suggested patch,
where ECONNRESET could be returned. But mostly it is EPIPE.
On Oct 10, 2017 12:17 AM, "Konstantin Belousov" wrote:
> On Mon, Oct 09, 2017 at 04:20:52PM -0700, Gleb Smirnoff wrote:
> > Sean & Jason,
> >
> > On Sat, Oct 07, 2017 at 11:30:57PM +, 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
> > 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.
> Do you mean that EPIPE could be returned from sendfile(2) after some
> round of changes ? It should be not.
>
> EPIPE is a workaround for naive programs that use stdio and cannot detect
> the broken pipe when used as an element in the shell plumbing. EPIPE
> results in SIGPIPE terminating such programs (instead of looping endlessly
> when write(2) returns error).
>
> Any application using sendfile(2) must understand the API to properly
> handle
> disconnects, so SIGPIPE workaround would be avoided. Often such
> appli
Re: svn commit: r324405 - head/sys/kern
https://reviews.freebsd.org/D12633
On Mon, Oct 9, 2017 at 5:01 PM, Jason Eggleston wrote:
> In tcp_input.c, this is where a RST is handled. so_error gets ECONNRESET,
> then tcp_close() is called.
>
> case TCPS_ESTABLISHED:
> case TCPS_FIN_WAIT_1:
> case TCPS_FIN_WAIT_2:
> case TCPS_CLOSE_WAIT:
> case TCPS_CLOSING:
> case TCPS_LAST_ACK:
> so->so_error = ECONNRESET;
> close:
> /* FALLTHROUGH */
> default:
> tp = tcp_close(tp);
>
> tcp_close() calls soisdisconnected() which sets this flag:
>
> so->so_state |= SS_ISDISCONNECTED;
>
> Followed by:
>
> ...
> socantrcvmore_locked(so);
> ...
> socantsendmore_locked(so);
> ...
>
> Those two functions set these flags:
>
> so->so_rcv.sb_state |= SBS_CANTRCVMORE;
> so->so_snd.sb_state |= SBS_CANTSENDMORE;
>
> A TCP session cannot survive a RST, and i/o calls will not work afterword,
> no matter how many times you try.
>
> Having said that, there's a race condition between when so_error is set
> and when the socket and socket buffer flags are set, and I agree with your
> strategy of:
>
> - have sendfile() match send()
> - The current send() behavior is probably fine as is
>
> I agree sendfile() should match write() and send(), by checking in this
> order:
>
> SBS_CANTSENDMORE
> so_error
> SS_ISCONNECTED
>
> and will prep a new review with your patch.
>
> On Mon, Oct 9, 2017 at 4:20 PM, Gleb Smirnoff wrote:
>
>> Sean & Jason,
>>
>> On Sat, Oct 07, 2017 at 11:30:57PM +, 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
>> 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.cSat Oct 7 23:10:16 2017
>> (r324404)
>> S> +++ head/sys/kern/kern_sendfile.cSat 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;
>>
Re: svn commit: r324405 - head/sys/kern
In tcp_input.c, this is where a RST is handled. so_error gets ECONNRESET,
then tcp_close() is called.
case TCPS_ESTABLISHED:
case TCPS_FIN_WAIT_1:
case TCPS_FIN_WAIT_2:
case TCPS_CLOSE_WAIT:
case TCPS_CLOSING:
case TCPS_LAST_ACK:
so->so_error = ECONNRESET;
close:
/* FALLTHROUGH */
default:
tp = tcp_close(tp);
tcp_close() calls soisdisconnected() which sets this flag:
so->so_state |= SS_ISDISCONNECTED;
Followed by:
...
socantrcvmore_locked(so);
...
socantsendmore_locked(so);
...
Those two functions set these flags:
so->so_rcv.sb_state |= SBS_CANTRCVMORE;
so->so_snd.sb_state |= SBS_CANTSENDMORE;
A TCP session cannot survive a RST, and i/o calls will not work afterword,
no matter how many times you try.
Having said that, there's a race condition between when so_error is set and
when the socket and socket buffer flags are set, and I agree with your
strategy of:
- have sendfile() match send()
- The current send() behavior is probably fine as is
I agree sendfile() should match write() and send(), by checking in this
order:
SBS_CANTSENDMORE
so_error
SS_ISCONNECTED
and will prep a new review with your patch.
On Mon, Oct 9, 2017 at 4:20 PM, Gleb Smirnoff wrote:
> Sean & Jason,
>
> On Sat, Oct 07, 2017 at 11:30:57PM +, 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
> 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.cSat Oct 7 23:10:16 2017
> (r324404)
> S> +++ head/sys/kern/kern_sendfile.cSat 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;
>
