From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ido Schimmel Subject: Re: [PATCH net] ipv4: Fix use-after-free when flushing FIB tables Date: Tue, 19 Dec 2017 20:49:13 +0200 Message-ID: <20171219184913.GA18980@splinter> References: <20171218081320.29442-1-idosch@mellanox.com> <20171219.113221.332118764426444996.davem@davemloft.net> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: David Miller , Ido Schimmel , Netdev , "Duyck, Alexander H" , Fengguang Wu , David Ahern , mlxsw@mellanox.com To: Alexander Duyck Return-path: Received: from out3-smtp.messagingengine.com ([66.111.4.27]:43051 "EHLO out3-smtp.messagingengine.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750825AbdLSStS (ORCPT ); Tue, 19 Dec 2017 13:49:18 -0500 Content-Disposition: inline In-Reply-To: Sender: netdev-owner@vger.kernel.org List-ID: On Tue, Dec 19, 2017 at 09:34:16AM -0800, Alexander Duyck wrote: > That seems like unneeded complexity when the issue is just the order > that these were created in versus the order they are freed in. As long > as we always destroy the one containing the alias before the one that > has the actual data we don't need to have a reference count. Basically > the issue is the bring-up and the tear-down order. It isn't something > that really needs a reference count since it would always be either 1 > or 2. My preference would be to just add a comment explaining that > local must always be destroyed before the main trie in order to > guarantee that there are no external references to the data contained > in main when it is freed. > > The one question I have in all this is if I did the bring-up in the > right order in the first place. I'm wondering if local should be where > the combined trie lives instead of main. Local is currently destroyed > after main anyway so I wonder if it wouldn't have been better if > everything lived in local since from what I can tell it looks like we > add rules for local first before we do so in main. The complexity of > that patch would be higher though since the patch would need to be > much larger and touch multiple files. I decided to go with the original patch because it resulted in a very small diff (patch is needed in -stable as well), but I agree with Dave about it not being explicit enough. How about I'll send v2 with a comment and then we can try Alex's suggestion in net-next? Thanks