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....@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.
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.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
