On Tue, 23 Sep 2008, Robert Watson wrote:

> On Tue, 23 Sep 2008, Stefan Ehmann wrote:
>
>> Also posted about this problem recently in stable@. 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)
@@ -2472,12 +2472,19 @@
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:
_______________________________________________
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/lis...reebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscribe@freebsd.org"