Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1210028 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCHv4 net-next 09/10] openvswitch: Allow matching on conntrack label Pravin Shelar <pshelar@nicira.com> - 2015-08-19 23:30 +0200
    Re: [PATCHv4 net-next 09/10] openvswitch: Allow matching on conntrack label Joe Stringer <joestringer@nicira.com> - 2015-08-20 01:10 +0200
      Re: [PATCHv4 net-next 09/10] openvswitch: Allow matching on conntrack label Pravin Shelar <pshelar@nicira.com> - 2015-08-20 17:50 +0200
        Re: [PATCHv4 net-next 09/10] openvswitch: Allow matching on conntrack label Joe Stringer <joestringer@nicira.com> - 2015-08-20 21:20 +0200
          Re: [PATCHv4 net-next 09/10] openvswitch: Allow matching on conntrack label Pravin Shelar <pshelar@nicira.com> - 2015-08-20 23:10 +0200
            Re: [PATCHv4 net-next 09/10] openvswitch: Allow matching on conntrack label Joe Stringer <joestringer@nicira.com> - 2015-08-21 02:50 +0200

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

On Tue, Aug 18, 2015 at 4:39 PM, Joe Stringer <joestringer@nicira.com> 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.
>
> Signed-off-by: Joe Stringer <joestringer@nicira.com>
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

> ---
> v2: Split out setting the connlabel size for the current namespace.
> v3: No change.
> v4: Only allow setting label via ct action.
>     Update documentation.
> ---
...
>                                   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.

>         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.

>  void ovs_ct_exit(struct net *net, struct ovs_ct_perdp_data *data)
> @@ -518,4 +615,5 @@ void ovs_ct_exit(struct net *net, struct ovs_ct_perdp_data *data)
>                 nf_ct_l3proto_module_put(PF_INET);
>         if (data->xt_v6)
>                 nf_ct_l3proto_module_put(PF_INET6);
> +       nf_connlabels_put(net);
>  }
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1210066

Thanks for the review,

On 19 August 2015 at 14:24, Pravin Shelar <pshelar@nicira.com> wrote:
> On Tue, Aug 18, 2015 at 4:39 PM, Joe Stringer <joestringer@nicira.com> 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.
>>
>> Signed-off-by: Joe Stringer <joestringer@nicira.com>
> 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.

>>                                   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.

>>         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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1210600

On Wed, Aug 19, 2015 at 4:04 PM, Joe Stringer <joestringer@nicira.com> wrote:
> Thanks for the review,
>
> On 19 August 2015 at 14:24, Pravin Shelar <pshelar@nicira.com> wrote:
>> On Tue, Aug 18, 2015 at 4:39 PM, Joe Stringer <joestringer@nicira.com> 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()
>
> 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.

Looks good.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1210688

On 20 August 2015 at 08:45, Pravin Shelar <pshelar@nicira.com> wrote:
> On Wed, Aug 19, 2015 at 4:04 PM, Joe Stringer <joestringer@nicira.com> wrote:
>> Thanks for the review,
>>
>> On 19 August 2015 at 14:24, Pravin Shelar <pshelar@nicira.com> wrote:
>>> On Tue, Aug 18, 2015 at 4:39 PM, Joe Stringer <joestringer@nicira.com> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1210732

On Thu, Aug 20, 2015 at 12:13 PM, Joe Stringer <joestringer@nicira.com> wrote:
> On 20 August 2015 at 08:45, Pravin Shelar <pshelar@nicira.com> wrote:
>> On Wed, Aug 19, 2015 at 4:04 PM, Joe Stringer <joestringer@nicira.com> wrote:
>>> Thanks for the review,
>>>
>>> On 19 August 2015 at 14:24, Pravin Shelar <pshelar@nicira.com> wrote:
>>>> On Tue, Aug 18, 2015 at 4:39 PM, Joe Stringer <joestringer@nicira.com> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1210819

On 20 August 2015 at 14:01, Pravin Shelar <pshelar@nicira.com> wrote:
> On Thu, Aug 20, 2015 at 12:13 PM, Joe Stringer <joestringer@nicira.com> wrote:
>> On 20 August 2015 at 08:45, Pravin Shelar <pshelar@nicira.com> wrote:
>>> On Wed, Aug 19, 2015 at 4:04 PM, Joe Stringer <joestringer@nicira.com> wrote:
>>>> Thanks for the review,
>>>>
>>>> On 19 August 2015 at 14:24, Pravin Shelar <pshelar@nicira.com> wrote:
>>>>> On Tue, Aug 18, 2015 at 4:39 PM, Joe Stringer <joestringer@nicira.com> 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web