Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752744AbbHTTOV (ORCPT ); Thu, 20 Aug 2015 15:14:21 -0400 Received: from mail-yk0-f176.google.com ([209.85.160.176]:33354 "EHLO mail-yk0-f176.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752340AbbHTTOT (ORCPT ); Thu, 20 Aug 2015 15:14:19 -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: Thu, 20 Aug 2015 12:13:59 -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 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2941 Lines: 65 On 20 August 2015 at 08:45, Pravin Shelar wrote: > On Wed, Aug 19, 2015 at 4:04 PM, Joe Stringer wrote: >> 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. >>>> > >> >>>> 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/