From owner-freebsd-hackers@FreeBSD.ORG Mon Jun 10 17:27:34 2013 Return-Path: Delivered-To: freebsd-hackers@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) by hub.freebsd.org (Postfix) with ESMTP id 984419FC; Mon, 10 Jun 2013 17:27:34 +0000 (UTC) (envelope-from luigi@onelab2.iet.unipi.it) Received: from onelab2.iet.unipi.it (onelab2.iet.unipi.it [131.114.59.238]) by mx1.freebsd.org (Postfix) with ESMTP id 555E9173F; Mon, 10 Jun 2013 17:27:34 +0000 (UTC) Received: by onelab2.iet.unipi.it (Postfix, from userid 275) id B21167300B; Mon, 10 Jun 2013 19:30:36 +0200 (CEST) Date: Mon, 10 Jun 2013 19:30:36 +0200 From: Luigi Rizzo To: Ermal Lu?i Subject: Re: [PATCH] multiple instances of ipfw(4) Message-ID: <20130610173036.GA916@onelab2.iet.unipi.it> References: <20120131110204.GA95472@onelab2.iet.unipi.it> <20120208133559.GK13554@FreeBSD.org> <20120208140921.GM13554@glebius.int.ru> <4F344CE4.301@freebsd.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.20 (2009-06-14) Cc: freebsd-net , freebsd-hackers@freebsd.org X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 10 Jun 2013 17:27:34 -0000 On Mon, Jun 10, 2013 at 06:52:01PM +0200, Ermal Lu?i wrote: > On Mon, Jun 10, 2013 at 5:01 PM, Luigi Rizzo wrote: ... > > if i understand well, this has no runtime overhead as the ifp has > > the index of the context it refers to ? > > Or you need an additional IPFW_CTX_RLOCK() ? > > > > Theoretically you would need for correctness the read lock. > It has never been hit in pfSense hence no further investigation on it has > been done. > It can be made even a read mostly lock or to prevent the race the write > lock > of the pfil hooks so no packets are passed through?! adding another lock (even just a read lock) around invocations is undesirable in my opinion. I'd rather check if there is already some other lock which is already held so we can use it to protect the list of contexts. > > Comments on the control/config path: > > - in ipfw_ctl(), handling IP_FW_CTX_GET i am worried that you might > > overflow the temporary buffer when building the list. You compute > > the length under rlock, release the lock, malloc(), then fill the > > list without checking if the total size is still correct. > > This kind of code is terribly boring to write, but essentially > > you need a bound check in the second loop and possibly > > retry if you notice that you need more memory. > > "ipfw show" addresses the problem by failing and requesting the > > user application to pass a larger buffer. > > > > Yeah that probably can be fixed. > During implementation it was considered enough rare operation to not > justify further thought. well, unlike the previous problem (locking), this has a very simple fix and no performance implications so there are really no excuses... > If you agree with the above i can redo the patch again with the above > changes for review? i would just be happy with the fix to IP_FW_CTX_GET and a big red flashing comment in the place where the context is being accessed. Or if you can find another lock to recycle, fine. cheers luigi