From owner-svn-src-head@freebsd.org Tue Oct 10 13:23:26 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 24CF7E319C8; Tue, 10 Oct 2017 13:23:26 +0000 (UTC) (envelope-from eggnet@gmail.com) Received: from mail-oi0-x235.google.com (mail-oi0-x235.google.com [IPv6:2607:f8b0:4003:c06::235]) (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 D4D12822D6; Tue, 10 Oct 2017 13:23:25 +0000 (UTC) (envelope-from eggnet@gmail.com) Received: by mail-oi0-x235.google.com with SMTP id j126so45097980oia.10; Tue, 10 Oct 2017 06:23:25 -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=w277796g0AvXYWxKgYN1kUUDcKx/aTFLFqN8QIVdVxo=; b=HQ6A6ZHIBRivIMC0307WSXCZvgfqf1S0wiMbHLgTDhToqiUyCwp7J0d0vVSzbdU++o XpECBd2okXWxj+2cD/EtC3ehv7JttdkceaDRK83TwNwtmA3+2LP8ZRv1Vb4NYrtq33bm RzvMQ/Hx0fbrnxrG1KlJ4I5w19i7efVuzGCG7IxaqYsaXJgndqdt0OkEaI+9ph+4Qo7J 2Zh+ORdy28Bfc+jHF5P8KDCFnz3/Y7m/3lEh4BtO85oKsRGxv4YI8KbiTweN1laYTyNG AE1TTvP7AdXqa2p6/5XjYr+QJQ8CsEywvbvV37RvxnrbwLymcqJJXxFavv149LiJf+HK vPnA== 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=w277796g0AvXYWxKgYN1kUUDcKx/aTFLFqN8QIVdVxo=; b=UasJU5gvGHT8dZOWNSlWzBOjjBAJXDWL2tRfwoVqUnS1CXeblViG9gmD92h5lh2qxv 7JIU/V7fL0YNhaLYq9h7rY4dSWWNb4xUnoXJ4Bt0ckaYkxms9MaMNm1drGy5zRHAjYnY KiLzquCmHRQYrNBpdqsrz8XtPOaPvKn1jYEWkFdw/QyLk7+1uHhsvcR3r21RcCB6PWwa f5USZqHonCrX7yOKHTOr8ljczFEI9Z3PP5xIpo4/MD40Y11fLWrWxo5yGPsx+CXirKSl SD77d5V+ljDzMe3/hhL0M/1g2dW4qyAQuNrjPL5Pd5BV4NvYoe1DKUZp6In+ecovaDnX PyOw== X-Gm-Message-State: AMCzsaXRr5B5RaJ8w0uip7X6QyHNLK604B6E9KHHm3K9oNRnI30ZwJHm 6C0BLE5Rhpfc0BQWpRcMYX1H+RsKPYjKT6i9MYk= X-Google-Smtp-Source: AOwi7QAU6JEDfHFdnM7CxELOxzU49hFha6CDKbQV+70UwMb3ef7v06/D+6x74pBRsTKQBzY0fveP6rVExS+mJHabvuA= X-Received: by 10.157.46.207 with SMTP id w73mr265663ota.227.1507641804946; Tue, 10 Oct 2017 06:23:24 -0700 (PDT) MIME-Version: 1.0 Received: by 10.74.10.211 with HTTP; Tue, 10 Oct 2017 06:23:24 -0700 (PDT) Received: by 10.74.10.211 with HTTP; Tue, 10 Oct 2017 06:23:24 -0700 (PDT) In-Reply-To: <20171010071703.GR95911@kib.kiev.ua> References: <201710072330.v97NUv7E040636@repo.freebsd.org> <20171009232052.GU1055@FreeBSD.org> <20171010071703.GR95911@kib.kiev.ua> From: Jason Eggleston Date: Tue, 10 Oct 2017 06:23:24 -0700 Message-ID: Subject: Re: svn commit: r324405 - head/sys/kern To: Konstantin Belousov Cc: Gleb Smirnoff , svn-src-all@freebsd.org, svn-src-head@freebsd.org, Sean Bruno , src-committers@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 13:23:26 -0000 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 +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. > 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 > applications > are already sofisticated enough to block SIGPIPE, but if blocking becomes > a new requirement because EPIPE was not returned in earlier version of > the system, they might not block it still. > > > > > 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 > > > Index: kern_sendfile.c > > =================================================================== > > --- kern_sendfile.c (revision 324457) > > +++ kern_sendfile.c (working copy) > > @@ -507,13 +507,6 @@ sendfile_getsock(struct thread *td, int s, struct > > *so = (*sock_fp)->f_data; > > if ((*so)->so_type != SOCK_STREAM) > > return (EINVAL); > > - if ((*so)->so_error) { > > - error = (*so)->so_error; > > - (*so)->so_error = 0; > > - return (error); > > - } > > - if (((*so)->so_state & SS_ISCONNECTED) == 0) > > - return (ENOTCONN); > > return (0); > > } > > > > @@ -622,6 +615,12 @@ retry_space: > > SOCKBUF_UNLOCK(&so->so_snd); > > goto done; > > } > > + if ((so->so_state & SS_ISCONNECTED) == 0) { > > + SOCKBUF_UNLOCK(&so->so_snd); > > + error = ENOTCONN; > > + goto done; > > + } > > + > > space = sbspace(&so->so_snd); > > if (space < rem && > > (space <= 0 || > >