Max Laier wrote:
On Friday 29 December 2006 16:41, Christian S.J. Peron wrote:
In IPFW, I changed the chain locks to use rw_lock(9), so we can get rid
of the mpsafenet requirement for IPFW.
I'm not 100% sure IPFW is safe yet. As we configure via raw sockets there
might be some common locks that are held when we enter ip_fw_ctl, which
then picks up the exclusive version of the chain lock. I'm not sure what
kind of locks are in play really, though - CC'ing Robert.
I agree, however, I am fairly sure that each protocol has it's own inpcb
locks. Since we never lookup the pcb for a raw socket (I.E. we currently
only support ucred based firewall rules for TCP/UDP packets).
So, in the IPFW configuration path we have:
ripcb lock (maybe?) -> rw_wlock() -> firewall modification -> rw_wunlock
-> ripcb unlock (maybe?)
In the firewall in/outbound paths we have:
{tcp|udp}pcb lock -> rw_rlock() -> process firewall chains ->
rw_runlock() -> {tcp|udp}pcblock
There is also the inp lock itself, but thats associated with the sockets
themselves... and again, the raw socket associated with configuring the
firewall should never be sending or receiving IP data.
I've thought about doing this in IPFW (looking up the ucred if there
are any uid/gid/jail rules) prior to picking up the
chain locks, but realized we could remove the lock ordering issue all
together if we anihilated the pfil lock.
As long as you are holding the IPFW rw_lock over the lookup there is a
potential problem as described above. The pfil rw_lock doesn't have any
exclusive interaction with any of the inpcb locks, so there is no
problem - as far as I understand at least
What I was thinking was this:
if (ucred_rule_count > 0) {
ucred = lookup_ucred(..);
}
rw_rlock(&chain_lock);
.
.
.
rw_runlock(&chain_lock);
So we would not be picking up any chain locks while the inp lock as
held, and vice versa.
It should be noted that this also allows us to cleanup the ucred stack
based caching code.
One idea I had was introducing PFIL_STATIC which requires that modules
wishing to register pfil hooks did so at
boot-time only. Something similar was done for the MAC framework to
avoid having to pickup policy list locks
for each security decision.
Hmmm ... how would we implement something like this? Can we easily keep
the non-static version? I'd like to be able to develop using modules ;)
Take a look at kern_mac.c and specifically how the MAC_STATIC kernel
option is handled.
My production firewalls never load/unload firewalls at run-time, I would
personally be using
PFIL_STATIC, just not on my development machines.
Again, I am proposing we do both:
(1) Implement ucred lookup PRIOR to picking up firewall chain locks
(2) Implement PFIL_STATIC to eliminate any WITNESS warnings between
pfil and inpcb locks
even though these warnings should be completely harmless imho.
We can also pickup the PFIL locks conditionally based on whether or not
PFIL_STATIC has been specified,
eliminating a couple more atomic instructions per packet in the fast path.
This would also have the nice side effect of eliminating a couple of
atomic instructions per packet in the fast path.
Taking this approach along with moving the inpcb lookup prior to the
firewall chain locks allows us to actually
eliminate the lock ordering issues. However, the layering violation
still exists. But it's harmless.
From the point of view of the locks the layering violation would be gone,
that's why it is harmless.
The layering violation occurs when we pickup layer 4 locks from layer 3,
however, with the
changes it's just bad programming practice instead of being fatal :)
_______________________________________________
[email protected] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-pf
To unsubscribe, send any mail to "[EMAIL PROTECTED]"
