* PANIC: divide by zero in xt_connbytes @ 2007-01-18 10:59 Jonas Berlin 2007-01-18 11:56 ` Bugzilla webserver misconfigured? (was: Re: PANIC: divide by zero in xt_connbytes) Maximilian Wilhelm 2007-01-18 13:28 ` PANIC: divide by zero in xt_connbytes Pablo Neira Ayuso 0 siblings, 2 replies; 10+ messages in thread From: Jonas Berlin @ 2007-01-18 10:59 UTC (permalink / raw) To: Patrick McHardy, Netfilter Development Mailinglist -----BEGIN PGP SIGNED MESSAGE----- Hash: RIPEMD160 Hi Patrick, Hidden asked me (on irc) to bring your attention to this bug: ~ http://bugzilla.netfilter.org/bugzilla/show_bug.cgi?id=533 He also said that the fix should maybe be considered for inclusion in the -stable branch. - -- ~ "if I'd make up my own definitions, unrational would be the the end ~ result after someone unrationalized something i.e. actively worked ~ to cut down on rationality, whereas irrational would be more a ~ result of laziness or lack of skill" ~ - me, 23.11.2006 -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.6 (GNU/Linux) iD8DBQFFr1MNxyF48ZTvn+4RA1woAKCxAkxl0gr5aiFP49NdZ+qnA2zJFACgxXNk CUOIgGSGWk7Nt4WODOizn+s= =CQkC -----END PGP SIGNATURE----- ^ permalink raw reply [flat|nested] 10+ messages in thread
* Bugzilla webserver misconfigured? (was: Re: PANIC: divide by zero in xt_connbytes) 2007-01-18 10:59 PANIC: divide by zero in xt_connbytes Jonas Berlin @ 2007-01-18 11:56 ` Maximilian Wilhelm 2007-01-19 2:43 ` Bugzilla webserver misconfigured? Jonas Berlin 2007-01-18 13:28 ` PANIC: divide by zero in xt_connbytes Pablo Neira Ayuso 1 sibling, 1 reply; 10+ messages in thread From: Maximilian Wilhelm @ 2007-01-18 11:56 UTC (permalink / raw) To: netfilter-devel Am Donnerstag, den 18. Januar hub Jonas Berlin folgendes in die Tasten: Hi! [...] > ~ http://bugzilla.netfilter.org/bugzilla/show_bug.cgi?id=533 When I entry this link into my browser I am redirected to https://bugzilla.netfilter.org/bugzilla/index.cgibugzilla/show_bug.cgi?id=533 and get a 404. Maybe one would look at the redirect rules? Ciao Max -- | | Follow the white penguin. | |\/| | |-----------------------------------------------------------. | | |/\| | Rechnerbetrieb Mathematik | Meine Baustellen: TSM | | | Universitaet Paderborn | Hostmaster, Linux, LDAP | ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Bugzilla webserver misconfigured? 2007-01-18 11:56 ` Bugzilla webserver misconfigured? (was: Re: PANIC: divide by zero in xt_connbytes) Maximilian Wilhelm @ 2007-01-19 2:43 ` Jonas Berlin 0 siblings, 0 replies; 10+ messages in thread From: Jonas Berlin @ 2007-01-19 2:43 UTC (permalink / raw) To: netfilter-devel -----BEGIN PGP SIGNED MESSAGE----- Hash: RIPEMD160 Quoting Maximilian Wilhelm on 01/18/2007 11:56 AM UTC: | Am Donnerstag, den 18. Januar hub Jonas Berlin folgendes in die Tasten: | | Hi! | | [...] |> ~ http://bugzilla.netfilter.org/bugzilla/show_bug.cgi?id=533 | | When I entry this link into my browser I am redirected to | | https://bugzilla.netfilter.org/bugzilla/index.cgibugzilla/show_bug.cgi?id=533 | | and get a 404. | | Maybe one would look at the redirect rules? Well I'll at least take blame for the http url.. I thought https was unnecessary and only a result of me being logged in.. so I manually removed the "s" but didn't test it :( But since the server seems to serve HTTP and indeed even does try to redirect, it seems it's supposed to be there and therefore could certainly be fixed too.. :) - -- - - xkr47 - -- ~ "if I'd make up my own definitions, unrational would be the end ~ result after someone unrationalized something i.e. actively worked ~ to cut down on rationality, whereas irrational would be more a ~ result of laziness or lack of skill" ~ - me, 23.11.2006 -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.6 (GNU/Linux) iD8DBQFFsDBOxyF48ZTvn+4RA4AFAKC7hwcGphYrjKZxB7AZlBonHYjZewCgx4pb pw1B08HPmG8c0ol9BZ4Qb2g= =/UHV -----END PGP SIGNATURE----- ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: PANIC: divide by zero in xt_connbytes 2007-01-18 10:59 PANIC: divide by zero in xt_connbytes Jonas Berlin 2007-01-18 11:56 ` Bugzilla webserver misconfigured? (was: Re: PANIC: divide by zero in xt_connbytes) Maximilian Wilhelm @ 2007-01-18 13:28 ` Pablo Neira Ayuso 2007-01-18 13:38 ` Jonas Berlin 1 sibling, 1 reply; 10+ messages in thread From: Pablo Neira Ayuso @ 2007-01-18 13:28 UTC (permalink / raw) To: Jonas Berlin; +Cc: Netfilter Development Mailinglist, Patrick McHardy Jonas Berlin wrote: > Hidden asked me (on irc) to bring your attention to this bug: > > ~ http://bugzilla.netfilter.org/bugzilla/show_bug.cgi?id=533 Copied from your patch available on bugzilla: > diff -ur linux-2.6.19/net/netfilter/xt_connbytes.c linux-2.6.19-xt_connbytes_fix/net/netfilter/xt_connbytes.c > --- linux-2.6.19/net/netfilter/xt_connbytes.c 2007-01-11 20:01:51.000000000 +0200 > +++ linux-2.6.19-xt_connbytes_fix/net/netfilter/xt_connbytes.c 2007-01-18 12:15:50.000000000 +0200 > @@ -89,26 +89,39 @@ > case XT_CONNBYTES_AVGPKT: > switch (sinfo->direction) { > case XT_CONNBYTES_DIR_ORIGINAL: > - what = div64_64(counters[IP_CT_DIR_ORIGINAL].bytes, > - counters[IP_CT_DIR_ORIGINAL].packets); Better check that divisor must be != 0 inside div64_64. > + if (counters[IP_CT_DIR_ORIGINAL].packets == 0) { > + what = 0; > + } else { > + what = div64_64(counters[IP_CT_DIR_ORIGINAL].bytes, > + counters[IP_CT_DIR_ORIGINAL].packets); > + } > break; > case XT_CONNBYTES_DIR_REPLY: > - what = div64_64(counters[IP_CT_DIR_REPLY].bytes, > - counters[IP_CT_DIR_REPLY].packets); > + if (counters[IP_CT_DIR_REPLY].packets == 0) { > + what = 0; > + } else { > + what = div64_64(counters[IP_CT_DIR_REPLY].bytes, > + counters[IP_CT_DIR_REPLY].packets); > + } > break; > case XT_CONNBYTES_DIR_BOTH: > { > - u_int64_t bytes; > u_int64_t pkts; > - bytes = counters[IP_CT_DIR_ORIGINAL].bytes + > - counters[IP_CT_DIR_REPLY].bytes; > pkts = counters[IP_CT_DIR_ORIGINAL].packets+ > counters[IP_CT_DIR_REPLY].packets; > > - /* FIXME_THEORETICAL: what to do if sum > - * overflows ? */ > + if (pkts == 0) { > + what = 0; > + } else { > + u_int64_t bytes; > + bytes = counters[IP_CT_DIR_ORIGINAL].bytes + > + counters[IP_CT_DIR_REPLY].bytes; > + > + /* FIXME_THEORETICAL: what to do if sum > + * overflows ? */ ^^^ Hm, already had this discussion: This is really hard to happen with 64 bits counters, it would take years even in a high performance network. -- The dawn of the fourth age of Linux firewalling is coming; a time of great struggle and heroic deeds -- J.Kadlecsik got inspired by J.Morris ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: PANIC: divide by zero in xt_connbytes 2007-01-18 13:28 ` PANIC: divide by zero in xt_connbytes Pablo Neira Ayuso @ 2007-01-18 13:38 ` Jonas Berlin 2007-01-18 14:22 ` KOVACS Krisztian 0 siblings, 1 reply; 10+ messages in thread From: Jonas Berlin @ 2007-01-18 13:38 UTC (permalink / raw) To: Pablo Neira Ayuso; +Cc: Netfilter Development Mailinglist, Patrick McHardy -----BEGIN PGP SIGNED MESSAGE----- Hash: RIPEMD160 Quoting Pablo Neira Ayuso on 01/18/2007 01:28 PM UTC: |> ~ http://bugzilla.netfilter.org/bugzilla/show_bug.cgi?id=533 | | Copied from your patch available on bugzilla: | |> diff -ur linux-2.6.19/net/netfilter/xt_connbytes.c linux-2.6.19-xt_connbytes_fix/net/netfilter/xt_connbytes.c |> --- linux-2.6.19/net/netfilter/xt_connbytes.c 2007-01-11 20:01:51.000000000 +0200 |> +++ linux-2.6.19-xt_connbytes_fix/net/netfilter/xt_connbytes.c 2007-01-18 12:15:50.000000000 +0200 |> @@ -89,26 +89,39 @@ |> case XT_CONNBYTES_AVGPKT: |> switch (sinfo->direction) { |> case XT_CONNBYTES_DIR_ORIGINAL: |> - what = div64_64(counters[IP_CT_DIR_ORIGINAL].bytes, |> - counters[IP_CT_DIR_ORIGINAL].packets); | | Better check that divisor must be != 0 inside div64_64. I initially suggested that too and would have renamed the function to div64_64_safe() or similar in the same go.. but Hidden (on irc) thought it was up to the caller to do the checking, and I felt he probably knows the kernel way of thinking better than me and succumbed. :) I think it's up to Patrick, I'm really only concerned with that it gets fixed :) If Patrick so requests, I'll redo it the other way.. |> + /* FIXME_THEORETICAL: what to do if sum |> + * overflows ? */ | ^^^ | Hm, already had this discussion: This is really hard to happen with 64 | bits counters, it would take years even in a high performance network. Well I didn't put it there and it was out of scope to do any modifications to it in this patch :) - - xkr47 - -- ~ "if I'd make up my own definitions, unrational would be the the end ~ result after someone unrationalized something i.e. actively worked ~ to cut down on rationality, whereas irrational would be more a ~ result of laziness or lack of skill" ~ - me, 23.11.2006 -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.6 (GNU/Linux) iD8DBQFFr3hexyF48ZTvn+4RAz6DAJ4kNXaOEPE3EFCKJsDvmCJVIOjD0gCfRVmV 8iX2XHtLXMcRy0+MnOnoryE= =fNp+ -----END PGP SIGNATURE----- ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: PANIC: divide by zero in xt_connbytes 2007-01-18 13:38 ` Jonas Berlin @ 2007-01-18 14:22 ` KOVACS Krisztian 2007-01-22 8:52 ` KOVACS Krisztian 2007-01-26 17:24 ` Patrick McHardy 0 siblings, 2 replies; 10+ messages in thread From: KOVACS Krisztian @ 2007-01-18 14:22 UTC (permalink / raw) To: netfilter-devel; +Cc: Patrick McHardy, Pablo Neira Ayuso Hi, On Thursday 18 January 2007 14:38, Jonas Berlin wrote: > | Better check that divisor must be != 0 inside div64_64. > > I initially suggested that too and would have renamed the function to > div64_64_safe() or similar in the same go.. but Hidden (on irc) thought > it was up to the caller to do the checking, and I felt he probably > knows the kernel way of thinking better than me and succumbed. :) I > think it's up to Patrick, I'm really only concerned with that it gets > fixed :) If Patrick so requests, I'll redo it the other way.. Not that it matters a lot, but I thought determining what to do if there have been no packets yet does not belong to the division routine logically. For me at least, it would introduce some kind of obscurity: at a first glance you won't be able to see what happens if packets==0, you'll have to scroll up to the div() function. Choosing zero as the average packet length in the corner case is appropriate only because that's whay usually makes sense in your ruleset. (For example if you use connbytes to classify traffic into bulk and interactive categories based on the average packet length, 0 as the "fallback" value is OK because it means that a brand new connection is considered interactive.) I guess this is a matter of personal preference: one way it's less code, the other way it's easier to read -- at least for me. Of course the only thing that matters is having this bug fixed, as it's quite ugly indeed. -- Regards, Krisztian Kovacs ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: PANIC: divide by zero in xt_connbytes 2007-01-18 14:22 ` KOVACS Krisztian @ 2007-01-22 8:52 ` KOVACS Krisztian 2007-01-26 17:24 ` Patrick McHardy 1 sibling, 0 replies; 10+ messages in thread From: KOVACS Krisztian @ 2007-01-22 8:52 UTC (permalink / raw) To: netfilter-devel Hi, > > I initially suggested that too and would have renamed the function to > > div64_64_safe() or similar in the same go.. but Hidden (on irc) As a side note: since counters have been changed to 32 bits, I think this div64_64 function is not necessary anymore. (Yes, I have read the discussion about reintroducing them, but that won't be in 2.6.20 I guess.) -- Regards, Krisztian Kovacs ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: PANIC: divide by zero in xt_connbytes 2007-01-18 14:22 ` KOVACS Krisztian 2007-01-22 8:52 ` KOVACS Krisztian @ 2007-01-26 17:24 ` Patrick McHardy 2007-01-26 20:11 ` KOVACS Krisztian 1 sibling, 1 reply; 10+ messages in thread From: Patrick McHardy @ 2007-01-26 17:24 UTC (permalink / raw) To: KOVACS Krisztian; +Cc: netfilter-devel, Pablo Neira Ayuso [-- Attachment #1: Type: text/plain, Size: 1985 bytes --] KOVACS Krisztian wrote: > Hi, > > On Thursday 18 January 2007 14:38, Jonas Berlin wrote: > >>| Better check that divisor must be != 0 inside div64_64. >> >>I initially suggested that too and would have renamed the function to >>div64_64_safe() or similar in the same go.. but Hidden (on irc) thought >>it was up to the caller to do the checking, and I felt he probably >>knows the kernel way of thinking better than me and succumbed. :) I >>think it's up to Patrick, I'm really only concerned with that it gets >>fixed :) If Patrick so requests, I'll redo it the other way.. > > > Not that it matters a lot, but I thought determining what to do if there > have been no packets yet does not belong to the division routine > logically. For me at least, it would introduce some kind of obscurity: at > a first glance you won't be able to see what happens if packets==0, > you'll have to scroll up to the div() function. Choosing zero as the > average packet length in the corner case is appropriate only because > that's whay usually makes sense in your ruleset. (For example if you use > connbytes to classify traffic into bulk and interactive categories based > on the average packet length, 0 as the "fallback" value is OK because it > means that a brand new connection is considered interactive.) > > I guess this is a matter of personal preference: one way it's less code, > the other way it's easier to read -- at least for me. Of course the only > thing that matters is having this bug fixed, as it's quite ugly indeed. I agree that having the caller check it is easier to read. I'm wondering what value to use when packets == 0 though, it can't happen for the first packet of a connection since it has already been accounted for before we can match, so the packets counter must have overflown at least once (and the byte counter at least as often as the packet counter). Well, I guess it doesn't matter since we've lost anyway, so I've queued this patch. [-- Attachment #2: x --] [-- Type: text/plain, Size: 2642 bytes --] [NETFILTER]: xt_connbytes: fix division by zero after overflow When the packet counter of a connection overflows a division by zero occurs in div64_64(). At that point we can't give calculate accurate values anymore, but at least make sure it doesn't crash. Additionally we're probably going to go back to 64 bit counters in 2.6.21. Based on patch from Jonas Berlin <xkr47@outerspace.dyndns.org>, with suggestions from KOVACS Krisztian <hidden@balabit.hu>. Signed-off-by: Patrick McHardy <kaber@trash.net> --- commit 57a227273018d7b507d01a67392a6faa082350e4 tree 01d37699758bd05f483b86ebe56405005c2e27c4 parent 9999a622b03b44e395c8388ff9ab99f99726dce0 author Patrick McHardy <kaber@trash.net> Fri, 26 Jan 2007 18:22:35 +0100 committer Patrick McHardy <kaber@trash.net> Fri, 26 Jan 2007 18:23:33 +0100 net/netfilter/xt_connbytes.c | 29 ++++++++++++----------------- 1 files changed, 12 insertions(+), 17 deletions(-) diff --git a/net/netfilter/xt_connbytes.c b/net/netfilter/xt_connbytes.c index d93cb09..6314601 100644 --- a/net/netfilter/xt_connbytes.c +++ b/net/netfilter/xt_connbytes.c @@ -53,6 +53,8 @@ match(const struct sk_buff *skb, const struct xt_connbytes_info *sinfo = matchinfo; u_int64_t what = 0; /* initialize to make gcc happy */ const struct ip_conntrack_counter *counters; + u_int64_t bytes; + u_int64_t pkts; if (!(counters = nf_ct_get_counters(skb))) return 0; /* no match */ @@ -89,29 +91,22 @@ match(const struct sk_buff *skb, case XT_CONNBYTES_AVGPKT: switch (sinfo->direction) { case XT_CONNBYTES_DIR_ORIGINAL: - what = div64_64(counters[IP_CT_DIR_ORIGINAL].bytes, - counters[IP_CT_DIR_ORIGINAL].packets); + bytes = counters[IP_CT_DIR_ORIGINAL].bytes; + pkts = counters[IP_CT_DIR_ORIGINAL].packets; break; case XT_CONNBYTES_DIR_REPLY: - what = div64_64(counters[IP_CT_DIR_REPLY].bytes, - counters[IP_CT_DIR_REPLY].packets); + bytes = counters[IP_CT_DIR_REPLY].bytes; + pkts = counters[IP_CT_DIR_REPLY].packets; break; case XT_CONNBYTES_DIR_BOTH: - { - u_int64_t bytes; - u_int64_t pkts; - bytes = counters[IP_CT_DIR_ORIGINAL].bytes + - counters[IP_CT_DIR_REPLY].bytes; - pkts = counters[IP_CT_DIR_ORIGINAL].packets+ - counters[IP_CT_DIR_REPLY].packets; - - /* FIXME_THEORETICAL: what to do if sum - * overflows ? */ - - what = div64_64(bytes, pkts); - } + bytes = counters[IP_CT_DIR_ORIGINAL].bytes + + counters[IP_CT_DIR_REPLY].bytes; + pkts = counters[IP_CT_DIR_ORIGINAL].packets + + counters[IP_CT_DIR_REPLY].packets; break; } + if (pkts != 0) + what = div64_64(bytes, pkts); break; } ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: PANIC: divide by zero in xt_connbytes 2007-01-26 17:24 ` Patrick McHardy @ 2007-01-26 20:11 ` KOVACS Krisztian 2007-01-27 16:36 ` Patrick McHardy 0 siblings, 1 reply; 10+ messages in thread From: KOVACS Krisztian @ 2007-01-26 20:11 UTC (permalink / raw) To: Patrick McHardy; +Cc: netfilter-devel, Pablo Neira Ayuso Hi, On Friday 26 January 2007 18:24, Patrick McHardy wrote: > I'm wondering what value to use when packets == 0 though, > it can't happen for the first packet of a connection since > it has already been accounted for before we can match, so > the packets counter must have overflown at least once (and > the byte counter at least as often as the packet counter). Ok, but what happens if you match on reply packets? I'm quite sure something like this will trigger a crash as soon as a new connection arrives: # iptables -A INPUT -m connbytes --connbytes 100: --connbytes-dir \ reply --connbytes-mode avgpkt -j ACCEPT -- Regards, Krisztian Kovacs ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: PANIC: divide by zero in xt_connbytes 2007-01-26 20:11 ` KOVACS Krisztian @ 2007-01-27 16:36 ` Patrick McHardy 0 siblings, 0 replies; 10+ messages in thread From: Patrick McHardy @ 2007-01-27 16:36 UTC (permalink / raw) To: KOVACS Krisztian; +Cc: netfilter-devel, Pablo Neira Ayuso KOVACS Krisztian wrote: > On Friday 26 January 2007 18:24, Patrick McHardy wrote: > >>I'm wondering what value to use when packets == 0 though, >>it can't happen for the first packet of a connection since >>it has already been accounted for before we can match, so >>the packets counter must have overflown at least once (and >>the byte counter at least as often as the packet counter). > > > Ok, but what happens if you match on reply packets? I'm quite sure > something like this will trigger a crash as soon as a new connection > arrives: > > # iptables -A INPUT -m connbytes --connbytes 100: --connbytes-dir \ > reply --connbytes-mode avgpkt -j ACCEPT You're right of course, I didn't think of that. The patches fixes this as well (using 0 as average value, which at least in the "no packets seen so far" case makes sense), but I'm going to fix the changelog. ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2007-01-27 16:36 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2007-01-18 10:59 PANIC: divide by zero in xt_connbytes Jonas Berlin 2007-01-18 11:56 ` Bugzilla webserver misconfigured? (was: Re: PANIC: divide by zero in xt_connbytes) Maximilian Wilhelm 2007-01-19 2:43 ` Bugzilla webserver misconfigured? Jonas Berlin 2007-01-18 13:28 ` PANIC: divide by zero in xt_connbytes Pablo Neira Ayuso 2007-01-18 13:38 ` Jonas Berlin 2007-01-18 14:22 ` KOVACS Krisztian 2007-01-22 8:52 ` KOVACS Krisztian 2007-01-26 17:24 ` Patrick McHardy 2007-01-26 20:11 ` KOVACS Krisztian 2007-01-27 16:36 ` Patrick McHardy
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).