net: em_cmp.c use unaligned access helpers
Subsystems (3)
PATCH 1/1 net: em_cmp.c use unaligned access helpers
Both locations are loading a big-endian value in cpu-endianness. The be32/be16_to_cpu immediately afterwards seems suspect.
--- net/sched/em_cmp.c | 11 +++-------- 1 files changed, 3 insertions(+), 8 deletions(-)
net/sched/em_cmp.c +3 -8
@@ -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 ;
Signature · 1 line
1.5.6.290.gc4e15
David Miller
maintainerFrom: Harvey Harrison <[email protected]>
Date: Wed, 25 Jun 2008 14:14:00 -0700
Quoted 4 lines
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 <[email protected]>
Smells like a bug, Thomas can you take a close look at this?
Quoted 49 lines
--- net/sched/em_cmp.c | 11 +++-------- 1 files changed, 3 insertions(+), 8 deletions(-)net/sched/em_cmp.c +3
-8@@ -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 ;Signature · 8 lines
1.5.6.290.gc4e15 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
* David Miller <[email protected]> 2008-06-27 20:16
Quoted 9 lines
From: Harvey Harrison <[email protected]>
Date: Wed, 25 Jun 2008 14:14:00 -0700Quoted 4 lines
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 <[email protected]>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.
* Thomas Graf <[email protected]> 2008-06-29 11:54
Quoted 1 line
I wasn't aware of these flags when I wrote the code initially.
macros I mean...
PATCH 1/1 net: em_cmp.c use unaligned access helpers
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.
---
Dave, here's a complete patch if you want to apply.
net/sched/em_cmp.c | 17 +++++++---------- 1 files changed, 7 insertions(+), 10 deletions(-)
net/sched/em_cmp.c +7 -10
@@ -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 :
Signature · 1 line
1.5.6.1.179.g18c1f
Harvey Harrison
Reviewed-byQuoted 7 lines · Thomas Graf
* David Miller <[email protected]> 2008-06-27 20:16
Quoted 6 lines
From: Harvey Harrison <[email protected]>
Date: Wed, 25 Jun 2008 14:14:00 -0700Quoted 3 lines
Both locations are loading a big-endian value in cpu-endianness. The be32/be16_to_cpu immediately afterwards seems suspect.
Quoted 8 lines
Quoted 3 lines
Quoted 1 line
Signed-off-by Harvey Harrison <[email protected]>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.
Quoted 10 lines
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.
* Harvey Harrison <[email protected]> 2008-06-29 11:01
Quoted 3 lines
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.
Quoted 10 lines · Thomas Graf
* Harvey Harrison <[email protected]> 2008-06-29 11:01
Quoted 3 lines
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 <[email protected]>
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.
--- net/sched/em_cmp.c | 20 +++----------------- 1 files changed, 3 insertions(+), 17 deletions(-)
net/sched/em_cmp.c +3 -17
@@ -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 :
Signature · 1 line
1.5.6.1.322.ge904b
* Harvey Harrison <[email protected]> 2008-07-04 19:10
Quoted 15 lines · Thomas Graf
Quoted 10 lines
* Harvey Harrison <[email protected]> 2008-06-29 11:01
Quoted 3 lines
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:
Quoted 6 lines
From: Harvey Harrison <[email protected]>
Subject: [PATCH] net:em_cmp.cuse unaligned access helperptr 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.