Re: [ovs-dev] [PATCH net] openvswitch: avoid allocating labels_ext in ovs_ct_set_labels

[Aaron Conole <aconole@redhat.com>]

Xin Long <lucien.xin@gmail.com> writes:

> Currently, ovs_ct_set_labels() is only called for *confirmed* conntrack
> entries (ct) within ovs_ct_commit(). However, if the conntrack entry
> does not have the labels_ext extension, attempting to allocate it in
> ovs_ct_get_conn_labels() for a confirmed entry triggers a warning in
> nf_ct_ext_add():
>
>   WARN_ON(nf_ct_is_confirmed(ct));
>
> This happens when the conntrack entry is created externally before OVS
> increases net->ct.labels_used. The issue has become more likely since
> commit fcb1aa5163b1 ("openvswitch: switch to per-action label counting
> in conntrack"), which switched to per-action label counting.
>
> To prevent this warning, this patch modifies ovs_ct_set_labels() to
> call nf_ct_labels_find() instead of ovs_ct_get_conn_labels() where
> it allocates the labels_ext if it does not exist, aligning its
> behavior with tcf_ct_act_set_labels().
>
> Fixes: fcb1aa5163b1 ("openvswitch: switch to per-action label counting in conntrack")
> Reported-by: Jianbo Liu <jianbol@nvidia.com>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---

Just a nit, but after this change, the only user of the
ovs_ct_get_conn_labels function is in the init path.  I think it might
make sense to also rename it to something like 'ovs_ct_init_labels_ext'.
Then hopefully it would be clear not to use it outside of the
initialization path.

>  net/openvswitch/conntrack.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
> index 3bb4810234aa..f13fbab4c942 100644
> --- a/net/openvswitch/conntrack.c
> +++ b/net/openvswitch/conntrack.c
> @@ -426,7 +426,7 @@ static int ovs_ct_set_labels(struct nf_conn *ct, struct sw_flow_key *key,
>  	struct nf_conn_labels *cl;
>  	int err;
>  
> -	cl = ovs_ct_get_conn_labels(ct);
> +	cl = nf_ct_labels_find(ct);
>  	if (!cl)
>  		return -ENOSPC;