From owner-svn-src-all@freebsd.org Tue Oct 10 00:09:51 2017 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 49A9CE3FD59; Tue, 10 Oct 2017 00:09:51 +0000 (UTC) (envelope-from eggnet@gmail.com) Received: from mail-oi0-x232.google.com (mail-oi0-x232.google.com [IPv6:2607:f8b0:4003:c06::232]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 0AD9817BD; Tue, 10 Oct 2017 00:09:51 +0000 (UTC) (envelope-from eggnet@gmail.com) Received: by mail-oi0-x232.google.com with SMTP id j126so42524578oia.10; Mon, 09 Oct 2017 17:09:51 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=8q7VMyLqrSo1ghQTe/Sr6Z65NBz5KrRuWT/vP+5bCqY=; b=mS7RVB2sG/ypS5CtHAgZjt5ZA+75KsuzzRl7mt5CQ8aAbkBF+HQJfd+yAFQWgQ6VYT yKpl/Adwng/uT4zWsD69U+SeeMNu/368yiSScEpbS6lot7gvvMuLJUDEjd4zSEmpsDBD aPCjng083UkX6hLxKj1PZ1jJwHRJcNpaXYh+GVdXAn4tP5g+mO5uAlzTImQtOkxV5QC+ q7fCgnWKTg7bjYV7tz4Sr2glZwm5wgK0Orb8W3o6gzBZTYmBGXiabSPIdn+Ae2cp4GEo reNqittL/5c0J4vKuN4K8/5XM3iRwtXZ5RjDWKRGFrZJcX+Ka3lIveWQ0Kn0a5jZFqUb q7BA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=8q7VMyLqrSo1ghQTe/Sr6Z65NBz5KrRuWT/vP+5bCqY=; b=IqgXqZDUNlr1bxPxrnlh9i+TawLBTUyMuoo1CuXKzQ8A6yNBTvffYQHcalsiXPGqph JT/ObDgrQrQqsHk60CsNiy+UP1rsyf3YMH0O3xMMQnvAGSuZgH9CYT8qmCHxUIcx9aBx PjQ5UPwnoysbqTq5374zSBl8erNJkUDjrKQpbyM2H6bDnJ3iDFhltEnNnH9/1PZ8aK5t CHlLaEfbNx6oXTN1HUsDW1aqxumgwNnaKUkxydg7MvhDOjt1S/jI6Sc8Pbmgs+KheQjc 7nvM0GG708FVRU2kq2migz08Kwmxd5zODLxs/qEBQyXIEwqyJ9g3/WR0FPmLBf8PQEAy r5WA== X-Gm-Message-State: AMCzsaXk12c86PsIu2Y/x+6KZA1NKrTMGgesIZ8zGdTTtt7kwwwLiYhK nuDlaOvW+AiPALdpsyocjYu75iUevnunLrZsqzKhHm+w X-Google-Smtp-Source: AOwi7QAK9g2fVoOF41lAyBGYgAUaFlbo08mPcAXY1N0v9FuHCBRXieK/s7AAwxdyFU8zkaBtsf7u9IYnUTGX3lP8zIw= X-Received: by 10.157.73.131 with SMTP id g3mr513500otf.451.1507594190136; Mon, 09 Oct 2017 17:09:50 -0700 (PDT) MIME-Version: 1.0 Received: by 10.74.10.211 with HTTP; Mon, 9 Oct 2017 17:09:49 -0700 (PDT) In-Reply-To: References: <201710072330.v97NUv7E040636@repo.freebsd.org> <20171009232052.GU1055@FreeBSD.org> From: Jason Eggleston Date: Mon, 9 Oct 2017 17:09:49 -0700 Message-ID: Subject: Re: svn commit: r324405 - head/sys/kern To: Gleb Smirnoff Cc: Sean Bruno , src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Content-Type: text/plain; charset="UTF-8" X-Content-Filtered-By: Mailman/MimeDel 2.1.23 X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 10 Oct 2017 00:09:51 -0000 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 +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 >> 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 >> > >