[PATCH] multiple instances of ipfw(4)

[Luigi Rizzo]

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