From owner-svn-src-head@freebsd.org Thu Feb 21 15:29:51 2019 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 DA27214E4C8D for ; Thu, 21 Feb 2019 15:29:50 +0000 (UTC) (envelope-from rrs@netflix.com) Received: from mail-qk1-x741.google.com (mail-qk1-x741.google.com [IPv6:2607:f8b0:4864:20::741]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id BE32F776A2 for ; Thu, 21 Feb 2019 15:29:49 +0000 (UTC) (envelope-from rrs@netflix.com) Received: by mail-qk1-x741.google.com with SMTP id z13so3173833qki.2 for ; Thu, 21 Feb 2019 07:29:49 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:subject:from:in-reply-to:date:cc :content-transfer-encoding:message-id:references:to; bh=7O8B8NiQCRhhilisquiHkWkqKzojKEJDUHqSyvgYLLM=; b=KRQQUSSk+M3DQVMS0PmerVgM79cJoCgOp9tCrpaUmm/PN1n8BySG/+k4K7/gErircg vzHAY8mNtiqLXVSXEPCZTG6sxnIfcPjSoDNGIsh+2V0UF9TcSnTR+v26rIReR665aou/ bkbO1uje9q9P2pVvEBu8Udl+S1ifMFndGX+BrApeiWLdH0EmjOrKJlTuZ++8ZPHvh/JO NqqUhZSADGS+GI1YlRJ+vsDYNZDo7Bg+IRWrjg5x80TWXDD4Xtj9JQT1LA7FwOaOHiu4 a/XoZuKnRNxuRkafrzwB0AmUem2R4/o9o7Q/ex1CDb2XqYdPr2BepzZ5dxyxviuCegmW 5dBw== X-Gm-Message-State: AHQUAuZnuXpnBAqsZIPkKvwlOEgfmexgviT0OyHexoc13LTcfeTI2elA cSRgljax6GIufFpT4BwQfO1bFA== X-Google-Smtp-Source: AHgI3Iaj6KvBeXPBcqwY01apj5i6ECMvcqH3rYd2DbDu+7+XPP007ZxHo0I9e9R/hJIHG9qwN9WQIA== X-Received: by 2002:a37:a783:: with SMTP id q125mr22998180qke.264.1550762988648; Thu, 21 Feb 2019 07:29:48 -0800 (PST) Received: from ?IPv6:2607:fb10:7061:7fd::8d34? ([2607:fb10:7061:7fd::8d34]) by smtp.gmail.com with ESMTPSA id k27sm2165688qki.19.2019.02.21.07.29.47 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 21 Feb 2019 07:29:47 -0800 (PST) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 11.5 \(3445.9.1\)) Subject: Re: svn commit: r344099 - head/sys/net From: Randall Stewart In-Reply-To: <89d15ffe-1bc9-adaf-9307-4bf6541cc5e1@FreeBSD.org> Date: Thu, 21 Feb 2019 10:29:46 -0500 Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Content-Transfer-Encoding: quoted-printable Message-Id: References: <201902131457.x1DEvx9V051533@repo.freebsd.org> <99453977-0f52-6050-3f40-e0fd7ea43d7f@FreeBSD.org> <80314D46-6FEC-462D-8EC5-FCE1ECFF81EF@netflix.com> <89d15ffe-1bc9-adaf-9307-4bf6541cc5e1@FreeBSD.org> To: John Baldwin X-Mailer: Apple Mail (2.3445.9.1) X-Rspamd-Queue-Id: BE32F776A2 X-Spamd-Bar: ------------- X-Spamd-Result: default: False [-13.61 / 15.00]; TO_DN_SOME(0.00)[]; R_SPF_ALLOW(-0.20)[+ip6:2607:f8b0:4000::/36]; MV_CASE(0.50)[]; RCVD_COUNT_THREE(0.00)[3]; DKIM_TRACE(0.00)[netflix.com:+]; DMARC_POLICY_ALLOW(-0.50)[netflix.com,reject]; MX_GOOD(-0.01)[alt1.aspmx.l.google.com,aspmx.l.google.com,aspmx2.googlemail.com,alt2.aspmx.l.google.com,aspmx3.googlemail.com]; NEURAL_HAM_SHORT(-0.94)[-0.935,0]; FROM_EQ_ENVFROM(0.00)[]; IP_SCORE(-0.17)[ip: (3.82), ipnet: 2607:f8b0::/32(-2.60), asn: 15169(-1.99), country: US(-0.07)]; MIME_TRACE(0.00)[0:+]; ASN(0.00)[asn:15169, ipnet:2607:f8b0::/32, country:US]; RCVD_TLS_LAST(0.00)[]; MID_RHS_MATCH_FROM(0.00)[]; ARC_NA(0.00)[]; NEURAL_HAM_MEDIUM(-1.00)[-1.000,0]; R_DKIM_ALLOW(-0.20)[netflix.com:s=google]; FROM_HAS_DN(0.00)[]; RCPT_COUNT_THREE(0.00)[4]; NEURAL_HAM_LONG(-1.00)[-1.000,0]; MIME_GOOD(-0.10)[text/plain]; PREVIOUSLY_DELIVERED(0.00)[svn-src-head@freebsd.org]; TO_MATCH_ENVRCPT_SOME(0.00)[]; WHITELIST_DMARC(-7.00)[netflix.com:D:+]; RCVD_IN_DNSWL_NONE(0.00)[1.4.7.0.0.0.0.0.0.0.0.0.0.0.0.0.0.2.0.0.4.6.8.4.0.b.8.f.7.0.6.2.list.dnswl.org : 127.0.5.0]; WHITELIST_SPF_DKIM(-3.00)[netflix.com:d:+,netflix.com:s:+] X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.29 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, 21 Feb 2019 15:29:51 -0000 > On Feb 13, 2019, at 1:10 PM, John Baldwin wrote: >=20 > On 2/13/19 10:03 AM, Randall Stewart wrote: >> oh and one other thing.. >>=20 >> It was *not* a random IFP.. it was the IFP to the lagg. >>=20 >> I.e. an alloc() was done to the lagg.. and the free was >> done back to the same IFP (that provided the allocate). >=20 > Yes, that's wrong. Suppose the route changes so that my traffic is = now over > em0 instead of lagg0 (where em0 isn't a member of the lagg), how do = you > expect if_lagg_free to invoke em0's free routine? In your case it = does, > but only by accident. It doesn't work in the other case I described = which > is if you have non-lagg interfaces and a route moves from cc0 to em0. = In > that case your existing code that is using the wrong ifp will just = panic. >=20 > These aren't real alloc routines as the lagg and vlan ones don't = allocate > anything, they pass along the request to the child and the child = allocates > the tag. Only ifnet's that actually allocate tags should need to free = them, > and you should be using tag->ifp to as the ifp whose if_snd_tag_free = works. But thats what the lagg=E2=80=99s routine does, use the tag sent in to find the real ifp (where the tag was allocated) and call the if_snd_tag_free() on that. Its not an accident it works, it calls the free of the actual interface where the allocation came from. I don=E2=80=99t see how it would panic. R >=20 >> R >>=20 >>> On Feb 13, 2019, at 1:02 PM, Randall Stewart = wrote: >>>=20 >>> I disagree. If you define an alloc it is only >>> reciprocal that you should define a free. >>>=20 >>> The code in question that hit this was changed (its in a version >>> of rack that has the rate-limit and TLS code).. but I think these >>> things *should* be balanced.. if you provide an Allocate, you >>> should also provide a Free=E2=80=A6=20 >>>=20 >>> R >>>=20 >>>=20 >>>> On Feb 13, 2019, at 12:09 PM, John Baldwin wrote: >>>>=20 >>>> On 2/13/19 6:57 AM, Randall Stewart wrote: >>>>> Author: rrs >>>>> Date: Wed Feb 13 14:57:59 2019 >>>>> New Revision: 344099 >>>>> URL: https://svnweb.freebsd.org/changeset/base/344099 >>>>>=20 >>>>> Log: >>>>> This commit adds the missing release mechanism for the >>>>> ratelimiting code. The two modules (lagg and vlan) did have >>>>> allocation routines, and even though they are indirect (and >>>>> vector down to the underlying interfaces) they both need to >>>>> have a free routine (that also vectors down to the actual = interface). >>>>>=20 >>>>> Sponsored by: Netflix Inc. >>>>> Differential Revision: https://reviews.freebsd.org/D19032 >>>>=20 >>>> Hmm, I don't understand why you'd ever invoke if_snd_tag_free from = anything >>>> but 'tag->ifp' rather than some other ifp. What if the route for a = connection >>>> moves so that a tag allocated on cc0 is now on a route that goes = over em0? >>>> You can't expect em0 to have an if_snd_tag_free routine that will = know to >>>> go invoke cxgbe's snd_tag_free. I think you should always be using >>>> 'tag->ifp->if_snd_tag_free' to free tags and never using any other = ifp. >>>>=20 >>>> That is, I think this should be reverted and that instead you need = to fix >>>> the code invoking if_snd_tag_free to invoke it on the tag's ifp = instead of >>>> some random other ifp. >>>>=20 >>>> --=20 >>>> John Baldwin >>>>=20 >>>>=20 >>>=20 >>> ------ >>> Randall Stewart >>> rrs@netflix.com >>>=20 >>>=20 >>>=20 >>=20 >> ------ >> Randall Stewart >> rrs@netflix.com >>=20 >>=20 >>=20 >=20 >=20 > --=20 > John Baldwin ------ Randall Stewart rrs@netflix.com