| 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 17:50:23 +0200
From: Ilya Maximets <i.maximets@....org>
To: Xin Long <lucien.xin@...il.com>
Cc: i.maximets@....org, 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 6/18/24 16:58, Xin Long wrote:
> 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?
You need to install lftp and pyftpdlib. The pyftpdlib may only be available
via pip on some systems.
>
> 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