Date: Thu, 6 May 2004 15:38:15 -0700 From: Luigi Rizzo <rizzo@icir.org> To: Oleg Bulyzhin <oleg@rinet.ru> Cc: freebsd-ipfw@freebsd.org Subject: Re: ipfw: ouch!, skip past end of rules, denying packet Message-ID: <20040506153815.A75812@xorpc.icir.org> In-Reply-To: <20040507013009.H1824@lath.rinet.ru>; from oleg@rinet.ru on Fri, May 07, 2004 at 01:35:11AM %2B0400 References: <104341060709.20040505171307@vkt.lt> <20040505194451.V9766@lath.rinet.ru> <158403649347.20040506103615@vkt.lt> <20040507013009.H1824@lath.rinet.ru>
next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, May 07, 2004 at 01:35:11AM +0400, Oleg Bulyzhin wrote: ... > i see. > There is a little bug (i'll PR it as soon i'll get enough time), you can > try attached patch(built on RELENG_4). very interesting that you found out what the bug was -- i couldn't realize it myself. Thanks! However, i believe the fix is incorrect and in principle can still trigger the problem (which is innocuous). The bug your patch addresses is the following: when a packet is stored in a pipe, dummynet keeps a pointer to the matching rule so if one_pass=0 it can restart processing from the following one. if the matching rule goes away while a packet is queued, my intention was to use the default rule as the next one, but i mistakenly used the default rule as the _matching_ one. Your patch replaces the matching rule with the next one, which however might still end up being the default rule; so it does not fix the proble, plus, it might completely subvert the packet's flow. I believe a proper fix is right before the main loop in ipfw_chk(), check if we enter with a NULL rule pointer and use the default rule instead. Alternatively, if we believe that when a rule goes away we should also drop queued packets because the resulting behaviour would be unpredictable or unsafe, then what happens now is basically correct (not by design, just pure luck), and we just need to remove the 'ouch...' message cheers luigi > > > > > > _______________________________________________ > > freebsd-ipfw@freebsd.org mailing list > > http://lists.freebsd.org/mailman/listinfo/freebsd-ipfw > > To unsubscribe, send any mail to "freebsd-ipfw-unsubscribe@freebsd.org" > > > > -- > Oleg. > > ================================================================ > === Oleg Bulyzhin -- OBUL-RIPN -- OBUL-RIPE -- oleg@rinet.ru === > ================================================================ Content-Description: ipfw patch > --- sys/netinet/ip_dummynet.c~ Tue Dec 30 15:28:09 2003 > +++ sys/netinet/ip_dummynet.c Wed May 5 21:41:09 2004 > @@ -1378,7 +1378,6 @@ > } > > > -extern struct ip_fw *ip_fw_default_rule ; > static void > dn_rule_delete_fs(struct dn_flow_set *fs, void *r) > { > @@ -1390,7 +1389,7 @@ > for (q = fs->rq[i] ; q ; q = q->next ) > for (pkt = q->head ; pkt ; pkt = DN_NEXT(pkt) ) > if (pkt->rule == r) > - pkt->rule = ip_fw_default_rule ; > + pkt->rule = lookup_next_rule(pkt->rule); > } > /* > * when a firewall rule is deleted, scan all queues and remove the flow-id > @@ -1415,7 +1414,7 @@ > dn_rule_delete_fs(fs, r); > for (pkt = p->head ; pkt ; pkt = DN_NEXT(pkt) ) > if (pkt->rule == r) > - pkt->rule = ip_fw_default_rule ; > + pkt->rule = lookup_next_rule(pkt->rule); > } > } > > --- sys/netinet/ip_fw.c~ Mon Jan 20 05:23:07 2003 > +++ sys/netinet/ip_fw.c Wed May 5 21:53:06 2004 > @@ -1023,9 +1023,7 @@ > * Backward jumps are not allowed, so start looking from the next > * rule... > */ > -static struct ip_fw * lookup_next_rule(struct ip_fw *me); > - > -static struct ip_fw * > +struct ip_fw * > lookup_next_rule(struct ip_fw *me) > { > struct ip_fw *rule ; > @@ -2066,16 +2064,6 @@ > return (error); > } > > -/** > - * dummynet needs a reference to the default rule, because rules can > - * be deleted while packets hold a reference to them (e.g. to resume > - * processing at the next rule). When this happens, dummynet changes > - * the reference to the default rule (probably it could well be a > - * NULL pointer, but this way we do not need to check for the special > - * case, plus here he have info on the default behaviour. > - */ > -struct ip_fw *ip_fw_default_rule ; > - > void > ip_fw_init(void) > { > @@ -2098,7 +2086,6 @@ > add_entry(&ip_fw_chain_head, &default_rule)) > panic("ip_fw_init"); > > - ip_fw_default_rule = LIST_FIRST(&ip_fw_chain_head) ; > printf("IP packet filtering initialized, " > #ifdef IPDIVERT > "divert enabled, " > --- sys/netinet/ip_fw.h~ Tue Jul 9 13:11:42 2002 > +++ sys/netinet/ip_fw.h Wed May 5 21:47:21 2004 > @@ -360,6 +360,7 @@ > struct sockopt; > struct dn_flow_set; > void flush_pipe_ptrs(struct dn_flow_set *match); /* used by dummynet */ > +struct ip_fw * lookup_next_rule(struct ip_fw *me); > > typedef int ip_fw_chk_t (struct ip_fw_args *args); > typedef int ip_fw_ctl_t (struct sockopt *); > --- sys/netinet/ip_fw2.c~ Fri Apr 2 21:15:44 2004 > +++ sys/netinet/ip_fw2.c Wed May 5 21:44:55 2004 > @@ -1221,7 +1221,7 @@ > * pointers are flushed so we are always correct. > */ > > -static struct ip_fw * > +struct ip_fw * > lookup_next_rule(struct ip_fw *me) > { > struct ip_fw *rule = NULL; > @@ -2721,15 +2721,6 @@ > return (error); > } > > -/** > - * dummynet needs a reference to the default rule, because rules can be > - * deleted while packets hold a reference to them. When this happens, > - * dummynet changes the reference to the default rule (it could well be a > - * NULL pointer, but this way we do not need to check for the special > - * case, plus here he have info on the default behaviour). > - */ > -struct ip_fw *ip_fw_default_rule; > - > /* > * This procedure is only used to handle keepalives. It is invoked > * every dyn_keepalive_period > @@ -2793,7 +2784,6 @@ > > add_rule(&layer3_chain, &default_rule); > > - ip_fw_default_rule = layer3_chain; > printf("ipfw2 initialized, divert %s, " > "rule-based forwarding enabled, default to %s, logging ", > #ifdef IPDIVERT > --- sys/netinet/ip_fw2.h~ Thu Jul 17 10:03:39 2003 > +++ sys/netinet/ip_fw2.h Wed May 5 21:44:10 2004 > @@ -413,6 +413,7 @@ > struct dn_flow_set; > > void flush_pipe_ptrs(struct dn_flow_set *match); /* used by dummynet */ > +struct ip_fw * lookup_next_rule(struct ip_fw *me); > > typedef int ip_fw_chk_t (struct ip_fw_args *args); > typedef int ip_fw_ctl_t (struct sockopt *); > _______________________________________________ > freebsd-ipfw@freebsd.org mailing list > http://lists.freebsd.org/mailman/listinfo/freebsd-ipfw > To unsubscribe, send any mail to "freebsd-ipfw-unsubscribe@freebsd.org"
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20040506153815.A75812>