From owner-freebsd-pf@FreeBSD.ORG Fri Dec 29 17:38:06 2006 Return-Path: X-Original-To: freebsd-pf@freebsd.org Delivered-To: freebsd-pf@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 2C7CE16A4D8 for ; Fri, 29 Dec 2006 17:38:06 +0000 (UTC) (envelope-from csjp@FreeBSD.ORG) Received: from ems01.seccuris.com (ems01.seccuris.com [204.112.0.35]) by mx1.freebsd.org (Postfix) with ESMTP id E321413C461 for ; Fri, 29 Dec 2006 17:38:04 +0000 (UTC) (envelope-from csjp@FreeBSD.ORG) Received: from [192.168.11.124] (dhcp-ws-060.ws.seccuris.com [192.168.11.124]) by ems01.seccuris.com (Postfix) with ESMTP id 8AFD3462EA5; Fri, 29 Dec 2006 12:43:29 -0600 (CST) Message-ID: <45955284.6000606@FreeBSD.ORG> Date: Fri, 29 Dec 2006 11:38:12 -0600 From: "Christian S.J. Peron" User-Agent: Thunderbird 1.5.0.9 (Macintosh/20061207) MIME-Version: 1.0 To: Max Laier References: <200612161335.kBGDZkMj012022@freefall.freebsd.org> <200612161709.48875.max@love2party.net> <45953727.7020405@FreeBSD.ORG> <200612291738.15541.max@love2party.net> In-Reply-To: <200612291738.15541.max@love2party.net> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: avatar@mmlab.cse.yzu.edu.tw, Robert Nicholas Maxwell Watson , freebsd-pf@freebsd.org Subject: Re: debug.mpsafenet=1 vs. user/group rules [Re: kern/106805: ...] X-BeenThere: freebsd-pf@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "Technical discussion and general questions about packet filter \(pf\)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 29 Dec 2006 17:38:06 -0000 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 :)