From owner-freebsd-hackers@FreeBSD.ORG Wed Jun 2 14:52:05 2004 Return-Path: Delivered-To: freebsd-hackers@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 089D916A4CE for ; Wed, 2 Jun 2004 14:52:05 -0700 (PDT) Received: from mailtoaster1.pipeline.ch (mailtoaster1.pipeline.ch [62.48.0.70]) by mx1.FreeBSD.org (Postfix) with ESMTP id 1674043D46 for ; Wed, 2 Jun 2004 14:52:04 -0700 (PDT) (envelope-from andre@freebsd.org) Received: (qmail 90081 invoked from network); 2 Jun 2004 21:52:02 -0000 Received: from unknown (HELO [62.48.0.47]) ([62.48.0.47]) (envelope-sender ) by mailtoaster1.pipeline.ch (qmail-ldap-1.03) with SMTP for ; 2 Jun 2004 21:52:02 -0000 Message-ID: <40BE4BFE.70204@freebsd.org> Date: Wed, 02 Jun 2004 23:51:58 +0200 From: Andre Oppermann User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7) Gecko/20040514 X-Accept-Language: en-us, en MIME-Version: 1.0 To: "Christian S.J. Peron" References: <20040602043537.GA42327@freefall.freebsd.org> <40BD9D3F.7090100@freebsd.org> <20040602135155.GA31642@freefall.freebsd.org> <40BE4536.4050905@freebsd.org> <20040602213515.GA90619@freefall.freebsd.org> In-Reply-To: <20040602213515.GA90619@freefall.freebsd.org> Content-Type: text/plain; charset=us-ascii; format=flowed Content-Transfer-Encoding: 7bit cc: hackers@freebsd.org cc: ipfw@freebsd.org Subject: Re: ipfw cached ucred patch X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.1 Precedence: list List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 02 Jun 2004 21:52:05 -0000 Christian S.J. Peron wrote: > Agreed, > > This was a concern for me as well, I was pretty carefull about > managing the reference counts, I am currently testing this patch > with a variety of rule types to check for ucred leaks. If/before this patch > gets committed, I plan on doing another carefull scrutinization > of the ipfw code to make sure there are no return, continues or breaks > etc which could cause the ucred to be leaked. This is exactly the problem I try to avoid. If there is the need to double and triple check then I think there is a design error. If that happens to any work I do I start over again unless I'm constrained by external code. For example when I replied to your original email with the patch I read it and said you'd re-do the lookup every time when there is a miss. This is actually not the case. However it is not really obvious what is going on there, even less so from just reading the patch. And I'm well familiar with the ipfw2 code. Only after putting it next to the full ip_fw2.c code and reading it slowly I saw the roundabout of the function and how it does its thing. What I want to say is that it is not obvious how it works and the next one working on this code is almost certainly breaking it or leaking ucreds. I am fully certain that you have tested it in any way to make sure it is correct and I can tell you that it is correct now that I have checked all function returns as well. But this is not write once and let it be but write once, modify often and read all the time. I have stumbled across too many 'short-cuts' in the past year and it made and makes locking the network stack a real pain. I do not object to your patch. I'm just highlighting the grief it might cause later on. ;-) -- Andre