Date: Fri, 31 Aug 2018 00:00:23 +0200 From: "Kristof Provost" <kp@FreeBSD.org> To: "Jonathan T. Looney" <jtl@FreeBSD.org> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r337776 - head/sys/netinet6 Message-ID: <7C9F31C2-839C-4537-A43D-F51A54380BDA@FreeBSD.org> In-Reply-To: <201808141717.w7EHHcPf010971@repo.freebsd.org> References: <201808141717.w7EHHcPf010971@repo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?7C9F31C2-839C-4537-A43D-F51A54380BDA>