Re: [ovs-dev] [PATCH v2] tc: Fix incorrect TC rule for decap+encap datapath flow
On 2021-12-02 2:38 PM, Roi Dayan wrote:
A datapath flow generated for traffic from vxlan port to another vxlan port
looks like this:
tunnel(tun_id=0x65,src=10.10.11.3,dst=10.10.11.2,ttl=0/0,tp_dst=4789,flags(+key)),...,in_port(vxlan_sys_4789),...,
actions:set(tunnel(tun_id=0x66,src=10.10.12.2,dst=10.10.12.3,tp_dst=4789,flags(key))),vxlan_sys_4789
The generated TC rule with explicit tunnel key unset action added after
tunnel key set action, which is wrong.
filter protocol ip pref 7 flower chain 0 handle 0x1
dst_mac fa:16:3e:2a:4e:23
eth_type ipv4
ip_tos 0x0/3
enc_dst_ip 10.10.11.2
enc_src_ip 10.10.11.3
enc_key_id 101
enc_dst_port 4789
ip_flags nofrag
not_in_hw
action order 1: tunnel_key set
src_ip 10.10.12.2
dst_ip 10.10.12.3
key_id 102
dst_port 4789
nocsum pipe
index 1 ref 1 bind 1 installed 568 sec used 0 sec
Action statistics:
Sent 46620 bytes 555 pkt (dropped 0, overlimits 0 requeues 0)
backlog 0b 0p requeues 0
action order 2: tunnel_key unset pipe
index 2 ref 1 bind 1 installed 568 sec used 0 sec
Action statistics:
Sent 46620 bytes 555 pkt (dropped 0, overlimits 0 requeues 0)
backlog 0b 0p requeues 0
action order 3: mirred (Egress Redirect to device vxlan_sys_4789)
stolen
index 1 ref 1 bind 1 installed 568 sec used 0 sec
Action statistics:
Sent 46620 bytes 555 pkt (dropped 0, overlimits 0 requeues 0)
backlog 0b 0p requeues 0
cookie e0c82bfd504b701428b00db6b08db3b2
Fix it by also adding the the tunnel key unset action before the tunnel
key set action and not only before output port.
Fixes: 7c53bd7839d8 ("tc: Move tunnel_key unset action before output ports")
Signed-off-by: Roi Dayan
Reviewed-by: Paul Blakey
---
Notes:
v2
- fix a mistake in the reviewed-by tag email domain.
lib/tc.c | 21 +
1 file changed, 17 insertions(+), 4 deletions(-)
diff --git a/lib/tc.c b/lib/tc.c
index 38a1dfc0ebc8..adb2d3182ad4 100644
--- a/lib/tc.c
+++ b/lib/tc.c
@@ -2545,6 +2545,17 @@ nl_msg_put_flower_rewrite_pedits(struct ofpbuf *request,
return 0;
}
+static void
+nl_msg_put_flower_acts_release(struct ofpbuf *request, uint16_t act_index)
+{
+size_t act_offset;
+
+act_offset = nl_msg_start_nested(request, act_index);
+nl_msg_put_act_tunnel_key_release(request);
+nl_msg_put_act_flags(request);
+nl_msg_end_nested(request, act_offset);
+}
+
static int
nl_msg_put_flower_acts(struct ofpbuf *request, struct tc_flower *flower)
{
@@ -2579,6 +2590,11 @@ nl_msg_put_flower_acts(struct ofpbuf *request, struct
tc_flower *flower)
}
break;
case TC_ACT_ENCAP: {
+if (!released && flower->tunnel) {
+nl_msg_put_flower_acts_release(request, act_index++);
+released = true;
+}
+
act_offset = nl_msg_start_nested(request, act_index++);
nl_msg_put_act_tunnel_key_set(request,
action->encap.id_present,
action->encap.id,
@@ -2636,10 +2652,7 @@ nl_msg_put_flower_acts(struct ofpbuf *request, struct
tc_flower *flower)
break;
case TC_ACT_OUTPUT: {
if (!released && flower->tunnel) {
-act_offset = nl_msg_start_nested(request, act_index++);
-nl_msg_put_act_tunnel_key_release(request);
-nl_msg_put_act_flags(request);
-nl_msg_end_nested(request, act_offset);
+nl_msg_put_flower_acts_release(request, act_index++);
released = true;
}
hi,
are there any comments on this small fix?
thanks,
Roi
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH RFC 3/5] conntrack: Replaces nat_conn introducing key directionality.
Hi, Paolo
well done.
the patch looks good to me.
and I am going through your other patches.
I wonder maybe finally we need a *conn_flags* to store all the informations
containing NAT, cleaned flag, etc.
Paolo Valerio 于2021年11月30日周二 02:06写道：
> From: Peng He
>
> Currently, when doing NAT, the userspace conntrack will use an extra
> conn for the two directions in a flow. However, each conn has actually
> the two keys for both orig and rev directions. This patch introduces a
> key_node[CT_DIR_MAX] member in the conn which consists of key and a
> cmap_node for hash lookup. Both keys can now be accessed in the
> following way:
>
> conn->key_node[CT_DIR_{FWD,REV}].key
>
> similarly to what Aaron Conole suggested.
>
> This patch avoids the extra allocation for nat_conn, and makes
> userspace code cleaner.
>
> Signed-off-by: Peng He
> Co-authored-by: Paolo Valerio
> Signed-off-by: Paolo Valerio
> ---
> Initial patch:
>
>
> https://patchwork.ozlabs.org/project/openvswitch/patch/20201129033255.64647-2-hepeng.0...@bytedance.com/
> ---
> lib/conntrack-private.h | 20 +-
> lib/conntrack-tp.c |6 -
> lib/conntrack.c | 499
> ---
> 3 files changed, 229 insertions(+), 296 deletions(-)
>
> diff --git a/lib/conntrack-private.h b/lib/conntrack-private.h
> index 34c688821..ea5ba3d9e 100644
> --- a/lib/conntrack-private.h
> +++ b/lib/conntrack-private.h
> @@ -48,9 +48,16 @@ struct ct_endpoint {
> * hashing in ct_endpoint_hash_add(). */
> BUILD_ASSERT_DECL(sizeof(struct ct_endpoint) == sizeof(union ct_addr) +
> 4);
>
> +enum key_dir {
> +CT_DIR_FWD = 0,
> +CT_DIR_REV,
> +CT_DIR_MAX,
> +};
> +
> /* Changes to this structure need to be reflected in conn_key_hash()
> * and conn_key_cmp(). */
> struct conn_key {
> +enum key_dir dir;
> struct ct_endpoint src;
> struct ct_endpoint dst;
>
> @@ -86,21 +93,19 @@ struct alg_exp_node {
> bool nat_rpl_dst;
> };
>
> -enum OVS_PACKED_ENUM ct_conn_type {
> -CT_CONN_TYPE_DEFAULT,
> -CT_CONN_TYPE_UN_NAT,
> +struct conn_key_node {
> +struct conn_key key;
> +struct cmap_node cm_node;
> };
>
> struct conn {
> /* Immutable data. */
> -struct conn_key key;
> -struct conn_key rev_key;
> +struct conn_key_node key_node[CT_DIR_MAX];
> struct conn_key parent_key; /* Only used for orig_tuple support. */
> struct ovs_list exp_node;
> -struct cmap_node cm_node;
> +
> uint16_t nat_action;
> char *alg;
> -struct conn *nat_conn; /* The NAT 'conn' context, if there is one. */
>
> /* Mutable data. */
> struct ovs_mutex lock; /* Guards all mutable fields. */
> @@ -120,7 +125,6 @@ struct conn {
>
> /* Immutable data. */
> bool alg_related; /* True if alg data connection. */
> -enum ct_conn_type conn_type;
>
> uint32_t tp_id; /* Timeout policy ID. */
> };
> diff --git a/lib/conntrack-tp.c b/lib/conntrack-tp.c
> index c2245038b..9ecb06978 100644
> --- a/lib/conntrack-tp.c
> +++ b/lib/conntrack-tp.c
> @@ -282,7 +282,8 @@ conn_update_expiration(struct conntrack *ct, struct
> conn *conn,
> ovs_mutex_lock(>lock);
> VLOG_DBG_RL(, "Update timeout %s zone=%u with policy id=%d "
> "val=%u sec.",
> -ct_timeout_str[tm], conn->key.zone, conn->tp_id, val);
> +ct_timeout_str[tm], conn->key_node[CT_DIR_FWD].key.zone,
> +conn->tp_id, val);
>
> conn_update_expiration__(ct, conn, tm, now, val);
> }
> @@ -313,7 +314,8 @@ conn_init_expiration(struct conntrack *ct, struct conn
> *conn,
> }
>
> VLOG_DBG_RL(, "Init timeout %s zone=%u with policy id=%d val=%u
> sec.",
> -ct_timeout_str[tm], conn->key.zone, conn->tp_id, val);
> +ct_timeout_str[tm], conn->key_node[CT_DIR_FWD].key.zone,
> +conn->tp_id, val);
>
> conn_init_expiration__(ct, conn, tm, now, val);
> }
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index 983f824d2..a284c57c0 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -96,7 +96,6 @@ static struct conn *new_conn(struct conntrack *ct,
> struct dp_packet *pkt,
> uint32_t tp_id);
> static void delete_conn_cmn(struct conn *);
> static void delete_conn(struct conn *);
> -static void delete_conn_one(struct conn *conn);
> static enum ct_update_res conn_update(struct conntrack *ct, struct conn
> *conn,
>struct dp_packet *pkt,
>struct conn_lookup_ctx *ctx,
> @@ -110,8 +109,7 @@ static void set_label(struct dp_packet *, struct conn
> *,
> static void *clean_thread_main(void *f_);
>
> static bool
> -nat_get_unique_tuple(struct conntrack *ct, const struct conn *conn,
> - struct conn *nat_conn,
> +nat_get_unique_tuple(struct conntrack *ct, struct conn *conn,
> const struct nat_action_info_t *nat_info);
>
> static uint8_t
>
Re: [ovs-dev] [PATCH net v2 2/3] net/sched: flow_dissector: Fix matching on zone id for invalid conns
On Fri, Dec 10, 2021 at 8:52 PM Jakub Kicinski wrote:
>
> On Thu, 9 Dec 2021 09:57:33 +0200 Paul Blakey wrote:
> > @@ -238,10 +239,12 @@ void
> > skb_flow_dissect_ct(const struct sk_buff *skb,
> > struct flow_dissector *flow_dissector,
> > void *target_container, u16 *ctinfo_map,
> > - size_t mapsize, bool post_ct)
> > + size_t mapsize)
> > {
> > #if IS_ENABLED(CONFIG_NF_CONNTRACK)
> > + bool post_ct = tc_skb_cb(skb)->post_ct;
> > struct flow_dissector_key_ct *key;
> > + u16 zone = tc_skb_cb(skb)->zone;
> > enum ip_conntrack_info ctinfo;
> > struct nf_conn_labels *cl;
> > struct nf_conn *ct;
> > @@ -260,6 +263,7 @@ skb_flow_dissect_ct(const struct sk_buff *skb,
> > if (!ct) {
> > key->ct_state = TCA_FLOWER_KEY_CT_FLAGS_TRACKED |
> > TCA_FLOWER_KEY_CT_FLAGS_INVALID;
> > + key->ct_zone = zone;
> > return;
> > }
> >
>
> Why is flow dissector expecting skb cb to be TC now?
> Please keep the appropriate abstractions intact.
Probably because only fl_classify() calls it, but I agree with you
on this point, this function is supposed to be TC independent.
Thanks.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
