Message-ID: <c7a9b481-190d-4b4b-a5ff-6f244c8c1abe@ovn.org>
Date: Wed, 5 Mar 2025 16:35:32 +0100
From: Ilya Maximets <i.maximets@....org>
To: Xin Long <lucien.xin@...il.com>, Jianbo Liu <jianbol@...dia.com>
Cc: i.maximets@....org, 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>,
 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 3/5/25 15:59, Xin Long wrote:
> 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 think, the test provided in the other thread is reasonable and logically
correct.   The link for the test:
  https://lore.kernel.org/all/2ee4d016-5e57-4d86-9dca-e4685cb183bb@nvidia.com/

We should be able to match on the label committed in the original direction.
The workaround doesn't really cover the use case, because the labels cover
a much larger scope that zones and it may be not possible to use zones instead
of labels.  Also, the ct entry obtained in the test is not from outside, AFAICT,
it is created inside the OVS pipeline and labels are also created inside the
OVS datapath, so it should work.

On a side note, the fact that it's allowed to modify labels for committed
connections, but it's not allowed to set one when there is none, seems like
an inconsistent behavior.  Maybe that should be fixed and the warning removed?

Best regards, Ilya Maximets.

> 
> I would also like to see the maintainer's option about this.
> 
> Thanks.

Powered by blists - more mailing lists