Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752603AbbHSXEc (ORCPT ); Wed, 19 Aug 2015 19:04:32 -0400 Received: from mail-yk0-f179.google.com ([209.85.160.179]:35466 "EHLO mail-yk0-f179.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751642AbbHSXEb convert rfc822-to-8bit (ORCPT ); Wed, 19 Aug 2015 19:04:31 -0400 MIME-Version: 1.0 In-Reply-To: References: <1439941188-19293-1-git-send-email-joestringer@nicira.com> <1439941188-19293-10-git-send-email-joestringer@nicira.com> From: Joe Stringer Date: Wed, 19 Aug 2015 16:04:10 -0700 Message-ID: Subject: Re: [PATCHv4 net-next 09/10] openvswitch: Allow matching on conntrack label To: Pravin Shelar Cc: netdev , LKML , pablo , Florian Westphal , Hannes Sowa , Thomas Graf , Justin Pettit , Jesse Gross Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3846 Lines: 97 Thanks for the review, On 19 August 2015 at 14:24, Pravin Shelar wrote: > On Tue, Aug 18, 2015 at 4:39 PM, Joe Stringer 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 > 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/