On Tue, Mar 4, 2025 at 8:00 PM Aaron Conole <[email protected]> wrote:
>
> Xin Long <[email protected]> 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 <[email protected]>
> > Signed-off-by: Xin Long <[email protected]>
> > ---
>
> 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.
Right. If you're okay with it, I'd like to delete this function and
directly use the following code in ovs_ct_init_labels():
cl = nf_ct_labels_find(ct);
if (!cl) {
nf_ct_labels_ext_add(ct);
cl = nf_ct_labels_find(ct);
}
since the function was originally introduced for only those two places.
Thanks.
>
> > 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;
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
