From owner-svn-src-all@freebsd.org Wed Feb 13 18:11:21 2019 Return-Path: Delivered-To: svn-src-all@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 CF50214D4367; Wed, 13 Feb 2019 18:11:20 +0000 (UTC) (envelope-from jhb@FreeBSD.org) Received: from smtp.freebsd.org (smtp.freebsd.org [IPv6:2610:1c1:1:606c::24b:4]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) server-signature RSA-PSS (4096 bits) client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "smtp.freebsd.org", Issuer "Let's Encrypt Authority X3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 6E3AE6E59F; Wed, 13 Feb 2019 18:11:20 +0000 (UTC) (envelope-from jhb@FreeBSD.org) Received: from John-Baldwins-MacBook-Pro-3.local (ralph.baldwin.cx [66.234.199.215]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client did not present a certificate) (Authenticated sender: jhb) by smtp.freebsd.org (Postfix) with ESMTPSA id C56D31D47B; Wed, 13 Feb 2019 18:11:19 +0000 (UTC) (envelope-from jhb@FreeBSD.org) Subject: Re: svn commit: r344099 - head/sys/net To: Randall Stewart Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org References: <201902131457.x1DEvx9V051533@repo.freebsd.org> <99453977-0f52-6050-3f40-e0fd7ea43d7f@FreeBSD.org> <80314D46-6FEC-462D-8EC5-FCE1ECFF81EF@netflix.com> From: John Baldwin Openpgp: preference=signencrypt Autocrypt: addr=jhb@FreeBSD.org; keydata= mQGiBETQ+XcRBADMFybiq69u+fJRy/0wzqTNS8jFfWaBTs5/OfcV7wWezVmf9sgwn8TW0Dk0 c9MBl0pz+H01dA2ZSGZ5fXlmFIsee1WEzqeJzpiwd/pejPgSzXB9ijbLHZ2/E0jhGBcVy5Yo /Tw5+U/+laeYKu2xb0XPvM0zMNls1ah5OnP9a6Ql6wCgupaoMySb7DXm2LHD1Z9jTsHcAQMD /1jzh2BoHriy/Q2s4KzzjVp/mQO5DSm2z14BvbQRcXU48oAosHA1u3Wrov6LfPY+0U1tG47X 1BGfnQH+rNAaH0livoSBQ0IPI/8WfIW7ub4qV6HYwWKVqkDkqwcpmGNDbz3gfaDht6nsie5Z pcuCcul4M9CW7Md6zzyvktjnbz61BADGDCopfZC4of0Z3Ka0u8Wik6UJOuqShBt1WcFS8ya1 oB4rc4tXfSHyMF63aPUBMxHR5DXeH+EO2edoSwViDMqWk1jTnYza51rbGY+pebLQOVOxAY7k do5Ordl3wklBPMVEPWoZ61SdbcjhHVwaC5zfiskcxj5wwXd2E9qYlBqRg7QeSm9obiBCYWxk d2luIDxqaGJARnJlZUJTRC5vcmc+iGAEExECACAFAkTQ+awCGwMGCwkIBwMCBBUCCAMEFgID AQIeAQIXgAAKCRBy3lIGd+N/BI6RAJ9S97fvbME+3hxzE3JUyUZ6vTewDACdE1stFuSfqMvM jomvZdYxIYyTUpC5Ag0ERND5ghAIAPwsO0B7BL+bz8sLlLoQktGxXwXQfS5cInvL17Dsgnr3 1AKa94j9EnXQyPEj7u0d+LmEe6CGEGDh1OcGFTMVrof2ZzkSy4+FkZwMKJpTiqeaShMh+Goj XlwIMDxyADYvBIg3eN5YdFKaPQpfgSqhT+7El7w+wSZZD8pPQuLAnie5iz9C8iKy4/cMSOrH YUK/tO+Nhw8Jjlw94Ik0T80iEhI2t+XBVjwdfjbq3HrJ0ehqdBwukyeJRYKmbn298KOFQVHO EVbHA4rF/37jzaMadK43FgJ0SAhPPF5l4l89z5oPu0b/+5e2inA3b8J3iGZxywjM+Csq1tqz hltEc7Q+E08AAwUIAL+15XH8bPbjNJdVyg2CMl10JNW2wWg2Q6qdljeaRqeR6zFus7EZTwtX sNzs5bP8y51PSUDJbeiy2RNCNKWFMndM22TZnk3GNG45nQd4OwYK0RZVrikalmJY5Q6m7Z16 4yrZgIXFdKj2t8F+x613/SJW1lIr9/bDp4U9tw0V1g3l2dFtD3p3ZrQ3hpoDtoK70ioIAjjH aIXIAcm3FGZFXy503DOA0KaTWwvOVdYCFLm3zWuSOmrX/GsEc7ovasOWwjPn878qVjbUKWwx Q4QkF4OhUV9zPtf9tDSAZ3x7QSwoKbCoRCZ/xbyTUPyQ1VvNy/mYrBcYlzHodsaqUDjHuW+I SQQYEQIACQUCRND5ggIbDAAKCRBy3lIGd+N/BCO8AJ9j1dWVQWxw/YdTbEyrRKOY8YZNwwCf afMAg8QvmOWnHx3wl8WslCaXaE8= Message-ID: <89d15ffe-1bc9-adaf-9307-4bf6541cc5e1@FreeBSD.org> Date: Wed, 13 Feb 2019 10:10:54 -0800 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:60.0) Gecko/20100101 Thunderbird/60.5.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit X-Rspamd-Queue-Id: 6E3AE6E59F X-Spamd-Bar: ------ Authentication-Results: mx1.freebsd.org X-Spamd-Result: default: False [-6.98 / 15.00]; NEURAL_HAM_MEDIUM(-1.00)[-0.999,0]; NEURAL_HAM_SHORT(-0.98)[-0.982,0]; REPLY(-4.00)[]; NEURAL_HAM_LONG(-1.00)[-1.000,0] X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 13 Feb 2019 18:11:21 -0000 On 2/13/19 10:03 AM, Randall Stewart wrote: > oh and one other thing.. > > It was *not* a random IFP.. it was the IFP to the lagg. > > I.e. an alloc() was done to the lagg.. and the free was > done back to the same IFP (that provided the allocate). 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. 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. > R > >> On Feb 13, 2019, at 1:02 PM, Randall Stewart wrote: >> >> I disagree. If you define an alloc it is only >> reciprocal that you should define a free. >> >> 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… >> >> R >> >> >>> On Feb 13, 2019, at 12:09 PM, John Baldwin wrote: >>> >>> 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 >>>> >>>> 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). >>>> >>>> Sponsored by: Netflix Inc. >>>> Differential Revision: https://reviews.freebsd.org/D19032 >>> >>> 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. >>> >>> 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. >>> >>> -- >>> John Baldwin >>> >>> >> >> ------ >> Randall Stewart >> rrs@netflix.com >> >> >> > > ------ > Randall Stewart > rrs@netflix.com > > > -- John Baldwin