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

[Xin Long <lucien.xin@gmail.com>]

On Tue, Jun 18, 2024 at 7:34 AM Ilya Maximets <i.maximets@ovn.org> wrote:
>
> On 6/17/24 22:10, Ilya Maximets wrote:
> > On 7/16/23 23:09, Xin Long wrote:
> >> By not setting IPS_CONFIRMED in tmpl that allows the exp not to be removed
> >> from the hashtable when lookup, we can simplify the exp processing code a
> >> lot in openvswitch conntrack.
> >>
> >> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> >> ---
> >>  net/openvswitch/conntrack.c | 78 +++++--------------------------------
> >>  1 file changed, 10 insertions(+), 68 deletions(-)
> >>
> >> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
> >> index 331730fd3580..fa955e892210 100644
> >> --- a/net/openvswitch/conntrack.c
> >> +++ b/net/openvswitch/conntrack.c
> >> @@ -455,45 +455,6 @@ static int ovs_ct_handle_fragments(struct net *net, struct sw_flow_key *key,
> >>      return 0;
> >>  }
> >>
> >> -static struct nf_conntrack_expect *
> >> -ovs_ct_expect_find(struct net *net, const struct nf_conntrack_zone *zone,
> >> -               u16 proto, const struct sk_buff *skb)
> >> -{
> >> -    struct nf_conntrack_tuple tuple;
> >> -    struct nf_conntrack_expect *exp;
> >> -
> >> -    if (!nf_ct_get_tuplepr(skb, skb_network_offset(skb), proto, net, &tuple))
> >> -            return NULL;
> >> -
> >> -    exp = __nf_ct_expect_find(net, zone, &tuple);
> >> -    if (exp) {
> >> -            struct nf_conntrack_tuple_hash *h;
> >> -
> >> -            /* Delete existing conntrack entry, if it clashes with the
> >> -             * expectation.  This can happen since conntrack ALGs do not
> >> -             * check for clashes between (new) expectations and existing
> >> -             * conntrack entries.  nf_conntrack_in() will check the
> >> -             * expectations only if a conntrack entry can not be found,
> >> -             * which can lead to OVS finding the expectation (here) in the
> >> -             * init direction, but which will not be removed by the
> >> -             * nf_conntrack_in() call, if a matching conntrack entry is
> >> -             * found instead.  In this case all init direction packets
> >> -             * would be reported as new related packets, while reply
> >> -             * direction packets would be reported as un-related
> >> -             * established packets.
> >> -             */
> >> -            h = nf_conntrack_find_get(net, zone, &tuple);
> >> -            if (h) {
> >> -                    struct nf_conn *ct = nf_ct_tuplehash_to_ctrack(h);
> >> -
> >> -                    nf_ct_delete(ct, 0, 0);
> >> -                    nf_ct_put(ct);
> >> -            }
> >> -    }
> >> -
> >> -    return exp;
> >> -}
> >> -
> >>  /* This replicates logic from nf_conntrack_core.c that is not exported. */
> >>  static enum ip_conntrack_info
> >>  ovs_ct_get_info(const struct nf_conntrack_tuple_hash *h)
> >> @@ -852,36 +813,16 @@ static int ovs_ct_lookup(struct net *net, struct sw_flow_key *key,
> >>                       const struct ovs_conntrack_info *info,
> >>                       struct sk_buff *skb)
> >>  {
> >> -    struct nf_conntrack_expect *exp;
> >> -
> >> -    /* If we pass an expected packet through nf_conntrack_in() the
> >> -     * expectation is typically removed, but the packet could still be
> >> -     * lost in upcall processing.  To prevent this from happening we
> >> -     * perform an explicit expectation lookup.  Expected connections are
> >> -     * always new, and will be passed through conntrack only when they are
> >> -     * committed, as it is OK to remove the expectation at that time.
> >> -     */
> >> -    exp = ovs_ct_expect_find(net, &info->zone, info->family, skb);
> >> -    if (exp) {
> >> -            u8 state;
> >> -
> >> -            /* NOTE: New connections are NATted and Helped only when
> >> -             * committed, so we are not calling into NAT here.
> >> -             */
> >> -            state = OVS_CS_F_TRACKED | OVS_CS_F_NEW | OVS_CS_F_RELATED;
> >> -            __ovs_ct_update_key(key, state, &info->zone, exp->master);
> >
> > Hi, Xin, others.
> >
> > Unfortunately, it seems like removal of this code broke the expected behavior.
> > OVS in userspace expects that SYN packet of a new related FTP connection will
> > get +new+rel+trk flags, but after this patch we're only getting +rel+trk and not
> > new.  This is a problem because we need to commit this connection with the label
> > and we do that for +new packets.  If we can't get +new packet we'll have to commit
> > every single +rel+trk packet, which doesn't make a lot of sense.  And it's a
> > significant behavior change regardless.
>
> Interestingly enough I see +new+rel+trk packets in cases without SNAT,
> but we can only get +rel+trk in cases with SNAT.  So, this may be just
> a generic conntrack bug somewhere.  At least the behavior seems fairly
> inconsistent.
>
In nf_conntrack, IP_CT_RELATED and IP_CT_NEW do not exist at the same
time. With this patch, we expect OVS_CS_F_RELATED and OVS_CS_F_NEW
are set at the same time by ovs_ct_update_key() when this related ct
is not confirmed.

The check-kernel test of "FTP SNAT orig tuple" skiped on my env somehow:

# make check-kernel
144: conntrack - FTP SNAT orig tuple   skipped (system-traffic.at:7295)

Any idea why? or do you know any other testcase that expects +new+rel+trk
but returns +rel+trk only?

Thanks.
> >
> > Could you, please, take a look?
> >
> > The issue can be reproduced by running check-kernel tests in OVS repo.
> > 'FTP SNAT orig tuple' tests fail 100% of the time.
> >
> > Best regards, Ilya Maximets.
>