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

[Aaron Conole <aconole@redhat.com>]

Xin Long <lucien.xin@gmail.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