[ovs-dev] [PATCH] datapath: Remove custom version of ipv6_skip_exthdr().
Jesse Gross
jesse at nicira.com
Fri Dec 2 02:31:28 UTC 2011
On Thu, Dec 1, 2011 at 4:31 PM, Ben Pfaff <blp at nicira.com> wrote:
> On Thu, Dec 01, 2011 at 04:24:07PM -0800, Jesse Gross wrote:
>> We currently have a version of ipv6_skip_exthdr() which is
>> identical to the main one with the addition of fragment reporting.
>> We can propose our version for upstream and then use it directly
>> without duplication.
>>
>> Signed-off-by: Jesse Gross <jesse at nicira.com>
>
> The case of a IPv6 fragment header with frag_off of 0 is really odd. I
> guess that this code handles it acceptably (treating it the same as an
> unfragmented packet) but this new behavior differs, I believe, from
> userspace treatment of such a packet, and so I believe that the patch
> should also update lib/flow.c
I don't think this should change behavior in the case of valid packets
but you're right that it affects invalid ones. I fixed up userspace
like this:
diff --git a/lib/flow.c b/lib/flow.c
index 3865e50..a491aff 100644
--- a/lib/flow.c
+++ b/lib/flow.c
@@ -203,11 +203,14 @@ parse_ipv6(struct ofpbuf *packet, struct flow *flow)
}
/* We only process the first fragment. */
- flow->nw_frag = FLOW_NW_FRAG_ANY;
- if ((frag_hdr->ip6f_offlg & IP6F_OFF_MASK) != htons(0)) {
- flow->nw_frag |= FLOW_NW_FRAG_LATER;
- nexthdr = IPPROTO_FRAGMENT;
- break;
+ if (frag_hdr->ip6f_offlg != htons(0)) {
+ if ((frag_hdr->ip6f_offlg & IP6F_OFF_MASK) == htons(0)) {
+ flow->nw_frag = FLOW_NW_FRAG_ANY;
+ } else {
+ flow->nw_frag |= FLOW_NW_FRAG_LATER;
+ nexthdr = IPPROTO_FRAGMENT;
+ break;
+ }
}
}
}
> I believe that "ntohs(*fp) & ~0x7" can be written slightly more
> efficiently, or maybe just in a way that is more obviously optimizable,
> as "*fp & htons(~0x7)".
That's how it is currently in the upstream version. I didn't see the
need to make unrelated changes so I left it as is but did use the
optimized version where I added a similar comparison to OVS.
> This is a pretty big function to mark inline.
Yeah, I originally thought there was only one user but it turns out
there was another. I applied this:
diff --git a/datapath/linux/Modules.mk b/datapath/linux/Modules.mk
index 1f9973b..bb8eff6 100644
--- a/datapath/linux/Modules.mk
+++ b/datapath/linux/Modules.mk
@@ -1,6 +1,7 @@
openvswitch_sources += \
linux/compat/addrconf_core-openvswitch.c \
linux/compat/dev-openvswitch.c \
+ linux/compat/exthdrs_core.c \
linux/compat/flex_array.c \
linux/compat/genetlink-openvswitch.c \
linux/compat/ip_output-openvswitch.c \
diff --git a/datapath/linux/compat/exthdrs_core.c
b/datapath/linux/compat/exthdrs_core.c
new file mode 100644
index 0000000..658e16a
--- /dev/null
+++ b/datapath/linux/compat/exthdrs_core.c
@@ -0,0 +1,48 @@
+#include <linux/ipv6.h>
+#include <net/ipv6.h>
+
+/* This function is upstream but not the version which supplies the
+ * fragment offset. We plan to propose the extended version.
+ */
+int rpl_ipv6_skip_exthdr(const struct sk_buff *skb, int start,
+ u8 *nexthdrp, __be16 *frag_offp)
+{
+ u8 nexthdr = *nexthdrp;
+
+ *frag_offp = 0;
+
+ while (ipv6_ext_hdr(nexthdr)) {
+ struct ipv6_opt_hdr _hdr, *hp;
+ int hdrlen;
+
+ if (nexthdr == NEXTHDR_NONE)
+ return -1;
+ hp = skb_header_pointer(skb, start, sizeof(_hdr), &_hdr);
+ if (hp == NULL)
+ return -1;
+ if (nexthdr == NEXTHDR_FRAGMENT) {
+ __be16 _frag_off, *fp;
+ fp = skb_header_pointer(skb,
+ start+offsetof(struct frag_hdr,
+ frag_off),
+ sizeof(_frag_off),
+ &_frag_off);
+ if (fp == NULL)
+ return -1;
+
+ *frag_offp = *fp;
+ if (ntohs(*frag_offp) & ~0x7)
+ break;
+ hdrlen = 8;
+ } else if (nexthdr == NEXTHDR_AUTH)
+ hdrlen = (hp->hdrlen+2)<<2;
+ else
+ hdrlen = ipv6_optlen(hp);
+
+ nexthdr = hp->nexthdr;
+ start += hdrlen;
+ }
+
+ *nexthdrp = nexthdr;
+ return start;
+}
diff --git a/datapath/linux/compat/include/linux/ipv6.h
b/datapath/linux/compat/include/linux/ipv6.h
index f047e60..fdbbe62 100644
--- a/datapath/linux/compat/include/linux/ipv6.h
+++ b/datapath/linux/compat/include/linux/ipv6.h
@@ -15,47 +15,7 @@ static inline struct ipv6hdr *ipv6_hdr(const struct
sk_buff *skb)
* fragment offset. We plan to propose the extended version.
*/
#define ipv6_skip_exthdr rpl_ipv6_skip_exthdr
-static inline int ipv6_skip_exthdr(const struct sk_buff *skb, int start,
- u8 *nexthdrp, __be16 *frag_offp)
-{
- u8 nexthdr = *nexthdrp;
-
- *frag_offp = 0;
-
- while (ipv6_ext_hdr(nexthdr)) {
- struct ipv6_opt_hdr _hdr, *hp;
- int hdrlen;
-
- if (nexthdr == NEXTHDR_NONE)
- return -1;
- hp = skb_header_pointer(skb, start, sizeof(_hdr), &_hdr);
- if (hp == NULL)
- return -1;
- if (nexthdr == NEXTHDR_FRAGMENT) {
- __be16 _frag_off, *fp;
- fp = skb_header_pointer(skb,
- start+offsetof(struct frag_hdr,
- frag_off),
- sizeof(_frag_off),
- &_frag_off);
- if (fp == NULL)
- return -1;
-
- *frag_offp = *fp;
- if (ntohs(*frag_offp) & ~0x7)
- break;
- hdrlen = 8;
- } else if (nexthdr == NEXTHDR_AUTH)
- hdrlen = (hp->hdrlen+2)<<2;
- else
- hdrlen = ipv6_optlen(hp);
-
- nexthdr = hp->nexthdr;
- start += hdrlen;
- }
-
- *nexthdrp = nexthdr;
- return start;
-}
+extern int rpl_ipv6_skip_exthdr(const struct sk_buff *skb, int start,
+ u8 *nexthdrp, __be16 *frag_offp);
#endif
More information about the dev
mailing list