[ovs-dev] [PATCH v2 06/22] datapath: Do not trigger events for unconfirmed connections.

[Jarno Rajahalme]

Upstream commit:

    commit 193e30967897f3a8b6f9f137ac30571d832c2c5c
    Author: Jarno Rajahalme <jarno at ovn.org>
    Date:   Thu Feb 9 11:21:54 2017 -0800

    openvswitch: Do not trigger events for unconfirmed connections.
    Receiving change events before the 'new' event for the connection has
    been received can be confusing.  Avoid triggering change events for
    setting conntrack mark or labels before the conntrack entry has been
    confirmed.

    Fixes: 182e3042e15d ("openvswitch: Allow matching on conntrack mark")
    Fixes: c2ac66735870 ("openvswitch: Allow matching on conntrack label")
    Signed-off-by: Jarno Rajahalme <jarno at ovn.org>
    Acked-by: Joe Stringer <joe at ovn.org>
    Acked-by: Pravin B Shelar <pshelar at ovn.org>
    Signed-off-by: David S. Miller <davem at davemloft.net>

Upstream commit:

    commit 2317c6b51e4249dbfa093e1b88cab0a9f0564b7f
    Author: Jarno Rajahalme <jarno at ovn.org>
    Date:   Fri Feb 17 18:11:58 2017 -0800

    openvswitch: Set event bit after initializing labels.

    Connlabels are included in conntrack netlink event messages only if
    the IPCT_LABEL bit is set in the event cache (see
    ctnetlink_conntrack_event()).  Set it after initializing labels for a
    new connection.

    Found upon further system testing, where it was noticed that labels
    were missing from the conntrack events.

    Fixes: 193e30967897 ("openvswitch: Do not trigger events for unconfirmed con
nections.")
    Signed-off-by: Jarno Rajahalme <jarno at ovn.org>
    Acked-by: Pravin B Shelar <pshelar at ovn.org>
    Signed-off-by: David S. Miller <davem at davemloft.net>

Fixes: 372ce9737d2b ("datapath: Allow matching on conntrack mark")
Fixes: 038e34abaa31 ("datapath: Allow matching on conntrack label")
Signed-off-by: Jarno Rajahalme <jarno at ovn.org>
---
 datapath/conntrack.c | 33 +++++++++++++++++++++++++++------
 1 file changed, 27 insertions(+), 6 deletions(-)

diff --git a/datapath/conntrack.c b/datapath/conntrack.c
index 10a7b91..9595fca 100644
--- a/datapath/conntrack.c
+++ b/datapath/conntrack.c
@@ -261,7 +261,8 @@ static int ovs_ct_set_mark(struct sk_buff *skb, struct sw_flow_key *key,
 	new_mark = ct_mark | (ct->mark & ~(mask));
 	if (ct->mark != new_mark) {
 		ct->mark = new_mark;
-		nf_conntrack_event_cache(IPCT_MARK, ct);
+		if (nf_ct_is_confirmed(ct))
+			nf_conntrack_event_cache(IPCT_MARK, ct);
 		key->ct.mark = new_mark;
 	}
 
@@ -278,7 +279,6 @@ static int ovs_ct_set_labels(struct sk_buff *skb, struct sw_flow_key *key,
 	enum ip_conntrack_info ctinfo;
 	struct nf_conn_labels *cl;
 	struct nf_conn *ct;
-	int err;
 
 	/* The connection could be invalid, in which case set_label is no-op.*/
 	ct = nf_ct_get(skb, &ctinfo);
@@ -294,10 +294,31 @@ static int ovs_ct_set_labels(struct sk_buff *skb, struct sw_flow_key *key,
 	if (!cl || ovs_ct_get_labels_len(cl) < OVS_CT_LABELS_LEN)
 		return -ENOSPC;
 
-	err = nf_connlabels_replace(ct, (u32 *)labels, (u32 *)mask,
-				    OVS_CT_LABELS_LEN / sizeof(u32));
-	if (err)
-		return err;
+	if (nf_ct_is_confirmed(ct)) {
+		/* Triggers a change event, which makes sense only for
+		 * confirmed connections.
+		 */
+		int err = nf_connlabels_replace(ct, (u32 *)labels, (u32 *)mask,
+						OVS_CT_LABELS_LEN / sizeof(u32));
+		if (err)
+			return err;
+	} else {
+		u32 *dst = (u32 *)cl->bits;
+		const u32 *msk = (const u32 *)mask->ct_labels;
+		const u32 *lbl = (const u32 *)labels->ct_labels;
+		int i;
+
+		/* No-one else has access to the non-confirmed entry, copy
+		 * labels over, keeping any bits we are not explicitly setting.
+		 */
+		for (i = 0; i < OVS_CT_LABELS_LEN / sizeof(u32); i++)
+			dst[i] = (dst[i] & ~msk[i]) | (lbl[i] & msk[i]);
+
+		/* Labels are included in the IPCTNL_MSG_CT_NEW event only if
+		 * the IPCT_LABEL bit it set in the event cache.
+		 */
+		nf_conntrack_event_cache(IPCT_LABEL, ct);
+	}
 
 	ovs_ct_get_labels(ct, &key->ct.labels);
 	return 0;
-- 
2.1.4