From owner-svn-src-head@freebsd.org Tue Oct 10 00:01:06 2017 Return-Path: Delivered-To: svn-src-head@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 071ACE3F8A6; Tue, 10 Oct 2017 00:01:06 +0000 (UTC) (envelope-from eggnet@gmail.com) Received: from mail-oi0-x244.google.com (mail-oi0-x244.google.com [IPv6:2607:f8b0:4003:c06::244]) (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 CBFBD1220; Tue, 10 Oct 2017 00:01:05 +0000 (UTC) (envelope-from eggnet@gmail.com) Received: by mail-oi0-x244.google.com with SMTP id f3so14370160oia.2; Mon, 09 Oct 2017 17:01:05 -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=kd+M/rO07n5xVOIwWxq+yjoljlc2j8tL8dCwcZMzWYo=; b=tvPHoJud6jsnfEpSL4N2lMm+7cOpkZkI+HXKTBPNu4zxIwvtpb6pm/vmaClG7Wb2Ku KdgjLLlEl6Fqx29YIElbL98pNjRQAyZwxC8lU5LFd19MO1sH8UGIuMupNZP4PFW+HSve vCLJkVGMHCKSWbk3mj5O6ArM5+4QPmvAOKQlbEmyk8RKhW2e6h+d9WpDM9VgyJ6XPCaL q4KRazI9N4UKoJEkjoZZrZeH8H8jPDYvqvnqfHsupZMvt6ZFodWkZLzm/YbuLUItcQtK e0Tw4ERtCqL8viWESZiOIgvr+bhUtCvvE0RkhI4adcpj3w21Nm3qgbCP2g8/OrUZfkZF KJ7A== 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=kd+M/rO07n5xVOIwWxq+yjoljlc2j8tL8dCwcZMzWYo=; b=nHMxiMB4OA9uTSFP3KrKUb7PrACfPeLBc3Cab4nq5VZxrZz6dGm7ncYG7DacS8jDvL OpUhJxjPSIQQkgAicGKMw1GNpYESNCC4UjKcAZnr0SN6Ge4su4pssWs0SJK49ksuiwMk E2AAF+gPu2cIKuqX0S5UwojCucsM+/ZXjgNLvozqvKY13CYDyupBebkUa03S4ChgDRPD f0UyeyH5tG3cnw003ubnKKVamunZFTQJWoP5KCafx1enn5/yg4+U7WUkFyi3eVR5kDQN JN0EvjWo1HXI171fWUEnDAJGqOMjqKc3RWoVWPA/C3xODqITw+iqZY1uZ9LMEa9s459D SH3A== X-Gm-Message-State: AMCzsaVKqw8TNRzdAPIIrCIpRGL93LL3Mo1/5PFfMNv5eto5pPUqQIuF XRsOFI19MTtOrfRwNdhkEo7+1zCm/EGLnY9y3b8bYdl+ X-Google-Smtp-Source: AOwi7QA3f1tWeMpoWStlq0VJx9XCR1cVVheIeaYD3Xfh+atxUjJJ9dyzoyZlTYLsHjmuH95eI8LtttIp1dFK4bsPGX0= X-Received: by 10.157.63.173 with SMTP id r42mr628849otc.107.1507593664612; Mon, 09 Oct 2017 17:01:04 -0700 (PDT) MIME-Version: 1.0 Received: by 10.74.10.211 with HTTP; Mon, 9 Oct 2017 17:01:03 -0700 (PDT) In-Reply-To: <20171009232052.GU1055@FreeBSD.org> References: <201710072330.v97NUv7E040636@repo.freebsd.org> <20171009232052.GU1055@FreeBSD.org> From: Jason Eggleston Date: Mon, 9 Oct 2017 17:01:03 -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-head@freebsd.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 10 Oct 2017 00:01:06 -0000 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 >