Date: Mon, 28 Dec 2015 10:23:19 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Ian Lepore <ian@freebsd.org> Cc: NGie Cooper <yaneurabeya@gmail.com>, Slawa Olhovchenkov <slw@zxy.spb.ru>, Dmitry Chagin <dchagin@freebsd.org>, src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r292777 - in head: lib/libc/sys sys/kern Message-ID: <20151228093724.D1014@besplex.bde.org> In-Reply-To: <1451247868.1369.16.camel@freebsd.org> References: <201512271537.tBRFb7nN095297@repo.freebsd.org> <1451236237.1369.9.camel@freebsd.org> <20151227184101.GG70867@zxy.spb.ru> <1451243810.1369.10.camel@freebsd.org> <20151227193046.GE4535@zxy.spb.ru> <8D7D617E-FF9E-4D74-87CB-1F3EE65D108A@gmail.com> <1451247868.1369.16.camel@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sun, 27 Dec 2015, Ian Lepore wrote: > On Sun, 2015-12-27 at 12:05 -0800, NGie Cooper wrote: >>> On Dec 27, 2015, at 11:30, Slawa Olhovchenkov <slw@zxy.spb.ru> >>> wrote: >> >> =85 >> >>>> I have no idea what you mean by that -- I didn't say anything at >>>> all >>>> about panic. >>> >>> As I understund commit log -- this is prevent kernel panic at some >>> call (with illegal arguments). This accpetable irrelevant to bugs >>> in >>> calling code. >> >> =09This also makes us POSIX compliant and more compatible with >> Linux: >> http://pubs.opengroup.org/onlinepubs/007908799/xsh/clock_settime.html >> (search for =93negative=94). >> Thanks kib! >> -NGie > > This thread just keeps becoming more and more surrealistic. First > someone tries to reply to the original commit (I guess?) by replying > with a complete non sequitur to my reply. Now you cite a document that > says nothing directly related to the commit at all. > > The only reference to "negative" in what you cited is part of > specifying how to truncate/round fractional values that fall between > two representable values given the resolution of the clock you're > setting. It also has an obfuscated verbose spelling of negative as "less than zero" in the description of [EINVAL]. This is the specification of a invalid timespec which is repeated ad nauseum. The upper limit is spelled even more verbosely as "greater than or equal to 1000 million". The correct spelling of this is ">=3D 1000000000" but that is hard to read in another way (too many 0's to count quickly). Spelling this value is remarkably difficult. There are about 10 different spellings that are no good since they depend on the locale or language (natural or programming). Mixing digits and words is ugly. 1 billion is shorter but is off by a factor of 1000 in some locales. I stared at this description many times. It doesn't allow considering negative times as invalid generally. setitimer(2) has to be specially broken to disallow them. This bug is missing for nanosleep(). FreeBSD still documents a non-POSIX limit of 100000000 seconds for setitimer(2), but its implementation has been broken to overflow instead of enforcing this limit. Note that this is 1 followed by 8 zeros and applies to the seconds value, while the limit for nanoseconds os 1 followed by 9 zeros. Different spellings of 1 followed by a large number of zeros makes thes value hard to grep for. 1 followed by 8 zeros is in about 50 man pages (counting links). It is documented as the limit on seconds in get/setitimer(2). mtree(8) misspells 1 followed by 9 zeros as 1 followed by 8 zeros. alarm(3) is implemented using itimers and documents the same limit. ularm(3) documents the bizarre limit of 1 followed by 14 zeros "in case this value fits in an the unsigned integer". This is alarm(3)'s documented but not actual limit of 10**8 seconds converted to microseconds. It is reachable on systems with >=3D 47 bit longs. This spelling is not used in any man page for the limit on the number of nanoseconds (as in POSIX). > Later in that document they specifically require EINVAL for negative > fractional second values. If they intended to to prohibit negative > whole-second values, that would certainly have been the place to > mention it, and they don't. This is the "obfuscated verbose spelling" part. This is not really a restriction on negative fractions. It is just that negative fractions are represented as a negative integer plus a proper fraction, where by definition a proper fraction is nonnegative and less than 1. Normalization gives this (except when it would overflow). The requirement is essentially that callers don't pass unnormalized values. Bruce From owner-svn-src-head@freebsd.org Sun Dec 27 23:26:52 2015 Return-Path: <owner-svn-src-head@freebsd.org> 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 03318A5288D; Sun, 27 Dec 2015 23:26:52 +0000 (UTC) (envelope-from cy.schubert@komquats.com) Received: from smtp-out-no.shaw.ca (smtp-out-no.shaw.ca [64.59.134.13]) by mx1.freebsd.org (Postfix) with ESMTP id ADC121BF7; Sun, 27 Dec 2015 23:26:51 +0000 (UTC) (envelope-from cy.schubert@komquats.com) Received: from spqr.komquats.com ([96.50.22.10]) by shaw.ca with SMTP id DKi4awFJHkK49DKi5a8KbA; Sun, 27 Dec 2015 16:26:51 -0700 X-Authority-Analysis: v=2.1 cv=AMkI9oPf c=1 sm=1 tr=0 a=jvE2nwUzI0ECrNeyr98KWA==:117 a=jvE2nwUzI0ECrNeyr98KWA==:17 a=BWvPGDcYAAAA:8 a=VxmjJ2MpAAAA:8 a=kj9zAlcOel0A:10 a=wUQvQvOEmiQA:10 a=6I5d2MoRAAAA:8 a=YxBL1-UpAAAA:8 a=hBRiNyo2w7lXQ92ch0sA:9 a=Q-0BtGJeCf-BjTat:21 a=HlmSUPbtQm4zg2Cc:21 a=-x0AuQ5L8kJXOgZ1:21 a=CjuIK1q_8ugA:10 Received: from slippy.cwsent.com (slippy [10.1.1.91]) by spqr.komquats.com (Postfix) with ESMTP id 1A2CB13752; Wed, 23 Dec 2015 16:28:14 -0800 (PST) Received: from slippy (localhost [127.0.0.1]) by slippy.cwsent.com (8.15.2/8.15.2) with ESMTP id tBRNQiQJ008124; Sun, 27 Dec 2015 15:26:44 -0800 (PST) (envelope-from Cy.Schubert@komquats.com) Message-Id: <201512272326.tBRNQiQJ008124@slippy.cwsent.com> X-Mailer: exmh version 2.8.0 04/21/2012 with nmh-1.6 Reply-to: Cy Schubert <Cy.Schubert@komquats.com> From: Cy Schubert <Cy.Schubert@komquats.com> X-os: FreeBSD X-Sender: cy@cwsent.com X-URL: http://www.komquats.com/ To: "George Neville-Neil" <gnn@freebsd.org> cc: "Cy Schubert" <Cy.Schubert@komquats.com>, src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r290383 - in head/sys: net netinet In-Reply-To: Message from "George Neville-Neil" <gnn@freebsd.org> of "Thu, 24 Dec 2015 16:18:52 -0500." <98F16C2B-3904-438D-912B-85C17ACFBDEA@freebsd.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Date: Sun, 27 Dec 2015 15:26:44 -0800 X-CMAE-Envelope: MS4wfMOUhXRNio73hSBoEAxh8H73GDgKkuQlSfWiv3oa+KL9mXRdOE3d5YBkeyRpMNyrlIutFvoiZPrYUzdirsWavZc38P8q8d5OfVZSg9zYqRiH+vwmmkK6 cRtI1orprSSwk66AdLVbMDF2EmpGq91oxjPfp4ocTNsNhWxiDPsHAwBWYW7an+xW0337IfPXoGf+yINy26tWb+04FtqpTRqi84mvsWD5cI8DYJz8izjeCL+p /EGrq4p0w0RF2BS4bvJo8wagkEb35IMANR7/ZmpX1KybbywT5KqpOuzaTF6vr6iU X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: SVN commit messages for the src tree for head/-current <svn-src-head.freebsd.org> List-Unsubscribe: <https://lists.freebsd.org/mailman/options/svn-src-head>, <mailto:svn-src-head-request@freebsd.org?subject=unsubscribe> List-Archive: <http://lists.freebsd.org/pipermail/svn-src-head/>; List-Post: <mailto:svn-src-head@freebsd.org> List-Help: <mailto:svn-src-head-request@freebsd.org?subject=help> List-Subscribe: <https://lists.freebsd.org/mailman/listinfo/svn-src-head>, <mailto:svn-src-head-request@freebsd.org?subject=subscribe> X-List-Received-Date: Sun, 27 Dec 2015 23:26:52 -0000 In message <98F16C2B-3904-438D-912B-85C17ACFBDEA@freebsd.org>, "George Neville- Neil" writes: > > > > On 20 Dec 2015, at 13:02, Cy Schubert wrote: > > > Cy Schubert writes: > >> In message <201511050726.tA57QXlu074213@repo.freebsd.org>, "George V. > >> Neville-N > >> eil" writes: > >>> Author: gnn > >>> Date: Thu Nov 5 07:26:32 2015 > >>> New Revision: 290383 > >>> URL: https://svnweb.freebsd.org/changeset/base/290383 > >>> > >>> Log: > >>> Replace the fastforward path with tryforward which does not require > >>> a > >>> sysctl and will always be on. The former split between default and > >>> fast forwarding is removed by this commit while preserving the > >>> ability > >>> to use all network stack features. > >>> > >>> Differential Revision: https://reviews.freebsd.org/D4042 > >>> Reviewed by: ae, melifaro, olivier, rwatson > >>> MFC after: 1 month > >>> Sponsored by: Rubicon Communications (Netgate) > >>> > >>> Modified: > >>> head/sys/net/if_arcsubr.c > >>> head/sys/net/if_ethersubr.c > >>> head/sys/net/if_fddisubr.c > >>> head/sys/net/if_fwsubr.c > >>> head/sys/net/if_iso88025subr.c > >>> head/sys/netinet/in_var.h > >>> head/sys/netinet/ip_fastfwd.c > >>> head/sys/netinet/ip_input.c > >>> > >>> Modified: head/sys/net/if_arcsubr.c > >>> ========================================================================= > == > >> == > >>> = > >>> --- head/sys/net/if_arcsubr.c Thu Nov 5 04:16:03 2015 (r29038 > >> 2) > >>> +++ head/sys/net/if_arcsubr.c Thu Nov 5 07:26:32 2015 (r29038 > >> 3) > >>> @@ -550,15 +550,11 @@ arc_input(struct ifnet *ifp, struct mbuf > >>> #ifdef INET > >>> case ARCTYPE_IP: > >>> m_adj(m, ARC_HDRNEWLEN); > >>> - if ((m = ip_fastforward(m)) == NULL) > >>> - return; > >>> isr = NETISR_IP; > >>> break; > >>> > >>> case ARCTYPE_IP_OLD: > >>> m_adj(m, ARC_HDRLEN); > >>> - if ((m = ip_fastforward(m)) == NULL) > >>> - return; > >>> isr = NETISR_IP; > >>> break; > >>> > >>> > >>> Modified: head/sys/net/if_ethersubr.c > >>> ========================================================================= > == > >> == > >>> = > >>> --- head/sys/net/if_ethersubr.c Thu Nov 5 04:16:03 2015 (r29038 > >>> 2) > >>> +++ head/sys/net/if_ethersubr.c Thu Nov 5 07:26:32 2015 (r29038 > >>> 3) > >>> @@ -722,8 +722,6 @@ ether_demux(struct ifnet *ifp, struct mb > >>> switch (ether_type) { > >>> #ifdef INET > >>> case ETHERTYPE_IP: > >>> - if ((m = ip_fastforward(m)) == NULL) > >>> - return; > >>> isr = NETISR_IP; > >>> break; > >>> > >>> > >>> Modified: head/sys/net/if_fddisubr.c > >>> ========================================================================= > == > >> == > >>> = > >>> --- head/sys/net/if_fddisubr.c Thu Nov 5 04:16:03 2015 (r29038 > >>> 2) > >>> +++ head/sys/net/if_fddisubr.c Thu Nov 5 07:26:32 2015 (r29038 > >>> 3) > >>> @@ -429,8 +429,6 @@ fddi_input(ifp, m) > >>> switch (type) { > >>> #ifdef INET > >>> case ETHERTYPE_IP: > >>> - if ((m = ip_fastforward(m)) == NULL) > >>> - return; > >>> isr = NETISR_IP; > >>> break; > >>> > >>> > >>> Modified: head/sys/net/if_fwsubr.c > >>> ========================================================================= > == > >> == > >>> = > >>> --- head/sys/net/if_fwsubr.c Thu Nov 5 04:16:03 2015 (r29038 > >> 2) > >>> +++ head/sys/net/if_fwsubr.c Thu Nov 5 07:26:32 2015 (r29038 > >> 3) > >>> @@ -605,8 +605,6 @@ firewire_input(struct ifnet *ifp, struct > >>> switch (type) { > >>> #ifdef INET > >>> case ETHERTYPE_IP: > >>> - if ((m = ip_fastforward(m)) == NULL) > >>> - return; > >>> isr = NETISR_IP; > >>> break; > >>> > >>> > >>> Modified: head/sys/net/if_iso88025subr.c > >>> ========================================================================= > == > >> == > >>> = > >>> --- head/sys/net/if_iso88025subr.c Thu Nov 5 04:16:03 2015 > (r29038 > >>> 2) > >>> +++ head/sys/net/if_iso88025subr.c Thu Nov 5 07:26:32 2015 > (r29038 > >>> 3) > >>> @@ -519,8 +519,6 @@ iso88025_input(ifp, m) > >>> #ifdef INET > >>> case ETHERTYPE_IP: > >>> th->iso88025_shost[0] &= ~(TR_RII); > >>> - if ((m = ip_fastforward(m)) == NULL) > >>> - return; > >>> isr = NETISR_IP; > >>> break; > >>> > >>> > >>> Modified: head/sys/netinet/in_var.h > >>> ========================================================================= > == > >> == > >>> = > >>> --- head/sys/netinet/in_var.h Thu Nov 5 04:16:03 2015 (r29038 > >> 2) > >>> +++ head/sys/netinet/in_var.h Thu Nov 5 07:26:32 2015 (r29038 > >> 3) > >>> @@ -380,7 +380,7 @@ int in_scrubprefix(struct in_ifaddr *, u > >>> void ip_input(struct mbuf *); > >>> void ip_direct_input(struct mbuf *); > >>> void in_ifadown(struct ifaddr *ifa, int); > >>> -struct mbuf *ip_fastforward(struct mbuf *); > >>> +struct mbuf *ip_tryforward(struct mbuf *); > >>> void *in_domifattach(struct ifnet *); > >>> void in_domifdetach(struct ifnet *, void *); > >>> > >>> > >>> Modified: head/sys/netinet/ip_fastfwd.c > >>> ========================================================================= > == > >> == > >>> = > >>> --- head/sys/netinet/ip_fastfwd.c Thu Nov 5 04:16:03 2015 (r29038 > >>> 2) > >>> +++ head/sys/netinet/ip_fastfwd.c Thu Nov 5 07:26:32 2015 (r29038 > >>> 3) > >>> @@ -108,12 +108,6 @@ __FBSDID("$FreeBSD$"); > >>> > >>> #include <machine/in_cksum.h> > >>> > >>> -static VNET_DEFINE(int, ipfastforward_active); > >>> -#define V_ipfastforward_active VNET(ipfastforward_active) > >>> - > >>> -SYSCTL_INT(_net_inet_ip, OID_AUTO, fastforwarding, CTLFLAG_VNET | > >>> CTLFLAG_ > >> RW > >>> , > >>> - &VNET_NAME(ipfastforward_active), 0, "Enable fast IP > >>> forwarding"); > >>> - > >>> static struct sockaddr_in * > >>> ip_findroute(struct route *ro, struct in_addr dest, struct mbuf *m) > >>> { > >>> @@ -158,7 +152,7 @@ ip_findroute(struct route *ro, struct in > >>> * to ip_input for full processing. > >>> */ > >>> struct mbuf * > >>> -ip_fastforward(struct mbuf *m) > >>> +ip_tryforward(struct mbuf *m) > >>> { > >>> struct ip *ip; > >>> struct mbuf *m0 = NULL; > >>> @@ -166,119 +160,20 @@ ip_fastforward(struct mbuf *m) > >>> struct sockaddr_in *dst = NULL; > >>> struct ifnet *ifp; > >>> struct in_addr odest, dest; > >>> - uint16_t sum, ip_len, ip_off; > >>> + uint16_t ip_len, ip_off; > >>> int error = 0; > >>> - int hlen, mtu; > >>> + int mtu; > >>> struct m_tag *fwd_tag = NULL; > >>> > >>> /* > >>> * Are we active and forwarding packets? > >>> */ > >>> - if (!V_ipfastforward_active || !V_ipforwarding) > >>> - return m; > >>> > >>> M_ASSERTVALID(m); > >>> M_ASSERTPKTHDR(m); > >>> > >>> bzero(&ro, sizeof(ro)); > >>> > >>> - /* > >>> - * Step 1: check for packet drop conditions (and sanity checks) > >>> - */ > >>> - > >>> - /* > >>> - * Is entire packet big enough? > >>> - */ > >>> - if (m->m_pkthdr.len < sizeof(struct ip)) { > >>> - IPSTAT_INC(ips_tooshort); > >>> - goto drop; > >>> - } > >>> - > >>> - /* > >>> - * Is first mbuf large enough for ip header and is header present? > >>> - */ > >>> - if (m->m_len < sizeof (struct ip) && > >>> - (m = m_pullup(m, sizeof (struct ip))) == NULL) { > >>> - IPSTAT_INC(ips_toosmall); > >>> - return NULL; /* mbuf already free'd */ > >>> - } > >>> - > >>> - ip = mtod(m, struct ip *); > >>> - > >>> - /* > >>> - * Is it IPv4? > >>> - */ > >>> - if (ip->ip_v != IPVERSION) { > >>> - IPSTAT_INC(ips_badvers); > >>> - goto drop; > >>> - } > >>> - > >>> - /* > >>> - * Is IP header length correct and is it in first mbuf? > >>> - */ > >>> - hlen = ip->ip_hl << 2; > >>> - if (hlen < sizeof(struct ip)) { /* minimum header length */ > >>> - IPSTAT_INC(ips_badhlen); > >>> - goto drop; > >>> - } > >>> - if (hlen > m->m_len) { > >>> - if ((m = m_pullup(m, hlen)) == NULL) { > >>> - IPSTAT_INC(ips_badhlen); > >>> - return NULL; /* mbuf already free'd */ > >>> - } > >>> - ip = mtod(m, struct ip *); > >>> - } > >>> - > >>> - /* > >>> - * Checksum correct? > >>> - */ > >>> - if (m->m_pkthdr.csum_flags & CSUM_IP_CHECKED) > >>> - sum = !(m->m_pkthdr.csum_flags & CSUM_IP_VALID); > >>> - else { > >>> - if (hlen == sizeof(struct ip)) > >>> - sum = in_cksum_hdr(ip); > >>> - else > >>> - sum = in_cksum(m, hlen); > >>> - } > >>> - if (sum) { > >>> - IPSTAT_INC(ips_badsum); > >>> - goto drop; > >>> - } > >>> - > >>> - /* > >>> - * Remember that we have checked the IP header and found it valid. > >>> - */ > >>> - m->m_pkthdr.csum_flags |= (CSUM_IP_CHECKED | CSUM_IP_VALID); > >>> - > >>> - ip_len = ntohs(ip->ip_len); > >>> - > >>> - /* > >>> - * Is IP length longer than packet we have got? > >>> - */ > >>> - if (m->m_pkthdr.len < ip_len) { > >>> - IPSTAT_INC(ips_tooshort); > >>> - goto drop; > >>> - } > >>> - > >>> - /* > >>> - * Is packet longer than IP header tells us? If yes, truncate > >>> packet. > >>> - */ > >>> - if (m->m_pkthdr.len > ip_len) { > >>> - if (m->m_len == m->m_pkthdr.len) { > >>> - m->m_len = ip_len; > >>> - m->m_pkthdr.len = ip_len; > >>> - } else > >>> - m_adj(m, ip_len - m->m_pkthdr.len); > >>> - } > >>> - > >>> - /* > >>> - * Is packet from or to 127/8? > >>> - */ > >>> - if ((ntohl(ip->ip_dst.s_addr) >> IN_CLASSA_NSHIFT) == > >>> IN_LOOPBACKNET || > >>> - (ntohl(ip->ip_src.s_addr) >> IN_CLASSA_NSHIFT) == > >>> IN_LOOPBACKNET) { > >>> - IPSTAT_INC(ips_badaddr); > >>> - goto drop; > >>> - } > >>> > >>> #ifdef ALTQ > >>> /* > >>> @@ -289,12 +184,10 @@ ip_fastforward(struct mbuf *m) > >>> #endif > >>> > >>> /* > >>> - * Step 2: fallback conditions to normal ip_input path processing > >>> - */ > >>> - > >>> - /* > >>> * Only IP packets without options > >>> */ > >>> + ip = mtod(m, struct ip *); > >>> + > >>> if (ip->ip_hl != (sizeof(struct ip) >> 2)) { > >>> if (V_ip_doopts == 1) > >>> return m; > >>> > >>> Modified: head/sys/netinet/ip_input.c > >>> ========================================================================= > == > >> == > >>> = > >>> --- head/sys/netinet/ip_input.c Thu Nov 5 04:16:03 2015 (r29038 > >>> 2) > >>> +++ head/sys/netinet/ip_input.c Thu Nov 5 07:26:32 2015 (r29038 > >>> 3) > >>> @@ -79,6 +79,8 @@ __FBSDID("$FreeBSD$"); > >>> #include <netinet/ip_carp.h> > >>> #ifdef IPSEC > >>> #include <netinet/ip_ipsec.h> > >>> +#include <netipsec/ipsec.h> > >>> +#include <netipsec/key.h> > >>> #endif /* IPSEC */ > >>> #include <netinet/in_rss.h> > >>> > >>> @@ -500,12 +502,22 @@ tooshort: > >>> m_adj(m, ip_len - m->m_pkthdr.len); > >>> } > >>> > >>> + /* Try to forward the packet, but if we fail continue */ > >>> #ifdef IPSEC > >>> + /* For now we do not handle IPSEC in tryforward. */ > >>> + if (!key_havesp(IPSEC_DIR_INBOUND) && > >>> !key_havesp(IPSEC_DIR_OUTBOUND) & > >>> & > >>> + (V_ipforwarding == 1)) > >>> + if (ip_tryforward(m) == NULL) > >>> + return; > >>> /* > >>> * Bypass packet filtering for packets previously handled by IPsec. > >>> */ > >>> if (ip_ipsec_filtertunnel(m)) > >>> goto passin; > >>> +#else > >>> + if (V_ipforwarding == 1) > >>> + if (ip_tryforward(m) == NULL) > >>> + return; > >>> #endif /* IPSEC */ > >>> > >>> /* > >>> > >>> > >> > >> Hi George, > >> > >> Sorry for the lateness of this reply, I finally got some time off for > >> Christmas and have time to myself to boot. > >> > >> This breaks ipfilter's ipnat. I want to let you know before anyone > >> MFCs > >> this. > > > > A fix to ipfilter has been committed to head and will be MFCed in a > > week. > > > > Let me know when that's done. It's been MFCd. Thanks for waiting. -- Cheers, Cy Schubert <Cy.Schubert@komquats.com> or <Cy.Schubert@cschubert.com> FreeBSD UNIX: <cy@FreeBSD.org> Web: http://www.FreeBSD.org The need of the many outweighs the greed of the few.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20151228093724.D1014>