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

[Joe Stringer]

On 20 August 2015 at 14:01, Pravin Shelar <pshelar@xxxxxxxxxx> wrote:
> On Thu, Aug 20, 2015 at 12:13 PM, Joe Stringer <joestringer@xxxxxxxxxx> wrote:
>> On 20 August 2015 at 08:45, Pravin Shelar <pshelar@xxxxxxxxxx> wrote:
>>> On Wed, Aug 19, 2015 at 4:04 PM, Joe Stringer <joestringer@xxxxxxxxxx> wrote:
>>>> Thanks for the review,
>>>>
>>>> On 19 August 2015 at 14:24, Pravin Shelar <pshelar@xxxxxxxxxx> wrote:
>>>>> On Tue, Aug 18, 2015 at 4:39 PM, Joe Stringer <joestringer@xxxxxxxxxx> wrote:
>>>>>> 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.
>>>>>>
>>>
>>>>
>>>>>>         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()
>>
>> Incidentally, we generally don't have the datapath by this point
>> (ovs_nla_get_match()). There'd need to be a bit of rearranging in the
>> ovs_flow_cmd_* functions, which would include holding the locks for
>> longer. Given that the two most common cases are that either A) The
>> kernel is configured with connlabel support, and built with support
>> for at least 128 bits of label, or B) the kernel is configured without
>> connlabel, and this is handled already in ovs_ct_verify(), I don't
>> think it's worth making this particular change.
>
> Actually I do not see need for this to be per datapath property.
> infact there is no need to have struct ovs_ct_perdp_data.
> ovs_ct_init can be called from ovs-module init.
> nf-connlabel bit length is per net-namespace property. So
> nf_connlabels_get()  should be called from ovs namespace init. This
> way you can move xt_label flag to ovs_net. ovs_net can be accessed
> from ovs_flow_cmd_* functions.

That sounds like a tidier approach, I'll roll it into the next version.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/