Date: Thu, 30 Aug 2018 22:07:28 +0000 From: "Bjoern A. Zeeb" <bzeeb-lists@lists.zabbadoz.net> To: "Kristof Provost" <kp@FreeBSD.org> Cc: "Jonathan T. Looney" <jtl@FreeBSD.org>, src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r337776 - head/sys/netinet6 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>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?0793AABF-F027-407B-8ACD-1CA4B67260EE>