Date: Thu, 21 Feb 2019 19:46:44 +0100 From: Hans Petter Selasky <hps@selasky.org> To: John Baldwin <jhb@FreeBSD.org>, 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: <47882eda-b33c-9577-14b3-5cd2402af5e7@selasky.org> In-Reply-To: <1dcae85d-2a3a-1e7c-4692-c62f87020096@FreeBSD.org> 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> <1dcae85d-2a3a-1e7c-4692-c62f87020096@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Hi,
On 2/21/19 7:28 PM, John Baldwin wrote:
> 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.
I haven't seen Randall's code, but unless he is using the same ifp to
free the send that that was used to allocate it, then John is right. The
send tag already contains an interface pointer, that should be used when
freeing it.
--HPS
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?47882eda-b33c-9577-14b3-5cd2402af5e7>