Re: [PATCHv2 net-next] openvswitch: switch to per-action label counting in conntrack

[Jianbo Liu <jianbol@nvidia.com>]

On 8/13/2024 1:17 AM, Xin Long wrote:
> Similar to commit 70f06c115bcc ("sched: act_ct: switch to per-action
> label counting"), we should also switch to per-action label counting
> in openvswitch conntrack, as Florian suggested.
> 
> The difference is that nf_connlabels_get() is called unconditionally
> when creating an ct action in ovs_ct_copy_action(). As with these
> flows:
> 
>    table=0,ip,actions=ct(commit,table=1)
>    table=1,ip,actions=ct(commit,exec(set_field:0xac->ct_label),table=2)
> 
> it needs to make sure the label ext is created in the 1st flow before
> the ct is committed in ovs_ct_commit(). Otherwise, the warning in
> nf_ct_ext_add() when creating the label ext in the 2nd flow will
> be triggered:
> 
>     WARN_ON(nf_ct_is_confirmed(ct));
> 

Hi Xin Long,

The ct can be committed before openvswitch handles packets with CT 
actions. And we can trigger the warning by creating VF and running ping 
over it with the following configurations:

ovs-vsctl add-br br
ovs-vsctl add-port br eth2
ovs-vsctl add-port br eth4
ovs-ofctl add-flow br "table=0, in_port=eth4,ip,ct_state=-trk 
actions=ct(table=1)"
ovs-ofctl add-flow br "table=1, in_port=eth4,ip,ct_state=+trk+new 
actions=ct(exec(set_field:0xef7d->ct_label), commit), output:eth2"
ovs-ofctl add-flow br "table=1, in_port=eth4,ip,ct_label=0xef7d, 
ct_state=+trk+est actions=output:eth2"

The eth2 is PF, and eth4 is VF's representor.
Would you like to fix it?

Thanks!
Jianbo

> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
> v2: move ovs_net into #if in ovs_ct_exit() as Jakub noticed.
> ---
>   net/openvswitch/conntrack.c | 30 ++++++++++++------------------
>   net/openvswitch/datapath.h  |  3 ---
>   2 files changed, 12 insertions(+), 21 deletions(-)
> 
> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
> index 8eb1d644b741..a3da5ee34f92 100644
> --- a/net/openvswitch/conntrack.c
> +++ b/net/openvswitch/conntrack.c
> @@ -1368,11 +1368,8 @@ bool ovs_ct_verify(struct net *net, enum ovs_key_attr attr)
>   	    attr == OVS_KEY_ATTR_CT_MARK)
>   		return true;
>   	if (IS_ENABLED(CONFIG_NF_CONNTRACK_LABELS) &&
> -	    attr == OVS_KEY_ATTR_CT_LABELS) {
> -		struct ovs_net *ovs_net = net_generic(net, ovs_net_id);
> -
> -		return ovs_net->xt_label;
> -	}
> +	    attr == OVS_KEY_ATTR_CT_LABELS)
> +		return true;
>   
>   	return false;
>   }
> @@ -1381,6 +1378,7 @@ int ovs_ct_copy_action(struct net *net, const struct nlattr *attr,
>   		       const struct sw_flow_key *key,
>   		       struct sw_flow_actions **sfa,  bool log)
>   {
> +	unsigned int n_bits = sizeof(struct ovs_key_ct_labels) * BITS_PER_BYTE;
>   	struct ovs_conntrack_info ct_info;
>   	const char *helper = NULL;
>   	u16 family;
> @@ -1409,6 +1407,12 @@ int ovs_ct_copy_action(struct net *net, const struct nlattr *attr,
>   		return -ENOMEM;
>   	}
>   
> +	if (nf_connlabels_get(net, n_bits - 1)) {
> +		nf_ct_tmpl_free(ct_info.ct);
> +		OVS_NLERR(log, "Failed to set connlabel length");
> +		return -EOPNOTSUPP;
> +	}
> +
>   	if (ct_info.timeout[0]) {
>   		if (nf_ct_set_timeout(net, ct_info.ct, family, key->ip.proto,
>   				      ct_info.timeout))
> @@ -1577,6 +1581,7 @@ static void __ovs_ct_free_action(struct ovs_conntrack_info *ct_info)
>   	if (ct_info->ct) {
>   		if (ct_info->timeout[0])
>   			nf_ct_destroy_timeout(ct_info->ct);
> +		nf_connlabels_put(nf_ct_net(ct_info->ct));
>   		nf_ct_tmpl_free(ct_info->ct);
>   	}
>   }
> @@ -2002,17 +2007,9 @@ struct genl_family dp_ct_limit_genl_family __ro_after_init = {
>   
>   int ovs_ct_init(struct net *net)
>   {
> -	unsigned int n_bits = sizeof(struct ovs_key_ct_labels) * BITS_PER_BYTE;
> +#if	IS_ENABLED(CONFIG_NETFILTER_CONNCOUNT)
>   	struct ovs_net *ovs_net = net_generic(net, ovs_net_id);
>   
> -	if (nf_connlabels_get(net, n_bits - 1)) {
> -		ovs_net->xt_label = false;
> -		OVS_NLERR(true, "Failed to set connlabel length");
> -	} else {
> -		ovs_net->xt_label = true;
> -	}
> -
> -#if	IS_ENABLED(CONFIG_NETFILTER_CONNCOUNT)
>   	return ovs_ct_limit_init(net, ovs_net);
>   #else
>   	return 0;
> @@ -2021,12 +2018,9 @@ int ovs_ct_init(struct net *net)
>   
>   void ovs_ct_exit(struct net *net)
>   {
> +#if	IS_ENABLED(CONFIG_NETFILTER_CONNCOUNT)
>   	struct ovs_net *ovs_net = net_generic(net, ovs_net_id);
>   
> -#if	IS_ENABLED(CONFIG_NETFILTER_CONNCOUNT)
>   	ovs_ct_limit_exit(net, ovs_net);
>   #endif
> -
> -	if (ovs_net->xt_label)
> -		nf_connlabels_put(net);
>   }
> diff --git a/net/openvswitch/datapath.h b/net/openvswitch/datapath.h
> index 9ca6231ea647..365b9bb7f546 100644
> --- a/net/openvswitch/datapath.h
> +++ b/net/openvswitch/datapath.h
> @@ -160,9 +160,6 @@ struct ovs_net {
>   #if	IS_ENABLED(CONFIG_NETFILTER_CONNCOUNT)
>   	struct ovs_ct_limit_info *ct_limit_info;
>   #endif
> -
> -	/* Module reference for configuring conntrack. */
> -	bool xt_label;
>   };
>   
>   /**