netdev - Re: [PATCH net-next 3/3] openvswitch: set IPS_CONFIRMED in tmpl status only when commit is set in conntrack

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CADvbK_cYpB07dvyMSGOU3XsAJmZV_feb76hVg9tCJeFjs5iuxA@mail.gmail.com>
Date: Mon, 8 Jul 2024 21:49:54 -0400
From: Xin Long <lucien.xin@...il.com>
To: Florian Westphal <fw@...len.de>
Cc: Ilya Maximets <i.maximets@....org>, network dev <netdev@...r.kernel.org>, dev@...nvswitch.org, 
	Marcelo Ricardo Leitner <marcelo.leitner@...il.com>, Jiri Pirko <jiri@...nulli.us>, 
	Davide Caratti <dcaratti@...hat.com>, Jamal Hadi Salim <jhs@...atatu.com>, 
	Eric Dumazet <edumazet@...gle.com>, Cong Wang <xiyou.wangcong@...il.com>, kuba@...nel.org, 
	Paolo Abeni <pabeni@...hat.com>, davem@...emloft.net, 
	Pablo Neira Ayuso <pablo@...filter.org>, Aaron Conole <aconole@...hat.com>
Subject: Re: [PATCH net-next 3/3] openvswitch: set IPS_CONFIRMED in tmpl
 status only when commit is set in conntrack

On Mon, Jul 8, 2024 at 6:38 PM Florian Westphal <fw@...len.de> wrote:
>
> Xin Long <lucien.xin@...il.com> wrote:
> > I can avoid this warning by not allocating ext for commit ct in ovs:
> >
> > @@ -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);
> >         if (!cl)
> >                 return -ENOSPC;
ovs_ct_get_conn_labels() must be replaced with nf_ct_labels_find() in here
anyway, thinking that the confirmed ct without labels was created in other
places (not by OVS conntrack), the warning may still be triggered when
trying to set labels in OVS after.

> >
> > However, the test case would fail, although the failure can be worked around
> > by setting ct_label in the 1st rule:
> >
> >   table=0,priority=30,in_port=1,ip,nw_dst=172.1.1.2,actions=ct(commit,nat(dst=10.1.1.2:80),exec(set_field:0x01->ct_label),table=1)
> >
> > So I'm worrying our change may break some existing OVS user cases.
>
> Then ovs_ct_limit_init() and nf_connlabels_get() need to be called
> once on the first conntrack operatation, regardless if labels are asked
> for or not.
>
> Not nice but still better than current state.
Right, not nice, it undermines the bits check against NF_CT_LABELS_MAX_SIZE.

>
> ovs_ct_execute() perhaps?
ovs_ct_execute() is in the hot path, and a better place would be
ovs_ct_copy_action() where the ct for an ovs flow is added.