Date: Mon, 25 May 2009 22:29:25 +0300 From: Mikolaj Golub <to.my.trociny@gmail.com> To: freebsd-net@FreeBSD.org Subject: panic with ng_ipfw+ng_car and net.inet.ip.fw.one_pass=0 Message-ID: <864ov9htgq.fsf@kopusha.onet>
next in thread | raw e-mail | index | archive | help
--=-=-= Hi, Some times ago it has been posted to fido7.ru.unix.bsd about panics when using ipfw + ng_ipfw + ng_car. http://groups.google.com/group/fido7.ru.unix.bsd/browse_thread/thread/5907d1ba4e76675d For those who haven't learnt Russian yet ;-) here are some details. Max Irgiznov reported that when ng_ipf+ng_car construction was used and net.inet.ip.fw.one_pass=0 was set, the system reliably panicked on ipfw rules reload if there was some traffic through ng_car. The problem here is in the following. When the packet is returning back from ng_car queue to ipfw_chk and one_pass is turned off the next rule is being tried. But if the rules were reloaded while the packet was sitting in ng_car, the next rule pointer might be dangling and the kernel will panic. (kgdb) bt #0 doadump () at pcpu.h:196 #1 0xc07e1f7e in boot (howto=260) at /usr/src/sys/kern/kern_shutdown.c:418 #2 0xc07e2252 in panic (fmt=Variable "fmt" is not available. ) at /usr/src/sys/kern/kern_shutdown.c:574 #3 0xc0495eb7 in db_panic (addr=Could not find the frame base for "db_panic". ) at /usr/src/sys/ddb/db_command.c:446 #4 0xc04968bc in db_command (last_cmdp=0xc0c97514, cmd_table=0x0, dopager=1) at /usr/src/sys/ddb/db_command.c:413 #5 0xc04969ca in db_command_loop () at /usr/src/sys/ddb/db_command.c:466 #6 0xc04981bd in db_trap (type=12, code=0) at /usr/src/sys/ddb/db_main.c:228 #7 0xc080ec76 in kdb_trap (type=12, code=0, tf=0xe6945774) at /usr/src/sys/kern/subr_kdb.c:524 #8 0xc0ad9e4f in trap_fatal (frame=0xe6945774, eva=3735929068) at /usr/src/sys/i386/i386/trap.c:930 #9 0xc0ada790 in trap (frame=0xe6945774) at /usr/src/sys/i386/i386/trap.c:320 #10 0xc0abeaab in calltrap () at /usr/src/sys/i386/i386/exception.s:159 #11 0xc903328c in ipfw_chk (args=0xe6945acc) at /usr/src/sys/modules/ipfw/../../netinet/ip_fw2.c:2516 #12 0xc90373f7 in ipfw_check_in (arg=0x0, m0=0xe6945bd0, ifp=0xc41f9000, dir=1, inp=0x0) at /usr/src/sys/modules/ipfw/../../netinet/ip_fw_pfil.c:125 #13 0xc088d6e8 in pfil_run_hooks (ph=0xc0d1f620, mp=0xe6945c24, ifp=0xc41f9000, dir=1, inp=0x0) at /usr/src/sys/net/pfil.c:78 #14 0xc08c766d in ip_input (m=0xc409ad00) at /usr/src/sys/netinet/ip_input.c:416 #15 0xc9011c39 in ng_ipfw_rcvdata (hook=0xc61a1780, item=0xc8fe5090) at /usr/src/sys/modules/netgraph/ipfw/../../../netgraph/ng_ipfw.c:250 #16 0xc68b80af in ng_apply_item (node=0xc7054c00, item=0xc8fe5090, rw=0) at /usr/src/sys/modules/netgraph/netgraph/../../../netgraph/ng_base.c:2336 #17 0xc68b939f in ngthread (arg=0x0) at /usr/src/sys/modules/netgraph/netgraph/../../../netgraph/ng_base.c:3304 #18 0xc07be4c8 in fork_exit (callout=0xc68b91f0 <ngthread>, arg=0x0, frame=0xe6945d38) at /usr/src/sys/kern/kern_fork.c:810 #19 0xc0abeb20 in fork_trampoline () at /usr/src/sys/i386/i386/exception.s:264 (kgdb) frame 11 #11 0xc903328c in ipfw_chk (args=0xe6945acc) at /usr/src/sys/modules/ipfw/../../netinet/ip_fw2.c:2516 warning: Source file is more recent than executable. 2516 if (set_disable & (1 << f->set) ) (kgdb) list 2511 ipfw_insn *cmd; 2512 uint32_t tablearg = 0; 2513 int l, cmdlen, skip_or; /* skip rest of OR block */ 2514 2515 again: 2516 if (set_disable & (1 << f->set) ) 2517 continue; 2518 2519 skip_or = 0; 2520 for (l = f->cmd_len, cmd = f->cmd ; l > 0 ; (kgdb) p f $1 = (struct ip_fw *) 0xdeadc0de (kgdb) DUMMYNET does not have such problems as ip_dn_ruledel_ptr(rule) is called when the rule is removed in reap_rules(). The first thought was to do the same here i.e. to broadcast "remove the rule" message to netgraph nodes, but glancing through the netgraph man I haven't figured out how it could be done if it is possible at all. So the other solution is to have some counter that increases every time when any rules are removed. When the packet is directed by ipfw to netgraph subsystem, the current value of the counter is stored in mtag. When the packet is coming back the current value of the counter is compared with one from the mtag and if they differ the packet is dropped. Just to prove the concept I have modified ip_fw2.c for 7.2-STABLE accordingly and it works for me. The patch is attached. I would like to hear other people opinion, first of all if the proposed idea is good enough or there might be other better solutions for the problem (e.g. "remove the rule" broadcasting is possible). But also if somebody have any remarks about the patch itself I would happy to see them. E.g. I have added the counter just as static variable but as for me struct ip_fw_chain could be better place for this. Also is there any need to mark the tag with MTAG_PERSISTENT bit? -- Mikolaj Golub --=-=-= Content-Type: text/x-diff Content-Disposition: attachment; filename=ip_fw2.c.patch --- sys/netinet/ip_fw2.c.orig 2009-05-24 14:25:30.000000000 +0300 +++ sys/netinet/ip_fw2.c 2009-05-25 19:30:33.000000000 +0300 @@ -111,6 +111,15 @@ static int fw_verbose; static struct callout ipfw_timeout; static int verbose_limit; +/* + * ip_fw_rm_cnt is a counter that is increased every time when reap_rules() is called. + * It is used when the packet is returning back from netgraph subsytem. The current value + * is compared with the old one stored in mtag. If they are not the same then some rules + * were removed from the firewall while the packet was in netgraph so it is not safe to + * use rule pointer as it can point to removed rule. In this case the packet is just dropped. + */ +static int ip_fw_rm_cnt; + static uma_zone_t ipfw_dyn_rule_zone; /* @@ -2458,6 +2467,18 @@ do { \ } IPFW_RLOCK(chain); + + /* + * Check if the packet has come back from netgraph. In this case check + * that we haven't remove some rules from the chain so we wouldn't reference + * deleted rule. + */ + mtag = m_tag_find(m, MTAG_IPFW_RMCNT, NULL); + if (mtag && (*(int*)(mtag + 1) != ip_fw_rm_cnt)) { + IPFW_RUNLOCK(chain); + return (IP_FW_DENY); /* invalid */ + } + mtag = m_tag_find(m, PACKET_TAG_DIVERT, NULL); if (args->rule) { /* @@ -3298,6 +3319,15 @@ check_body: args->cookie = tablearg; else args->cookie = cmd->arg1; + /* + * Store the current value of ip_fw_rm_cnt. We will + * check it when the packet has come back. + */ + if ((mtag = m_tag_alloc(MTAG_IPFW, MTAG_IPFW_RMCNT, + sizeof(int), M_NOWAIT)) != NULL) { + m_tag_prepend(m, mtag); + *(int*)(mtag + 1) = ip_fw_rm_cnt; + } retval = (cmd->opcode == O_NETGRAPH) ? IP_FW_NETGRAPH : IP_FW_NGTEE; goto done; @@ -3511,6 +3541,7 @@ reap_rules(struct ip_fw *head) { struct ip_fw *rule; + ip_fw_rm_cnt++; while ((rule = head) != NULL) { head = head->next; if (DUMMYNET_LOADED) --=-=-=--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?864ov9htgq.fsf>