[PATCH] net: em_cmp.c use unaligned access helpers

[PATCH] net: em_cmp.c use unaligned access helpers

[Harvey Harrison]

Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
---
 net/sched/em_cmp.c |    9 +++------
 1 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/net/sched/em_cmp.c b/net/sched/em_cmp.c
index cc49c93..bc45039 100644
--- a/net/sched/em_cmp.c
+++ b/net/sched/em_cmp.c
@@ -14,6 +14,7 @@
 #include <linux/kernel.h>
 #include <linux/skbuff.h>
 #include <linux/tc_ematch/tc_em_cmp.h>
+#include <asm/unaligned.h>
 #include <net/pkt_cls.h>
 
 static inline int cmp_needs_transformation(struct tcf_em_cmp *cmp)
@@ -37,8 +38,7 @@ static int em_cmp_match(struct sk_buff *skb, struct tcf_ematch *em,
 			break;
 
 		case TCF_EM_ALIGN_U16:
-			val = *ptr << 8;
-			val |= *(ptr+1);
+			val = get_unaligned_be16(ptr);
 
 			if (cmp_needs_transformation(cmp))
 				val = be16_to_cpu(val);
@@ -47,10 +47,7 @@ static int em_cmp_match(struct sk_buff *skb, struct tcf_ematch *em,
 		case TCF_EM_ALIGN_U32:
 			/* Worth checking boundries? The branching seems
 			 * to get worse. Visit again. */
-			val = *ptr << 24;
-			val |= *(ptr+1) << 16;
-			val |= *(ptr+2) << 8;
-			val |= *(ptr+3);
+			val = get_unaligned_be32(ptr);
 
 			if (cmp_needs_transformation(cmp))
 				val = be32_to_cpu(val);
-- 
1.6.0.2.405.g3cc38

[PATCH] net: em_cmp.c use unaligned access helpers

[Harvey Harrison]

Both locations are loading a big-endian value in cpu-endianness.  The
be32/be16_to_cpu immediately afterwards seems suspect.

Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
---
 net/sched/em_cmp.c |   11 +++--------
 1 files changed, 3 insertions(+), 8 deletions(-)

diff --git a/net/sched/em_cmp.c b/net/sched/em_cmp.c
index cc49c93..56f1eae 100644
--- a/net/sched/em_cmp.c
+++ b/net/sched/em_cmp.c
@@ -15,6 +15,7 @@
 #include <linux/skbuff.h>
 #include <linux/tc_ematch/tc_em_cmp.h>
 #include <net/pkt_cls.h>
+#include <asm/unaligned.h>
 
 static inline int cmp_needs_transformation(struct tcf_em_cmp *cmp)
 {
@@ -37,9 +38,7 @@ static int em_cmp_match(struct sk_buff *skb, struct tcf_ematch *em,
 			break;
 
 		case TCF_EM_ALIGN_U16:
-			val = *ptr << 8;
-			val |= *(ptr+1);
-
+			val = get_unaligned_be16(ptr);
 			if (cmp_needs_transformation(cmp))
 				val = be16_to_cpu(val);
 			break;
@@ -47,11 +46,7 @@ static int em_cmp_match(struct sk_buff *skb, struct tcf_ematch *em,
 		case TCF_EM_ALIGN_U32:
 			/* Worth checking boundries? The branching seems
 			 * to get worse. Visit again. */
-			val = *ptr << 24;
-			val |= *(ptr+1) << 16;
-			val |= *(ptr+2) << 8;
-			val |= *(ptr+3);
-
+			val = get_unaligned_be32(ptr);
 			if (cmp_needs_transformation(cmp))
 				val = be32_to_cpu(val);
 			break;
-- 
1.5.6.290.gc4e15

Re: [PATCH] net: em_cmp.c use unaligned access helpers

[David Miller]

From: Harvey Harrison <harvey.harrison@gmail.com>
Date: Wed, 25 Jun 2008 14:14:00 -0700

> Both locations are loading a big-endian value in cpu-endianness.  The
> be32/be16_to_cpu immediately afterwards seems suspect.
> 
> Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>

Smells like a bug, Thomas can you take a close look at this?

> ---
>  net/sched/em_cmp.c |   11 +++--------
>  1 files changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/net/sched/em_cmp.c b/net/sched/em_cmp.c
> index cc49c93..56f1eae 100644
> --- a/net/sched/em_cmp.c
> +++ b/net/sched/em_cmp.c
> @@ -15,6 +15,7 @@
>  #include <linux/skbuff.h>
>  #include <linux/tc_ematch/tc_em_cmp.h>
>  #include <net/pkt_cls.h>
> +#include <asm/unaligned.h>
>  
>  static inline int cmp_needs_transformation(struct tcf_em_cmp *cmp)
>  {
> @@ -37,9 +38,7 @@ static int em_cmp_match(struct sk_buff *skb, struct tcf_ematch *em,
>  			break;
>  
>  		case TCF_EM_ALIGN_U16:
> -			val = *ptr << 8;
> -			val |= *(ptr+1);
> -
> +			val = get_unaligned_be16(ptr);
>  			if (cmp_needs_transformation(cmp))
>  				val = be16_to_cpu(val);
>  			break;
> @@ -47,11 +46,7 @@ static int em_cmp_match(struct sk_buff *skb, struct tcf_ematch *em,
>  		case TCF_EM_ALIGN_U32:
>  			/* Worth checking boundries? The branching seems
>  			 * to get worse. Visit again. */
> -			val = *ptr << 24;
> -			val |= *(ptr+1) << 16;
> -			val |= *(ptr+2) << 8;
> -			val |= *(ptr+3);
> -
> +			val = get_unaligned_be32(ptr);
>  			if (cmp_needs_transformation(cmp))
>  				val = be32_to_cpu(val);
>  			break;
> -- 
> 1.5.6.290.gc4e15
> 
> 
> 
> --
> 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] net: em_cmp.c use unaligned access helpers

[Thomas Graf]

* David Miller <davem@davemloft.net> 2008-06-27 20:16
> From: Harvey Harrison <harvey.harrison@gmail.com>
> Date: Wed, 25 Jun 2008 14:14:00 -0700
> 
> > Both locations are loading a big-endian value in cpu-endianness.  The
> > be32/be16_to_cpu immediately afterwards seems suspect.
> > 
> > Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
> 
> Smells like a bug, Thomas can you take a close look at this?

It's not a bug, the u16|u32 pointed to by ptr can be at any offset in the
skb buffer and is not necessarly in big endian, it can be both. Only the
user knows so he has to specify a flag if the value should be converted
from be to cpu.

I think the code is correct, although it could be simplified to:

case TCF_EM_ALIGN_U16:
	if (!cmp_needs_transformation(cmp))
		val = get_unaligned_le16(ptr);
	else
		val = get_unaligned_be16(ptr);

I wasn't aware of these flags when I wrote the code initially.

Re: [PATCH] net: em_cmp.c use unaligned access helpers

[Thomas Graf]

* Thomas Graf <tgraf@suug.ch> 2008-06-29 11:54
> I wasn't aware of these flags when I wrote the code initially.

macros I mean...

[PATCH] net: em_cmp.c use unaligned access helpers

[Harvey Harrison]

Explicitly check the endianness and use the appropriate unaligned
access helpers rather than conditionally byteswapping after the
unaligned access.

Fixes a bug on big-endian machines where be16/be32_to_cpu are no-ops
and would miss byteswapping in the le case.

Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
---
Dave, here's a complete patch if you want to apply.

 net/sched/em_cmp.c |   17 +++++++----------
 1 files changed, 7 insertions(+), 10 deletions(-)

diff --git a/net/sched/em_cmp.c b/net/sched/em_cmp.c
index cc49c93..afd2d7a 100644
--- a/net/sched/em_cmp.c
+++ b/net/sched/em_cmp.c
@@ -14,6 +14,7 @@
 #include <linux/kernel.h>
 #include <linux/skbuff.h>
 #include <linux/tc_ematch/tc_em_cmp.h>
+#include <asm/unaligned.h>
 #include <net/pkt_cls.h>
 
 static inline int cmp_needs_transformation(struct tcf_em_cmp *cmp)
@@ -37,23 +38,19 @@ static int em_cmp_match(struct sk_buff *skb, struct tcf_ematch *em,
 			break;
 
 		case TCF_EM_ALIGN_U16:
-			val = *ptr << 8;
-			val |= *(ptr+1);
-
 			if (cmp_needs_transformation(cmp))
-				val = be16_to_cpu(val);
+				val = get_unaligned_le16(ptr);
+			else
+				val = get_unaligned_be16(ptr);
 			break;
 
 		case TCF_EM_ALIGN_U32:
 			/* Worth checking boundries? The branching seems
 			 * to get worse. Visit again. */
-			val = *ptr << 24;
-			val |= *(ptr+1) << 16;
-			val |= *(ptr+2) << 8;
-			val |= *(ptr+3);
-
 			if (cmp_needs_transformation(cmp))
-				val = be32_to_cpu(val);
+				val = get_unaligned_le32(ptr);
+			else
+				val = get_unaligned_be32(ptr);
 			break;
 
 		default:
-- 
1.5.6.1.179.g18c1f

Re: [PATCH] net: em_cmp.c use unaligned access helpers

[Harvey Harrison]

On Sun, 2008-06-29 at 11:54 +0200, Thomas Graf wrote:
> * David Miller <davem@davemloft.net> 2008-06-27 20:16
> > From: Harvey Harrison <harvey.harrison@gmail.com>
> > Date: Wed, 25 Jun 2008 14:14:00 -0700
> > 
> > > Both locations are loading a big-endian value in cpu-endianness.  The
> > > be32/be16_to_cpu immediately afterwards seems suspect.
> > > 
> > > Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
> > 
> > Smells like a bug, Thomas can you take a close look at this?
> 
> It's not a bug, the u16|u32 pointed to by ptr can be at any offset in the
> skb buffer and is not necessarly in big endian, it can be both. Only the
> user knows so he has to specify a flag if the value should be converted
> from be to cpu.

OK, but be16/32_to_cpu is a no-op on be-arches, so there is a bug here on
big-endian machines as they won't switch it back, your simplified patch
is actually a bugfix in that case.

> 
> I think the code is correct, although it could be simplified to:
> 
> case TCF_EM_ALIGN_U16:
> 	if (!cmp_needs_transformation(cmp))
> 		val = get_unaligned_le16(ptr);
> 	else
> 		val = get_unaligned_be16(ptr);
> 
> I wasn't aware of these flags when I wrote the code initially.

I introduced them recently, your patch is probably applicable to
2.6.26 as a bugfix.

Reviewed-by: Harvey Harrison <harvey.harrison@gmail.com>

Re: [PATCH] net: em_cmp.c use unaligned access helpers

[Thomas Graf]

* Harvey Harrison <harvey.harrison@gmail.com> 2008-06-29 11:01
> OK, but be16/32_to_cpu is a no-op on be-arches, so there is a bug here on
> big-endian machines as they won't switch it back, your simplified patch
> is actually a bugfix in that case.

I noticed that my change wouldn't be correct. The point is to allow
userspace to convert the filter values to big endian once, supply them
to the kernel and compare big endian values directly. The be16/32_to_cpu
convertion is available to support greater-than/lesser-than on little
endian architectures.

Re: [PATCH] net: em_cmp.c use unaligned access helpers

[Harvey Harrison]

On Mon, 2008-06-30 at 01:26 +0200, Thomas Graf wrote:
> * Harvey Harrison <harvey.harrison@gmail.com> 2008-06-29 11:01
> > OK, but be16/32_to_cpu is a no-op on be-arches, so there is a bug here on
> > big-endian machines as they won't switch it back, your simplified patch
> > is actually a bugfix in that case.
> 
> I noticed that my change wouldn't be correct. The point is to allow
> userspace to convert the filter values to big endian once, supply them
> to the kernel and compare big endian values directly. The be16/32_to_cpu
> convertion is available to support greater-than/lesser-than on little
> endian architectures.

Thomas, maybe I'm just being slow, put if it's always a be-value, why
not the following:

From: Harvey Harrison <harvey.harrison@gmail.com>
Subject: [PATCH] net: em_cmp.c use unaligned access helper

ptr pointers to a big endian value, instead of constructing a be
value and conditionally byteswapping on little-endian arches, use
the unaligned to access helpers return a cpu-endian value.

Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
---
 net/sched/em_cmp.c |   20 +++-----------------
 1 files changed, 3 insertions(+), 17 deletions(-)

diff --git a/net/sched/em_cmp.c b/net/sched/em_cmp.c
index cc49c93..6e64af1 100644
--- a/net/sched/em_cmp.c
+++ b/net/sched/em_cmp.c
@@ -14,13 +14,9 @@
 #include <linux/kernel.h>
 #include <linux/skbuff.h>
 #include <linux/tc_ematch/tc_em_cmp.h>
+#include <asm/unaligned.h>
 #include <net/pkt_cls.h>
 
-static inline int cmp_needs_transformation(struct tcf_em_cmp *cmp)
-{
-	return unlikely(cmp->flags & TCF_EM_CMP_TRANS);
-}
-
 static int em_cmp_match(struct sk_buff *skb, struct tcf_ematch *em,
 			struct tcf_pkt_info *info)
 {
@@ -37,23 +33,13 @@ static int em_cmp_match(struct sk_buff *skb, struct tcf_ematch *em,
 			break;
 
 		case TCF_EM_ALIGN_U16:
-			val = *ptr << 8;
-			val |= *(ptr+1);
-
-			if (cmp_needs_transformation(cmp))
-				val = be16_to_cpu(val);
+			val = get_unaligned_be16(ptr);
 			break;
 
 		case TCF_EM_ALIGN_U32:
 			/* Worth checking boundries? The branching seems
 			 * to get worse. Visit again. */
-			val = *ptr << 24;
-			val |= *(ptr+1) << 16;
-			val |= *(ptr+2) << 8;
-			val |= *(ptr+3);
-
-			if (cmp_needs_transformation(cmp))
-				val = be32_to_cpu(val);
+			val = get_unaligned_be32(ptr);
 			break;
 
 		default:
-- 
1.5.6.1.322.ge904b

Re: [PATCH] net: em_cmp.c use unaligned access helpers

[Thomas Graf]

* Harvey Harrison <harvey.harrison@gmail.com> 2008-07-04 19:10
> On Mon, 2008-06-30 at 01:26 +0200, Thomas Graf wrote:
> > * Harvey Harrison <harvey.harrison@gmail.com> 2008-06-29 11:01
> > > OK, but be16/32_to_cpu is a no-op on be-arches, so there is a bug here on
> > > big-endian machines as they won't switch it back, your simplified patch
> > > is actually a bugfix in that case.
> > 
> > I noticed that my change wouldn't be correct. The point is to allow
> > userspace to convert the filter values to big endian once, supply them
> > to the kernel and compare big endian values directly. The be16/32_to_cpu
> > convertion is available to support greater-than/lesser-than on little
> > endian architectures.
> 
> Thomas, maybe I'm just being slow, put if it's always a be-value, why
> not the following:
> 
> From: Harvey Harrison <harvey.harrison@gmail.com>
> Subject: [PATCH] net: em_cmp.c use unaligned access helper
> 
> ptr pointers to a big endian value, instead of constructing a be
> value and conditionally byteswapping on little-endian arches, use
> the unaligned to access helpers return a cpu-endian value.

em_cmp is based on the u32 classifier which compares everything in
be, it is faster than swapping every value but only works for eq/ne
comparisons. Some people have just gotten used to work with be
when filtering on addresses and port numbers.

em_cmp additionally supports gt/lt operations which only work in
host byte order, therefore the flag TCP_EM_CMP_TRANS was added to
trigger this.

Enforcing host byte order in all cases would probably be simpler
but it can't be changed now as it would break scripts.