netdev - Re: [PATCH net-next 0/3] net: handle the exp removal problem with ovs upcall properly

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: <f7tedl46nah.fsf@redhat.com>
Date: Wed, 19 Jul 2023 09:31:02 -0400
From: Aaron Conole <aconole@...hat.com>
To: Xin Long <lucien.xin@...il.com>
Cc: network dev <netdev@...r.kernel.org>,  dev@...nvswitch.org,
  davem@...emloft.net,  kuba@...nel.org,  Eric Dumazet
 <edumazet@...gle.com>,  Paolo Abeni <pabeni@...hat.com>,  Pravin B Shelar
 <pshelar@....org>,  Jamal Hadi Salim <jhs@...atatu.com>,  Cong Wang
 <xiyou.wangcong@...il.com>,  Jiri Pirko <jiri@...nulli.us>,  Pablo Neira
 Ayuso <pablo@...filter.org>,  Florian Westphal <fw@...len.de>,  Marcelo
 Ricardo Leitner <marcelo.leitner@...il.com>,  Davide Caratti
 <dcaratti@...hat.com>
Subject: Re: [PATCH net-next 0/3] net: handle the exp removal problem with
 ovs upcall properly

Xin Long <lucien.xin@...il.com> writes:

> With the OVS upcall, the original ct in the skb will be dropped, and when
> the skb comes back from userspace it has to create a new ct again through
> nf_conntrack_in() in either OVS __ovs_ct_lookup() or TC tcf_ct_act().
>
> However, the new ct will not be able to have the exp as the original ct
> has taken it away from the hash table in nf_ct_find_expectation(). This
> will cause some flow never to be matched, like:
>
>   'ip,ct_state=-trk,in_port=1 actions=ct(zone=1)'
>   'ip,ct_state=+trk+new+rel,in_port=1 actions=ct(commit,zone=1)'
>   'ip,ct_state=+trk+new+rel,in_port=1 actions=ct(commit,zone=2),normal'
>
> if the 2nd flow triggers the OVS upcall, the 3rd flow will never get
> matched.
>
> OVS conntrack works around this by adding its own exp lookup function to
> not remove the exp from the hash table and saving the exp and its master
> info to the flow keys instead of create a real ct. But this way doesn't
> work for TC act_ct.
>
> The patch 1/3 allows nf_ct_find_expectation() not to remove the exp from
> the hash table if tmpl is set with IPS_CONFIRMED when doing lookup. This
> allows both OVS conntrack and TC act_ct to have a simple and clear fix
> for this problem in the patch 2/3 and 3/3.
>
> Xin Long (3):
>   netfilter: allow exp not to be removed in nf_ct_find_expectation
>   net: sched: set IPS_CONFIRMED in tmpl status only when commit is set
>     in act_ct
>   openvswitch: set IPS_CONFIRMED in tmpl status only when commit is set
>     in conntrack
>
>  include/net/netfilter/nf_conntrack_expect.h |  2 +-
>  net/netfilter/nf_conntrack_core.c           |  2 +-
>  net/netfilter/nf_conntrack_expect.c         |  4 +-
>  net/netfilter/nft_ct.c                      |  2 +
>  net/openvswitch/conntrack.c                 | 78 +++------------------
>  net/sched/act_ct.c                          |  3 +-
>  6 files changed, 18 insertions(+), 73 deletions(-)

Hi Xin,

The changes look okay to me, and I don't see anything that immediately
jumps out.  I would like to test it today / tomorrow with the ovs
check-kernel testsuite if you haven't done so already.

-Aaron