Date: Sat, 8 Dec 2001 07:45:40 -0500 (EST) From: Dan Pelleg <peldan@yahoo.com> To: FreeBSD-gnats-submit@freebsd.org Subject: kern/32600: [PATCH] incorrect handling of parent rules in ipfw Message-ID: <200112081245.fB8Cjel13266@palraz.wburn>
next in thread | raw e-mail | index | archive | help
>Number: 32600
>Category: kern
>Synopsis: [PATCH] incorrect handling of parent rules in ipfw
>Confidential: no
>Severity: serious
>Priority: low
>Responsible: freebsd-bugs
>State: open
>Quarter:
>Keywords:
>Date-Required:
>Class: sw-bug
>Submitter-Id: current-users
>Arrival-Date: Sat Dec 08 04:50:00 PST 2001
>Closed-Date:
>Last-Modified:
>Originator: Dan Pelleg
>Release: FreeBSD 4.4-STABLE i386
>Organization:
>Environment:
System: FreeBSD p 4.4-STABLE FreeBSD 4.4-STABLE #0: Sat Nov 10 15:10:25 EST 2001 d@k:/usr/obj/usr/src/sys/P i386
>Description:
when I started to use limit rules, I noticed ipfw was emitting lots
of messages:
OUCH! cannot remove rule, count 2
Inspecting ip_fw.c, this is caused when a parent rule with a nonzero count
is detected to have expired.
Here is one scenario how this can legally happen:
For example, lookup_dyn_parent() increases the expiry by
dyn_short_lifetime, whereas add_dyn_rule() will create it with time_second +
dyn_syn_lifetime.
So, when a second limit rule is created, the parent's expire field is not
extended by enough time to match this second child.
Luigi Rizzo confirmed this analysis and on his advice I patched to ignore
the expire field altogether for limit rules.
Note that the (userland) ipfw still takes the expire field into account, as
exemplified by the following scenario:
Suppose you have a "limit" rule, and a rule is created for it, and a minute
later 4 more rules are created for it. So the count is 5. Now terminate the
first connection. Suppose you have net.inet.ip.fw.*lifetime set to huge
values like I do. After a while, the LIMIT rule will expire, and "ipfw -d
show" will not show it anymore. However, only when it gets freed will the
count value for its parent be decreased. So, in the meantime, when you do a
"ipfw -d" show, you see "PARENT 5", but only 4 rules that match it.
Another thing that can happen is for the PARENT rule to expire, and then
you see LIMIT rules listed but not their parent.
At that point I added a patch to ipfw to list PARENT rules with count > 0
regardless of their expire value.
(this still doesn't solve the problem of the number of counts for the
parent being larger than the number of children ipfw lists; I started
solving this one by having ipfw update the reference counter in the parent,
and then realized that ip_fw.c doesn't pass parent pointers - at least not
usable ones - the patch includes a comment about this).
Now, running this patched code gave me kernel panics in add_dyn_rule(),
called from install_state(). I came up with this explanation for them:
Suppose you have plenty of rules (ie, conn_limit of them) for a
parent, but they are all expired. This happens to me when I use Mozilla,
which opens dozens of connections, and then leave that window alone for a
while.
In the DYN_LIMIT case of install_state(), the lookup_dyn_parent finds the
parent, which is found out to have too large a count. Then EXPIRE_DYN_CHAIN
is called, the count goes down to zero in the first pass, and the parent is
removed in the second. But install_state is still holding a pointer to the
freed structure, and later passes it on to add_dyn_rule().
I fixed this by re-looking the parent up after the expiry code, which has
solved the problem.
>How-To-Repeat:
see above.
>Fix:
apply provided patch.
*** sys/netinet/ip_fw.c.orig Sun Nov 18 18:29:23 2001
--- sys/netinet/ip_fw.c Mon Nov 26 07:03:08 2001
***************
*** 649,655 ****
/* remove a refcount to the parent */ \
if (q->dyn_type == DYN_LIMIT) \
q->parent->count--; \
! DEB(printf("-- unlink entry 0x%08x %d -> 0x%08x %d, %d left\n", \
(q->id.src_ip), (q->id.src_port), \
(q->id.dst_ip), (q->id.dst_port), dyn_count-1 ); ) \
if (prev != NULL) \
--- 649,656 ----
/* remove a refcount to the parent */ \
if (q->dyn_type == DYN_LIMIT) \
q->parent->count--; \
! DEB(printf("-- unlink entry %p 0x%08x %d -> 0x%08x %d, %d left\n", \
! q, \
(q->id.src_ip), (q->id.src_port), \
(q->id.dst_ip), (q->id.dst_port), dyn_count-1 ); ) \
if (prev != NULL) \
***************
*** 694,710 ****
* and possibly more in the future.
*/
int zap = ( rule == NULL || rule == q->rule);
! if (zap)
! zap = force || TIME_LEQ( q->expire , time_second );
/* do not zap parent in first pass, record we need a second pass */
if (q->dyn_type == DYN_LIMIT_PARENT) {
max_pass = 1; /* we need a second pass */
if (zap == 1 && (pass == 0 || q->count != 0) ) {
zap = 0 ;
! if (pass == 1) /* should not happen */
! printf("OUCH! cannot remove rule, count %d\n",
! q->count);
}
}
if (zap) {
UNLINK_DYN_RULE(prev, ipfw_dyn_v[i], q);
--- 695,718 ----
* and possibly more in the future.
*/
int zap = ( rule == NULL || rule == q->rule);
!
/* do not zap parent in first pass, record we need a second pass */
if (q->dyn_type == DYN_LIMIT_PARENT) {
max_pass = 1; /* we need a second pass */
if (zap == 1 && (pass == 0 || q->count != 0) ) {
zap = 0 ;
! if (force && pass == 1) { /* should not happen */
! printf("OUCH! cannot remove rule %p 0x%08x %d -> 0x%08x %d, count %d, bucket %d\n",
! q,
! (q->id.src_ip), (q->id.src_port),
! (q->id.dst_ip), (q->id.dst_port),
! q->count,
! i);
! }
}
+ } else {
+ if (zap)
+ zap = force || TIME_LEQ( q->expire , time_second );
}
if (zap) {
UNLINK_DYN_RULE(prev, ipfw_dyn_v[i], q);
***************
*** 882,891 ****
r->next = ipfw_dyn_v[i] ;
ipfw_dyn_v[i] = r ;
dyn_count++ ;
! DEB(printf("-- add entry 0x%08x %d -> 0x%08x %d, total %d\n",
(r->id.src_ip), (r->id.src_port),
(r->id.dst_ip), (r->id.dst_port),
! dyn_count ); )
return r;
}
--- 890,901 ----
r->next = ipfw_dyn_v[i] ;
ipfw_dyn_v[i] = r ;
dyn_count++ ;
! DEB(printf("-- add entry %p 0x%08x %d -> 0x%08x %d, total %d, bucket %d\n",
! r,
(r->id.src_ip), (r->id.src_port),
(r->id.dst_ip), (r->id.dst_port),
! dyn_count,
! i); )
return r;
}
***************
*** 988,995 ****
--- 998,1017 ----
}
if (parent->count >= conn_limit) {
EXPIRE_DYN_CHAIN(rule); /* try to expire some */
+ /*
+ The expiry might have removed the parent too.
+ We lookup again, which will re-create if necessary.
+ */
+ parent = lookup_dyn_parent(&id, rule);
+ if (parent == NULL) {
+ printf("add parent failed\n");
+ return 1;
+ }
if (parent->count >= conn_limit) {
+ if (last_log != time_second) {
+ last_log = time_second ;
printf("drop session, too many entries\n");
+ }
return 1;
}
}
***************
*** 1929,1934 ****
--- 1951,1962 ----
bcopy(p, dst, sizeof *p);
(int)dst->rule = p->rule->fw_number ;
/*
+ * we should really set the parent field
+ * to the corresponding parent in the new
+ * structure. For now, just set it to NULL.
+ */
+ dst->parent = NULL ;
+ /*
* store a non-null value in "next". The userland
* code will interpret a NULL here as a marker
* for the last dynamic rule.
*** sbin/ipfw/ipfw.c.orig Sun Nov 18 18:42:51 2001
--- sbin/ipfw/ipfw.c Sat Nov 24 12:18:27 2001
***************
*** 813,822 ****
(struct ipfw_dyn_rule *)&rules[num];
struct in_addr a;
struct protoent *pe;
printf("## Dynamic rules:\n");
for (;; d++) {
! if (d->expire == 0 && !do_expired) {
if (d->next == NULL)
break;
continue;
--- 813,830 ----
(struct ipfw_dyn_rule *)&rules[num];
struct in_addr a;
struct protoent *pe;
+ int skip;
printf("## Dynamic rules:\n");
for (;; d++) {
! /* determine whether to skip this rule */
! skip = !do_expired;
! if( d->dyn_type == DYN_LIMIT_PARENT) {
! skip = skip && d->count == 0;
! } else {
! skip = skip && d->expire == 0;
! }
! if(skip) {
if (d->next == NULL)
break;
continue;
>Release-Note:
>Audit-Trail:
>Unformatted:
To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-bugs" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200112081245.fB8Cjel13266>