Hello Luigi,
On Mon, Jun 10, 2013 at 7:30 PM, Luigi Rizzo <ri...@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 <ri...@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 _______________________________________________ freebsd-net@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-net To unsubscribe, send any mail to "freebsd-net-unsubscr...@freebsd.org"
