| 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
| ||
|
Date: Tue, 18 Jun 2024 10:58:54 -0400
From: Xin Long <lucien.xin@...il.com>
To: Ilya Maximets <i.maximets@....org>
Cc: network dev <netdev@...r.kernel.org>, dev@...nvswitch.org,
Marcelo Ricardo Leitner <marcelo.leitner@...il.com>, Jiri Pirko <jiri@...nulli.us>,
Davide Caratti <dcaratti@...hat.com>, Florian Westphal <fw@...len.de>, Jamal Hadi Salim <jhs@...atatu.com>,
Eric Dumazet <edumazet@...gle.com>, Cong Wang <xiyou.wangcong@...il.com>, kuba@...nel.org,
Paolo Abeni <pabeni@...hat.com>, davem@...emloft.net,
Pablo Neira Ayuso <pablo@...filter.org>, Aaron Conole <aconole@...hat.com>
Subject: Re: [PATCH net-next 3/3] openvswitch: set IPS_CONFIRMED in tmpl
status only when commit is set in conntrack
On Tue, Jun 18, 2024 at 7:34 AM Ilya Maximets <i.maximets@....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@...il.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.
>
Powered by blists - more mailing lists