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 <[email protected]> >> --- >> 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. > > 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 [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
