From owner-svn-src-head@freebsd.org Thu Aug 30 22:07:52 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 12C8DEFA02C; Thu, 30 Aug 2018 22:07:52 +0000 (UTC) (envelope-from bzeeb-lists@lists.zabbadoz.net) Received: from mx1.sbone.de (mx1.sbone.de [IPv6:2a01:4f8:13b:39f::9f:25]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client CN "mx1.sbone.de", Issuer "SBone.DE" (not verified)) by mx1.freebsd.org (Postfix) with ESMTPS id 8A05074704; Thu, 30 Aug 2018 22:07:51 +0000 (UTC) (envelope-from bzeeb-lists@lists.zabbadoz.net) Received: from mail.sbone.de (mail.sbone.de [IPv6:fde9:577b:c1a9:31::2013:587]) (using TLSv1 with cipher ADH-CAMELLIA256-SHA (256/256 bits)) (No client certificate requested) by mx1.sbone.de (Postfix) with ESMTPS id E66958D4A13F; Thu, 30 Aug 2018 22:07:49 +0000 (UTC) Received: from content-filter.sbone.de (content-filter.sbone.de [IPv6:fde9:577b:c1a9:31::2013:2742]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by mail.sbone.de (Postfix) with ESMTPS id 426FCD1F83F; Thu, 30 Aug 2018 22:07:49 +0000 (UTC) X-Virus-Scanned: amavisd-new at sbone.de Received: from mail.sbone.de ([IPv6:fde9:577b:c1a9:31::2013:587]) by content-filter.sbone.de (content-filter.sbone.de [fde9:577b:c1a9:31::2013:2742]) (amavisd-new, port 10024) with ESMTP id Z-KCP7ALD71y; Thu, 30 Aug 2018 22:07:46 +0000 (UTC) Received: from [192.168.124.1] (fresh-ayiya.sbone.de [IPv6:fde9:577b:c1a9:f001::2]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by mail.sbone.de (Postfix) with ESMTPSA id B2C1DD1F7EB; Thu, 30 Aug 2018 22:07:38 +0000 (UTC) From: "Bjoern A. Zeeb" To: "Kristof Provost" Cc: "Jonathan T. Looney" , src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r337776 - head/sys/netinet6 Date: Thu, 30 Aug 2018 22:07:28 +0000 X-Mailer: MailMate (2.0BETAr6116) Message-ID: <0793AABF-F027-407B-8ACD-1CA4B67260EE@lists.zabbadoz.net> In-Reply-To: <7C9F31C2-839C-4537-A43D-F51A54380BDA@FreeBSD.org> References: <201808141717.w7EHHcPf010971@repo.freebsd.org> <7C9F31C2-839C-4537-A43D-F51A54380BDA@FreeBSD.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: quoted-printable 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:07:52 -0000 On 30 Aug 2018, at 22:00, Kristof Provost wrote: > 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 >> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D >> --- 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 =3D *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 =3D *offp, nxt, i, next; >> int first_frag =3D 0; >> int fragoff, frgpartlen; /* must be larger than u_int16_t */ >> + uint32_t hash, hashkey[sizeof(struct in6_addr) * 2 + 1], *hashkeyp; > > I=E2=80=99m pretty sure you didn=E2=80=99t mean for the hashkey to be 1= 028 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 =3D hashkey; >> + memcpy(hashkeyp, &ip6->ip6_src, sizeof(struct in6_addr)); >> + hashkeyp +=3D sizeof(struct in6_addr) / sizeof(*hashkeyp); >> + memcpy(hashkeyp, &ip6->ip6_dst, sizeof(struct in6_addr)); >> + hashkeyp +=3D sizeof(struct in6_addr) / sizeof(*hashkeyp); >> + *hashkeyp =3D ip6f->ip6f_ident; >> + hash =3D 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=E2=80=99t guara= nteed = > to be the same every time. > So we end up not always being able to reassemble the packet. > > It=E2=80=99s 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=E2=80=99ve 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 =3D *offp, nxt, i, next; > int first_frag =3D 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)) / Can we actually make it the size of the field rather than uint32_t (not = u_int32_t)? I guess not easily but at least change the type spelling = and leave a comment what it is? > + sizeof(uint32_t)]; > + uint32_t hash, *hashkeyp; > struct ifnet *dstifp; > u_int8_t ecn, ecn0; > #ifdef RSS > > Regards, > Kristof