netdev - Re: [PATCH net] openvswitch: avoid allocating labels_ext in ovs_ct_set

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CADvbK_dWq8Wff5vmY6tMvQ7Pe+LSo5VBH04HREWmn6PdnCF8mA@mail.gmail.com>
Date: Wed, 5 Mar 2025 11:13:55 -0500
From: Xin Long <lucien.xin@...il.com>
To: Ilya Maximets <i.maximets@....org>
Cc: Jianbo Liu <jianbol@...dia.com>, 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 Wed, Mar 5, 2025 at 10:35 AM Ilya Maximets <i.maximets@....org> wrote:
>
> 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.
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.
>