Re: [PATCHv4 net-next 09/10] openvswitch: Allow matching on conntrack label

Thanks for the review,

On 19 August 2015 at 14:24, Pravin Shelar [off-list ref] wrote:

On Tue, Aug 18, 2015 at 4:39 PM, Joe Stringer [off-list ref] wrote:

quoted

Allow matching and setting the conntrack label field. As with ct_mark,
this is populated by executing the CT action, and is a writable field.
Specifying a label and optional mask allows the label to be modified,
which takes effect on the entry found by the lookup of the CT action.

E.g.: actions:ct(zone=1,label=1)

This will perform conntrack lookup in zone 1, then modify the label for
that entry. The conntrack entry itself must be committed using the
"commit" flag in the conntrack action flags for this change to persist.

Signed-off-by: Joe Stringer <redacted>

I got compilation error after applying this patch:
net/openvswitch/conntrack.c: In function ‘ovs_ct_init’:
net/openvswitch/conntrack.c:713: error: expected ‘)’ before ‘;’ token
net/openvswitch/conntrack.c:715: error: expected expression before ‘}’ token

Sorry about that, missed it in my final refactoring round. I'll fix this.

quoted

                                  type);

@@ -432,6 +521,10 @@ bool ovs_ct_verify(enum ovs_key_attr attr)
        if (attr & OVS_KEY_ATTR_CT_MARK)
                return true;
 #endif
+#ifdef CONFIG_NF_CONNTRACK_LABELS
+       if (attr & OVS_KEY_ATTR_CT_LABEL)
+               return true;
+#endif

OVS_KEY_ATTR_CT_LABEL is not bit field so bitwise AND operation does
not work here. This applies to all check done in this function.

Should be BIT(...), I'll fix this.

quoted

        return false;
 }

@@ -508,8 +601,12 @@ void ovs_ct_free_action(const struct nlattr *a)

 void ovs_ct_init(struct net *net, struct ovs_ct_perdp_data *data)
 {
+       unsigned int n_bits = sizeof(struct ovs_key_ct_label) * BITS_PER_BYTE;
+
        data->xt_v4 = !nf_ct_l3proto_try_module_get(PF_INET);
        data->xt_v6 = !nf_ct_l3proto_try_module_get(PF_INET6);
+       if (nf_connlabels_get(net, n_bits);
+               OVS_NLERR(true, "Failed to set connlabel length");
 }

In case of error should we reject conntrack label actions? Otherwise
user will never see any error. But action could drop packets.

I suspect that currently errors would be seen from ovs_ct_set_label():

.......if (!cl || cl->words * sizeof(long) < OVS_CT_LABEL_LEN)
.......>.......return -ENOSPC;

So, for cmd_execute, userspace would see this. For regular handling,
pipeline processing would stop (so, drop).

However, I agree it would be more friendly to have the attribute
rejected up-front. Just means we'll pass the datapath all the way
down:
ovs_nla_get_match()
--> ovs_key_from_nlattrs()
--> metadata_from_nlattrs()
--> ovs_ct_verify()

And rather than simply reporting the error in ovs_ct_init() there,
we'll store the success condition something like:

@@ -721,8 +721,12 @@ void ovs_ct_init(struct net *net, struct

ovs_ct_perdp_data *data)

        data->xt_v4 = !nf_ct_l3proto_try_module_get(PF_INET);
        data->xt_v6 = !nf_ct_l3proto_try_module_get(PF_INET6);
-       if (nf_connlabels_get(net, n_bits));
+       if (nf_connlabels_get(net, n_bits)) {
+               data->xt_label = false;
                OVS_NLERR(true, "Failed to set connlabel length");
+       } else {
+               data->xt_label = true;
+       }
 }

 void ovs_ct_exit(struct net *net, struct ovs_ct_perdp_data *data)

ovs_ct_exit() also needs to be updated to ensure that if this fails,
we don't try to put the connlabel use back.