On 3/5/25 15:59, Xin Long wrote: > On Tue, Mar 4, 2025 at 8:31 PM Jianbo Liu <[email protected]> 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 <[email protected]> >>> Signed-off-by: Xin Long <[email protected]> >>> --- >>> 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/[email protected]/ 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. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
