[PATCH] netfilter: xt_HMARK: endian bugs

[PATCH] netfilter: xt_HMARK: endian bugs

[Hans Schillstrom]

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 <dan.carpenter@oracle.com>
Signed-off-by: Hans Schillstrom <hans.schillstrom@ericsson.com>
---
 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

[Pablo Neira Ayuso]

On Mon, May 14, 2012 at 03:42:23PM +0200, Hans Schillstrom wrote:
> 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 <dan.carpenter@oracle.com>
> Signed-off-by: Hans Schillstrom <hans.schillstrom@ericsson.com>
> ---
>  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?

>  			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.

> @@ -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

[Hans Schillstrom]

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:
> > 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 <dan.carpenter@oracle.com>
> > Signed-off-by: Hans Schillstrom <hans.schillstrom@ericsson.com>
> > ---
> >  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 

> 
> >  			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

> 
> > @@ -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 <hans.schillstrom@ericsson.com>

Re: [PATCH] netfilter: xt_HMARK: endian bugs

[Jan Engelhardt]

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

>> -		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

[Eric Dumazet]

On Mon, 2012-05-14 at 17:05 +0200, Jan Engelhardt wrote:
> On Monday 2012-05-14 16:40, Pablo Neira Ayuso wrote:
> 
> >> -		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

[Hans Schillstrom]

On Monday 14 May 2012 17:24:39 Eric Dumazet wrote:
> On Mon, 2012-05-14 at 17:05 +0200, Jan Engelhardt wrote:
> > On Monday 2012-05-14 16:40, Pablo Neira Ayuso wrote:
> > 
> > >> -		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 <hans.schillstrom@ericsson.com>

Re: [PATCH] netfilter: xt_HMARK: endian bugs

[Eric Dumazet]

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

[Hans Schillstrom]

On Monday 14 May 2012 18:24:34 Eric Dumazet wrote:
> 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 ?

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

[Jan Engelhardt]

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

>On Monday 14 May 2012 18:24:34 Eric Dumazet wrote:
>> 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 ?
>
>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

[Eric Dumazet]

On Mon, 2012-05-14 at 19:51 +0200, Hans Schillstrom wrote:
> On Monday 14 May 2012 18:24:34 Eric Dumazet wrote:
> > 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 ?
> 
> 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

[Jozsef Kadlecsik]

On Mon, 14 May 2012, Hans Schillstrom wrote:

> On Monday 14 May 2012 18:24:34 Eric Dumazet wrote:
> > 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 ?
> 
> 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

[Pablo Neira Ayuso]

On Mon, May 14, 2012 at 08:35:26PM +0200, Jozsef Kadlecsik wrote:
> On Mon, 14 May 2012, Hans Schillstrom wrote:
> 
> > On Monday 14 May 2012 18:24:34 Eric Dumazet wrote:
> > > 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 ?
> > 
> > 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

[Eric Dumazet]

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

[Hans Schillström]

>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.

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

[Hans Schillström]

On Mon, 2012-05-14 at 17:05 +0200, Jan Engelhardt wrote:
> On Monday 2012-05-14 16:40, Pablo Neira Ayuso wrote:
>
> >> -          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 ?