| Message ID | 1337002943-16374-1-git-send-email-hans.schillstrom@ericsson.com |
|---|---|
| State | Not Applicable |
| Headers | show |
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 -- 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
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 > >
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 -- 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
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(...) -- 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
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(...) > >
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. -- 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
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. > -- 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
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. -- 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
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. -- 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
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 -- 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
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. -- 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
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. -- 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
>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 -- 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
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 ?-- 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
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;
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(-)