linux-kernel - Re: [PATCH 5/5] net/netfilter/nf_conntrack

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 for Android: free password hash cracker in your pocket

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

Date:   Wed, 31 Aug 2016 07:15:24 -0700
From:   Eric Dumazet <eric.dumazet@...il.com>
To:     Manfred Spraul <manfred@...orfullife.com>
Cc:     benh@...nel.crashing.org, paulmck@...ux.vnet.ibm.com,
        Ingo Molnar <mingo@...e.hu>, Boqun Feng <boqun.feng@...il.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        LKML <linux-kernel@...r.kernel.org>, 1vier1@....de,
        Davidlohr Bueso <dave@...olabs.net>,
        Pablo Neira Ayuso <pablo@...filter.org>,
        netfilter-devel@...r.kernel.org
Subject: Re: [PATCH 5/5] net/netfilter/nf_conntrack_core: update memory
 barriers.

On Wed, 2016-08-31 at 15:42 +0200, Manfred Spraul wrote:
> As explained in commit 51d7d5205d33
> ("powerpc: Add smp_mb() to arch_spin_is_locked()", for some architectures
> the ACQUIRE during spin_lock only applies to loading the lock, not to
> storing the lock state.
> 
> nf_conntrack_lock() does not handle this correctly:
>     /* 1) Acquire the lock */
>     spin_lock(lock);
>     while (unlikely(nf_conntrack_locks_all)) {
>         spin_unlock(lock);
> 
> spinlock_store_acquire() is missing between spin_lock and reading
> nf_conntrack_locks_all. In addition, reading nf_conntrack_locks_all
> needs ACQUIRE memory ordering.
> 
> 2nd, minor issue: If there would be many nf_conntrack_all_lock() callers,
> then nf_conntrack_lock() would loop forever.
> 
> Therefore: Change nf_conntrack_lock and nf_conntract_lock_all() to the
> approach used by ipc/sem.c:
> 
> - add spinlock_store_acquire()
> - add smp_load_acquire()
> - for nf_conntrack_lock, use spin_lock(&global_lock) instead of
>   spin_unlock_wait(&global_lock) and loop backward.
> - use smp_store_mb() instead of a raw smp_mb()
> 
> Signed-off-by: Manfred Spraul <manfred@...orfullife.com>
> Cc: Pablo Neira Ayuso <pablo@...filter.org>
> Cc: netfilter-devel@...r.kernel.org
> 
> ---
> 
> Question: Should I split this patch?
> First a patch that uses smp_mb(), with Cc: stable.
> The replace the smp_mb() with spinlock_store_acquire, not for stable

I guess it all depends on stable backports you believe are needed.

You probably should add the tags :
Fixes: <12-digit-sha1> "patch title"
that introduced the bug(s) you fix.

By doing this archaeological research you will likely have a better
answer ?

Thanks !