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]

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