Date: Thu, 21 Feb 2019 10:28:02 -0800 From: John Baldwin <jhb@FreeBSD.org> To: Randall Stewart <rrs@netflix.com> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r344099 - head/sys/net Message-ID: <1dcae85d-2a3a-1e7c-4692-c62f87020096@FreeBSD.org> In-Reply-To: <E0A0F0A9-18E2-47B0-9BB3-CF90C666ADA4@netflix.com> References: <201902131457.x1DEvx9V051533@repo.freebsd.org> <99453977-0f52-6050-3f40-e0fd7ea43d7f@FreeBSD.org> <80314D46-6FEC-462D-8EC5-FCE1ECFF81EF@netflix.com> <DC8566A0-7C0E-41E9-AA6A-7CDCE58976B4@netflix.com> <89d15ffe-1bc9-adaf-9307-4bf6541cc5e1@FreeBSD.org> <E0A0F0A9-18E2-47B0-9BB3-CF90C666ADA4@netflix.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On 2/21/19 7:29 AM, Randall Stewart wrote:
>
>
>> On Feb 13, 2019, at 1:10 PM, John Baldwin <jhb@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.
My point is that your calling code should do this.
Suppose I have a socket over cc0. It allocates a send tag. Later, the
connection is rerouted over em0 and em(4) doesn't support send tags at all.
If you use the ifp from the route to free the tag, you will try to use
em's if_snd_tag_free routine and that will be NULL and panic. The fix is
that you shouldn't be using the route ifp to free the tag, only to note that
the tag is stale, so something like:
ifp = route.rt_ifp;
if (ifp != m->m_snd_tag->ifp) {
/* Need to free tag. */
tag = m->m_snd_tag;
/* Free mbuf or clear tag or whatever */
tag->ifp->if_snd_tag_free(tag);
}
if_snd_tag_free should always be called from 'tag->ifp' as that's the ifp
that owns that tag and knows how to free it.
Your patch happens to work for the case of the route going over a lagg and
the connection switching between lagg ports only because you've made the
lagg routine do what the calling code should be doing in the first place.
--
John Baldwin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?1dcae85d-2a3a-1e7c-4692-c62f87020096>