Date: Mon, 01 Jun 2009 11:12:45 +0300 From: Mikolaj Golub <to.my.trociny@gmail.com> To: freebsd-net@FreeBSD.org Subject: Re: panic with ng_ipfw+ng_car and net.inet.ip.fw.one_pass=0 Message-ID: <81bpp8l6de.fsf@zhuzha.ua1> In-Reply-To: <864ov9htgq.fsf@kopusha.onet> (Mikolaj Golub's message of "Mon\, 25 May 2009 22\:29\:25 %2B0300") References: <864ov9htgq.fsf@kopusha.onet>
next in thread | previous in thread | raw e-mail | index | archive | help
--=-=-= On Mon, 25 May 2009 22:29:25 +0300 Mikolaj Golub wrote: > 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. It looks the problem has not drawn much attention :-). Anyway, another version of the patch is attached. This time almost all of the necessary modifications are done in ng_ipfw module. Only the small changes have been made in ip_fw module and I tried to make them in the same manner as it is done for dummynet. The main logic is the same as in the previous patch: have internal counter ng_ipfw_rdcnt that is increased every time when some rule is deleted from the chain and compare it with one stored in ng_ipfw_tag when a packet passes ng_ipfw_rcvdata(). The patch is against 8-CURRENT but it is applied (and has been tested) to 7-STABLE too. Actually with this version of patch it looks like there is still small chance of race when ng_ipfw_rdcnt is going to be increased and in the same time the current value is stored in packet arrived to ng_ipfw. But running attached test script in loop I was not able to crash patched system while without the patch the system reliably crashes on the second run of the script. It would be nice to have this patch at least in CURRENT. Although I think that some generic mechanism should be developed in ipfw to validate rule pointers of second pass packets to have net.inet.ip.fw.one_pass=0 feature safe. AFAIK ipfw improvements is this year Summer of Code project so this problem could be addressed there. At least it should be documented in ipfw in BUGS section that the currrent implementation of net.inet.ip.fw.one_pass=0 could panic the system when is used with netgraph. -- Mikolaj Golub --=-=-= Content-Type: text/x-diff Content-Disposition: attachment; filename=ng_ipfw.patch --- sys/netinet/ip_fw2.c.orig 2009-06-01 10:28:33.000000000 +0300 +++ sys/netinet/ip_fw2.c 2009-06-01 10:29:33.000000000 +0300 @@ -3616,6 +3616,8 @@ reap_rules(struct ip_fw *head) head = head->next; if (DUMMYNET_LOADED) ip_dn_ruledel_ptr(rule); + if (NG_IPFW_LOADED) + ng_ipfw_ruledel_p(rule); free(rule, M_IPFW); } } --- sys/netinet/ip_fw_pfil.c.orig 2009-06-01 10:28:33.000000000 +0300 +++ sys/netinet/ip_fw_pfil.c 2009-06-01 10:29:33.000000000 +0300 @@ -85,6 +85,7 @@ ip_divert_packet_t *ip_divert_ptr = NULL /* ng_ipfw hooks. */ ng_ipfw_input_t *ng_ipfw_input_p = NULL; +ng_ipfw_ruledel_t *ng_ipfw_ruledel_p = NULL; /* Forward declarations. */ static int ipfw_divert(struct mbuf **, int, int); --- sys/netgraph/ng_ipfw.c.orig 2009-06-01 10:28:12.000000000 +0300 +++ sys/netgraph/ng_ipfw.c 2009-06-01 10:29:33.000000000 +0300 @@ -84,6 +84,25 @@ static struct ng_type ng_ipfw_typestruct NETGRAPH_INIT(ipfw, &ng_ipfw_typestruct); MODULE_DEPEND(ng_ipfw, ipfw, 2, 2, 2); +extern struct ip_fw *ip_fw_default_rule; + +/* + * ng_ipfw_rdcnt is a counter that is increased every time when some + * rule is deleted from ipfw chain. When a packet passes ng_ipfw_rcvdata() + * the current value of ng_ipfw_rdcnt is compared with the old one stored + * in ng_ipfw_tag. 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 might point to the removed rule and it is replaced + * with ip_fw_default_rule. + */ + +static int ng_ipfw_rdcnt; + +static void +ng_ipfw_ruledel(void* r __unused) { + ng_ipfw_rdcnt++; +} + /* Information we store for each hook */ struct ng_ipfw_hook_priv { hook_p hook; @@ -118,6 +137,7 @@ ng_ipfw_mod_event(module_t mod, int even /* Register hook */ ng_ipfw_input_p = ng_ipfw_input; + ng_ipfw_ruledel_p = ng_ipfw_ruledel; break; case MOD_UNLOAD: @@ -232,6 +252,9 @@ ng_ipfw_rcvdata(hook_p hook, item_p item return (EINVAL); /* XXX: find smth better */ }; + if (ngit->ipfw_rdcnt != ng_ipfw_rdcnt) + ngit->rule = ip_fw_default_rule; + switch (ngit->dir) { case NG_IPFW_OUT: { @@ -293,6 +316,7 @@ ng_ipfw_input(struct mbuf **m0, int dir, return (ENOMEM); } ngit->rule = fwa->rule; + ngit->ipfw_rdcnt = ng_ipfw_rdcnt; ngit->dir = dir; ngit->ifp = fwa->oif; m_tag_prepend(m, &ngit->mt); @@ -324,6 +348,7 @@ ng_ipfw_shutdown(node_p node) * 'kldunload ng_ipfw.ko' */ ng_ipfw_input_p = NULL; + ng_ipfw_ruledel_p = NULL; NG_NODE_UNREF(node); return (0); } --- sys/netgraph/ng_ipfw.h.orig 2009-06-01 10:28:12.000000000 +0300 +++ sys/netgraph/ng_ipfw.h 2009-06-01 10:29:33.000000000 +0300 @@ -34,10 +34,13 @@ typedef int ng_ipfw_input_t(struct mbuf **, int, struct ip_fw_args *, int); extern ng_ipfw_input_t *ng_ipfw_input_p; #define NG_IPFW_LOADED (ng_ipfw_input_p != NULL) +typedef void ng_ipfw_ruledel_t(void *); /* ip_fw.c */ +extern ng_ipfw_ruledel_t *ng_ipfw_ruledel_p; struct ng_ipfw_tag { struct m_tag mt; /* tag header */ struct ip_fw *rule; /* matching rule */ + int ipfw_rdcnt; /* value of ng_ipfw_rdcnt counter when tag was attached */ struct ifnet *ifp; /* interface, for ip_output */ int dir; #define NG_IPFW_OUT 0 --=-=-=--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?81bpp8l6de.fsf>