From owner-freebsd-hackers@FreeBSD.ORG Tue Jun 11 11:57:35 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 8FADFC38; Tue, 11 Jun 2013 11:57:35 +0000 (UTC) (envelope-from ermal.luci@gmail.com) Received: from mail-qa0-x232.google.com (mail-qa0-x232.google.com [IPv6:2607:f8b0:400d:c00::232]) by mx1.freebsd.org (Postfix) with ESMTP id 2DBD51E6A; Tue, 11 Jun 2013 11:57:35 +0000 (UTC) Received: by mail-qa0-f50.google.com with SMTP id l18so2912082qak.9 for ; Tue, 11 Jun 2013 04:57:34 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:sender:in-reply-to:references:date :x-google-sender-auth:message-id:subject:from:to:cc:content-type; bh=kyAft7s/XLwkghxceoOMrG/LivZ91Ijy0yTzIz2gevE=; b=n2/I4dU2N5EHC892PqN3pS90otNwWcem7BeZc8lxjajgiEEvk1ZyRXZsAnR/scPqLw 3Mxl8qCXjWsb+qKbMSB6+lWou8ccWZjBaJ1VgDDkAh9WXw782Z64aAPAWqb0WaKxiUdW lW/6t31LaBUMqmR9MVbjzDfj7MC5WuJzfl57AKipTzjcqYMAE1arg1eix2mAQn7IknyU MsXgv8RVZbfGe7FnxRe5S4hoR4vnrb+Vp/dNo7iFljoS/P1S0pkRxoryU3JRntrqcJ44 /dMEma++XlwKtp/RBBlct6Br6nIEpVZuz/zPAybPbv/2xLSWeotZ49MNiSIud2OvkyW8 DB8g== MIME-Version: 1.0 X-Received: by 10.229.152.4 with SMTP id e4mr5571136qcw.109.1370951854249; Tue, 11 Jun 2013 04:57:34 -0700 (PDT) Sender: ermal.luci@gmail.com Received: by 10.49.51.8 with HTTP; Tue, 11 Jun 2013 04:57:34 -0700 (PDT) In-Reply-To: <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> <20130610173036.GA916@onelab2.iet.unipi.it> Date: Tue, 11 Jun 2013 13:57:34 +0200 X-Google-Sender-Auth: ZASgw6JHD5aEzelV4cNDSXhKTVI Message-ID: Subject: Re: [PATCH] multiple instances of ipfw(4) From: =?ISO-8859-1?Q?Ermal_Lu=E7i?= To: Luigi Rizzo Content-Type: text/plain; charset=ISO-8859-1 X-Content-Filtered-By: Mailman/MimeDel 2.1.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: Tue, 11 Jun 2013 11:57:35 -0000 Hello Luigi, On Mon, Jun 10, 2013 at 7:30 PM, Luigi Rizzo wrote: > 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 > regenerated patch at the same location https://github.com/pfsense/pfsense-tools/blob/master/patches/RELENG_10_0/CP_multi_instance_ipfw.diff I actually changed some more things to provide a more correct implementation. The only thing about this is that manual page and rc scripts need to be changed for this as well. How you propose to handle this? -- Ermal