linux-kernel - Re: [PATCH] SUNRPC: Use atomic(64)_t for seq

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: <65bc05f9685faf5a29f47805e4675f229f32531c.camel@hammerspace.com>
Date:   Thu, 1 Nov 2018 17:57:43 +0000
From:   Trond Myklebust <trondmy@...merspace.com>
To:     "anna.schumaker@...app.com" <anna.schumaker@...app.com>,
        "linux-nfs@...r.kernel.org" <linux-nfs@...r.kernel.org>,
        "paul.burton@...s.com" <paul.burton@...s.com>
CC:     "bfields@...ldses.org" <bfields@...ldses.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "pburton@...ecomp.com" <pburton@...ecomp.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "jlayton@...nel.org" <jlayton@...nel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH] SUNRPC: Use atomic(64)_t for seq_send(64)

On Thu, 2018-11-01 at 17:51 +0000, Paul Burton wrote:
> The seq_send & seq_send64 fields in struct krb5_ctx are used as
> atomically incrementing counters. This is implemented using cmpxchg()
> &
> cmpxchg64() to implement what amount to custom versions of
> atomic_fetch_inc() & atomic64_fetch_inc().
> 
> Besides the duplication, using cmpxchg64() has another major drawback
> in
> that some 32 bit architectures don't provide it. As such commit
> 571ed1fd2390 ("SUNRPC: Replace krb5_seq_lock with a lockless scheme")
> resulted in build failures for some architectures.
> 
> Change seq_send to be an atomic_t and seq_send64 to be an atomic64_t,
> then use atomic(64)_* functions to manipulate the values. The
> atomic64_t
> type & associated functions are provided even on architectures which
> lack real 64 bit atomic memory access via CONFIG_GENERIC_ATOMIC64
> which
> uses spinlocks to serialize access. This fixes the build failures for
> architectures lacking cmpxchg64().
> 
> A potential alternative that was raised would be to provide
> cmpxchg64()
> on the 32 bit architectures that currently lack it, using spinlocks.
> However this would provide a version of cmpxchg64() with semantics a
> little different to the implementations on architectures with real 64
> bit atomics - the spinlock-based implementation would only work if
> all
> access to the memory used with cmpxchg64() is *always* performed
> using
> cmpxchg64(). That is not currently a requirement for users of
> cmpxchg64(), and making it one seems questionable. As such avoiding
> cmpxchg64() outside of architecture-specific code seems best,
> particularly in cases where atomic64_t seems like a better fit
> anyway.
> 
> The CONFIG_GENERIC_ATOMIC64 implementation of atomic64_* functions
> will
> use spinlocks & so faces the same issue, but with the key difference
> that the memory backing an atomic64_t ought to always be accessed via
> the atomic64_* functions anyway making the issue moot.
> 
> Signed-off-by: Paul Burton <paul.burton@...s.com>
> Fixes: 571ed1fd2390 ("SUNRPC: Replace krb5_seq_lock with a lockless
> scheme")
> Cc: Trond Myklebust <trond.myklebust@...merspace.com>
> Cc: Anna Schumaker <anna.schumaker@...app.com>
> Cc: J. Bruce Fields <bfields@...ldses.org>
> Cc: Jeff Layton <jlayton@...nel.org>
> Cc: David S. Miller <davem@...emloft.net>
> Cc: linux-nfs@...r.kernel.org
> Cc: netdev@...r.kernel.org
> ---
>  include/linux/sunrpc/gss_krb5.h     |  7 ++-----
>  net/sunrpc/auth_gss/gss_krb5_mech.c | 16 ++++++++++------
>  net/sunrpc/auth_gss/gss_krb5_seal.c | 28 ++-------------------------
> -
>  net/sunrpc/auth_gss/gss_krb5_wrap.c |  4 ++--
>  4 files changed, 16 insertions(+), 39 deletions(-)
> 

Thanks Paul! ...and thanks for your patience in working out the
atomicity wraparound issues. Applied..

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@...merspace.com