netdev - Re: Multi-thread udp 4.7 regression, bisected to 71d8c47fc653

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAB9dFds=qY=Dk++p7qVX7a8aOOH4wn0rtL3m4poO6HMQPuPrnA@mail.gmail.com>
Date:	Mon, 27 Jun 2016 11:46:02 -0300
From:	Marc Dionne <marc.c.dionne@...il.com>
To:	Florian Westphal <fw@...len.de>
Cc:	Pablo Neira Ayuso <pablo@...filter.org>,
	netdev <netdev@...r.kernel.org>
Subject: Re: Multi-thread udp 4.7 regression, bisected to 71d8c47fc653

On Mon, Jun 27, 2016 at 11:22 AM, Florian Westphal <fw@...len.de> wrote:
> Marc Dionne <marc.c.dionne@...il.com> wrote:
>> Hi,
>>
>> I've been seeing issues with 4.7-rc kernels with some of our
>> multi-thread test cases.  I've bisected it down to this commit:
>>
>>   commit 71d8c47fc653711c41bc3282e5b0e605b3727956
>>   Author: Pablo Neira Ayuso <pablo@...filter.org>
>>   Date:   Sun May 1 00:28:40 2016 +0200
>>
>>       netfilter: conntrack: introduce clash resolution on insertion race
>>
>>
>> .. and verified that reverting the commit restores the expected behaviour.
>
> Does this change help?
>
> Subject: Restrict clash resolution to original direction
>
> diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
> --- a/net/netfilter/nf_conntrack_core.c
> +++ b/net/netfilter/nf_conntrack_core.c
> @@ -721,13 +721,18 @@ __nf_conntrack_confirm(struct sk_buff *skb)
>            not in the hash.  If there is, we lost race. */
>         hlist_nulls_for_each_entry(h, n, &nf_conntrack_hash[hash], hnnode)
>                 if (nf_ct_key_equal(h, &ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple,
> -                                   zone, net))
> -                       goto out;
> +                                   zone, net)) {
> +                       nf_ct_add_to_dying_list(ct);
> +                       ret = nf_ct_resolve_clash(net, skb, ctinfo, h);
> +                       goto dying;
> +               }
>
>         hlist_nulls_for_each_entry(h, n, &nf_conntrack_hash[reply_hash], hnnode)
>                 if (nf_ct_key_equal(h, &ct->tuplehash[IP_CT_DIR_REPLY].tuple,
> -                                   zone, net))
> -                       goto out;
> +                                   zone, net)) {
> +                       nf_ct_add_to_dying_list(ct);
> +                       goto dying;
> +               }
>
>         /* Timer relative to confirmation time, not original
>            setting time, otherwise we'd get timer wrap in
> @@ -763,9 +768,6 @@ __nf_conntrack_confirm(struct sk_buff *skb)
>                                  IPCT_RELATED : IPCT_NEW, ct);
>         return NF_ACCEPT;
>
> -out:
> -       nf_ct_add_to_dying_list(ct);
> -       ret = nf_ct_resolve_clash(net, skb, ctinfo, h);
>  dying:
>         nf_conntrack_double_unlock(hash, reply_hash);
>         NF_CT_STAT_INC(net, insert_failed);

No, that doesn't seem to make any noticeable difference.

Marc