Re: ipfw: LOR/panic with uid rules

On Tue, 23 Sep 2008, Robert Watson wrote:

> On Tue, 23 Sep 2008, Stefan Ehmann wrote:
>
>> Also posted about this problem recently in stable_at_. But got no replies 
>> there. So I tried on a recent CURRENT but the problem persists:
>> 
>> ipfw rules using uid are causing a deadlock. eg. allow ip from any to any 
>> uid root A simple HTTP fetch triggers this problem nearly instantly.
>> 
>> For me, this problem existed in 6.x with PREEMPTION enabled. It was fixed 
>> in 7.0. But in RELENG_7 and head it's back. This is a single processor i386 
>> machine.
>
> This is an interesting edge case -- to prevent lookup of an inpcb in the 
> output path, we normally pass the inpcb reference down to the firewall so it 
> can directly access the cred rather than looking it up.  Thus, we don't 
> recurse the global tcbinfo or inpcb locks normally on the transmit path. 
> However, it looks like we have an edge case here where we've freed the inpcb 
> but not yet unlocked the tcbinfo, and since the inpcb is freed we don't pass 
> it down--the firewall code tries to look up the inpcb and improperly 
> recurses the tcbinfo lock, boom.
>
> The uid/gid/jail code in ipfw is undesirable for a number of reasons, not 
> least because it's a layering violation.  Historically, layering violations 
> meant slightly awkward and risky recursion, but now they also mean lock 
> recursion, which has more serious consequences.  I'll investigate tomorrow 
> and see what the best solution is -- probably to drop the lock before 
> calling tcp_dropwithreset() on a NULL inpcb, which is a workaround/hack, but 
> I think our hands are forced in this case.  I'll follow up with a patch 
> then.

Here is a possible candidate patch, could you see if it resolves all of the 
issues you reported?  (Possibly I have missed other similar cases as well...)

Index: tcp_input.c
===================================================================
--- tcp_input.c	(revision 183235)
+++ tcp_input.c	(working copy)
_at__at_ -2472,12 +2472,19 _at__at_
  dropwithreset:
  	KASSERT(headlocked, ("%s: dropwithreset: head not locked", __func__));

-	tcp_dropwithreset(m, th, tp, tlen, rstreason);
-
-	if (tp != NULL)
+	/*
+	 * If tp is non-NULL, we call tcp_dropwithreset() holding both inpcb
+	 * and global locks.  However, if NULL, we must hold neither as
+	 * firewalls may acquire the global lock in order to look for a
+	 * matching inpcb.
+	 */
+	if (tp != NULL) {
+		tcp_dropwithreset(m, th, tp, tlen, rstreason);
  		INP_WUNLOCK(tp->t_inpcb);
-	if (headlocked)
-		INP_INFO_WUNLOCK(&V_tcbinfo);
+	}
+	INP_INFO_WUNLOCK(&V_tcbinfo);
+	if (tp == NULL)
+		tcp_dropwithreset(m, th, NULL, tlen, rstreason);
  	return;

  drop: