linux-kernel - 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: <CANr6G5yp1cj6GuhXDU6jvpaYAzb20-aiTUrpPLrBQAStwxpDVA@mail.gmail.com>
Date:	Wed, 19 Aug 2015 16:04:10 -0700
From:	Joe Stringer <joestringer@...ira.com>
To:	Pravin Shelar <pshelar@...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

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.
>>
>> Signed-off-by: Joe Stringer <joestringer@...ira.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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/