svn commit: r344099 - head/sys/net

[John Baldwin]

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 <rrs at netflix.com> 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 <jhb at freebsd.org> 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 at netflix.com
>>
>>
>>
> 
> ------
> Randall Stewart
> rrs at netflix.com
> 
> 
> 


-- 
John Baldwin