On Wed, Mar 5, 2025 at 10:35 AM Ilya Maximets <i.maxim...@ovn.org> wrote:
>
> On 3/5/25 15:59, Xin Long wrote:
> > On Tue, Mar 4, 2025 at 8:31 PM Jianbo Liu <jian...@nvidia.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 <jian...@nvidia.com>
> >>> Signed-off-by: Xin Long <lucien....@gmail.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-e4685cb18...@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.
All ct entries created by action=ct() will have label exts allocated.
This is because any flow with ct inserted in the kernel increases
net->ct.labels_used via nf_connlabels_get(), which ensures that the
new ct entry has label exts allocated during its creation. Therefore,
I believed this ct entry was created outside of OVS. But I might not
be aware of other scenarios where OVS itself might create ct entries.
>
> 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?
I need to check with Netfilter developers regarding this.
If I can't find a solution this week, I will revert
"openvswitch: switch to per-action label counting in conntrack"
as requested by Jianbo.
Thanks.
>
> Best regards, Ilya Maximets.
>
> >
> > I would also like to see the maintainer's option about this.
> >
> > Thanks.
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
