svn commit: r344099 - head/sys/net

[Randall Stewart]

> On Feb 13, 2019, at 1:10 PM, John Baldwin <jhb at freebsd.org> wrote:
> 
> 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.

But thats what the lagg’s 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’t see how it would panic.

R

> 
>> 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

------
Randall Stewart
rrs at netflix.com