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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CALnjE+qPfACh-ykS6HWSmLVQF=sJRrQfA231urskMB01eW=qNA@mail.gmail.com>
Date:	Thu, 20 Aug 2015 14:01:00 -0700
From:	Pravin Shelar <pshelar@...ira.com>
To:	Joe Stringer <joestringer@...ira.com>
Cc:	netdev <netdev@...r.kernel.org>,
	LKML <linux-kernel@...r.kernel.org>, pablo <pablo@...filter.org>,
	Florian Westphal <fwestpha@...hat.com>,
	Hannes Sowa <hannes@...hat.com>, Thomas Graf <tgraf@...g.ch>,
	Justin Pettit <jpettit@...ira.com>,
	Jesse Gross <jesse@...ira.com>
Subject: Re: [PATCHv4 net-next 09/10] openvswitch: Allow matching on conntrack label

On Thu, Aug 20, 2015 at 12:13 PM, Joe Stringer <joestringer@...ira.com> wrote:
> On 20 August 2015 at 08:45, Pravin Shelar <pshelar@...ira.com> wrote:
>> On Wed, Aug 19, 2015 at 4:04 PM, Joe Stringer <joestringer@...ira.com> wrote:
>>> Thanks for the review,
>>>
>>> On 19 August 2015 at 14:24, Pravin Shelar <pshelar@...ira.com> wrote:
>>>> On Tue, Aug 18, 2015 at 4:39 PM, Joe Stringer <joestringer@...ira.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 netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html