From owner-svn-src-head@freebsd.org Thu Feb 21 18:49:14 2019 Return-Path: Delivered-To: svn-src-head@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 D5DE214EC36E; Thu, 21 Feb 2019 18:49:13 +0000 (UTC) (envelope-from hps@selasky.org) Received: from mail.turbocat.net (turbocat.net [IPv6:2a01:4f8:c17:6c4b::2]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 68A9E88347; Thu, 21 Feb 2019 18:49:13 +0000 (UTC) (envelope-from hps@selasky.org) Received: from hps2016.home.selasky.org (unknown [176.74.212.121]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.turbocat.net (Postfix) with ESMTPSA id D0694260296; Thu, 21 Feb 2019 19:49:10 +0100 (CET) Subject: Re: svn commit: r344099 - head/sys/net To: John Baldwin , 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> <89d15ffe-1bc9-adaf-9307-4bf6541cc5e1@FreeBSD.org> <1dcae85d-2a3a-1e7c-4692-c62f87020096@FreeBSD.org> From: Hans Petter Selasky Message-ID: <47882eda-b33c-9577-14b3-5cd2402af5e7@selasky.org> Date: Thu, 21 Feb 2019 19:46:44 +0100 User-Agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:60.0) Gecko/20100101 Thunderbird/60.4.0 MIME-Version: 1.0 In-Reply-To: <1dcae85d-2a3a-1e7c-4692-c62f87020096@FreeBSD.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit X-Rspamd-Queue-Id: 68A9E88347 X-Spamd-Bar: ------ Authentication-Results: mx1.freebsd.org X-Spamd-Result: default: False [-6.94 / 15.00]; NEURAL_HAM_MEDIUM(-1.00)[-1.000,0]; NEURAL_HAM_SHORT(-0.94)[-0.945,0]; REPLY(-4.00)[]; NEURAL_HAM_LONG(-1.00)[-1.000,0] X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 21 Feb 2019 18:49:14 -0000 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 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