cvs commit: src/share/man/man4 ipfirewall.4 src/share/man/man9
pfil.9 src/sys/alpha/conf GENERIC src/sys/amd64/conf GENERIC
src/sys/conf NOTES files options src/sys/i386/conf GENERIC
src/sys/ia64/conf GENERIC SKI src/sys/modules/bridge Makefile ...
Andre Oppermann
andre at freebsd.org
Fri Aug 27 12:45:42 PDT 2004
Darren Reed wrote:
>
> On Fri, Aug 27, 2004 at 06:12:11PM +0200, Max Laier wrote:
> > Maybe we should hide:
> > if (inet_pfil_hook.ph_busy_count == -1)
>
> There's now a double check on ph_busy_count for inet & inet6 as it's
> first statement in pfil_run_hooks(). Seems a bit silly...
It's not. There is quite a bit of code that follows the pfil_run_hooks()
to look for certain things that might have happend to a packet. If no
hooks are connected we can save the work and simply jump over it. Take
a look the code that is jumped over.
> > behind a macro in case we modify the locking for pfil_hooks in the future. I
> > am thinking of something like:
> > if (PFIL_IS_EMPTY(&inet_pfil_hook))
>
> Unless pfil(9) is being used with a protocol that has been loaded via
> an LKM (and can therefore disappear), there should be no risk here to
> warrant the use of locking.
Locking is used to protect changes to the hooks. If you hook/unhook
there might be another CPU traversing the hooks while you yank them
underneath it. Panic.
> The pfil locking should be overhauled, however, with the main aim
> to replace PFIL_*LOCK() with the use of sx(9).
Please read the reply from Max. He already explained why sx(9) is
inappropriate.
> As an example of the evilness of this, if you've got (say) ipfw loaded
> and you want to enable ipf or pf, there's a security hole that could
> allow a packet to flow through the system without any of them kicking
> in (ph_want_write.)
I agree that this is not perfect. Max already said he wants to revisit
pfil_hooks locking in the future.
--
Andre
More information about the cvs-src
mailing list