[PATCH] netfilter: xt_HMARK: endian bugs

Subsystems: netfilter, networking [general], the rest

STALE5229d

15 messages, 5 authors, 2012-05-15 · open the first message on its own page

[PATCH] netfilter: xt_HMARK: endian bugs

From: Hans Schillstrom <hidden>
Date: 2012-05-14 13:42:26

A mix of u32 and __be32 causes endian warning.
Switch to __be32 and __be16 for addresses and ports.
Added (__force u32) at some places.

Reported-by: Dan Carpenter <redacted>
Signed-off-by: Hans Schillstrom <redacted>
---
 include/linux/netfilter/xt_HMARK.h |    4 ++--
 net/netfilter/xt_HMARK.c           |   35 ++++++++++++++++++-----------------
 2 files changed, 20 insertions(+), 19 deletions(-)

diff --git a/include/linux/netfilter/xt_HMARK.h b/include/linux/netfilter/xt_HMARK.h
index abb1650..e2af67e 100644
--- a/include/linux/netfilter/xt_HMARK.h
+++ b/include/linux/netfilter/xt_HMARK.h

@@ -24,8 +24,8 @@ enum {
 
 union hmark_ports {
 	struct {
-		__u16	src;
-		__u16	dst;
+		__be16	src;
+		__be16	dst;
 	} p16;
 	__u32	v32;
 };

diff --git a/net/netfilter/xt_HMARK.c b/net/netfilter/xt_HMARK.c
index 32fbd73..38ed442 100644
--- a/net/netfilter/xt_HMARK.c
+++ b/net/netfilter/xt_HMARK.c

@@ -32,13 +32,13 @@ MODULE_ALIAS("ipt_HMARK");
 MODULE_ALIAS("ip6t_HMARK");
 
 struct hmark_tuple {
-	u32			src;
-	u32			dst;
+	__be32			src;
+	__be32			dst;
 	union hmark_ports	uports;
 	uint8_t			proto;
 };
 
-static inline u32 hmark_addr6_mask(const __u32 *addr32, const __u32 *mask)
+static inline __be32 hmark_addr6_mask(const __be32 *addr32, const __be32 *mask)
 {
 	return (addr32[0] & mask[0]) ^
 	       (addr32[1] & mask[1]) ^

@@ -46,8 +46,8 @@ static inline u32 hmark_addr6_mask(const __u32 *addr32, const __u32 *mask)
 	       (addr32[3] & mask[3]);
 }
 
-static inline u32
-hmark_addr_mask(int l3num, const __u32 *addr32, const __u32 *mask)
+static inline __be32
+hmark_addr_mask(int l3num, const __be32 *addr32, const __be32 *mask)
 {
 	switch (l3num) {
 	case AF_INET:

@@ -74,10 +74,10 @@ hmark_ct_set_htuple(const struct sk_buff *skb, struct hmark_tuple *t,
 	otuple = &ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple;
 	rtuple = &ct->tuplehash[IP_CT_DIR_REPLY].tuple;
 
-	t->src = hmark_addr_mask(otuple->src.l3num, otuple->src.u3.all,
-				 info->src_mask.all);
-	t->dst = hmark_addr_mask(otuple->src.l3num, rtuple->src.u3.all,
-				 info->dst_mask.all);
+	t->src = hmark_addr_mask(otuple->src.l3num, otuple->src.u3.ip6,
+				 info->src_mask.ip6);
+	t->dst = hmark_addr_mask(otuple->src.l3num, rtuple->src.u3.ip6,
+				 info->dst_mask.ip6);
 
 	if (info->flags & XT_HMARK_FLAG(XT_HMARK_METHOD_L3))
 		return 0;

@@ -88,7 +88,7 @@ hmark_ct_set_htuple(const struct sk_buff *skb, struct hmark_tuple *t,
 		t->uports.p16.dst = rtuple->src.u.all;
 		t->uports.v32 = (t->uports.v32 & info->port_mask.v32) |
 				info->port_set.v32;
-		if (t->uports.p16.dst < t->uports.p16.src)
+		if (ntohs(t->uports.p16.dst) < ntohs(t->uports.p16.src))
 			swap(t->uports.p16.dst, t->uports.p16.src);
 	}

@@ -103,10 +103,11 @@ hmark_hash(struct hmark_tuple *t, const struct xt_hmark_info *info)
 {
 	u32 hash;
 
-	if (t->dst < t->src)
+	if (ntohl(t->dst) < ntohl(t->src))
 		swap(t->src, t->dst);
 
-	hash = jhash_3words(t->src, t->dst, t->uports.v32, info->hashrnd);
+	hash = jhash_3words((__force u32) t->src, (__force u32) t->dst,
+			    t->uports.v32, info->hashrnd);
 	hash = hash ^ (t->proto & info->proto_mask);
 
 	return (hash % info->hmodulus) + info->hoffset;

@@ -129,7 +130,7 @@ hmark_set_tuple_ports(const struct sk_buff *skb, unsigned int nhoff,
 	t->uports.v32 = (t->uports.v32 & info->port_mask.v32) |
 			info->port_set.v32;
 
-	if (t->uports.p16.dst < t->uports.p16.src)
+	if (ntohs(t->uports.p16.dst) < ntohs(t->uports.p16.src))
 		swap(t->uports.p16.dst, t->uports.p16.src);
 }

@@ -178,8 +179,8 @@ hmark_pkt_set_htuple_ipv6(const struct sk_buff *skb, struct hmark_tuple *t,
 			return -1;
 	}
 noicmp:
-	t->src = hmark_addr6_mask(ip6->saddr.s6_addr32, info->src_mask.all);
-	t->dst = hmark_addr6_mask(ip6->daddr.s6_addr32, info->dst_mask.all);
+	t->src = hmark_addr6_mask(ip6->saddr.s6_addr32, info->src_mask.ip6);
+	t->dst = hmark_addr6_mask(ip6->daddr.s6_addr32, info->dst_mask.ip6);
 
 	if (info->flags & XT_HMARK_FLAG(XT_HMARK_METHOD_L3))
 		return 0;

@@ -255,8 +256,8 @@ hmark_pkt_set_htuple_ipv4(const struct sk_buff *skb, struct hmark_tuple *t,
 		}
 	}
 
-	t->src = (__force u32) ip->saddr;
-	t->dst = (__force u32) ip->daddr;
+	t->src = ip->saddr;
+	t->dst = ip->daddr;
 
 	t->src &= info->src_mask.ip;
 	t->dst &= info->dst_mask.ip;

-- 
1.7.2.3

Re: [PATCH] netfilter: xt_HMARK: endian bugs

From: Pablo Neira Ayuso <pablo@netfilter.org>
Date: 2012-05-14 14:40:52

On Mon, May 14, 2012 at 03:42:23PM +0200, Hans Schillstrom wrote:

quoted hunk

A mix of u32 and __be32 causes endian warning.
Switch to __be32 and __be16 for addresses and ports.
Added (__force u32) at some places.

Reported-by: Dan Carpenter <redacted>
Signed-off-by: Hans Schillstrom <redacted>
---
 include/linux/netfilter/xt_HMARK.h |    4 ++--
 net/netfilter/xt_HMARK.c           |   35 ++++++++++++++++++-----------------
 2 files changed, 20 insertions(+), 19 deletions(-)

diff --git a/include/linux/netfilter/xt_HMARK.h b/include/linux/netfilter/xt_HMARK.h
index abb1650..e2af67e 100644
--- a/include/linux/netfilter/xt_HMARK.h
+++ b/include/linux/netfilter/xt_HMARK.h

@@ -24,8 +24,8 @@ enum {
 
 union hmark_ports {
 	struct {
-		__u16	src;
-		__u16	dst;
+		__be16	src;
+		__be16	dst;
 	} p16;
 	__u32	v32;
 };

diff --git a/net/netfilter/xt_HMARK.c b/net/netfilter/xt_HMARK.c
index 32fbd73..38ed442 100644
--- a/net/netfilter/xt_HMARK.c
+++ b/net/netfilter/xt_HMARK.c

@@ -32,13 +32,13 @@ MODULE_ALIAS("ipt_HMARK");
 MODULE_ALIAS("ip6t_HMARK");
 
 struct hmark_tuple {
-	u32			src;
-	u32			dst;
+	__be32			src;
+	__be32			dst;
 	union hmark_ports	uports;
 	uint8_t			proto;
 };
 
-static inline u32 hmark_addr6_mask(const __u32 *addr32, const __u32 *mask)
+static inline __be32 hmark_addr6_mask(const __be32 *addr32, const __be32 *mask)
 {
 	return (addr32[0] & mask[0]) ^
 	       (addr32[1] & mask[1]) ^

@@ -46,8 +46,8 @@ static inline u32 hmark_addr6_mask(const __u32 *addr32, const __u32 *mask)
 	       (addr32[3] & mask[3]);
 }
 
-static inline u32
-hmark_addr_mask(int l3num, const __u32 *addr32, const __u32 *mask)
+static inline __be32
+hmark_addr_mask(int l3num, const __be32 *addr32, const __be32 *mask)
 {
 	switch (l3num) {
 	case AF_INET:

@@ -74,10 +74,10 @@ hmark_ct_set_htuple(const struct sk_buff *skb, struct hmark_tuple *t,
 	otuple = &ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple;
 	rtuple = &ct->tuplehash[IP_CT_DIR_REPLY].tuple;
 
-	t->src = hmark_addr_mask(otuple->src.l3num, otuple->src.u3.all,
-				 info->src_mask.all);
-	t->dst = hmark_addr_mask(otuple->src.l3num, rtuple->src.u3.all,
-				 info->dst_mask.all);
+	t->src = hmark_addr_mask(otuple->src.l3num, otuple->src.u3.ip6,
+				 info->src_mask.ip6);
+	t->dst = hmark_addr_mask(otuple->src.l3num, rtuple->src.u3.ip6,
+				 info->dst_mask.ip6);
 
 	if (info->flags & XT_HMARK_FLAG(XT_HMARK_METHOD_L3))
 		return 0;

@@ -88,7 +88,7 @@ hmark_ct_set_htuple(const struct sk_buff *skb, struct hmark_tuple *t,
 		t->uports.p16.dst = rtuple->src.u.all;
 		t->uports.v32 = (t->uports.v32 & info->port_mask.v32) |
 				info->port_set.v32;
-		if (t->uports.p16.dst < t->uports.p16.src)
+		if (ntohs(t->uports.p16.dst) < ntohs(t->uports.p16.src))

Do we really need this to make sparse happy?

quoted hunk

 			swap(t->uports.p16.dst, t->uports.p16.src);
 	}

@@ -103,10 +103,11 @@ hmark_hash(struct hmark_tuple *t, const struct xt_hmark_info *info)
 {
 	u32 hash;
 
-	if (t->dst < t->src)
+	if (ntohl(t->dst) < ntohl(t->src))
 		swap(t->src, t->dst);
 
-	hash = jhash_3words(t->src, t->dst, t->uports.v32, info->hashrnd);
+	hash = jhash_3words((__force u32) t->src, (__force u32) t->dst,
+			    t->uports.v32, info->hashrnd);
 	hash = hash ^ (t->proto & info->proto_mask);
 
 	return (hash % info->hmodulus) + info->hoffset;

This will clash with my patch. No problem, I'll manually fix it
myself.

quoted hunk

@@ -129,7 +130,7 @@ hmark_set_tuple_ports(const struct sk_buff *skb, unsigned int nhoff,
 	t->uports.v32 = (t->uports.v32 & info->port_mask.v32) |
 			info->port_set.v32;
 
-	if (t->uports.p16.dst < t->uports.p16.src)
+	if (ntohs(t->uports.p16.dst) < ntohs(t->uports.p16.src))
 		swap(t->uports.p16.dst, t->uports.p16.src);
 }

@@ -178,8 +179,8 @@ hmark_pkt_set_htuple_ipv6(const struct sk_buff *skb, struct hmark_tuple *t,
 			return -1;
 	}
 noicmp:
-	t->src = hmark_addr6_mask(ip6->saddr.s6_addr32, info->src_mask.all);
-	t->dst = hmark_addr6_mask(ip6->daddr.s6_addr32, info->dst_mask.all);
+	t->src = hmark_addr6_mask(ip6->saddr.s6_addr32, info->src_mask.ip6);
+	t->dst = hmark_addr6_mask(ip6->daddr.s6_addr32, info->dst_mask.ip6);
 
 	if (info->flags & XT_HMARK_FLAG(XT_HMARK_METHOD_L3))
 		return 0;

@@ -255,8 +256,8 @@ hmark_pkt_set_htuple_ipv4(const struct sk_buff *skb, struct hmark_tuple *t,
 		}
 	}
 
-	t->src = (__force u32) ip->saddr;
-	t->dst = (__force u32) ip->daddr;
+	t->src = ip->saddr;
+	t->dst = ip->daddr;
 
 	t->src &= info->src_mask.ip;
 	t->dst &= info->dst_mask.ip;

-- 
1.7.2.3

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] netfilter: xt_HMARK: endian bugs

From: Hans Schillstrom <hidden>
Date: 2012-05-14 15:05:57

On Monday 14 May 2012 16:40:52 Pablo Neira Ayuso wrote:

On Mon, May 14, 2012 at 03:42:23PM +0200, Hans Schillstrom wrote:

quoted

A mix of u32 and __be32 causes endian warning.
Switch to __be32 and __be16 for addresses and ports.
Added (__force u32) at some places.

Reported-by: Dan Carpenter <redacted>
Signed-off-by: Hans Schillstrom <redacted>
---
 include/linux/netfilter/xt_HMARK.h |    4 ++--
 net/netfilter/xt_HMARK.c           |   35 ++++++++++++++++++-----------------
 2 files changed, 20 insertions(+), 19 deletions(-)

diff --git a/include/linux/netfilter/xt_HMARK.h b/include/linux/netfilter/xt_HMARK.h
index abb1650..e2af67e 100644
--- a/include/linux/netfilter/xt_HMARK.h
+++ b/include/linux/netfilter/xt_HMARK.h

@@ -24,8 +24,8 @@ enum {
 
 union hmark_ports {
 	struct {
-		__u16	src;
-		__u16	dst;
+		__be16	src;
+		__be16	dst;
 	} p16;
 	__u32	v32;
 };

diff --git a/net/netfilter/xt_HMARK.c b/net/netfilter/xt_HMARK.c
index 32fbd73..38ed442 100644
--- a/net/netfilter/xt_HMARK.c
+++ b/net/netfilter/xt_HMARK.c

@@ -32,13 +32,13 @@ MODULE_ALIAS("ipt_HMARK");
 MODULE_ALIAS("ip6t_HMARK");
 
 struct hmark_tuple {
-	u32			src;
-	u32			dst;
+	__be32			src;
+	__be32			dst;
 	union hmark_ports	uports;
 	uint8_t			proto;
 };
 
-static inline u32 hmark_addr6_mask(const __u32 *addr32, const __u32 *mask)
+static inline __be32 hmark_addr6_mask(const __be32 *addr32, const __be32 *mask)
 {
 	return (addr32[0] & mask[0]) ^
 	       (addr32[1] & mask[1]) ^

@@ -46,8 +46,8 @@ static inline u32 hmark_addr6_mask(const __u32 *addr32, const __u32 *mask)
 	       (addr32[3] & mask[3]);
 }
 
-static inline u32
-hmark_addr_mask(int l3num, const __u32 *addr32, const __u32 *mask)
+static inline __be32
+hmark_addr_mask(int l3num, const __be32 *addr32, const __be32 *mask)
 {
 	switch (l3num) {
 	case AF_INET:

@@ -74,10 +74,10 @@ hmark_ct_set_htuple(const struct sk_buff *skb, struct hmark_tuple *t,
 	otuple = &ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple;
 	rtuple = &ct->tuplehash[IP_CT_DIR_REPLY].tuple;
 
-	t->src = hmark_addr_mask(otuple->src.l3num, otuple->src.u3.all,
-				 info->src_mask.all);
-	t->dst = hmark_addr_mask(otuple->src.l3num, rtuple->src.u3.all,
-				 info->dst_mask.all);
+	t->src = hmark_addr_mask(otuple->src.l3num, otuple->src.u3.ip6,
+				 info->src_mask.ip6);
+	t->dst = hmark_addr_mask(otuple->src.l3num, rtuple->src.u3.ip6,
+				 info->dst_mask.ip6);
 
 	if (info->flags & XT_HMARK_FLAG(XT_HMARK_METHOD_L3))
 		return 0;

@@ -88,7 +88,7 @@ hmark_ct_set_htuple(const struct sk_buff *skb, struct hmark_tuple *t,
 		t->uports.p16.dst = rtuple->src.u.all;
 		t->uports.v32 = (t->uports.v32 & info->port_mask.v32) |
 				info->port_set.v32;
-		if (t->uports.p16.dst < t->uports.p16.src)
+		if (ntohs(t->uports.p16.dst) < ntohs(t->uports.p16.src))

Do we really need this to make sparse happy?

__force is ok for Sparse,
but I realize that the mixing little and big endian machines will not work

quoted

 			swap(t->uports.p16.dst, t->uports.p16.src);
 	}

@@ -103,10 +103,11 @@ hmark_hash(struct hmark_tuple *t, const struct xt_hmark_info *info)
 {
 	u32 hash;
 
-	if (t->dst < t->src)
+	if (ntohl(t->dst) < ntohl(t->src))
 		swap(t->src, t->dst);
 
-	hash = jhash_3words(t->src, t->dst, t->uports.v32, info->hashrnd);
+	hash = jhash_3words((__force u32) t->src, (__force u32) t->dst,
+			    t->uports.v32, info->hashrnd);
 	hash = hash ^ (t->proto & info->proto_mask);
 
 	return (hash % info->hmodulus) + info->hoffset;

This will clash with my patch. No problem, I'll manually fix it
myself.

Thanks

quoted

@@ -129,7 +130,7 @@ hmark_set_tuple_ports(const struct sk_buff *skb, unsigned int nhoff,
 	t->uports.v32 = (t->uports.v32 & info->port_mask.v32) |
 			info->port_set.v32;
 
-	if (t->uports.p16.dst < t->uports.p16.src)
+	if (ntohs(t->uports.p16.dst) < ntohs(t->uports.p16.src))
 		swap(t->uports.p16.dst, t->uports.p16.src);
 }

@@ -178,8 +179,8 @@ hmark_pkt_set_htuple_ipv6(const struct sk_buff *skb, struct hmark_tuple *t,
 			return -1;
 	}
 noicmp:
-	t->src = hmark_addr6_mask(ip6->saddr.s6_addr32, info->src_mask.all);
-	t->dst = hmark_addr6_mask(ip6->daddr.s6_addr32, info->dst_mask.all);
+	t->src = hmark_addr6_mask(ip6->saddr.s6_addr32, info->src_mask.ip6);
+	t->dst = hmark_addr6_mask(ip6->daddr.s6_addr32, info->dst_mask.ip6);
 
 	if (info->flags & XT_HMARK_FLAG(XT_HMARK_METHOD_L3))
 		return 0;

@@ -255,8 +256,8 @@ hmark_pkt_set_htuple_ipv4(const struct sk_buff *skb, struct hmark_tuple *t,
 		}
 	}
 
-	t->src = (__force u32) ip->saddr;
-	t->dst = (__force u32) ip->daddr;
+	t->src = ip->saddr;
+	t->dst = ip->daddr;
 
 	t->src &= info->src_mask.ip;
 	t->dst &= info->dst_mask.ip;

-- 
1.7.2.3

-- 
Regards
Hans Schillstrom [off-list ref]

Re: [PATCH] netfilter: xt_HMARK: endian bugs

From: Jan Engelhardt <hidden>
Date: 2012-05-14 15:05:58

On Monday 2012-05-14 16:40, Pablo Neira Ayuso wrote:

quoted

-		if (t->uports.p16.dst < t->uports.p16.src)
+		if (ntohs(t->uports.p16.dst) < ntohs(t->uports.p16.src))

Do we really need this to make sparse happy?

You need it to make *maths* happy.

Consider

	384 < 65407

but

	    ntohs(384) > ntohs(65407)
	<=> 32769 > 32767

Re: [PATCH] netfilter: xt_HMARK: endian bugs

From: Eric Dumazet <hidden>
Date: 2012-05-14 15:24:39

On Mon, 2012-05-14 at 17:05 +0200, Jan Engelhardt wrote:

On Monday 2012-05-14 16:40, Pablo Neira Ayuso wrote:

quoted

-		if (t->uports.p16.dst < t->uports.p16.src)
+		if (ntohs(t->uports.p16.dst) < ntohs(t->uports.p16.src))

Do we really need this to make sparse happy?

You need it to make *maths* happy.

Consider

	384 < 65407

but

	    ntohs(384) > ntohs(65407)
	<=> 32769 > 32767
--

Doesnt matter at all in this context.

Take a look at 

void __skb_get_rxhash(struct sk_buff *skb)

if ((__force u16)keys.port16[1] < (__force u16)keys.port16[0])
	swap(...)

Re: [PATCH] netfilter: xt_HMARK: endian bugs

From: Hans Schillstrom <hidden>
Date: 2012-05-14 16:09:16

On Monday 14 May 2012 17:24:39 Eric Dumazet wrote:

On Mon, 2012-05-14 at 17:05 +0200, Jan Engelhardt wrote:

quoted

On Monday 2012-05-14 16:40, Pablo Neira Ayuso wrote:

quoted

-		if (t->uports.p16.dst < t->uports.p16.src)
+		if (ntohs(t->uports.p16.dst) < ntohs(t->uports.p16.src))

Do we really need this to make sparse happy?

You need it to make *maths* happy.

Consider

	384 < 65407

but

	    ntohs(384) > ntohs(65407)
	<=> 32769 > 32767
--

Doesnt matter at all in this context.

This context can contain both le & be machines,
so at least in hmark it make sense

Take a look at 

void __skb_get_rxhash(struct sk_buff *skb)

if ((__force u16)keys.port16[1] < (__force u16)keys.port16[0])
	swap(...)



-- 
Regards
Hans Schillstrom [off-list ref]

Re: [PATCH] netfilter: xt_HMARK: endian bugs

From: Eric Dumazet <hidden>
Date: 2012-05-14 16:24:34

On Mon, 2012-05-14 at 18:09 +0200, Hans Schillstrom wrote:

This context can contain both le & be machines,
so at least in hmark it make sense

Before jhash() and its shuffle ? What do you mean ?

Please respin your patch using (__force u16/u32) instead of
useless/expensive ntohs() / ntohl() (in _this_ context of hashing)

If you compare two 32bits values, of course they must have same
ordering, but seeding jhash() is another matter.

(Granted all calls use the same ordering of course)

sparse is great tool, but if you add useless ntohl() calls to make
sparse silent, then its probably better to not use sparse.

Re: [PATCH] netfilter: xt_HMARK: endian bugs

From: Hans Schillstrom <hidden>
Date: 2012-05-14 17:51:34

On Monday 14 May 2012 18:24:34 Eric Dumazet wrote:

On Mon, 2012-05-14 at 18:09 +0200, Hans Schillstrom wrote:

quoted

This context can contain both le & be machines,
so at least in hmark it make sense

Before jhash() and its shuffle ? What do you mean ?

I want that a Big endian machine should produce the same
hash value independent of flow direction as a Little endian.

OK, I missed ntohl() before calling jhash_3words()

Correct me if I'm wrong here (have no big endian machine available for test)
jhash_3words() and __jhash_final() seems to be "endian" safe.

So by doing the expensive ntohl on addresses and ports into jhash_3words()
it will produce the same value on both be and le.

That's why I want to have the ntohs() / ntohl() when comparing.

Please respin your patch using (__force u16/u32) instead of
useless/expensive ntohs() / ntohl() (in _this_ context of hashing)

If you compare two 32bits values, of course they must have same
ordering, but seeding jhash() is another matter.

(Granted all calls use the same ordering of course)

sparse is great tool, but if you add useless ntohl() calls to make
sparse silent, then its probably better to not use sparse.

Re: [PATCH] netfilter: xt_HMARK: endian bugs

From: Jan Engelhardt <hidden>
Date: 2012-05-14 18:24:53

On Monday 2012-05-14 19:51, Hans Schillstrom wrote:

On Monday 14 May 2012 18:24:34 Eric Dumazet wrote:

quoted

On Mon, 2012-05-14 at 18:09 +0200, Hans Schillstrom wrote:

quoted

This context can contain both le & be machines,
so at least in hmark it make sense

Before jhash() and its shuffle ? What do you mean ?

I want that a Big endian machine should produce the same
hash value independent of flow direction as a Little endian.

But one does not really need that, since the hash is only used locally.

Re: [PATCH] netfilter: xt_HMARK: endian bugs

From: Eric Dumazet <hidden>
Date: 2012-05-14 18:28:52

On Mon, 2012-05-14 at 19:51 +0200, Hans Schillstrom wrote:

On Monday 14 May 2012 18:24:34 Eric Dumazet wrote:

quoted

On Mon, 2012-05-14 at 18:09 +0200, Hans Schillstrom wrote:

quoted

This context can contain both le & be machines,
so at least in hmark it make sense

Before jhash() and its shuffle ? What do you mean ?

I want that a Big endian machine should produce the same
hash value independent of flow direction as a Little endian.

So one machine can be both le and be ? at the same time ?

OK, I missed ntohl() before calling jhash_3words()

Correct me if I'm wrong here (have no big endian machine available for test)
jhash_3words() and __jhash_final() seems to be "endian" safe.

So by doing the expensive ntohl on addresses and ports into jhash_3words()
it will produce the same value on both be and le.

And what is the purpose of the jhash output ? Is is sent to other
machines on the network, or only localy used ?

That's why I want to have the ntohs() / ntohl() when comparing.

If xt_HMARK depends on a particular bit ordering to jhash() input, then
something is really wrong. I mean it.

jhash() primary purpose it to shuffle input.

We use (__force u32) everywhere in network tree to avoid sparse
warnings. Please grep for them.

Re: [PATCH] netfilter: xt_HMARK: endian bugs

From: Jozsef Kadlecsik <hidden>
Date: 2012-05-14 18:35:29

On Mon, 14 May 2012, Hans Schillstrom wrote:

On Monday 14 May 2012 18:24:34 Eric Dumazet wrote:

quoted

On Mon, 2012-05-14 at 18:09 +0200, Hans Schillstrom wrote:

quoted

This context can contain both le & be machines,
so at least in hmark it make sense

Before jhash() and its shuffle ? What do you mean ?

I want that a Big endian machine should produce the same
hash value independent of flow direction as a Little endian.

OK, I missed ntohl() before calling jhash_3words()

Correct me if I'm wrong here (have no big endian machine available for test)
jhash_3words() and __jhash_final() seems to be "endian" safe.

No, but as Eric wrote: what is the point in forcing the same hash value 
for the same input on big endian and little endian machines? Are you going 
to transfer the hash value between machines?

Best regards,
Jozsef
-
E-mail  : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.mta.hu
PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : Wigner Research Centre for Physics, Hungarian Academy of Sciences
          H-1525 Budapest 114, POB. 49, Hungary

Re: [PATCH] netfilter: xt_HMARK: endian bugs

From: Pablo Neira Ayuso <pablo@netfilter.org>
Date: 2012-05-14 19:02:11

On Mon, May 14, 2012 at 08:35:26PM +0200, Jozsef Kadlecsik wrote:

On Mon, 14 May 2012, Hans Schillstrom wrote:

quoted

On Monday 14 May 2012 18:24:34 Eric Dumazet wrote:

quoted

On Mon, 2012-05-14 at 18:09 +0200, Hans Schillstrom wrote:

quoted

This context can contain both le & be machines,
so at least in hmark it make sense

Before jhash() and its shuffle ? What do you mean ?

I want that a Big endian machine should produce the same
hash value independent of flow direction as a Little endian.

OK, I missed ntohl() before calling jhash_3words()

Correct me if I'm wrong here (have no big endian machine available for test)
jhash_3words() and __jhash_final() seems to be "endian" safe.

No, but as Eric wrote: what is the point in forcing the same hash value 
for the same input on big endian and little endian machines? Are you going 
to transfer the hash value between machines?

IIRC, Hans wants that, in case you have a cluster composed of system
with different endianess, the hash mark calculated will be the same
in both systems. To ensure that the distribution is consistent with
independency of the endianess.

Re: [PATCH] netfilter: xt_HMARK: endian bugs

From: Eric Dumazet <hidden>
Date: 2012-05-14 19:13:55

On Mon, 2012-05-14 at 21:02 +0200, Pablo Neira Ayuso wrote:

IIRC, Hans wants that, in case you have a cluster composed of system
with different endianess, the hash mark calculated will be the same
in both systems. To ensure that the distribution is consistent with
independency of the endianess.

Then jhash() must be audited to make sure its output is OK with this
requirement.

RE: [PATCH] netfilter: xt_HMARK: endian bugs

From: Hans Schillström <hidden>
Date: 2012-05-15 05:57:22

On Mon, 2012-05-14 at 21:02 +0200, Pablo Neira Ayuso wrote:

quoted

IIRC, Hans wants that, in case you have a cluster composed of system
with different endianess, the hash mark calculated will be the same
in both systems. To ensure that the distribution is consistent with
independency of the endianess.

Then jhash() must be audited to make sure its output is OK with this
requirement.

Have done that, and made tests on a mips 32 running debian
It was as expected jhash3_words is endian safe, while jhash() is not

RE: [PATCH] netfilter: xt_HMARK: endian bugs

From: Hans Schillström <hidden>
Date: 2012-05-15 07:33:17

On Mon, 2012-05-14 at 17:05 +0200, Jan Engelhardt wrote:

On Monday 2012-05-14 16:40, Pablo Neira Ayuso wrote:

quoted

-          if (t->uports.p16.dst < t->uports.p16.src)
+          if (ntohs(t->uports.p16.dst) < ntohs(t->uports.p16.src))

Do we really need this to make sparse happy?

This looks insane to make sparse happy 

static inline u32 addr_mask(const __be32 *addr32, const __be32 *mask)
{
                return (__force u32)htonl((__force u32)(*addr32 & *mask));
}

with the "more logic"  way to write it  sparse complains on everything...

static inline u32 addr_mask(const __be32 *addr32, const __be32 *mask)
{
                return htonl(*addr32 & *mask);
}

Is there a better way to do this ?

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help