[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CADvbK_cD7gVbrOH3Ps6SXhbwyxka_BaMH+NvRY6rKBgwvORiRw@mail.gmail.com>
Date: Wed, 5 Mar 2025 09:59:12 -0500
From: Xin Long <lucien.xin@...il.com>
To: Jianbo Liu <jianbol@...dia.com>
Cc: network dev <netdev@...r.kernel.org>, dev@...nvswitch.org, ovs-dev@...nvswitch.org,
davem@...emloft.net, kuba@...nel.org, Eric Dumazet <edumazet@...gle.com>,
Paolo Abeni <pabeni@...hat.com>, Pravin B Shelar <pshelar@....org>, Ilya Maximets <i.maximets@....org>,
Aaron Conole <aconole@...hat.com>, Marcelo Ricardo Leitner <marcelo.leitner@...il.com>,
Florian Westphal <fw@...len.de>
Subject: Re: [PATCH net] openvswitch: avoid allocating labels_ext in ovs_ct_set_labels
On Tue, Mar 4, 2025 at 8:31 PM Jianbo Liu <jianbol@...dia.com> wrote:
>
>
>
> On 3/5/2025 1:15 AM, Xin Long wrote:
> > 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@...dia.com>
> > Signed-off-by: Xin Long <lucien.xin@...il.com>
> > ---
> > 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);
>
> I don't think it's correct fix. The label is not added and packets can't
> pass the next rule to match ct_label.
>
That's expected, external ct may not work in the flow with extensions.
Again, "openvswitch: switch to per-action label counting in conntrack"
only makes it easier to be triggered.
> I tested it and used the configuration posted before, ping can't work.
I've provided the 'workaround' with the ct zone for this in the other email.
I would also like to see the maintainer's option about this.
Thanks.
Powered by blists - more mailing lists