Re: [ovs-dev] [patch v9 02/11] flow: Enhance parse_ipv6_ext_hdrs.

2018-12-10 Thread Darrell Ball
On Mon, Dec 10, 2018 at 5:48 PM Ben Pfaff  wrote:

> On Mon, Nov 19, 2018 at 11:09:21AM -0800, Darrell Ball wrote:
> > Acked-by: Justin Pettit 
> > Signed-off-by: Darrell Ball 
>
> Thanks for adding the documentation comment to parse_ipv6_ext_hdrs().
> Please update the documentation comment to describe the return value.
>

sure


>
> I don't see anything that sets *frag_hdr to NULL if there is no fragment
> header.  This seems risky at best.
>

Agreed; there is no guarantee the user would check 'nw_frag' before
accessing frag_hdr
and/or set *frag_hdr to NULL in the caller.


>
> Thanks,
>
> Ben.
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev


Re: [ovs-dev] [patch v9 02/11] flow: Enhance parse_ipv6_ext_hdrs.

2018-12-10 Thread Ben Pfaff
On Mon, Nov 19, 2018 at 11:09:21AM -0800, Darrell Ball wrote:
> Acked-by: Justin Pettit 
> Signed-off-by: Darrell Ball 

Thanks for adding the documentation comment to parse_ipv6_ext_hdrs().
Please update the documentation comment to describe the return value.

I don't see anything that sets *frag_hdr to NULL if there is no fragment
header.  This seems risky at best.

Thanks,

Ben.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev


[ovs-dev] [patch v9 02/11] flow: Enhance parse_ipv6_ext_hdrs.

2018-11-19 Thread Darrell Ball
Acked-by: Justin Pettit 
Signed-off-by: Darrell Ball 
---
 lib/conntrack.c |  4 ++--
 lib/flow.c  | 40 ++--
 lib/flow.h  |  3 ++-
 3 files changed, 34 insertions(+), 13 deletions(-)

diff --git a/lib/conntrack.c b/lib/conntrack.c
index 974f985..682feb9 100644
--- a/lib/conntrack.c
+++ b/lib/conntrack.c
@@ -1310,7 +1310,6 @@ conntrack_execute(struct conntrack *ct, struct 
dp_packet_batch *pkt_batch,
   const struct nat_action_info_t *nat_action_info,
   long long now)
 {
-
 struct dp_packet *packet;
 struct conn_lookup_ctx ctx;
 
@@ -1558,7 +1557,8 @@ extract_l3_ipv6(struct conn_key *key, const void *data, 
size_t size,
 uint8_t nw_proto = ip6->ip6_nxt;
 uint8_t nw_frag = 0;
 
-if (!parse_ipv6_ext_hdrs(, , _proto, _frag)) {
+const struct ovs_16aligned_ip6_frag *frag_hdr;
+if (!parse_ipv6_ext_hdrs(, , _proto, _frag, _hdr)) {
 return false;
 }
 
diff --git a/lib/flow.c b/lib/flow.c
index c60446f..a2ce5fd 100644
--- a/lib/flow.c
+++ b/lib/flow.c
@@ -458,7 +458,8 @@ invalid:
 
 static inline bool
 parse_ipv6_ext_hdrs__(const void **datap, size_t *sizep, uint8_t *nw_proto,
-  uint8_t *nw_frag)
+  uint8_t *nw_frag,
+  const struct ovs_16aligned_ip6_frag **frag_hdr)
 {
 while (1) {
 if (OVS_LIKELY((*nw_proto != IPPROTO_HOPOPTS)
@@ -505,17 +506,17 @@ parse_ipv6_ext_hdrs__(const void **datap, size_t *sizep, 
uint8_t *nw_proto,
 return false;
 }
 } else if (*nw_proto == IPPROTO_FRAGMENT) {
-const struct ovs_16aligned_ip6_frag *frag_hdr = *datap;
+*frag_hdr = *datap;
 
-*nw_proto = frag_hdr->ip6f_nxt;
-if (!data_try_pull(datap, sizep, sizeof *frag_hdr)) {
+*nw_proto = (*frag_hdr)->ip6f_nxt;
+if (!data_try_pull(datap, sizep, sizeof **frag_hdr)) {
 return false;
 }
 
 /* We only process the first fragment. */
-if (frag_hdr->ip6f_offlg != htons(0)) {
+if ((*frag_hdr)->ip6f_offlg != htons(0)) {
 *nw_frag = FLOW_NW_FRAG_ANY;
-if ((frag_hdr->ip6f_offlg & IP6F_OFF_MASK) != htons(0)) {
+if (((*frag_hdr)->ip6f_offlg & IP6F_OFF_MASK) != htons(0)) {
 *nw_frag |= FLOW_NW_FRAG_LATER;
 *nw_proto = IPPROTO_FRAGMENT;
 return true;
@@ -525,11 +526,26 @@ parse_ipv6_ext_hdrs__(const void **datap, size_t *sizep, 
uint8_t *nw_proto,
 }
 }
 
+/* Parses IPv6 extension headers until a terminal header (or header we
+ * don't understand) is found.  'datap' points to the first extension
+ * header and advances as parsing occurs; 'sizep' is the remaining size
+ * and is decreased accordingly.  'nw_proto' starts as the first
+ * extension header to process and is updated as the extension headers
+ * are parsed.
+ *
+ * If a fragment header is found, '*frag_hdr' is set to the fragment
+ * header.  If it is the first fragment, extension header parsing
+ * otherwise continues as usual.  If it's not the first fragment,
+ * 'nw_proto' is set to IPPROTO_FRAGMENT and 'nw_frag' has
+ * FLOW_NW_FRAG_LATER set.  Both first and later fragments have
+ * FLOW_NW_FRAG_ANY set in 'nw_frag'. */
 bool
 parse_ipv6_ext_hdrs(const void **datap, size_t *sizep, uint8_t *nw_proto,
-uint8_t *nw_frag)
+uint8_t *nw_frag,
+const struct ovs_16aligned_ip6_frag **frag_hdr)
 {
-return parse_ipv6_ext_hdrs__(datap, sizep, nw_proto, nw_frag);
+return parse_ipv6_ext_hdrs__(datap, sizep, nw_proto, nw_frag,
+ frag_hdr);
 }
 
 bool
@@ -876,7 +892,9 @@ miniflow_extract(struct dp_packet *packet, struct miniflow 
*dst)
 nw_ttl = nh->ip6_hlim;
 nw_proto = nh->ip6_nxt;
 
-if (!parse_ipv6_ext_hdrs__(, , _proto, _frag)) {
+const struct ovs_16aligned_ip6_frag *frag_hdr;
+if (!parse_ipv6_ext_hdrs__(, , _proto, _frag,
+   _hdr)) {
 goto out;
 }
 
@@ -1077,7 +1095,9 @@ parse_tcp_flags(struct dp_packet *packet)
 plen = ntohs(nh->ip6_plen); /* Never pull padding. */
 dp_packet_set_l2_pad_size(packet, size - plen);
 size = plen;
-if (!parse_ipv6_ext_hdrs__(, , _proto, _frag)) {
+const struct ovs_16aligned_ip6_frag *frag_hdr;
+if (!parse_ipv6_ext_hdrs__(, , _proto, _frag,
+_hdr)) {
 return 0;
 }
 nw_proto = nh->ip6_nxt;
diff --git a/lib/flow.h b/lib/flow.h
index 5ebdb1f..7298c71 100644
--- a/lib/flow.h
+++ b/lib/flow.h
@@ -131,7 +131,8 @@ void flow_compose(struct dp_packet *, const struct flow *,
 void packet_expand(struct dp_packet *, const struct flow *, size_t size);
 
 bool parse_ipv6_ext_hdrs(const void **datap, size_t *sizep, uint8_t *nw_proto,
-