All of lore.kernel.org
 help / color / mirror / Atom feed
* xt_ACCOUNT and big endian machines
@ 2009-10-23 14:23 Thomas Jarosch
  2009-10-23 15:48 ` Jan Engelhardt
  0 siblings, 1 reply; 14+ messages in thread
From: Thomas Jarosch @ 2009-10-23 14:23 UTC (permalink / raw)
  To: Jan Engelhardt; +Cc: Netfilter Developer Mailing List

[-- Attachment #1: Type: Text/Plain, Size: 314 bytes --]

Hello Jan,

a user of ipt_ACCOUNT recently sent me some patches regarding
big endian support in ipt_ACCOUNT/xt_ACCOUNT:
http://developer.intra2net.com/mailarchive/html/ipt_ACCOUNT/2009/msg00014.html

I'm wondering if this is the best approach to support big endian machines
with all those #ifdefs?

Cheers,
Thomas

[-- Attachment #2: ipt_ACCOUNT-1.15-byteorder.patch --]
[-- Type: text/x-patch, Size: 4864 bytes --]

diff -ru ACCOUNT/linux-2.6/net/ipv4/netfilter/ipt_ACCOUNT.c ACCOUNT.byteorder/linux-2.6/net/ipv4/netfilter/ipt_ACCOUNT.c
--- ACCOUNT/linux-2.6/net/ipv4/netfilter/ipt_ACCOUNT.c	2009-04-08 15:35:42.000000000 +0200
+++ ACCOUNT.byteorder/linux-2.6/net/ipv4/netfilter/ipt_ACCOUNT.c	2009-10-13 15:11:52.000000000 +0200
@@ -32,6 +32,7 @@
 #include <linux/string.h>
 #include <linux/spinlock.h>
 #include <asm/uaccess.h>
+#include <asm/byteorder.h>
 
 #include <net/route.h>
 #include <linux/netfilter_ipv4/ipt_ACCOUNT.h>
@@ -362,8 +363,15 @@
     }
 
     /* Calculate array positions */
+#if defined(__LITTLE_ENDIAN_BITFIELD)
     src_slot = (unsigned char)((src_ip&0xFF000000) >> 24);
     dst_slot = (unsigned char)((dst_ip&0xFF000000) >> 24);
+#elif defined(__BIG_ENDIAN_BITFIELD)
+    src_slot = (unsigned char)(src_ip&0x000000FF);
+    dst_slot = (unsigned char)(dst_ip&0x000000FF);
+#else
+#error  "Please fix <asm/byteorder.h>"
+#endif
 
     /* Increase size counters */
     if (is_src) {
@@ -414,7 +422,13 @@
 {
     /* Do we need to process src IP? */
     if ((net_ip&netmask) == (src_ip&netmask)) {
+#if defined(__LITTLE_ENDIAN_BITFIELD)
         unsigned char slot = (unsigned char)((src_ip&0x00FF0000) >> 16);
+#elif defined(__BIG_ENDIAN_BITFIELD)
+        unsigned char slot = (unsigned char)((src_ip&0x0000FF00) >> 8);
+#else
+#error  "Please fix <asm/byteorder.h>"
+#endif
         DEBUGP("ACCOUNT: Calculated SRC 16 bit network slot: %d\n", slot);
 
         /* Do we need to create a new mask_24 bucket? */
@@ -430,7 +444,13 @@
 
     /* Do we need to process dst IP? */
     if ((net_ip&netmask) == (dst_ip&netmask)) {
+#if defined(__LITTLE_ENDIAN_BITFIELD)
         unsigned char slot = (unsigned char)((dst_ip&0x00FF0000) >> 16);
+#elif defined(__BIG_ENDIAN_BITFIELD)
+        unsigned char slot = (unsigned char)((dst_ip&0x0000FF00) >> 8);
+#else
+#error  "Please fix <asm/byteorder.h>"
+#endif
         DEBUGP("ACCOUNT: Calculated DST 16 bit network slot: %d\n", slot);
 
         /* Do we need to create a new mask_24 bucket? */
@@ -452,7 +472,13 @@
 {
     /* Do we need to process src IP? */
     if ((net_ip&netmask) == (src_ip&netmask)) {
+#if defined(__LITTLE_ENDIAN_BITFIELD)
         unsigned char slot = (unsigned char)((src_ip&0x0000FF00) >> 8);
+#elif defined(__BIG_ENDIAN_BITFIELD)
+        unsigned char slot = (unsigned char)((src_ip&0x00FF0000) >> 16);
+#else
+#error  "Please fix <asm/byteorder.h>"
+#endif
         DEBUGP("ACCOUNT: Calculated SRC 24 bit network slot: %d\n", slot);
 
         /* Do we need to create a new mask_24 bucket? */
@@ -468,7 +494,13 @@
 
     /* Do we need to process dst IP? */
     if ((net_ip&netmask) == (dst_ip&netmask)) {
+#if defined(__LITTLE_ENDIAN_BITFIELD)
         unsigned char slot = (unsigned char)((dst_ip&0x0000FF00) >> 8);
+#elif defined(__BIG_ENDIAN_BITFIELD)
+        unsigned char slot = (unsigned char)((dst_ip&0x00FF0000) >> 16);
+#else
+#error  "Please fix <asm/byteorder.h>"
+#endif
         DEBUGP("ACCOUNT: Calculated DST 24 bit network slot: %d\n", slot);
 
         /* Do we need to create a new mask_24 bucket? */
@@ -789,7 +821,13 @@
     
     for (i = 0; i <= 255; i++) {
         if (data->ip[i].src_packets || data->ip[i].dst_packets) {
+#if defined(__LITTLE_ENDIAN_BITFIELD)
             handle_ip.ip = net_ip | net_OR_mask | (i<<24);
+#elif defined(__BIG_ENDIAN_BITFIELD)
+            handle_ip.ip = net_ip | net_OR_mask | i;
+#else
+#error  "Please fix <asm/byteorder.h>"
+#endif
             
             handle_ip.src_packets = data->ip[i].src_packets;
             handle_ip.src_bytes = data->ip[i].src_bytes;
@@ -862,7 +900,15 @@
                 struct ipt_acc_mask_24 *network = 
                     (struct ipt_acc_mask_24*)network_16->mask_24[b];
                 if (ipt_acc_handle_copy_data(to_user, &to_user_pos,
-                                      &tmpbuf_pos, network, net_ip, (b << 16)))
+                                      &tmpbuf_pos, network, net_ip,
+#if defined(__LITTLE_ENDIAN_BITFIELD)
+				     (b << 16)
+#elif defined(__BIG_ENDIAN_BITFIELD)
+				     (b << 8)
+#else
+#error  "Please fix <asm/byteorder.h>"
+#endif
+		))
                     return -1;
             }
         }
@@ -890,7 +936,15 @@
                             (struct ipt_acc_mask_24*)network_16->mask_24[b];
                         if (ipt_acc_handle_copy_data(to_user,
                                        &to_user_pos, &tmpbuf_pos,
-                                       network, net_ip, (a << 8) | (b << 16)))
+                                       network, net_ip,
+#if defined(__LITTLE_ENDIAN_BITFIELD)
+				       (a << 8) | (b << 16)
+#elif defined(__BIG_ENDIAN_BITFIELD)
+				       (a << 16) | (b << 8)
+#else
+#error  "Please fix <asm/byteorder.h>"
+#endif
+			))
                             return -1;
                     }
                 }

[-- Attachment #3: libipt_ACCOUNT-1.3-byteorder.patch --]
[-- Type: text/x-patch, Size: 677 bytes --]

diff -ru libipt_ACCOUNT-1.3/iptaccount/iptaccount.c libipt_ACCOUNT-1.3.byteorder/iptaccount/iptaccount.c
--- libipt_ACCOUNT-1.3/iptaccount/iptaccount.c	2007-12-14 10:42:42.000000000 +0100
+++ libipt_ACCOUNT-1.3.byteorder/iptaccount/iptaccount.c	2009-10-13 15:13:05.000000000 +0200
@@ -34,8 +34,9 @@
     static char buf[17];
     const unsigned char *bytep;
 
+    addr = htonl(addr);
     bytep = (const unsigned char *) &addr;
-    snprintf(buf, 16, "%u.%u.%u.%u", bytep[0], bytep[1], bytep[2], bytep[3]);
+    snprintf(buf, 16, "%u.%u.%u.%u", ((addr&0xFF000000)>>24), ((addr&0x00FF0000)>>16), ((addr&0x0000FF00)>>8),(addr&0x000000FF));
     buf[16] = 0;
     return buf;
 }

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: xt_ACCOUNT and big endian machines
  2009-10-23 14:23 xt_ACCOUNT and big endian machines Thomas Jarosch
@ 2009-10-23 15:48 ` Jan Engelhardt
  2009-10-23 15:49   ` Jan Engelhardt
  2009-10-26  9:03   ` Thomas Jarosch
  0 siblings, 2 replies; 14+ messages in thread
From: Jan Engelhardt @ 2009-10-23 15:48 UTC (permalink / raw)
  To: Thomas Jarosch; +Cc: Netfilter Developer Mailing List


On Friday 2009-10-23 16:23, Thomas Jarosch wrote:

>Hello Jan,
>
>a user of ipt_ACCOUNT recently sent me some patches regarding
>big endian support in ipt_ACCOUNT/xt_ACCOUNT:
>http://developer.intra2net.com/mailarchive/html/ipt_ACCOUNT/2009/msg00014.html
>
>I'm wondering if this is the best approach to support big endian machines
>with all those #ifdefs?

The second is ok, though I dunno why he did not chose to do the
same in the first. Hence:

parent 304e5e52ca600c4d7bc68722a514c49bbac45195 (v1.19-8-g304e5e5)
commit d0fd08cd390fde4698010d7e50ffb5b8aea3fbeb
Author: Jan Engelhardt <jengelh@medozas.de>
Date:   Fri Oct 23 17:37:54 2009 +0200

ACCOUNT: correctly account for network-order addresses on BE arches
---
 extensions/ACCOUNT/xt_ACCOUNT.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/extensions/ACCOUNT/xt_ACCOUNT.c b/extensions/ACCOUNT/xt_ACCOUNT.c
index 34e16b1..251ebe1 100644
--- a/extensions/ACCOUNT/xt_ACCOUNT.c
+++ b/extensions/ACCOUNT/xt_ACCOUNT.c
@@ -293,8 +293,8 @@ static void ipt_acc_depth0_insert(struct ipt_acc_mask_24 *mask_24,
 	}
 
 	/* Calculate array positions */
-	src_slot = (src_ip & 0xFF000000) >> 24;
-	dst_slot = (dst_ip & 0xFF000000) >> 24;
+	src_slot = ntohl(src_ip) & 0xFF;
+	dst_slot = ntohl(dst_ip) & 0xFF;
 
 	/* Increase size counters */
 	if (is_src) {
@@ -345,7 +345,7 @@ static void ipt_acc_depth1_insert(struct ipt_acc_mask_16 *mask_16,
 {
 	/* Do we need to process src IP? */
 	if ((net_ip & netmask) == (src_ip & netmask)) {
-		unsigned char slot = (src_ip & 0x00FF0000) >> 16;
+		unsigned char slot = (ntohl(src_ip) & 0xFF00) >> 8;
 		pr_debug("ACCOUNT: Calculated SRC 16 bit network slot: %d\n", slot);
 
 		/* Do we need to create a new mask_24 bucket? */
@@ -361,7 +361,7 @@ static void ipt_acc_depth1_insert(struct ipt_acc_mask_16 *mask_16,
 
 	/* Do we need to process dst IP? */
 	if ((net_ip & netmask) == (dst_ip & netmask)) {
-		unsigned char slot = (dst_ip & 0x00FF0000) >> 16;
+		unsigned char slot = (ntohl(dst_ip) & 0xFF00) >> 8;
 		pr_debug("ACCOUNT: Calculated DST 16 bit network slot: %d\n", slot);
 
 		/* Do we need to create a new mask_24 bucket? */
@@ -383,7 +383,7 @@ static void ipt_acc_depth2_insert(struct ipt_acc_mask_8 *mask_8,
 {
 	/* Do we need to process src IP? */
 	if ((net_ip & netmask) == (src_ip & netmask)) {
-		unsigned char slot = (src_ip & 0x0000FF00) >> 8;
+		unsigned char slot = (ntohl(src_ip) & 0xFF0000) >> 16;
 		pr_debug("ACCOUNT: Calculated SRC 24 bit network slot: %d\n", slot);
 
 		/* Do we need to create a new mask_24 bucket? */
@@ -399,7 +399,7 @@ static void ipt_acc_depth2_insert(struct ipt_acc_mask_8 *mask_8,
 
 	/* Do we need to process dst IP? */
 	if ((net_ip & netmask) == (dst_ip & netmask)) {
-		unsigned char slot = (dst_ip & 0x0000FF00) >> 8;
+		unsigned char slot = (ntohl(dst_ip) & 0xFF0000) >> 16;
 		pr_debug("ACCOUNT: Calculated DST 24 bit network slot: %d\n", slot);
 
 		/* Do we need to create a new mask_24 bucket? */
-- 
# Created with git-export-patch

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: xt_ACCOUNT and big endian machines
  2009-10-23 15:48 ` Jan Engelhardt
@ 2009-10-23 15:49   ` Jan Engelhardt
  2009-10-26  9:03   ` Thomas Jarosch
  1 sibling, 0 replies; 14+ messages in thread
From: Jan Engelhardt @ 2009-10-23 15:49 UTC (permalink / raw)
  To: Thomas Jarosch; +Cc: Netfilter Developer Mailing List

On Friday 2009-10-23 17:48, Jan Engelhardt wrote:

>
>On Friday 2009-10-23 16:23, Thomas Jarosch wrote:
>
>>Hello Jan,
>>
>>a user of ipt_ACCOUNT recently sent me some patches regarding
>>big endian support in ipt_ACCOUNT/xt_ACCOUNT:
>>http://developer.intra2net.com/mailarchive/html/ipt_ACCOUNT/2009/msg00014.html
>>
>>I'm wondering if this is the best approach to support big endian machines
>>with all those #ifdefs?
>
>The second is ok, though I dunno why he did not chose to do the
>same in the first. Hence:

Though the iptaccount.c patch could have continued using the byte trick.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: xt_ACCOUNT and big endian machines
  2009-10-23 15:48 ` Jan Engelhardt
  2009-10-23 15:49   ` Jan Engelhardt
@ 2009-10-26  9:03   ` Thomas Jarosch
  2009-10-27 10:04     ` Jan Engelhardt
  1 sibling, 1 reply; 14+ messages in thread
From: Thomas Jarosch @ 2009-10-26  9:03 UTC (permalink / raw)
  To: Jan Engelhardt; +Cc: Netfilter Developer Mailing List

Hello Jan,

On Friday, 23. October 2009 17:48:04 you wrote:
> >I'm wondering if this is the best approach to support big endian
> > machines with all those #ifdefs?
> 
> The second is ok, though I dunno why he did not chose to do the
> same in the first. Hence:

Thanks for the fix! What about the left shift operations f.e. in 
ipt_acc_handle_get_data()? The original patch touched those, too.

Cheers,
Thomas

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: xt_ACCOUNT and big endian machines
  2009-10-26  9:03   ` Thomas Jarosch
@ 2009-10-27 10:04     ` Jan Engelhardt
  2009-10-27 10:49       ` Thomas Jarosch
  0 siblings, 1 reply; 14+ messages in thread
From: Jan Engelhardt @ 2009-10-27 10:04 UTC (permalink / raw)
  To: Thomas Jarosch; +Cc: Netfilter Developer Mailing List


On Monday 2009-10-26 10:03, Thomas Jarosch wrote:
>On Friday, 23. October 2009 17:48:04 you wrote:
>> >I'm wondering if this is the best approach to support big endian
>> > machines with all those #ifdefs?
>> 
>> The second is ok, though I dunno why he did not chose to do the
>> same in the first. Hence:
>
>Thanks for the fix! What about the left shift operations f.e. in 
>ipt_acc_handle_get_data()? The original patch touched those, too.

Adding this in.

diff --git a/extensions/ACCOUNT/iptaccount.c b/extensions/ACCOUNT/iptaccount.c
index 2d4556f..b4fddf9 100644
--- a/extensions/ACCOUNT/iptaccount.c
+++ b/extensions/ACCOUNT/iptaccount.c
@@ -39,8 +39,9 @@ char *addr_to_dotted(unsigned int addr)
 	static char buf[17];
 	const unsigned char *bytep;
 
+	addr = ntohl(addr);
 	bytep = (const unsigned char *)&addr;
-	snprintf(buf, 16, "%u.%u.%u.%u", bytep[0], bytep[1], bytep[2], bytep[3]);
+	snprintf(buf, 16, "%u.%u.%u.%u", bytep[3], bytep[2], bytep[1], bytep[0]);
 	buf[16] = 0;
 	return buf;
 }
diff --git a/extensions/ACCOUNT/xt_ACCOUNT.c b/extensions/ACCOUNT/xt_ACCOUNT.c
index 6154bbe..c7e5aae 100644
--- a/extensions/ACCOUNT/xt_ACCOUNT.c
+++ b/extensions/ACCOUNT/xt_ACCOUNT.c
@@ -744,7 +744,7 @@ static int ipt_acc_handle_copy_data(void *to_user, unsigned long *to_user_pos,
 
 	for (i = 0; i <= 255; i++) {
 		if (data->ip[i].src_packets || data->ip[i].dst_packets) {
-			handle_ip.ip = net_ip | net_OR_mask | (i << 24);
+			handle_ip.ip = htonl(net_ip | net_OR_mask | i);
 
 			handle_ip.src_packets = data->ip[i].src_packets;
 			handle_ip.src_bytes = data->ip[i].src_bytes;
@@ -796,7 +796,7 @@ static int ipt_acc_handle_get_data(uint32_t handle, void *to_user)
 		struct ipt_acc_mask_24 *network =
 			ipt_acc_handles[handle].data;
 		if (ipt_acc_handle_copy_data(to_user, &to_user_pos, &tmpbuf_pos,
-		    network, net_ip, 0))
+		    network, ntohl(net_ip), 0))
 			return -1;
 
 		/* Flush remaining data to userspace */
@@ -817,7 +817,7 @@ static int ipt_acc_handle_get_data(uint32_t handle, void *to_user)
 				struct ipt_acc_mask_24 *network =
 					network_16->mask_24[b];
 				if (ipt_acc_handle_copy_data(to_user, &to_user_pos,
-				    &tmpbuf_pos, network, net_ip, (b << 16)))
+				    &tmpbuf_pos, network, ntohl(net_ip), (b << 16)))
 					return -1;
 			}
 		}
@@ -845,7 +845,7 @@ static int ipt_acc_handle_get_data(uint32_t handle, void *to_user)
 							network_16->mask_24[b];
 						if (ipt_acc_handle_copy_data(to_user,
 						    &to_user_pos, &tmpbuf_pos,
-						    network, net_ip, (a << 8) | (b << 16)))
+						    network, ntohl(net_ip), (a << 16) | (b << 8)))
 							return -1;
 					}
 				}

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: xt_ACCOUNT and big endian machines
  2009-10-27 10:04     ` Jan Engelhardt
@ 2009-10-27 10:49       ` Thomas Jarosch
  2009-10-28 17:51         ` Jan Engelhardt
  0 siblings, 1 reply; 14+ messages in thread
From: Thomas Jarosch @ 2009-10-27 10:49 UTC (permalink / raw)
  To: Jan Engelhardt; +Cc: Netfilter Developer Mailing List

Hello Jan,

On Tuesday, 27. October 2009 11:04:38 Jan Engelhardt wrote:
> On Monday 2009-10-26 10:03, Thomas Jarosch wrote:
> >On Friday, 23. October 2009 17:48:04 you wrote:
> >> >I'm wondering if this is the best approach to support big endian
> >> > machines with all those #ifdefs?
> >>
> >> The second is ok, though I dunno why he did not chose to do the
> >> same in the first. Hence:
> >
> >Thanks for the fix! What about the left shift operations f.e. in
> >ipt_acc_handle_get_data()? The original patch touched those, too.
> 
> Adding this in.

Thanks! See one little remark below.

> @@ -817,7 +817,7 @@ static int ipt_acc_handle_get_data(uint32_t handle,
>  void *to_user) struct ipt_acc_mask_24 *network =
>  					network_16->mask_24[b];
>  				if (ipt_acc_handle_copy_data(to_user, &to_user_pos,
> -				    &tmpbuf_pos, network, net_ip, (b << 16)))
> +				    &tmpbuf_pos, network, ntohl(net_ip), (b << 16)))
>  					return -1;
>  			}
>  		}

I'm starting to get my mind ready for endian issues :)
The "net_OR_mask" is in host order if I understood it correctly,
so shouldn't this read "(b << 8)"?

Cheers,
Thomas

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: xt_ACCOUNT and big endian machines
  2009-10-27 10:49       ` Thomas Jarosch
@ 2009-10-28 17:51         ` Jan Engelhardt
  2009-10-30 16:07           ` Thomas Jarosch
  0 siblings, 1 reply; 14+ messages in thread
From: Jan Engelhardt @ 2009-10-28 17:51 UTC (permalink / raw)
  To: Thomas Jarosch; +Cc: Netfilter Developer Mailing List


On Tuesday 2009-10-27 11:49, Thomas Jarosch wrote:
>
>> @@ -817,7 +817,7 @@ static int ipt_acc_handle_get_data(uint32_t handle,
>>  void *to_user) struct ipt_acc_mask_24 *network =
>>  					network_16->mask_24[b];
>>  				if (ipt_acc_handle_copy_data(to_user, &to_user_pos,
>> -				    &tmpbuf_pos, network, net_ip, (b << 16)))
>> +				    &tmpbuf_pos, network, ntohl(net_ip), (b << 16)))
>>  					return -1;
>>  			}
>>  		}
>
>I'm starting to get my mind ready for endian issues :)
>The "net_OR_mask" is in host order if I understood it correctly,
>so shouldn't this read "(b << 8)"?

Yes it should be <<8.

Besides, "the 8 bit" comment is confusing, because both CIDR/8 and
CIDR/24 have eight bits either way - just one has eight 1s, one has
eight 0s.

    →   /* 8 bit network */ 
    →   if (depth == 0) { 
    →       →   struct ipt_acc_mask_24 *network = 


I am in the process of annotating the entire source with the BE
markers and I am already getting into situations where I think
"there's more bswapping amiss"
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: xt_ACCOUNT and big endian machines
  2009-10-28 17:51         ` Jan Engelhardt
@ 2009-10-30 16:07           ` Thomas Jarosch
  2009-10-30 17:49             ` Jan Engelhardt
  0 siblings, 1 reply; 14+ messages in thread
From: Thomas Jarosch @ 2009-10-30 16:07 UTC (permalink / raw)
  To: Jan Engelhardt; +Cc: Netfilter Developer Mailing List

On Wednesday, 28. October 2009 18:51:14 Jan Engelhardt wrote:
> >I'm starting to get my mind ready for endian issues :)
> >The "net_OR_mask" is in host order if I understood it correctly,
> >so shouldn't this read "(b << 8)"?
> 
> Yes it should be <<8.

Hehe :)

> Besides, "the 8 bit" comment is confusing, because both CIDR/8 and
> CIDR/24 have eight bits either way - just one has eight 1s, one has
> eight 0s.

True that. Didn't came up with a better description back
in the days when I wrote it.

> I am in the process of annotating the entire source with the BE
> markers and I am already getting into situations where I think
> "there's more bswapping amiss"

One more thing came to my mind: The new code transfers the data in
network order from kernel to userspace. This will break existing 
applications which make use of libipt_ACCOUNT like the included "iptaccount"
tool or other applications. We should keep it in host order if possible
or transform it inside libipt_ACCOUNT. Even if we transform it,
it will break systems without updated libipt_ACCOUNT versions.

What do you think?

Cheers,
Thomas

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: xt_ACCOUNT and big endian machines
  2009-10-30 16:07           ` Thomas Jarosch
@ 2009-10-30 17:49             ` Jan Engelhardt
  2009-11-04  8:42               ` Thomas Jarosch
  0 siblings, 1 reply; 14+ messages in thread
From: Jan Engelhardt @ 2009-10-30 17:49 UTC (permalink / raw)
  To: Thomas Jarosch; +Cc: Netfilter Developer Mailing List


On Friday 2009-10-30 17:07, Thomas Jarosch wrote:
>
>> I am in the process of annotating the entire source with the BE
>> markers and I am already getting into situations where I think
>> "there's more bswapping amiss"
>
>One more thing came to my mind: The new code transfers the data in
>network order from kernel to userspace. This will break existing 
>applications which make use of libipt_ACCOUNT like the included "iptaccount"
>tool or other applications. We should keep it in host order if possible
>or transform it inside libipt_ACCOUNT. Even if we transform it,
>it will break systems without updated libipt_ACCOUNT versions.
>
>What do you think?

I think it is a non-issue, because all three of libxt_ACCOUNT,
xt_ACCOUNT.ko and iptaccount are updated at the same time.
Especially so if you are an Xtables-addons user!

I will bump the .revision too so that the chance of accidental 
collisions is lowered.

Transfer via IPT_SO_... still uses host order.

And I went through the source and annotated it with appropriate __be 
markers plus a htonl->ntohl fix.


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: xt_ACCOUNT and big endian machines
  2009-10-30 17:49             ` Jan Engelhardt
@ 2009-11-04  8:42               ` Thomas Jarosch
  2009-11-04  9:27                 ` Jan Engelhardt
  0 siblings, 1 reply; 14+ messages in thread
From: Thomas Jarosch @ 2009-11-04  8:42 UTC (permalink / raw)
  To: Jan Engelhardt; +Cc: Netfilter Developer Mailing List

Hello Jan,

On Friday, 30. October 2009 18:49:21 you wrote:
> >What do you think?
> 
> I think it is a non-issue, because all three of libxt_ACCOUNT,
> xt_ACCOUNT.ko and iptaccount are updated at the same time.
> Especially so if you are an Xtables-addons user!

Yes, though this would break external applications that use
libxt_ACCOUNT as they expect the data to be in host order.
Not everyone is just using the command line "iptaccount" tool.

> I will bump the .revision too so that the chance of accidental
> collisions is lowered.

Ok

> Transfer via IPT_SO_... still uses host order.

>From the new code:

xt_ACCOUNT.c: ipt_acc_handle_copy_data():
handle_ip.ip = htonl(net_ip | net_OR_mask | i);

This really looks like network order for
the kernel -> userspace transfer.

-> Breakage for libxt_ACCOUNT library users.

> And I went through the source and annotated it with appropriate __be
> markers plus a htonl->ntohl fix.

Thanks!

Cheers,
Thomas

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: xt_ACCOUNT and big endian machines
  2009-11-04  8:42               ` Thomas Jarosch
@ 2009-11-04  9:27                 ` Jan Engelhardt
  2009-11-04  9:45                   ` Thomas Jarosch
  0 siblings, 1 reply; 14+ messages in thread
From: Jan Engelhardt @ 2009-11-04  9:27 UTC (permalink / raw)
  To: Thomas Jarosch; +Cc: Netfilter Developer Mailing List


On Wednesday 2009-11-04 09:42, Thomas Jarosch wrote:
>On Friday, 30. October 2009 18:49:21 you wrote:
>
>> Transfer via IPT_SO_... still uses host order.
>
>From the new code:
>
>xt_ACCOUNT.c: ipt_acc_handle_copy_data():
>handle_ip.ip = htonl(net_ip | net_OR_mask | i);
>
>This really looks like network order for
>the kernel -> userspace transfer.

Blame that on the original submission, which changed that ;-)

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: xt_ACCOUNT and big endian machines
  2009-11-04  9:27                 ` Jan Engelhardt
@ 2009-11-04  9:45                   ` Thomas Jarosch
  2009-11-04 22:38                     ` Jan Engelhardt
  0 siblings, 1 reply; 14+ messages in thread
From: Thomas Jarosch @ 2009-11-04  9:45 UTC (permalink / raw)
  To: Jan Engelhardt; +Cc: Netfilter Developer Mailing List

On Wednesday, 4. November 2009 10:27:56 you wrote:
> >> Transfer via IPT_SO_... still uses host order.
> >
> >From the new code:
> >
> >xt_ACCOUNT.c: ipt_acc_handle_copy_data():
> >handle_ip.ip = htonl(net_ip | net_OR_mask | i);
> >
> >This really looks like network order for
> >the kernel -> userspace transfer.
> 
> Blame that on the original submission, which changed that ;-)

Would you bet on it? ;)

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: xt_ACCOUNT and big endian machines
  2009-11-04  9:45                   ` Thomas Jarosch
@ 2009-11-04 22:38                     ` Jan Engelhardt
  2009-11-05 16:06                       ` Thomas Jarosch
  0 siblings, 1 reply; 14+ messages in thread
From: Jan Engelhardt @ 2009-11-04 22:38 UTC (permalink / raw)
  To: Thomas Jarosch; +Cc: Netfilter Developer Mailing List


On Wednesday 2009-11-04 10:45, Thomas Jarosch wrote:
>On Wednesday, 4. November 2009 10:27:56 you wrote:
>> >> Transfer via IPT_SO_... still uses host order.
>> >
>> >From the new code:
>> >
>> >xt_ACCOUNT.c: ipt_acc_handle_copy_data():
>> >handle_ip.ip = htonl(net_ip | net_OR_mask | i);
>> >
>> >This really looks like network order for
>> >the kernel -> userspace transfer.
>> 
>> Blame that on the original submission, which changed that ;-)
>
>Would you bet on it? ;)
>
No, but I can fix it anyway.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: xt_ACCOUNT and big endian machines
  2009-11-04 22:38                     ` Jan Engelhardt
@ 2009-11-05 16:06                       ` Thomas Jarosch
  0 siblings, 0 replies; 14+ messages in thread
From: Thomas Jarosch @ 2009-11-05 16:06 UTC (permalink / raw)
  To: Jan Engelhardt; +Cc: Netfilter Developer Mailing List

On Wednesday, 4. November 2009 23:38:03 Jan Engelhardt wrote:
> >> >This really looks like network order for
> >> >the kernel -> userspace transfer.
> >>
> >> Blame that on the original submission, which changed that ;-)
> >
> >Would you bet on it? ;)
> 
> No, but I can fix it anyway.

Thanks for fixing it! I'll port your changes to my tree (including proper 
credits) the next days and will give the new code a test run.
Leonardo Secci will also test the changes on a big endian box.

Cheers,
Thomas

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2009-11-05 16:06 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-10-23 14:23 xt_ACCOUNT and big endian machines Thomas Jarosch
2009-10-23 15:48 ` Jan Engelhardt
2009-10-23 15:49   ` Jan Engelhardt
2009-10-26  9:03   ` Thomas Jarosch
2009-10-27 10:04     ` Jan Engelhardt
2009-10-27 10:49       ` Thomas Jarosch
2009-10-28 17:51         ` Jan Engelhardt
2009-10-30 16:07           ` Thomas Jarosch
2009-10-30 17:49             ` Jan Engelhardt
2009-11-04  8:42               ` Thomas Jarosch
2009-11-04  9:27                 ` Jan Engelhardt
2009-11-04  9:45                   ` Thomas Jarosch
2009-11-04 22:38                     ` Jan Engelhardt
2009-11-05 16:06                       ` Thomas Jarosch

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.