From owner-svn-src-head@freebsd.org Thu Aug 30 22:00:28 2018 Return-Path: Delivered-To: svn-src-head@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 8F1BBEF9D70; Thu, 30 Aug 2018 22:00:28 +0000 (UTC) (envelope-from kp@FreeBSD.org) Received: from smtp.freebsd.org (smtp.freebsd.org [IPv6:2610:1c1:1:606c::24b:4]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client CN "smtp.freebsd.org", Issuer "Let's Encrypt Authority X3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 36813742D2; Thu, 30 Aug 2018 22:00:28 +0000 (UTC) (envelope-from kp@FreeBSD.org) Received: from venus.codepro.be (venus.codepro.be [IPv6:2a01:4f8:162:1127::2]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client CN "*.codepro.be", Issuer "Gandi Standard SSL CA 2" (verified OK)) (Authenticated sender: kp) by smtp.freebsd.org (Postfix) with ESMTPSA id B7AEB20B77; Thu, 30 Aug 2018 22:00:27 +0000 (UTC) (envelope-from kp@FreeBSD.org) Received: from [10.0.2.164] (ptr-8rgnodvsrj9z8q8721q.18120a2.ip6.access.telenet.be [IPv6:2a02:1811:240b:b802:9df3:70fc:be86:f61e]) (Authenticated sender: kp) by venus.codepro.be (Postfix) with ESMTPSA id ADFF84BE31; Fri, 31 Aug 2018 00:00:24 +0200 (CEST) From: "Kristof Provost" To: "Jonathan T. Looney" Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r337776 - head/sys/netinet6 Date: Fri, 31 Aug 2018 00:00:23 +0200 X-Mailer: MailMate (2.0BETAr6116) Message-ID: <7C9F31C2-839C-4537-A43D-F51A54380BDA@FreeBSD.org> In-Reply-To: <201808141717.w7EHHcPf010971@repo.freebsd.org> References: <201808141717.w7EHHcPf010971@repo.freebsd.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit X-Content-Filtered-By: Mailman/MimeDel 2.1.27 X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.27 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: Thu, 30 Aug 2018 22:00:28 -0000 On 14 Aug 2018, at 19:17, Jonathan T. Looney wrote: > Author: jtl > Date: Tue Aug 14 17:17:37 2018 > New Revision: 337776 > URL: https://svnweb.freebsd.org/changeset/base/337776 > > Log: > Improve IPv6 reassembly performance by hashing fragments into > buckets. > > Currently, all IPv6 fragment reassembly queues are kept in a flat > linked list. This has a number of implications. Two significant > implications are: all reassembly operations share a common lock, > and it is possible for the linked list to grow quite large. > > Improve IPv6 reassembly performance by hashing fragments into > buckets, > each of which has its own lock. Calculate the hash key using a > Jenkins > hash with a random seed. > > Reviewed by: jhb > Security: FreeBSD-SA-18:10.ip > Security: CVE-2018-6923 > > Modified: > head/sys/netinet6/frag6.c > > Modified: head/sys/netinet6/frag6.c > ============================================================================== > --- head/sys/netinet6/frag6.c Tue Aug 14 17:15:47 2018 (r337775) > +++ head/sys/netinet6/frag6.c Tue Aug 14 17:17:37 2018 (r337776) > @@ -157,12 +176,13 @@ frag6_input(struct mbuf **mp, int *offp, int > proto) > struct mbuf *m = *mp, *t; > struct ip6_hdr *ip6; > struct ip6_frag *ip6f; > - struct ip6q *q6; > + struct ip6q *head, *q6; > struct ip6asfrag *af6, *ip6af, *af6dwn; > struct in6_ifaddr *ia; > int offset = *offp, nxt, i, next; > int first_frag = 0; > int fragoff, frgpartlen; /* must be larger than u_int16_t */ > + uint32_t hash, hashkey[sizeof(struct in6_addr) * 2 + 1], *hashkeyp; I’m pretty sure you didn’t mean for the hashkey to be 1028 bytes long. > struct ifnet *dstifp; > u_int8_t ecn, ecn0; > #ifdef RSS > @@ -231,7 +251,16 @@ frag6_input(struct mbuf **mp, int *offp, int > proto) > return (ip6f->ip6f_nxt); > } > > - IP6Q_LOCK(); > + hashkeyp = hashkey; > + memcpy(hashkeyp, &ip6->ip6_src, sizeof(struct in6_addr)); > + hashkeyp += sizeof(struct in6_addr) / sizeof(*hashkeyp); > + memcpy(hashkeyp, &ip6->ip6_dst, sizeof(struct in6_addr)); > + hashkeyp += sizeof(struct in6_addr) / sizeof(*hashkeyp); > + *hashkeyp = ip6f->ip6f_ident; > + hash = jenkins_hash32(hashkey, nitems(hashkey), V_ip6q_hashseed); Especially combined with this, because it means we hash a lot more than the src/dst dress and ident. We end up hashing random stack garbage, so the hash isn’t guaranteed to be the same every time. So we end up not always being able to reassemble the packet. It’s especially fun when you consider that a Dtrace fbt:::entry probe will leave a consistent stack state, so when you try to probe frag6_input() the problem magically goes away. This broke the sys.netpfil.pf.fragmentation.v6 test. I’ve done this, which fixes the problem: diff --git a/sys/netinet6/frag6.c b/sys/netinet6/frag6.c index 0f30801540a..e1f2b3f5842 100644 --- a/sys/netinet6/frag6.c +++ b/sys/netinet6/frag6.c @@ -218,7 +218,9 @@ frag6_input(struct mbuf **mp, int *offp, int proto) int offset = *offp, nxt, i, next; int first_frag = 0; int fragoff, frgpartlen; /* must be larger than u_int16_t */ - uint32_t hash, hashkey[sizeof(struct in6_addr) * 2 + 1], *hashkeyp; + uint32_t hashkey[(sizeof(struct in6_addr) * 2 + sizeof(u_int32_t)) / + sizeof(uint32_t)]; + uint32_t hash, *hashkeyp; struct ifnet *dstifp; u_int8_t ecn, ecn0; #ifdef RSS Regards, Kristof