debug.mpsafenet=1 vs. user/group rules [Re: kern/106805: ...]

[Christian S.J. Peron]

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 :)