Re: [PATCH] add new iptables ipt_connbytes match
Amin Azez wrote: Work well done, so oughtn't div64_64 to go in include/asm-generic/div64.h one day, to be available kernel wide, as do_div64_64 I see that net/core/pktgen.c is full of 64 bit division and maybe could benefit from this, and perhaps a version that does remainders too. So far nothing besides ipt_connbytes and pktgen seems to need it, and they use different versions. I'd rather leave it where it is to not encourage more use of it. - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] add new iptables ipt_connbytes match
Work well done, so oughtn't div64_64 to go in include/asm-generic/div64.h one day, to be available kernel wide, as do_div64_64 I see that net/core/pktgen.c is full of 64 bit division and maybe could benefit from this, and perhaps a version that does remainders too. Amin Patrick McHardy wrote: Harald Welte wrote: Just send two incremental patches to Dave. Here they are. The first patch fixes the div64_64 function, the second one renames some constants. [NETFILTER]: Fix div64_64 in ipt_connbytes Signded-off-by: Patrick McHardy [EMAIL PROTECTED] --- commit 62084bc1a04e2fbc492566fa30997bd0a7aa2d0a tree 083c8042609e0da81f0be9e15583d5d31b54e685 parent 68e734a5864ba568058c3b8ea63fac3d7d567542 author Patrick McHardy [EMAIL PROTECTED] Sat, 13 Aug 2005 03:16:32 +0200 committer Patrick McHardy [EMAIL PROTECTED] Sat, 13 Aug 2005 03:16:32 +0200 net/ipv4/netfilter/ipt_connbytes.c | 22 +- 1 files changed, 9 insertions(+), 13 deletions(-) diff --git a/net/ipv4/netfilter/ipt_connbytes.c b/net/ipv4/netfilter/ipt_connbytes.c --- a/net/ipv4/netfilter/ipt_connbytes.c +++ b/net/ipv4/netfilter/ipt_connbytes.c @@ -22,23 +22,19 @@ MODULE_AUTHOR(Harald Welte [EMAIL PROTECTED] MODULE_DESCRIPTION(iptables match for matching number of pkts/bytes per connection); /* 64bit divisor, dividend and result. dynamic precision */ -static u_int64_t div64_64(u_int64_t divisor, u_int64_t dividend) +static u_int64_t div64_64(u_int64_t dividend, u_int64_t divisor) ... - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] add new iptables ipt_connbytes match
On Sat, Aug 13, 2005 at 03:20:06AM +0200, Patrick McHardy wrote: Harald Welte wrote: Just send two incremental patches to Dave. Here they are. The first patch fixes the div64_64 function, the second one renames some constants. Ok, just in case Dave was waiting for my comments (which are usually not required since Patricks patches tend to have a higher quality than mine): ACK-ed-by: Harald Welte [EMAIL PROTECTED] -- - Harald Welte [EMAIL PROTECTED] http://netfilter.org/ Fragmentation is like classful addressing -- an interesting early architectural error that shows how much experimentation was going on while IP was being designed.-- Paul Vixie pgpDaohn4PDtx.pgp Description: PGP signature
Re: [PATCH] add new iptables ipt_connbytes match
On Fri, Aug 12, 2005 at 12:09:04PM -0700, David S. Miller wrote: From: Harald Welte [EMAIL PROTECTED] Date: Fri, 12 Aug 2005 21:03:43 +0200 Ok, I hope everyone is fine with this patch: It is, but I did not add the connbytes patch into my tree so I can't use this patch as-is. That's why I replied this is broken, fix u64 alignment to the connbytes patch instead of applied, thanks :-) Please untangle this stuff. This is how we end up with a big mess of noise changesets in the tree, due to how we have been putting half-working changes in first then a bunch of fixup patches. I'd like to avoid that, because I then spend a lot of time redoing things when I rebase the tree later. Fine, I can understand that in the case of the connbytes/unaligned case. Speaking more generally: I know I've been the exact opposite with all the nfnetlink_* stuff. But the problem is to a certain degree that bugs aren't discovered until it's in some tree that a lot of people use... I think our whole usual strategy 'put it in patch-o-matic and wait until bugs have been shaken out' doesn't really work because there's too little people actually using the code from there. So for new development, I'm now more inclined to push things sooner to you - even more for code that only adds new featurss. If you generally dislike that, please let me know. So in this case, send me the aligned_u64 patch seperately which doesn't assume connbytes is in the tree. Then another patch which adds connbytes with the proper usage of aligned_u64. sure. no problem. Will be sent soon. -- - Harald Welte [EMAIL PROTECTED] http://netfilter.org/ Fragmentation is like classful addressing -- an interesting early architectural error that shows how much experimentation was going on while IP was being designed.-- Paul Vixie pgpFPXD2h0WcZ.pgp Description: PGP signature
Re: [PATCH] add new iptables ipt_connbytes match
From: Patrick McHardy [EMAIL PROTECTED] Subject: Re: [PATCH] add new iptables ipt_connbytes match Date: Sat, 13 Aug 2005 03:20:06 +0200 Harald Welte wrote: Just send two incremental patches to Dave. Here they are. The first patch fixes the div64_64 function, the second one renames some constants. Both applied, thanks Patrick. - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] add new iptables ipt_connbytes match
From: Harald Welte [EMAIL PROTECTED] Date: Sat, 13 Aug 2005 16:51:57 +0200 Ok, just in case Dave was waiting for my comments (which are usually not required since Patricks patches tend to have a higher quality than mine): ACK-ed-by: Harald Welte [EMAIL PROTECTED] I like to see ACKs, because it shows explicit agreement amongst the various developers involved. Thanks. - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] add new iptables ipt_connbytes match
From: Harald Welte [EMAIL PROTECTED] Date: Sat, 13 Aug 2005 16:50:23 +0200 So for new development, I'm now more inclined to push things sooner to you - even more for code that only adds new featurss. If you generally dislike that, please let me know. I think this is the way to go. - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] add new iptables ipt_connbytes match
From: Harald Welte [EMAIL PROTECTED] Date: Sat, 13 Aug 2005 17:46:19 +0200 [NETFILTER] Add new iptables connbytes match Applied. - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] add new iptables ipt_connbytes match
Andi Kleen wrote: David S. Miller [EMAIL PROTECTED] writes: Won't work in x86 -- x86_64 compat environments. Thanks for catching it. The aligned u64 trick probably will #define aligned_u64 unsigned long long __attribute__((aligned(8))) It just forces i386 to be aligned too. Then use aligned_u64 instead of u64/__u64/u_int64_t in all user visible places. Similar for signed types. Unfortunately one of the iptables structures which is needed to get the ruleset in the kernel (ipt_replace) is differently sized when compiled for 32/64 bit. IIRC it doesn't work at all currently. - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] add new iptables ipt_connbytes match
On Fri, Aug 12, 2005 at 04:52:49AM +0200, Patrick McHardy wrote: This functions looks broken. I feared it... Divisor and divident are mixed up, the shifted result variable is not used in the actual division, the first bit has to be 32 assumption is wrong and num_shift is calculated incorrectly. To find a 32-bit divisor consisting of the most-significant 32 bits we need to find the highest bit set and subtract 32 from this, then right-shift by that value if it is larger than 0. I can send a fixed patch tomorrow but I'm too tired now. Thanks. +case IPT_CONNBYTES_WHAT_PKTS: I would really prefer the name IPT_CONNBYTES_PKTS :) I _think_ it's sure to change it, since we don't include ipt_connbytes.h in the iptables package. Just send two incremental patches to Dave. Cheers, Harald -- - Harald Welte [EMAIL PROTECTED] http://netfilter.org/ Fragmentation is like classful addressing -- an interesting early architectural error that shows how much experimentation was going on while IP was being designed.-- Paul Vixie pgpgwRWcZpriU.pgp Description: PGP signature
Re: [PATCH] add new iptables ipt_connbytes match
On Fri, Aug 12, 2005 at 02:03:20PM +0200, Andi Kleen wrote:
Unfortunately one of the iptables structures which is needed to get the
ruleset in the kernel (ipt_replace) is differently sized when compiled
for 32/64 bit. IIRC it doesn't work at all currently.
Yes that's the old bug and cannot be fixed without breaking compatibility.
But we hope that ctnetlink will not repeat that mistake. That is why I'm
suggesting
to use aligned_u64 in all new interfaces
I'll soon push a patch for all nfnetlink_{conntrack,queue,log} stuff for
net-2.6.14. Don't worry about that.
But getting back to the original connbytes issue. Is it worth fixing
it, if the core iptables doesn't even work (the old bug)?
I don't think that we're ever going to fix that bug in the old
{get,set}sockopt interface, but rather introduce a netlink interface
when pkt_tables matures.
--
- Harald Welte [EMAIL PROTECTED] http://netfilter.org/
Fragmentation is like classful addressing -- an interesting early
architectural error that shows how much experimentation was going
on while IP was being designed.-- Paul Vixie
pgp23UNUvw65R.pgp
Description: PGP signature
Re: [PATCH] add new iptables ipt_connbytes match
I don't think that we're ever going to fix that bug in the old
{get,set}sockopt interface, but rather introduce a netlink interface
when pkt_tables matures.
All new interfaces should be emulation clean, so that if the old interface
is replaced later it should eventually work. The best way to do that
is to use aligned_u64. Should probably put that into linux/types.h
-Andi
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] add new iptables ipt_connbytes match
On Fri, Aug 12, 2005 at 08:23:55PM +0200, Andi Kleen wrote:
I don't think that we're ever going to fix that bug in the old
{get,set}sockopt interface, but rather introduce a netlink interface
when pkt_tables matures.
All new interfaces should be emulation clean, so that if the old interface
is replaced later it should eventually work. The best way to do that
is to use aligned_u64. Should probably put that into linux/types.h
Ok, I hope everyone is fine with this patch:
--
- Harald Welte [EMAIL PROTECTED] http://netfilter.org/
Fragmentation is like classful addressing -- an interesting early
architectural error that shows how much experimentation was going
on while IP was being designed.-- Paul Vixie
[NETFILTER] introduce and use aligned_u64 data type
As proposed by Andi Kleen, this is required esp. for x86_64 architecture,
where 64bit code needs 8byte aligned 64bit data types, but 32bit userspace
apps will only align to 4bytes.
Signed-off-by: Harald Welte [EMAIL PROTECTED]
---
commit 30da9a3da187af74b2e2d00becf2d9cab3624ddd
tree 7666f6ce67e96beedc8884f1aba18ea80a20e2b1
parent 7c249f391a3b9bc86ec07d734959c532a3c7a3f6
author Harald Welte [EMAIL PROTECTED] Fr, 12 Aug 2005 21:00:28 +0200
committer Harald Welte [EMAIL PROTECTED] Fr, 12 Aug 2005 21:00:28 +0200
include/linux/netfilter/nfnetlink_log.h |5 +++--
include/linux/netfilter/nfnetlink_queue.h|5 +++--
include/linux/netfilter_ipv4/ipt_connbytes.h |4 ++--
include/linux/types.h|3 +++
4 files changed, 11 insertions(+), 6 deletions(-)
diff --git a/include/linux/netfilter/nfnetlink_log.h
b/include/linux/netfilter/nfnetlink_log.h
--- a/include/linux/netfilter/nfnetlink_log.h
+++ b/include/linux/netfilter/nfnetlink_log.h
@@ -5,6 +5,7 @@
* and not any kind of function definitions. It is shared between kernel and
* userspace. Don't put kernel specific stuff in here */
+#include linux/types.h
#include linux/netfilter/nfnetlink.h
enum nfulnl_msg_types {
@@ -27,8 +28,8 @@ struct nfulnl_msg_packet_hw {
} __attribute__ ((packed));
struct nfulnl_msg_packet_timestamp {
- u_int64_t sec;
- u_int64_t usec;
+ aligned_u64 sec;
+ aligned_u64 usec;
} __attribute__ ((packed));
#define NFULNL_PREFIXLEN 30 /* just like old log target */
diff --git a/include/linux/netfilter/nfnetlink_queue.h
b/include/linux/netfilter/nfnetlink_queue.h
--- a/include/linux/netfilter/nfnetlink_queue.h
+++ b/include/linux/netfilter/nfnetlink_queue.h
@@ -1,6 +1,7 @@
#ifndef _NFNETLINK_QUEUE_H
#define _NFNETLINK_QUEUE_H
+#include linux/types.h
#include linux/netfilter/nfnetlink.h
enum nfqnl_msg_types {
@@ -24,8 +25,8 @@ struct nfqnl_msg_packet_hw {
} __attribute__ ((packed));
struct nfqnl_msg_packet_timestamp {
- u_int64_t sec;
- u_int64_t usec;
+ aligned_u64 sec;
+ aligned_u64 usec;
} __attribute__ ((packed));
enum nfqnl_attr_type {
diff --git a/include/linux/netfilter_ipv4/ipt_connbytes.h
b/include/linux/netfilter_ipv4/ipt_connbytes.h
--- a/include/linux/netfilter_ipv4/ipt_connbytes.h
+++ b/include/linux/netfilter_ipv4/ipt_connbytes.h
@@ -16,8 +16,8 @@ enum ipt_connbytes_direction {
struct ipt_connbytes_info
{
struct {
- u_int64_t from; /* count to be matched */
- u_int64_t to; /* count to be matched */
+ aligned_u64 from; /* count to be matched */
+ aligned_u64 to; /* count to be matched */
} count;
u_int8_t what; /* ipt_connbytes_what */
u_int8_t direction; /* ipt_connbytes_direction */
diff --git a/include/linux/types.h b/include/linux/types.h
--- a/include/linux/types.h
+++ b/include/linux/types.h
@@ -123,6 +123,9 @@ typedef __u64 u_int64_t;
typedef__s64 int64_t;
#endif
+/* this is a special 64bit data type that is 8-byte aligned */
+#define aligned_u64 unsigned long long __attribute__((aligned(8)))
+
/*
* The type used for indexing onto a disc or disc partition.
* If required, asm/types.h can override it and define
pgpTd8fpnsZCU.pgp
Description: PGP signature
Re: [PATCH] add new iptables ipt_connbytes match
From: Harald Welte [EMAIL PROTECTED] Date: Fri, 12 Aug 2005 21:03:43 +0200 Ok, I hope everyone is fine with this patch: It is, but I did not add the connbytes patch into my tree so I can't use this patch as-is. That's why I replied this is broken, fix u64 alignment to the connbytes patch instead of applied, thanks :-) Please untangle this stuff. This is how we end up with a big mess of noise changesets in the tree, due to how we have been putting half-working changes in first then a bunch of fixup patches. I'd like to avoid that, because I then spend a lot of time redoing things when I rebase the tree later. So in this case, send me the aligned_u64 patch seperately which doesn't assume connbytes is in the tree. Then another patch which adds connbytes with the proper usage of aligned_u64. Thanks Harald. - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] add new iptables ipt_connbytes match
From: Harald Welte [EMAIL PROTECTED]
Date: Thu, 11 Aug 2005 22:03:49 +0200
+struct ipt_connbytes_info
+{
+ struct {
+ u_int64_t from; /* count to be matched */
+ u_int64_t to; /* count to be matched */
+ } count;
+ u_int8_t what; /* ipt_connbytes_what */
+ u_int8_t direction; /* ipt_connbytes_direction */
+};
Won't work in x86 -- x86_64 compat environments.
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] add new iptables ipt_connbytes match
Harald Welte wrote:
+/* 64bit divisor, dividend and result. dynamic precision */
+static u_int64_t div64_64(u_int64_t divisor, u_int64_t dividend)
+{
+ u_int64_t result = divisor;
+
+ if (dividend 0x) {
+ int first_bit = find_first_bit((unsigned long *) dividend,
sizeof(dividend));
+ /* calculate number of bits to shift. shift exactly enough
+* bits to make dividend fit in 32bits. */
+ int num_shift = (64 - 32 - first_bit);
+ /* first bit has to be 32, since dividend was 0x */
+ result = result num_shift;
+ dividend = dividend num_shift;
+ }
+
+ do_div(divisor, dividend);
+
+ return divisor;
+}
This functions looks broken. Divisor and divident are mixed up, the
shifted result variable is not used in the actual division, the
first bit has to be 32 assumption is wrong and num_shift is
calculated incorrectly. To find a 32-bit divisor consisting of the
most-significant 32 bits we need to find the highest bit set and
subtract 32 from this, then right-shift by that value if it is larger
than 0. I can send a fixed patch tomorrow but I'm too tired now.
+ case IPT_CONNBYTES_WHAT_PKTS:
I would really prefer the name IPT_CONNBYTES_PKTS :)
Regards
Patrick
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
