Re: [ovs-dev] [patch_v7 5/9] dpdk: Add orig tuple context recovery.

[Darrell Ball]

On Sat, Apr 29, 2017 at 7:01 PM, Daniele Di Proietto 
wrote:

> 2017-03-24 2:15 GMT-07:00 Darrell Ball :
> > This patch adds orig tuple checking and context
> > recovery; NAT interactions are factored in.
> > Orig tuple support exists to better handle policy
> > changes.
> >
> > Signed-off-by: Darrell Ball 
> > ---
> >  lib/conntrack.c | 69 ++
> +++
> >  1 file changed, 69 insertions(+)
> >
> > diff --git a/lib/conntrack.c b/lib/conntrack.c
> > index ee22280..5524495 100644
> > --- a/lib/conntrack.c
> > +++ b/lib/conntrack.c
> > @@ -679,6 +679,72 @@ handle_nat(struct dp_packet *pkt, struct conn *conn,
> >  }
> >  }
> >
> > +static bool
> > +check_orig_tuple(struct conntrack *ct, struct dp_packet *pkt,
> > + struct conn_lookup_ctx *ctx_in, long long now,
> > + unsigned *bucket, struct conn **conn,
> > + const struct nat_action_info_t *nat_action_info)
> > +OVS_REQUIRES(ct->buckets[*bucket].lock)
> > +{
> > +if ((ctx_in->key.dl_type == htons(ETH_TYPE_IP) &&
> > + !pkt->md.ct_orig_tuple.ipv4.ipv4_proto) ||
> > +(ctx_in->key.dl_type == htons(ETH_TYPE_IPV6) &&
> > + !pkt->md.ct_orig_tuple.ipv6.ipv6_proto) ||
> > +!(pkt->md.ct_state & (CS_SRC_NAT | CS_DST_NAT)) ||
> > +nat_action_info){
> > +return false;
> > +}
> > +
> > +ct_lock_unlock(>buckets[*bucket].lock);
> > +struct conn_lookup_ctx ctx;
> > +memset(, 0 , sizeof ctx);
> > +ctx.conn = NULL;
> > +
> > +if (ctx_in->key.dl_type == htons(ETH_TYPE_IP)) {
> > +
> > +ctx.key.src.addr.ipv4_aligned = pkt->md.ct_orig_tuple.ipv4.
> ipv4_src;
> > +ctx.key.dst.addr.ipv4_aligned = pkt->md.ct_orig_tuple.ipv4.
> ipv4_dst;
> > +
> > +if (ctx_in->key.nw_proto == IPPROTO_ICMP) {
> > +ctx.key.src.icmp_id = ctx_in->key.src.icmp_id;
> > +ctx.key.dst.icmp_id = ctx_in->key.dst.icmp_id;
> > +uint16_t src_port = ntohs(pkt->md.ct_orig_tuple.
> ipv4.src_port);
> > +ctx.key.src.icmp_type = (uint8_t) src_port;
> > +ctx.key.dst.icmp_type = reverse_icmp_type(ctx.key.src.
> icmp_type);
> > +} else {
> > +ctx.key.src.port = pkt->md.ct_orig_tuple.ipv4.src_port;
> > +ctx.key.dst.port = pkt->md.ct_orig_tuple.ipv4.dst_port;
> > +}
> > +ctx.key.nw_proto = pkt->md.ct_orig_tuple.ipv4.ipv4_proto;
> > +} else {
> > +ctx.key.src.addr.ipv6_aligned = pkt->md.ct_orig_tuple.ipv6.
> ipv6_src;
> > +ctx.key.dst.addr.ipv6_aligned = pkt->md.ct_orig_tuple.ipv6.
> ipv6_dst;
> > +
> > +if (ctx_in->key.nw_proto == IPPROTO_ICMPV6) {
> > +ctx.key.src.icmp_id = ctx_in->key.src.icmp_id;
> > +ctx.key.dst.icmp_id = ctx_in->key.dst.icmp_id;
> > +uint16_t src_port = ntohs(pkt->md.ct_orig_tuple.
> ipv6.src_port);
> > +ctx.key.src.icmp_type = (uint8_t) src_port;
> > +ctx.key.dst.icmp_type = reverse_icmp6_type(ctx.key.
> src.icmp_type);
> > +} else {
> > +ctx.key.src.port = pkt->md.ct_orig_tuple.ipv6.src_port;
> > +ctx.key.dst.port = pkt->md.ct_orig_tuple.ipv6.dst_port;
> > +}
> > +ctx.key.nw_proto = pkt->md.ct_orig_tuple.ipv6.ipv6_proto;
> > +}
> > +
> > +ctx.key.dl_type = ctx_in->key.dl_type;
> > +ctx.key.zone = pkt->md.ct_zone;
> > +
> > +ctx.hash = conn_key_hash(, ct->hash_basis);
> > +*bucket = hash_to_bucket(ctx.hash);
> > +ct_lock_lock(>buckets[*bucket].lock);
> > +conn_key_lookup(>buckets[*bucket], , now);
> > +*conn = ctx.conn;
> > +
> > +return *conn ? true : false;
> > +}
> > +
> >  static void
> >  process_one(struct conntrack *ct, struct dp_packet *pkt,
> >  struct conn_lookup_ctx *ctx, uint16_t zone,
> > @@ -735,6 +801,9 @@ process_one(struct conntrack *ct, struct dp_packet
> *pkt,
> >  if (nat_action_info && !create_new_conn) {
> >  handle_nat(pkt, conn, zone, ctx->reply, ctx->related);
> >  }
> > +
> > +}else if (check_orig_tuple(ct, pkt, ctx, now, , ,
> > +   nat_action_info)) {
>
> Sorry, I don't understand the feature very much, so I'm going to ask a
> couple of dumb
> questions :-)
>
> Why is the body of this 'else if' empty? Could you explain a little bit
> more
> in the commit message?
>

The original tuple is another way to an existed conn entry, when NAT is
done on the packet. It's value is mainly for related packets.
I'll add that this to the commit message.

As we discussed offline, you had a concern about conn_update_state()
not being called; this is bug - good catch !
I added the exact same call to the "else if" case as in the "if" case
i.e.
create_new_conn = conn_update_state(ct, pkt, ctx, , now, bucket);



>
> Thanks
>
> >  } else {
> >  if (ctx->related) {
> >  

Re: [ovs-dev] [patch_v7 5/9] dpdk: Add orig tuple context recovery.

[Daniele Di Proietto]

2017-03-24 2:15 GMT-07:00 Darrell Ball :
> This patch adds orig tuple checking and context
> recovery; NAT interactions are factored in.
> Orig tuple support exists to better handle policy
> changes.
>
> Signed-off-by: Darrell Ball 
> ---
>  lib/conntrack.c | 69 
> +
>  1 file changed, 69 insertions(+)
>
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index ee22280..5524495 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -679,6 +679,72 @@ handle_nat(struct dp_packet *pkt, struct conn *conn,
>  }
>  }
>
> +static bool
> +check_orig_tuple(struct conntrack *ct, struct dp_packet *pkt,
> + struct conn_lookup_ctx *ctx_in, long long now,
> + unsigned *bucket, struct conn **conn,
> + const struct nat_action_info_t *nat_action_info)
> +OVS_REQUIRES(ct->buckets[*bucket].lock)
> +{
> +if ((ctx_in->key.dl_type == htons(ETH_TYPE_IP) &&
> + !pkt->md.ct_orig_tuple.ipv4.ipv4_proto) ||
> +(ctx_in->key.dl_type == htons(ETH_TYPE_IPV6) &&
> + !pkt->md.ct_orig_tuple.ipv6.ipv6_proto) ||
> +!(pkt->md.ct_state & (CS_SRC_NAT | CS_DST_NAT)) ||
> +nat_action_info){
> +return false;
> +}
> +
> +ct_lock_unlock(>buckets[*bucket].lock);
> +struct conn_lookup_ctx ctx;
> +memset(, 0 , sizeof ctx);
> +ctx.conn = NULL;
> +
> +if (ctx_in->key.dl_type == htons(ETH_TYPE_IP)) {
> +
> +ctx.key.src.addr.ipv4_aligned = pkt->md.ct_orig_tuple.ipv4.ipv4_src;
> +ctx.key.dst.addr.ipv4_aligned = pkt->md.ct_orig_tuple.ipv4.ipv4_dst;
> +
> +if (ctx_in->key.nw_proto == IPPROTO_ICMP) {
> +ctx.key.src.icmp_id = ctx_in->key.src.icmp_id;
> +ctx.key.dst.icmp_id = ctx_in->key.dst.icmp_id;
> +uint16_t src_port = ntohs(pkt->md.ct_orig_tuple.ipv4.src_port);
> +ctx.key.src.icmp_type = (uint8_t) src_port;
> +ctx.key.dst.icmp_type = reverse_icmp_type(ctx.key.src.icmp_type);
> +} else {
> +ctx.key.src.port = pkt->md.ct_orig_tuple.ipv4.src_port;
> +ctx.key.dst.port = pkt->md.ct_orig_tuple.ipv4.dst_port;
> +}
> +ctx.key.nw_proto = pkt->md.ct_orig_tuple.ipv4.ipv4_proto;
> +} else {
> +ctx.key.src.addr.ipv6_aligned = pkt->md.ct_orig_tuple.ipv6.ipv6_src;
> +ctx.key.dst.addr.ipv6_aligned = pkt->md.ct_orig_tuple.ipv6.ipv6_dst;
> +
> +if (ctx_in->key.nw_proto == IPPROTO_ICMPV6) {
> +ctx.key.src.icmp_id = ctx_in->key.src.icmp_id;
> +ctx.key.dst.icmp_id = ctx_in->key.dst.icmp_id;
> +uint16_t src_port = ntohs(pkt->md.ct_orig_tuple.ipv6.src_port);
> +ctx.key.src.icmp_type = (uint8_t) src_port;
> +ctx.key.dst.icmp_type = 
> reverse_icmp6_type(ctx.key.src.icmp_type);
> +} else {
> +ctx.key.src.port = pkt->md.ct_orig_tuple.ipv6.src_port;
> +ctx.key.dst.port = pkt->md.ct_orig_tuple.ipv6.dst_port;
> +}
> +ctx.key.nw_proto = pkt->md.ct_orig_tuple.ipv6.ipv6_proto;
> +}
> +
> +ctx.key.dl_type = ctx_in->key.dl_type;
> +ctx.key.zone = pkt->md.ct_zone;
> +
> +ctx.hash = conn_key_hash(, ct->hash_basis);
> +*bucket = hash_to_bucket(ctx.hash);
> +ct_lock_lock(>buckets[*bucket].lock);
> +conn_key_lookup(>buckets[*bucket], , now);
> +*conn = ctx.conn;
> +
> +return *conn ? true : false;
> +}
> +
>  static void
>  process_one(struct conntrack *ct, struct dp_packet *pkt,
>  struct conn_lookup_ctx *ctx, uint16_t zone,
> @@ -735,6 +801,9 @@ process_one(struct conntrack *ct, struct dp_packet *pkt,
>  if (nat_action_info && !create_new_conn) {
>  handle_nat(pkt, conn, zone, ctx->reply, ctx->related);
>  }
> +
> +}else if (check_orig_tuple(ct, pkt, ctx, now, , ,
> +   nat_action_info)) {

Sorry, I don't understand the feature very much, so I'm going to ask a
couple of dumb
questions :-)

Why is the body of this 'else if' empty? Could you explain a little bit more
in the commit message?

Thanks

>  } else {
>  if (ctx->related) {
>  pkt->md.ct_state = CS_INVALID;
> --
> 1.9.1
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [patch_v7 5/9] dpdk: Add orig tuple context recovery.

[Darrell Ball]

This patch adds orig tuple checking and context
recovery; NAT interactions are factored in.
Orig tuple support exists to better handle policy
changes.

Signed-off-by: Darrell Ball 
---
 lib/conntrack.c | 69 +
 1 file changed, 69 insertions(+)

diff --git a/lib/conntrack.c b/lib/conntrack.c
index ee22280..5524495 100644
--- a/lib/conntrack.c
+++ b/lib/conntrack.c
@@ -679,6 +679,72 @@ handle_nat(struct dp_packet *pkt, struct conn *conn,
 }
 }
 
+static bool
+check_orig_tuple(struct conntrack *ct, struct dp_packet *pkt,
+ struct conn_lookup_ctx *ctx_in, long long now,
+ unsigned *bucket, struct conn **conn,
+ const struct nat_action_info_t *nat_action_info)
+OVS_REQUIRES(ct->buckets[*bucket].lock)
+{
+if ((ctx_in->key.dl_type == htons(ETH_TYPE_IP) &&
+ !pkt->md.ct_orig_tuple.ipv4.ipv4_proto) ||
+(ctx_in->key.dl_type == htons(ETH_TYPE_IPV6) &&
+ !pkt->md.ct_orig_tuple.ipv6.ipv6_proto) ||
+!(pkt->md.ct_state & (CS_SRC_NAT | CS_DST_NAT)) ||
+nat_action_info){
+return false;
+}
+
+ct_lock_unlock(>buckets[*bucket].lock);
+struct conn_lookup_ctx ctx;
+memset(, 0 , sizeof ctx);
+ctx.conn = NULL;
+
+if (ctx_in->key.dl_type == htons(ETH_TYPE_IP)) {
+
+ctx.key.src.addr.ipv4_aligned = pkt->md.ct_orig_tuple.ipv4.ipv4_src;
+ctx.key.dst.addr.ipv4_aligned = pkt->md.ct_orig_tuple.ipv4.ipv4_dst;
+
+if (ctx_in->key.nw_proto == IPPROTO_ICMP) {
+ctx.key.src.icmp_id = ctx_in->key.src.icmp_id;
+ctx.key.dst.icmp_id = ctx_in->key.dst.icmp_id;
+uint16_t src_port = ntohs(pkt->md.ct_orig_tuple.ipv4.src_port);
+ctx.key.src.icmp_type = (uint8_t) src_port;
+ctx.key.dst.icmp_type = reverse_icmp_type(ctx.key.src.icmp_type);
+} else {
+ctx.key.src.port = pkt->md.ct_orig_tuple.ipv4.src_port;
+ctx.key.dst.port = pkt->md.ct_orig_tuple.ipv4.dst_port;
+}
+ctx.key.nw_proto = pkt->md.ct_orig_tuple.ipv4.ipv4_proto;
+} else {
+ctx.key.src.addr.ipv6_aligned = pkt->md.ct_orig_tuple.ipv6.ipv6_src;
+ctx.key.dst.addr.ipv6_aligned = pkt->md.ct_orig_tuple.ipv6.ipv6_dst;
+
+if (ctx_in->key.nw_proto == IPPROTO_ICMPV6) {
+ctx.key.src.icmp_id = ctx_in->key.src.icmp_id;
+ctx.key.dst.icmp_id = ctx_in->key.dst.icmp_id;
+uint16_t src_port = ntohs(pkt->md.ct_orig_tuple.ipv6.src_port);
+ctx.key.src.icmp_type = (uint8_t) src_port;
+ctx.key.dst.icmp_type = reverse_icmp6_type(ctx.key.src.icmp_type);
+} else {
+ctx.key.src.port = pkt->md.ct_orig_tuple.ipv6.src_port;
+ctx.key.dst.port = pkt->md.ct_orig_tuple.ipv6.dst_port;
+}
+ctx.key.nw_proto = pkt->md.ct_orig_tuple.ipv6.ipv6_proto;
+}
+
+ctx.key.dl_type = ctx_in->key.dl_type;
+ctx.key.zone = pkt->md.ct_zone;
+
+ctx.hash = conn_key_hash(, ct->hash_basis);
+*bucket = hash_to_bucket(ctx.hash);
+ct_lock_lock(>buckets[*bucket].lock);
+conn_key_lookup(>buckets[*bucket], , now);
+*conn = ctx.conn;
+
+return *conn ? true : false;
+}
+
 static void
 process_one(struct conntrack *ct, struct dp_packet *pkt,
 struct conn_lookup_ctx *ctx, uint16_t zone,
@@ -735,6 +801,9 @@ process_one(struct conntrack *ct, struct dp_packet *pkt,
 if (nat_action_info && !create_new_conn) {
 handle_nat(pkt, conn, zone, ctx->reply, ctx->related);
 }
+
+}else if (check_orig_tuple(ct, pkt, ctx, now, , ,
+   nat_action_info)) {
 } else {
 if (ctx->related) {
 pkt->md.ct_state = CS_INVALID;
-- 
1.9.1

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