From owner-svn-src-all@FreeBSD.ORG Wed Apr 1 23:26:03 2015 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 09FA3E66; Wed, 1 Apr 2015 23:26:03 +0000 (UTC) Received: from relay.mailchannels.net (si-002-i69.relay.mailchannels.net [184.154.112.243]) by mx1.freebsd.org (Postfix) with ESMTP id 86EF7C59; Wed, 1 Apr 2015 23:26:00 +0000 (UTC) X-Sender-Id: duocircle|x-authuser|hippie Received: from smtp5.ore.mailhop.org (ip-10-204-4-183.us-west-2.compute.internal [10.204.4.183]) by relay.mailchannels.net (Postfix) with ESMTPA id A44651D0195; Wed, 1 Apr 2015 23:08:00 +0000 (UTC) X-Sender-Id: duocircle|x-authuser|hippie Received: from smtp5.ore.mailhop.org (smtp5.ore.mailhop.org [10.21.145.197]) (using TLSv1 with cipher DHE-RSA-AES256-SHA) by 0.0.0.0:2500 (trex/5.4.8); Wed, 01 Apr 2015 23:08:00 +0000 X-MC-Relay: Neutral X-MailChannels-SenderId: duocircle|x-authuser|hippie X-MailChannels-Auth-Id: duocircle X-MC-Loop-Signature: 1427929680804:2776705836 X-MC-Ingress-Time: 1427929680804 Received: from c-73-34-117-227.hsd1.co.comcast.net ([73.34.117.227] helo=ilsoft.org) by smtp5.ore.mailhop.org with esmtpsa (TLSv1.2:DHE-RSA-AES256-GCM-SHA384:256) (Exim 4.82) (envelope-from ) id 1YdRjq-0003G8-Rr; Wed, 01 Apr 2015 23:07:59 +0000 Received: from revolution.hippie.lan (revolution.hippie.lan [172.22.42.240]) by ilsoft.org (8.14.9/8.14.9) with ESMTP id t31N7uvQ062422; Wed, 1 Apr 2015 17:07:56 -0600 (MDT) (envelope-from ian@freebsd.org) X-Mail-Handler: DuoCircle Outbound SMTP X-Originating-IP: 73.34.117.227 X-Report-Abuse-To: abuse@duocircle.com (see https://support.duocircle.com/support/solutions/articles/5000540958-duocircle-standard-smtp-abuse-information for abuse reporting information) X-MHO-User: U2FsdGVkX1/kUCmAFny9qAzcRZ0mNxYX Message-ID: <1427929676.82583.103.camel@freebsd.org> Subject: Re: svn commit: r280971 - in head: contrib/ipfilter/tools share/man/man4 sys/contrib/ipfilter/netinet sys/netinet sys/netipsec sys/netpfil/pf From: Ian Lepore To: Gleb Smirnoff Date: Wed, 01 Apr 2015 17:07:56 -0600 In-Reply-To: <201504012226.t31MQedN044443@svn.freebsd.org> References: <201504012226.t31MQedN044443@svn.freebsd.org> Content-Type: text/plain; charset="us-ascii" X-Mailer: Evolution 3.12.10 FreeBSD GNOME Team Port Mime-Version: 1.0 Content-Transfer-Encoding: 7bit X-AuthUser: hippie Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.18-1 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: Wed, 01 Apr 2015 23:26:03 -0000 On Wed, 2015-04-01 at 22:26 +0000, Gleb Smirnoff wrote: > Author: glebius > Date: Wed Apr 1 22:26:39 2015 > New Revision: 280971 > URL: https://svnweb.freebsd.org/changeset/base/280971 > > Log: > o Use new function ip_fillid() in all places throughout the kernel, > where we want to create a new IP datagram. > o Add support for RFC6864, which allows to set IP ID for atomic IP > datagrams to any value, to improve performance. The behaviour is > controlled by net.inet.ip.rfc6864 sysctl knob, which is enabled by > default. > o In case if we generate IP ID, use counter(9) to improve performance. > o Gather all code related to IP ID into ip_id.c. > > Differential Revision: https://reviews.freebsd.org/D2177 > Reviewed by: adrian, cy, rpaulo > Tested by: Emeric POUPON > Sponsored by: Netflix > Sponsored by: Nginx, Inc. > Relnotes: yes > [...] > +void > +ip_fillid(struct ip *ip) > +{ > + > + /* > + * Per RFC6864 Section 4 > + * > + * o Atomic datagrams: (DF==1) && (MF==0) && (frag_offset==0) > + * o Non-atomic datagrams: (DF==0) || (MF==1) || (frag_offset>0) > + */ > + if (V_ip_rfc6864 && (ip->ip_off & htons(IP_DF)) == htons(IP_DF)) > + ip->ip_id = 0; > + else if (V_ip_do_randomid) > + ip->ip_id = ip_randomid(); > + else { > + counter_u64_add(V_ip_id, 1); > + ip->ip_id = htons((*(uint64_t *)zpcpu_get(V_ip_id)) & 0xffff); > + } > +} > + This is completely bogus. It's a big opacity violation (it relies on what should be opaque private internal implementation details of counter(9)). The fact that the counter api doesn't provide a function for retrieving one cpu's counter value should be a big clue there -- the fact that you know the internals doesn't make it okay to reach behind the counter and grab a value like that. It may not even be safe to do so on any given architecture; it certainly isn't safe on arm, and that line of code above will work only by accident because you're throwing way all but 16 bits. But even more importantly, this WILL result in multiple threads using the same value at the same time... - Thread A on CPU 1 and thread B on CPU 2 both begin executing here at the same time, and both get through counter_u64_add(). - Thread A keeps running and uses CPU 1's new value, call it 27. - Thread B gets prempted between counter_u64_add() and zpcpu_get(). When it resumes it's now on CPU 1, so it retrieves value 27 as well. -- Ian