[PATCH] multiple instances of ipfw(4)

[Ermal Luçi]

Hello Luigi,


On Mon, Jun 10, 2013 at 7:30 PM, Luigi Rizzo <rizzo at 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 at 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