Date: Tue, 11 Jun 2013 13:57:34 +0200 From: =?ISO-8859-1?Q?Ermal_Lu=E7i?= <eri@freebsd.org> To: Luigi Rizzo <rizzo@iet.unipi.it> Cc: freebsd-net <freebsd-net@freebsd.org>, freebsd-hackers@freebsd.org Subject: Re: [PATCH] multiple instances of ipfw(4) Message-ID: <CAPBZQG1G_M7K1Tc4PJH-_6nkz%2BjvmU5AfhVksDAzCja4QuK1fA@mail.gmail.com> In-Reply-To: <20130610173036.GA916@onelab2.iet.unipi.it> References: <CAPBZQG32iyzkec4PG%2Bqay9bKfd0GiffKyRBapLkATKvHr7cVww@mail.gmail.com> <20120131110204.GA95472@onelab2.iet.unipi.it> <20120208133559.GK13554@FreeBSD.org> <CAPBZQG0edS3sru=D_iGMsNDC5EA8H=A=wwRUDOGZi9DtU5-CkQ@mail.gmail.com> <20120208140921.GM13554@glebius.int.ru> <4F344CE4.301@freebsd.org> <CAPBZQG3X4da93rvORyF169yWUokS5V_HhazN-wMeiJEDvDB-4Q@mail.gmail.com> <CA%2BhQ2%2BgnY64x_2uk0FUD%2BLL7Wm9i%2BUtQU-pxhW5KsEkb44mfeQ@mail.gmail.com> <CAPBZQG32LBXx2LTwPzwdY-fBZH9C8mZvj8b%2BWK183xTixa2XOQ@mail.gmail.com> <20130610173036.GA916@onelab2.iet.unipi.it>
next in thread | previous in thread | raw e-mail | index | archive | help
Hello Luigi, On Mon, Jun 10, 2013 at 7:30 PM, Luigi Rizzo <rizzo@iet.unipi.it> 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 <rizzo@iet.unipi.it> 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAPBZQG1G_M7K1Tc4PJH-_6nkz%2BjvmU5AfhVksDAzCja4QuK1fA>