Re: [RFC PATCH] lib: Introduce generic __cmpxchg_u64() and use it where needed

Re: [RFC PATCH] lib: Introduce generic __cmpxchg_u64() and use it where needed

[Paul Burton]

(Copying SunRPC & net maintainers.)

Hi Guenter,

On Wed, Oct 31, 2018 at 03:02:53PM -0700, Guenter Roeck wrote:
> The alternatives I can see are
> - Do not use cmpxchg64() outside architecture code (ie drop its use from
>   the offending driver, and keep doing the same whenever the problem comes
>   up again).
> or
> - Introduce something like ARCH_HAS_CMPXCHG64 and use it to determine
>   if cmpxchg64 is supported or not.
> 
> Any preference ?

My preference would be option 1 - avoiding cmpxchg64() where possible in
generic code. I wouldn't be opposed to the Kconfig option if there are
cases where cmpxchg64() can really help performance though.

The last time I'm aware of this coming up the affected driver was
modified to avoid cmpxchg64() [1].

In this particular case I have no idea why
net/sunrpc/auth_gss/gss_krb5_seal.c is using cmpxchg64() at all. It's
essentially reinventing atomic64_fetch_inc() which is already provided
everywhere via CONFIG_GENERIC_ATOMIC64 & the spinlock approach. At least
for atomic64_* functions the assumption that all access will be
performed using those same functions seems somewhat reasonable.

So how does the below look? Trond?

Thanks,
    Paul

[1] https://patchwork.ozlabs.org/cover/891284/

---
diff --git a/include/linux/sunrpc/gss_krb5.h b/include/linux/sunrpc/gss_krb5.h
index 131424cefc6a..02c0412e368c 100644
--- a/include/linux/sunrpc/gss_krb5.h
+++ b/include/linux/sunrpc/gss_krb5.h
@@ -107,8 +107,8 @@ struct krb5_ctx {
 	u8			Ksess[GSS_KRB5_MAX_KEYLEN]; /* session key */
 	u8			cksum[GSS_KRB5_MAX_KEYLEN];
 	s32			endtime;
-	u32			seq_send;
-	u64			seq_send64;
+	atomic_t		seq_send;
+	atomic64_t		seq_send64;
 	struct xdr_netobj	mech_used;
 	u8			initiator_sign[GSS_KRB5_MAX_KEYLEN];
 	u8			acceptor_sign[GSS_KRB5_MAX_KEYLEN];
@@ -118,9 +118,6 @@ struct krb5_ctx {
 	u8			acceptor_integ[GSS_KRB5_MAX_KEYLEN];
 };
 
-extern u32 gss_seq_send_fetch_and_inc(struct krb5_ctx *ctx);
-extern u64 gss_seq_send64_fetch_and_inc(struct krb5_ctx *ctx);
-
 /* The length of the Kerberos GSS token header */
 #define GSS_KRB5_TOK_HDR_LEN	(16)
 
diff --git a/net/sunrpc/auth_gss/gss_krb5_mech.c b/net/sunrpc/auth_gss/gss_krb5_mech.c
index 7f0424dfa8f6..eab71fc7af3e 100644
--- a/net/sunrpc/auth_gss/gss_krb5_mech.c
+++ b/net/sunrpc/auth_gss/gss_krb5_mech.c
@@ -274,6 +274,7 @@ get_key(const void *p, const void *end,
 static int
 gss_import_v1_context(const void *p, const void *end, struct krb5_ctx *ctx)
 {
+	u32 seq_send;
 	int tmp;
 
 	p = simple_get_bytes(p, end, &ctx->initiate, sizeof(ctx->initiate));
@@ -315,9 +316,10 @@ gss_import_v1_context(const void *p, const void *end, struct krb5_ctx *ctx)
 	p = simple_get_bytes(p, end, &ctx->endtime, sizeof(ctx->endtime));
 	if (IS_ERR(p))
 		goto out_err;
-	p = simple_get_bytes(p, end, &ctx->seq_send, sizeof(ctx->seq_send));
+	p = simple_get_bytes(p, end, &seq_send, sizeof(seq_send));
 	if (IS_ERR(p))
 		goto out_err;
+	atomic_set(&ctx->seq_send, seq_send);
 	p = simple_get_netobj(p, end, &ctx->mech_used);
 	if (IS_ERR(p))
 		goto out_err;
@@ -607,6 +609,7 @@ static int
 gss_import_v2_context(const void *p, const void *end, struct krb5_ctx *ctx,
 		gfp_t gfp_mask)
 {
+	u64 seq_send64;
 	int keylen;
 
 	p = simple_get_bytes(p, end, &ctx->flags, sizeof(ctx->flags));
@@ -617,14 +620,15 @@ gss_import_v2_context(const void *p, const void *end, struct krb5_ctx *ctx,
 	p = simple_get_bytes(p, end, &ctx->endtime, sizeof(ctx->endtime));
 	if (IS_ERR(p))
 		goto out_err;
-	p = simple_get_bytes(p, end, &ctx->seq_send64, sizeof(ctx->seq_send64));
+	p = simple_get_bytes(p, end, &seq_send64, sizeof(seq_send64));
 	if (IS_ERR(p))
 		goto out_err;
+	atomic64_set(&ctx->seq_send64, seq_send64);
 	/* set seq_send for use by "older" enctypes */
-	ctx->seq_send = ctx->seq_send64;
-	if (ctx->seq_send64 != ctx->seq_send) {
-		dprintk("%s: seq_send64 %lx, seq_send %x overflow?\n", __func__,
-			(unsigned long)ctx->seq_send64, ctx->seq_send);
+	atomic_set(&ctx->seq_send, seq_send64);
+	if (seq_send64 != atomic_read(&ctx->seq_send)) {
+		dprintk("%s: seq_send64 %llx, seq_send %x overflow?\n", __func__,
+			seq_send64, atomic_read(&ctx->seq_send));
 		p = ERR_PTR(-EINVAL);
 		goto out_err;
 	}
diff --git a/net/sunrpc/auth_gss/gss_krb5_seal.c b/net/sunrpc/auth_gss/gss_krb5_seal.c
index b4adeb06660b..48fe4a591b54 100644
--- a/net/sunrpc/auth_gss/gss_krb5_seal.c
+++ b/net/sunrpc/auth_gss/gss_krb5_seal.c
@@ -123,30 +123,6 @@ setup_token_v2(struct krb5_ctx *ctx, struct xdr_netobj *token)
 	return krb5_hdr;
 }
 
-u32
-gss_seq_send_fetch_and_inc(struct krb5_ctx *ctx)
-{
-	u32 old, seq_send = READ_ONCE(ctx->seq_send);
-
-	do {
-		old = seq_send;
-		seq_send = cmpxchg(&ctx->seq_send, old, old + 1);
-	} while (old != seq_send);
-	return seq_send;
-}
-
-u64
-gss_seq_send64_fetch_and_inc(struct krb5_ctx *ctx)
-{
-	u64 old, seq_send = READ_ONCE(ctx->seq_send);
-
-	do {
-		old = seq_send;
-		seq_send = cmpxchg64(&ctx->seq_send64, old, old + 1);
-	} while (old != seq_send);
-	return seq_send;
-}
-
 static u32
 gss_get_mic_v1(struct krb5_ctx *ctx, struct xdr_buf *text,
 		struct xdr_netobj *token)
@@ -177,7 +153,7 @@ gss_get_mic_v1(struct krb5_ctx *ctx, struct xdr_buf *text,
 
 	memcpy(ptr + GSS_KRB5_TOK_HDR_LEN, md5cksum.data, md5cksum.len);
 
-	seq_send = gss_seq_send_fetch_and_inc(ctx);
+	seq_send = atomic_fetch_inc(&ctx->seq_send);
 
 	if (krb5_make_seq_num(ctx, ctx->seq, ctx->initiate ? 0 : 0xff,
 			      seq_send, ptr + GSS_KRB5_TOK_HDR_LEN, ptr + 8))
@@ -205,7 +181,7 @@ gss_get_mic_v2(struct krb5_ctx *ctx, struct xdr_buf *text,
 
 	/* Set up the sequence number. Now 64-bits in clear
 	 * text and w/o direction indicator */
-	seq_send_be64 = cpu_to_be64(gss_seq_send64_fetch_and_inc(ctx));
+	seq_send_be64 = cpu_to_be64(atomic64_fetch_inc(&ctx->seq_send64));
 	memcpy(krb5_hdr + 8, (char *) &seq_send_be64, 8);
 
 	if (ctx->initiate) {
diff --git a/net/sunrpc/auth_gss/gss_krb5_wrap.c b/net/sunrpc/auth_gss/gss_krb5_wrap.c
index 962fa84e6db1..5cdde6cb703a 100644
--- a/net/sunrpc/auth_gss/gss_krb5_wrap.c
+++ b/net/sunrpc/auth_gss/gss_krb5_wrap.c
@@ -228,7 +228,7 @@ gss_wrap_kerberos_v1(struct krb5_ctx *kctx, int offset,
 
 	memcpy(ptr + GSS_KRB5_TOK_HDR_LEN, md5cksum.data, md5cksum.len);
 
-	seq_send = gss_seq_send_fetch_and_inc(kctx);
+	seq_send = atomic_fetch_inc(&kctx->seq_send);
 
 	/* XXX would probably be more efficient to compute checksum
 	 * and encrypt at the same time: */
@@ -475,7 +475,7 @@ gss_wrap_kerberos_v2(struct krb5_ctx *kctx, u32 offset,
 	*be16ptr++ = 0;
 
 	be64ptr = (__be64 *)be16ptr;
-	*be64ptr = cpu_to_be64(gss_seq_send64_fetch_and_inc(kctx));
+	*be64ptr = cpu_to_be64(atomic64_fetch_inc(&kctx->seq_send64));
 
 	err = (*kctx->gk5e->encrypt_v2)(kctx, offset, buf, pages);
 	if (err)

Re: [RFC PATCH] lib: Introduce generic __cmpxchg_u64() and use it where needed

[Trond Myklebust]

Hi Paul,

On Wed, 2018-10-31 at 23:32 +0000, Paul Burton wrote:
> (Copying SunRPC & net maintainers.)
> 
> Hi Guenter,
> 
> On Wed, Oct 31, 2018 at 03:02:53PM -0700, Guenter Roeck wrote:
> > The alternatives I can see are
> > - Do not use cmpxchg64() outside architecture code (ie drop its use
> > from
> >   the offending driver, and keep doing the same whenever the
> > problem comes
> >   up again).
> > or
> > - Introduce something like ARCH_HAS_CMPXCHG64 and use it to
> > determine
> >   if cmpxchg64 is supported or not.
> > 
> > Any preference ?
> 
> My preference would be option 1 - avoiding cmpxchg64() where possible
> in
> generic code. I wouldn't be opposed to the Kconfig option if there
> are
> cases where cmpxchg64() can really help performance though.
> 
> The last time I'm aware of this coming up the affected driver was
> modified to avoid cmpxchg64() [1].
> 
> In this particular case I have no idea why
> net/sunrpc/auth_gss/gss_krb5_seal.c is using cmpxchg64() at all. It's
> essentially reinventing atomic64_fetch_inc() which is already
> provided
> everywhere via CONFIG_GENERIC_ATOMIC64 & the spinlock approach. At
> least
> for atomic64_* functions the assumption that all access will be
> performed using those same functions seems somewhat reasonable.
> 
> So how does the below look? Trond?
> 

My one question (and the reason why I went with cmpxchg() in the first
place) would be about the overflow behaviour for atomic_fetch_inc() and
friends. I believe those functions should be OK on x86, so that when we
overflow the counter, it behaves like an unsigned value and wraps back
around.  Is that the case for all architectures?

i.e. are atomic_t/atomic64_t always guaranteed to behave like u32/u64
on increment?

I could not find any documentation that explicitly stated that they
should.

Cheers
  Trond

> Thanks,
>     Paul
> 
> [1] https://patchwork.ozlabs.org/cover/891284/
> 
> ---
> diff --git a/include/linux/sunrpc/gss_krb5.h
> b/include/linux/sunrpc/gss_krb5.h
> index 131424cefc6a..02c0412e368c 100644
> --- a/include/linux/sunrpc/gss_krb5.h
> +++ b/include/linux/sunrpc/gss_krb5.h
> @@ -107,8 +107,8 @@ struct krb5_ctx {
>  	u8			Ksess[GSS_KRB5_MAX_KEYLEN]; /*
> session key */
>  	u8			cksum[GSS_KRB5_MAX_KEYLEN];
>  	s32			endtime;
> -	u32			seq_send;
> -	u64			seq_send64;
> +	atomic_t		seq_send;
> +	atomic64_t		seq_send64;
>  	struct xdr_netobj	mech_used;
>  	u8			initiator_sign[GSS_KRB5_MAX_KEYLEN];
>  	u8			acceptor_sign[GSS_KRB5_MAX_KEYLEN];
> @@ -118,9 +118,6 @@ struct krb5_ctx {
>  	u8			acceptor_integ[GSS_KRB5_MAX_KEYLEN];
>  };
>  
> -extern u32 gss_seq_send_fetch_and_inc(struct krb5_ctx *ctx);
> -extern u64 gss_seq_send64_fetch_and_inc(struct krb5_ctx *ctx);
> -
>  /* The length of the Kerberos GSS token header */
>  #define GSS_KRB5_TOK_HDR_LEN	(16)
>  
> diff --git a/net/sunrpc/auth_gss/gss_krb5_mech.c
> b/net/sunrpc/auth_gss/gss_krb5_mech.c
> index 7f0424dfa8f6..eab71fc7af3e 100644
> --- a/net/sunrpc/auth_gss/gss_krb5_mech.c
> +++ b/net/sunrpc/auth_gss/gss_krb5_mech.c
> @@ -274,6 +274,7 @@ get_key(const void *p, const void *end,
>  static int
>  gss_import_v1_context(const void *p, const void *end, struct
> krb5_ctx *ctx)
>  {
> +	u32 seq_send;
>  	int tmp;
>  
>  	p = simple_get_bytes(p, end, &ctx->initiate, sizeof(ctx-
> >initiate));
> @@ -315,9 +316,10 @@ gss_import_v1_context(const void *p, const void
> *end, struct krb5_ctx *ctx)
>  	p = simple_get_bytes(p, end, &ctx->endtime, sizeof(ctx-
> >endtime));
>  	if (IS_ERR(p))
>  		goto out_err;
> -	p = simple_get_bytes(p, end, &ctx->seq_send, sizeof(ctx-
> >seq_send));
> +	p = simple_get_bytes(p, end, &seq_send, sizeof(seq_send));
>  	if (IS_ERR(p))
>  		goto out_err;
> +	atomic_set(&ctx->seq_send, seq_send);
>  	p = simple_get_netobj(p, end, &ctx->mech_used);
>  	if (IS_ERR(p))
>  		goto out_err;
> @@ -607,6 +609,7 @@ static int
>  gss_import_v2_context(const void *p, const void *end, struct
> krb5_ctx *ctx,
>  		gfp_t gfp_mask)
>  {
> +	u64 seq_send64;
>  	int keylen;
>  
>  	p = simple_get_bytes(p, end, &ctx->flags, sizeof(ctx->flags));
> @@ -617,14 +620,15 @@ gss_import_v2_context(const void *p, const void
> *end, struct krb5_ctx *ctx,
>  	p = simple_get_bytes(p, end, &ctx->endtime, sizeof(ctx-
> >endtime));
>  	if (IS_ERR(p))
>  		goto out_err;
> -	p = simple_get_bytes(p, end, &ctx->seq_send64, sizeof(ctx-
> >seq_send64));
> +	p = simple_get_bytes(p, end, &seq_send64, sizeof(seq_send64));
>  	if (IS_ERR(p))
>  		goto out_err;
> +	atomic64_set(&ctx->seq_send64, seq_send64);
>  	/* set seq_send for use by "older" enctypes */
> -	ctx->seq_send = ctx->seq_send64;
> -	if (ctx->seq_send64 != ctx->seq_send) {
> -		dprintk("%s: seq_send64 %lx, seq_send %x overflow?\n",
> __func__,
> -			(unsigned long)ctx->seq_send64, ctx->seq_send);
> +	atomic_set(&ctx->seq_send, seq_send64);
> +	if (seq_send64 != atomic_read(&ctx->seq_send)) {
> +		dprintk("%s: seq_send64 %llx, seq_send %x overflow?\n",
> __func__,
> +			seq_send64, atomic_read(&ctx->seq_send));
>  		p = ERR_PTR(-EINVAL);
>  		goto out_err;
>  	}
> diff --git a/net/sunrpc/auth_gss/gss_krb5_seal.c
> b/net/sunrpc/auth_gss/gss_krb5_seal.c
> index b4adeb06660b..48fe4a591b54 100644
> --- a/net/sunrpc/auth_gss/gss_krb5_seal.c
> +++ b/net/sunrpc/auth_gss/gss_krb5_seal.c
> @@ -123,30 +123,6 @@ setup_token_v2(struct krb5_ctx *ctx, struct
> xdr_netobj *token)
>  	return krb5_hdr;
>  }
>  
> -u32
> -gss_seq_send_fetch_and_inc(struct krb5_ctx *ctx)
> -{
> -	u32 old, seq_send = READ_ONCE(ctx->seq_send);
> -
> -	do {
> -		old = seq_send;
> -		seq_send = cmpxchg(&ctx->seq_send, old, old + 1);
> -	} while (old != seq_send);
> -	return seq_send;
> -}
> -
> -u64
> -gss_seq_send64_fetch_and_inc(struct krb5_ctx *ctx)
> -{
> -	u64 old, seq_send = READ_ONCE(ctx->seq_send);
> -
> -	do {
> -		old = seq_send;
> -		seq_send = cmpxchg64(&ctx->seq_send64, old, old + 1);
> -	} while (old != seq_send);
> -	return seq_send;
> -}
> -
>  static u32
>  gss_get_mic_v1(struct krb5_ctx *ctx, struct xdr_buf *text,
>  		struct xdr_netobj *token)
> @@ -177,7 +153,7 @@ gss_get_mic_v1(struct krb5_ctx *ctx, struct
> xdr_buf *text,
>  
>  	memcpy(ptr + GSS_KRB5_TOK_HDR_LEN, md5cksum.data,
> md5cksum.len);
>  
> -	seq_send = gss_seq_send_fetch_and_inc(ctx);
> +	seq_send = atomic_fetch_inc(&ctx->seq_send);
>  
>  	if (krb5_make_seq_num(ctx, ctx->seq, ctx->initiate ? 0 : 0xff,
>  			      seq_send, ptr + GSS_KRB5_TOK_HDR_LEN, ptr
> + 8))
> @@ -205,7 +181,7 @@ gss_get_mic_v2(struct krb5_ctx *ctx, struct
> xdr_buf *text,
>  
>  	/* Set up the sequence number. Now 64-bits in clear
>  	 * text and w/o direction indicator */
> -	seq_send_be64 = cpu_to_be64(gss_seq_send64_fetch_and_inc(ctx));
> +	seq_send_be64 = cpu_to_be64(atomic64_fetch_inc(&ctx-
> >seq_send64));
>  	memcpy(krb5_hdr + 8, (char *) &seq_send_be64, 8);
>  
>  	if (ctx->initiate) {
> diff --git a/net/sunrpc/auth_gss/gss_krb5_wrap.c
> b/net/sunrpc/auth_gss/gss_krb5_wrap.c
> index 962fa84e6db1..5cdde6cb703a 100644
> --- a/net/sunrpc/auth_gss/gss_krb5_wrap.c
> +++ b/net/sunrpc/auth_gss/gss_krb5_wrap.c
> @@ -228,7 +228,7 @@ gss_wrap_kerberos_v1(struct krb5_ctx *kctx, int
> offset,
>  
>  	memcpy(ptr + GSS_KRB5_TOK_HDR_LEN, md5cksum.data,
> md5cksum.len);
>  
> -	seq_send = gss_seq_send_fetch_and_inc(kctx);
> +	seq_send = atomic_fetch_inc(&kctx->seq_send);
>  
>  	/* XXX would probably be more efficient to compute checksum
>  	 * and encrypt at the same time: */
> @@ -475,7 +475,7 @@ gss_wrap_kerberos_v2(struct krb5_ctx *kctx, u32
> offset,
>  	*be16ptr++ = 0;
>  
>  	be64ptr = (__be64 *)be16ptr;
> -	*be64ptr = cpu_to_be64(gss_seq_send64_fetch_and_inc(kctx));
> +	*be64ptr = cpu_to_be64(atomic64_fetch_inc(&kctx->seq_send64));
>  
>  	err = (*kctx->gk5e->encrypt_v2)(kctx, offset, buf, pages);
>  	if (err)
-- 
Trond Myklebust
CTO, Hammerspace Inc
4300 El Camino Real, Suite 105
Los Altos, CA 94022
www.hammer.space

Re: [RFC PATCH] lib: Introduce generic __cmpxchg_u64() and use it where needed

[Mark Rutland]

[adding the atomic maintainers]

On Thu, Nov 01, 2018 at 12:17:31AM +0000, Trond Myklebust wrote:
> On Wed, 2018-10-31 at 23:32 +0000, Paul Burton wrote:
> > (Copying SunRPC & net maintainers.)
> > 
> > Hi Guenter,
> > 
> > On Wed, Oct 31, 2018 at 03:02:53PM -0700, Guenter Roeck wrote:
> > > The alternatives I can see are
> > > - Do not use cmpxchg64() outside architecture code (ie drop its use
> > > from the offending driver, and keep doing the same whenever the
> > > problem comes up again).
> > > or
> > > - Introduce something like ARCH_HAS_CMPXCHG64 and use it to
> > > determine if cmpxchg64 is supported or not.
> > > 
> > > Any preference ?
> > 
> > My preference would be option 1 - avoiding cmpxchg64() where
> > possible in generic code. I wouldn't be opposed to the Kconfig
> > option if there are cases where cmpxchg64() can really help
> > performance though.
> > 
> > The last time I'm aware of this coming up the affected driver was
> > modified to avoid cmpxchg64() [1].
> > 
> > In this particular case I have no idea why
> > net/sunrpc/auth_gss/gss_krb5_seal.c is using cmpxchg64() at all.
> > It's essentially reinventing atomic64_fetch_inc() which is already
> > provided everywhere via CONFIG_GENERIC_ATOMIC64 & the spinlock
> > approach. At least for atomic64_* functions the assumption that all
> > access will be performed using those same functions seems somewhat
> > reasonable.
> > 
> > So how does the below look? Trond?
> 
> My one question (and the reason why I went with cmpxchg() in the first
> place) would be about the overflow behaviour for atomic_fetch_inc() and
> friends. I believe those functions should be OK on x86, so that when we
> overflow the counter, it behaves like an unsigned value and wraps back
> around.  Is that the case for all architectures?
> 
> i.e. are atomic_t/atomic64_t always guaranteed to behave like u32/u64
> on increment?
> 
> I could not find any documentation that explicitly stated that they
> should.

Peter, Will, I understand that the atomic_t/atomic64_t ops are required
to wrap per 2's-complement. IIUC the refcount code relies on this.

Can you confirm?

Thanks,
Mark.

> Cheers
>   Trond
> 
> > Thanks,
> >     Paul
> > 
> > [1] https://patchwork.ozlabs.org/cover/891284/
> > 
> > ---
> > diff --git a/include/linux/sunrpc/gss_krb5.h
> > b/include/linux/sunrpc/gss_krb5.h
> > index 131424cefc6a..02c0412e368c 100644
> > --- a/include/linux/sunrpc/gss_krb5.h
> > +++ b/include/linux/sunrpc/gss_krb5.h
> > @@ -107,8 +107,8 @@ struct krb5_ctx {
> >  	u8			Ksess[GSS_KRB5_MAX_KEYLEN]; /*
> > session key */
> >  	u8			cksum[GSS_KRB5_MAX_KEYLEN];
> >  	s32			endtime;
> > -	u32			seq_send;
> > -	u64			seq_send64;
> > +	atomic_t		seq_send;
> > +	atomic64_t		seq_send64;
> >  	struct xdr_netobj	mech_used;
> >  	u8			initiator_sign[GSS_KRB5_MAX_KEYLEN];
> >  	u8			acceptor_sign[GSS_KRB5_MAX_KEYLEN];
> > @@ -118,9 +118,6 @@ struct krb5_ctx {
> >  	u8			acceptor_integ[GSS_KRB5_MAX_KEYLEN];
> >  };
> >  
> > -extern u32 gss_seq_send_fetch_and_inc(struct krb5_ctx *ctx);
> > -extern u64 gss_seq_send64_fetch_and_inc(struct krb5_ctx *ctx);
> > -
> >  /* The length of the Kerberos GSS token header */
> >  #define GSS_KRB5_TOK_HDR_LEN	(16)
> >  
> > diff --git a/net/sunrpc/auth_gss/gss_krb5_mech.c
> > b/net/sunrpc/auth_gss/gss_krb5_mech.c
> > index 7f0424dfa8f6..eab71fc7af3e 100644
> > --- a/net/sunrpc/auth_gss/gss_krb5_mech.c
> > +++ b/net/sunrpc/auth_gss/gss_krb5_mech.c
> > @@ -274,6 +274,7 @@ get_key(const void *p, const void *end,
> >  static int
> >  gss_import_v1_context(const void *p, const void *end, struct
> > krb5_ctx *ctx)
> >  {
> > +	u32 seq_send;
> >  	int tmp;
> >  
> >  	p = simple_get_bytes(p, end, &ctx->initiate, sizeof(ctx-
> > >initiate));
> > @@ -315,9 +316,10 @@ gss_import_v1_context(const void *p, const void
> > *end, struct krb5_ctx *ctx)
> >  	p = simple_get_bytes(p, end, &ctx->endtime, sizeof(ctx-
> > >endtime));
> >  	if (IS_ERR(p))
> >  		goto out_err;
> > -	p = simple_get_bytes(p, end, &ctx->seq_send, sizeof(ctx-
> > >seq_send));
> > +	p = simple_get_bytes(p, end, &seq_send, sizeof(seq_send));
> >  	if (IS_ERR(p))
> >  		goto out_err;
> > +	atomic_set(&ctx->seq_send, seq_send);
> >  	p = simple_get_netobj(p, end, &ctx->mech_used);
> >  	if (IS_ERR(p))
> >  		goto out_err;
> > @@ -607,6 +609,7 @@ static int
> >  gss_import_v2_context(const void *p, const void *end, struct
> > krb5_ctx *ctx,
> >  		gfp_t gfp_mask)
> >  {
> > +	u64 seq_send64;
> >  	int keylen;
> >  
> >  	p = simple_get_bytes(p, end, &ctx->flags, sizeof(ctx->flags));
> > @@ -617,14 +620,15 @@ gss_import_v2_context(const void *p, const void
> > *end, struct krb5_ctx *ctx,
> >  	p = simple_get_bytes(p, end, &ctx->endtime, sizeof(ctx-
> > >endtime));
> >  	if (IS_ERR(p))
> >  		goto out_err;
> > -	p = simple_get_bytes(p, end, &ctx->seq_send64, sizeof(ctx-
> > >seq_send64));
> > +	p = simple_get_bytes(p, end, &seq_send64, sizeof(seq_send64));
> >  	if (IS_ERR(p))
> >  		goto out_err;
> > +	atomic64_set(&ctx->seq_send64, seq_send64);
> >  	/* set seq_send for use by "older" enctypes */
> > -	ctx->seq_send = ctx->seq_send64;
> > -	if (ctx->seq_send64 != ctx->seq_send) {
> > -		dprintk("%s: seq_send64 %lx, seq_send %x overflow?\n",
> > __func__,
> > -			(unsigned long)ctx->seq_send64, ctx->seq_send);
> > +	atomic_set(&ctx->seq_send, seq_send64);
> > +	if (seq_send64 != atomic_read(&ctx->seq_send)) {
> > +		dprintk("%s: seq_send64 %llx, seq_send %x overflow?\n",
> > __func__,
> > +			seq_send64, atomic_read(&ctx->seq_send));
> >  		p = ERR_PTR(-EINVAL);
> >  		goto out_err;
> >  	}
> > diff --git a/net/sunrpc/auth_gss/gss_krb5_seal.c
> > b/net/sunrpc/auth_gss/gss_krb5_seal.c
> > index b4adeb06660b..48fe4a591b54 100644
> > --- a/net/sunrpc/auth_gss/gss_krb5_seal.c
> > +++ b/net/sunrpc/auth_gss/gss_krb5_seal.c
> > @@ -123,30 +123,6 @@ setup_token_v2(struct krb5_ctx *ctx, struct
> > xdr_netobj *token)
> >  	return krb5_hdr;
> >  }
> >  
> > -u32
> > -gss_seq_send_fetch_and_inc(struct krb5_ctx *ctx)
> > -{
> > -	u32 old, seq_send = READ_ONCE(ctx->seq_send);
> > -
> > -	do {
> > -		old = seq_send;
> > -		seq_send = cmpxchg(&ctx->seq_send, old, old + 1);
> > -	} while (old != seq_send);
> > -	return seq_send;
> > -}
> > -
> > -u64
> > -gss_seq_send64_fetch_and_inc(struct krb5_ctx *ctx)
> > -{
> > -	u64 old, seq_send = READ_ONCE(ctx->seq_send);
> > -
> > -	do {
> > -		old = seq_send;
> > -		seq_send = cmpxchg64(&ctx->seq_send64, old, old + 1);
> > -	} while (old != seq_send);
> > -	return seq_send;
> > -}
> > -
> >  static u32
> >  gss_get_mic_v1(struct krb5_ctx *ctx, struct xdr_buf *text,
> >  		struct xdr_netobj *token)
> > @@ -177,7 +153,7 @@ gss_get_mic_v1(struct krb5_ctx *ctx, struct
> > xdr_buf *text,
> >  
> >  	memcpy(ptr + GSS_KRB5_TOK_HDR_LEN, md5cksum.data,
> > md5cksum.len);
> >  
> > -	seq_send = gss_seq_send_fetch_and_inc(ctx);
> > +	seq_send = atomic_fetch_inc(&ctx->seq_send);
> >  
> >  	if (krb5_make_seq_num(ctx, ctx->seq, ctx->initiate ? 0 : 0xff,
> >  			      seq_send, ptr + GSS_KRB5_TOK_HDR_LEN, ptr
> > + 8))
> > @@ -205,7 +181,7 @@ gss_get_mic_v2(struct krb5_ctx *ctx, struct
> > xdr_buf *text,
> >  
> >  	/* Set up the sequence number. Now 64-bits in clear
> >  	 * text and w/o direction indicator */
> > -	seq_send_be64 = cpu_to_be64(gss_seq_send64_fetch_and_inc(ctx));
> > +	seq_send_be64 = cpu_to_be64(atomic64_fetch_inc(&ctx-
> > >seq_send64));
> >  	memcpy(krb5_hdr + 8, (char *) &seq_send_be64, 8);
> >  
> >  	if (ctx->initiate) {
> > diff --git a/net/sunrpc/auth_gss/gss_krb5_wrap.c
> > b/net/sunrpc/auth_gss/gss_krb5_wrap.c
> > index 962fa84e6db1..5cdde6cb703a 100644
> > --- a/net/sunrpc/auth_gss/gss_krb5_wrap.c
> > +++ b/net/sunrpc/auth_gss/gss_krb5_wrap.c
> > @@ -228,7 +228,7 @@ gss_wrap_kerberos_v1(struct krb5_ctx *kctx, int
> > offset,
> >  
> >  	memcpy(ptr + GSS_KRB5_TOK_HDR_LEN, md5cksum.data,
> > md5cksum.len);
> >  
> > -	seq_send = gss_seq_send_fetch_and_inc(kctx);
> > +	seq_send = atomic_fetch_inc(&kctx->seq_send);
> >  
> >  	/* XXX would probably be more efficient to compute checksum
> >  	 * and encrypt at the same time: */
> > @@ -475,7 +475,7 @@ gss_wrap_kerberos_v2(struct krb5_ctx *kctx, u32
> > offset,
> >  	*be16ptr++ = 0;
> >  
> >  	be64ptr = (__be64 *)be16ptr;
> > -	*be64ptr = cpu_to_be64(gss_seq_send64_fetch_and_inc(kctx));
> > +	*be64ptr = cpu_to_be64(atomic64_fetch_inc(&kctx->seq_send64));
> >  
> >  	err = (*kctx->gk5e->encrypt_v2)(kctx, offset, buf, pages);
> >  	if (err)
> -- 
> Trond Myklebust
> CTO, Hammerspace Inc
> 4300 El Camino Real, Suite 105
> Los Altos, CA 94022
> www.hammer.space
> 
> 

Re: [RFC PATCH] lib: Introduce generic __cmpxchg_u64() and use it where needed

[Peter Zijlstra]

On Thu, Nov 01, 2018 at 01:18:46PM +0000, Mark Rutland wrote:
> > My one question (and the reason why I went with cmpxchg() in the first
> > place) would be about the overflow behaviour for atomic_fetch_inc() and
> > friends. I believe those functions should be OK on x86, so that when we
> > overflow the counter, it behaves like an unsigned value and wraps back
> > around.  Is that the case for all architectures?
> > 
> > i.e. are atomic_t/atomic64_t always guaranteed to behave like u32/u64
> > on increment?
> > 
> > I could not find any documentation that explicitly stated that they
> > should.
> 
> Peter, Will, I understand that the atomic_t/atomic64_t ops are required
> to wrap per 2's-complement. IIUC the refcount code relies on this.
> 
> Can you confirm?

There is quite a bit of core code that hard assumes 2s-complement. Not
only for atomics but for any signed integer type. Also see the kernel
using -fno-strict-overflow which implies -fwrapv, which defines signed
overflow to behave like 2s-complement (and rids us of that particular
UB).

Re: [RFC PATCH] lib: Introduce generic __cmpxchg_u64() and use it where needed

[Trond Myklebust]

On Thu, 2018-11-01 at 15:59 +0100, Peter Zijlstra wrote:
> On Thu, Nov 01, 2018 at 01:18:46PM +0000, Mark Rutland wrote:
> > > My one question (and the reason why I went with cmpxchg() in the
> > > first
> > > place) would be about the overflow behaviour for
> > > atomic_fetch_inc() and
> > > friends. I believe those functions should be OK on x86, so that
> > > when we
> > > overflow the counter, it behaves like an unsigned value and wraps
> > > back
> > > around.  Is that the case for all architectures?
> > > 
> > > i.e. are atomic_t/atomic64_t always guaranteed to behave like
> > > u32/u64
> > > on increment?
> > > 
> > > I could not find any documentation that explicitly stated that
> > > they
> > > should.
> > 
> > Peter, Will, I understand that the atomic_t/atomic64_t ops are
> > required
> > to wrap per 2's-complement. IIUC the refcount code relies on this.
> > 
> > Can you confirm?
> 
> There is quite a bit of core code that hard assumes 2s-complement.
> Not
> only for atomics but for any signed integer type. Also see the kernel
> using -fno-strict-overflow which implies -fwrapv, which defines
> signed
> overflow to behave like 2s-complement (and rids us of that particular
> UB).

Fair enough, but there have also been bugfixes to explicitly fix unsafe
C standards assumptions for signed integers. See, for instance commit
5a581b367b5d "jiffies: Avoid undefined behavior from signed overflow"
from Paul McKenney.

Anyhow, if the atomic maintainers are willing to stand up and state for
the record that the atomic counters are guaranteed to wrap modulo 2^n
just like unsigned integers, then I'm happy to take Paul's patch.

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

Re: [RFC PATCH] lib: Introduce generic __cmpxchg_u64() and use it where needed

[Peter Zijlstra]

On Thu, Nov 01, 2018 at 03:22:15PM +0000, Trond Myklebust wrote:
> On Thu, 2018-11-01 at 15:59 +0100, Peter Zijlstra wrote:
> > On Thu, Nov 01, 2018 at 01:18:46PM +0000, Mark Rutland wrote:

> > > > My one question (and the reason why I went with cmpxchg() in the
> > > > first place) would be about the overflow behaviour for
> > > > atomic_fetch_inc() and friends. I believe those functions should
> > > > be OK on x86, so that when we overflow the counter, it behaves
> > > > like an unsigned value and wraps back around.  Is that the case
> > > > for all architectures?
> > > > 
> > > > i.e. are atomic_t/atomic64_t always guaranteed to behave like
> > > > u32/u64 on increment?
> > > > 
> > > > I could not find any documentation that explicitly stated that
> > > > they should.
> > > 
> > > Peter, Will, I understand that the atomic_t/atomic64_t ops are
> > > required to wrap per 2's-complement. IIUC the refcount code relies
> > > on this.
> > > 
> > > Can you confirm?
> > 
> > There is quite a bit of core code that hard assumes 2s-complement.
> > Not only for atomics but for any signed integer type. Also see the
> > kernel using -fno-strict-overflow which implies -fwrapv, which
> > defines signed overflow to behave like 2s-complement (and rids us of
> > that particular UB).
> 
> Fair enough, but there have also been bugfixes to explicitly fix unsafe
> C standards assumptions for signed integers. See, for instance commit
> 5a581b367b5d "jiffies: Avoid undefined behavior from signed overflow"
> from Paul McKenney.

Yes, I feel Paul has been to too many C/C++ committee meetings and got
properly paranoid. Which isn't always a bad thing :-)

But for us using -fno-strict-overflow which actually defines signed
overflow, I myself am really not worried. I'm also not sure if KASAN has
been taught about this, or if it will still (incorrectly) warn about UB
for signed types.

> Anyhow, if the atomic maintainers are willing to stand up and state for
> the record that the atomic counters are guaranteed to wrap modulo 2^n
> just like unsigned integers, then I'm happy to take Paul's patch.

I myself am certainly relying on it.

Re: [RFC PATCH] lib: Introduce generic __cmpxchg_u64() and use it where needed

[Eric Dumazet]

On 11/01/2018 09:32 AM, Peter Zijlstra wrote:

>> Anyhow, if the atomic maintainers are willing to stand up and state for
>> the record that the atomic counters are guaranteed to wrap modulo 2^n
>> just like unsigned integers, then I'm happy to take Paul's patch.
> 
> I myself am certainly relying on it.


Could we get uatomic_t support maybe ?

This reminds me of this sooooo silly patch :/

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=adb03115f4590baa280ddc440a8eff08a6be0cb7

Re: [RFC PATCH] lib: Introduce generic __cmpxchg_u64() and use it where needed

[Peter Zijlstra]

On Thu, Nov 01, 2018 at 09:59:38AM -0700, Eric Dumazet wrote:
> On 11/01/2018 09:32 AM, Peter Zijlstra wrote:
> 
> >> Anyhow, if the atomic maintainers are willing to stand up and state for
> >> the record that the atomic counters are guaranteed to wrap modulo 2^n
> >> just like unsigned integers, then I'm happy to take Paul's patch.
> > 
> > I myself am certainly relying on it.
> 
> Could we get uatomic_t support maybe ?

Whatever for; it'd be the exact identical same functions as for
atomic_t, except for a giant amount of code duplication to deal with the
new type.

That is; today we merged a bunch of scripts that generates most of
atomic*_t, so we could probably script uatomic*_t wrappers with minimal
effort, but it would add several thousand lines of code to each compile
for absolutely no reason what so ever.

> This reminds me of this sooooo silly patch :/
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=adb03115f4590baa280ddc440a8eff08a6be0cb7

Yes, that's stupid. UBSAN is just wrong there.

Re: [RFC PATCH] lib: Introduce generic __cmpxchg_u64() and use it where needed

[Peter Zijlstra]

On Thu, Nov 01, 2018 at 06:14:32PM +0100, Peter Zijlstra wrote:
> > This reminds me of this sooooo silly patch :/
> > 
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=adb03115f4590baa280ddc440a8eff08a6be0cb7

You'd probably want to write it like so; +- some ordering stuff, that
code didn't look like it really needs the memory barriers implied by
these, but I didn't look too hard.

diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index c0a9d26c06ce..11deb1d7e96b 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -485,16 +485,10 @@ u32 ip_idents_reserve(u32 hash, int segs)
 	u32 now = (u32)jiffies;
 	u32 new, delta = 0;
 
-	if (old != now && cmpxchg(p_tstamp, old, now) == old)
+	if (old != now && try_cmpxchg(p_tstamp, &old, now))
 		delta = prandom_u32_max(now - old);
 
-	/* Do not use atomic_add_return() as it makes UBSAN unhappy */
-	do {
-		old = (u32)atomic_read(p_id);
-		new = old + delta + segs;
-	} while (atomic_cmpxchg(p_id, old, new) != old);
-
-	return new - segs;
+	return atomic_fetch_add(segs + delta, p_id) + delta;
 }
 EXPORT_SYMBOL(ip_idents_reserve);
 

Re: [RFC PATCH] lib: Introduce generic __cmpxchg_u64() and use it where needed

[Paul E. McKenney]

On Thu, Nov 01, 2018 at 06:27:39PM +0100, Peter Zijlstra wrote:
> On Thu, Nov 01, 2018 at 06:14:32PM +0100, Peter Zijlstra wrote:
> > > This reminds me of this sooooo silly patch :/
> > > 
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=adb03115f4590baa280ddc440a8eff08a6be0cb7
> 
> You'd probably want to write it like so; +- some ordering stuff, that
> code didn't look like it really needs the memory barriers implied by
> these, but I didn't look too hard.

The atomic_fetch_add() API would need to be propagated out to the other
architectures, correct?

							Thanx, Paul

> diff --git a/net/ipv4/route.c b/net/ipv4/route.c
> index c0a9d26c06ce..11deb1d7e96b 100644
> --- a/net/ipv4/route.c
> +++ b/net/ipv4/route.c
> @@ -485,16 +485,10 @@ u32 ip_idents_reserve(u32 hash, int segs)
>  	u32 now = (u32)jiffies;
>  	u32 new, delta = 0;
> 
> -	if (old != now && cmpxchg(p_tstamp, old, now) == old)
> +	if (old != now && try_cmpxchg(p_tstamp, &old, now))
>  		delta = prandom_u32_max(now - old);
> 
> -	/* Do not use atomic_add_return() as it makes UBSAN unhappy */
> -	do {
> -		old = (u32)atomic_read(p_id);
> -		new = old + delta + segs;
> -	} while (atomic_cmpxchg(p_id, old, new) != old);
> -
> -	return new - segs;
> +	return atomic_fetch_add(segs + delta, p_id) + delta;
>  }
>  EXPORT_SYMBOL(ip_idents_reserve);
> 
> 

Re: [RFC PATCH] lib: Introduce generic __cmpxchg_u64() and use it where needed

[Peter Zijlstra]

On Thu, Nov 01, 2018 at 01:29:10PM -0700, Paul E. McKenney wrote:
> On Thu, Nov 01, 2018 at 06:27:39PM +0100, Peter Zijlstra wrote:
> > On Thu, Nov 01, 2018 at 06:14:32PM +0100, Peter Zijlstra wrote:
> > > > This reminds me of this sooooo silly patch :/
> > > > 
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=adb03115f4590baa280ddc440a8eff08a6be0cb7
> > 
> > You'd probably want to write it like so; +- some ordering stuff, that
> > code didn't look like it really needs the memory barriers implied by
> > these, but I didn't look too hard.
> 
> The atomic_fetch_add() API would need to be propagated out to the other
> architectures, correct?

Like these commits I did like 2 years ago ? :-)

$ git log --oneline 6dc25876cdb1...1f51dee7ca74
6dc25876cdb1 locking/atomic, arch/xtensa: Implement atomic_fetch_{add,sub,and,or,xor}()
a8bcccaba162 locking/atomic, arch/x86: Implement atomic{,64}_fetch_{add,sub,and,or,xor}()
1af5de9af138 locking/atomic, arch/tile: Implement atomic{,64}_fetch_{add,sub,and,or,xor}()
3a1adb23a52c locking/atomic, arch/sparc: Implement atomic{,64}_fetch_{add,sub,and,or,xor}()
7d9794e75237 locking/atomic, arch/sh: Implement atomic_fetch_{add,sub,and,or,xor}()
56fefbbc3f13 locking/atomic, arch/s390: Implement atomic{,64}_fetch_{add,sub,and,or,xor}()
a28cc7bbe8e3 locking/atomic, arch/powerpc: Implement atomic{,64}_fetch_{add,sub,and,or,xor}{,_relaxed,_acquire,_release}()
e5857a6ed600 locking/atomic, arch/parisc: Implement atomic{,64}_fetch_{add,sub,and,or,xor}()
f8d638e28d7c locking/atomic, arch/mn10300: Implement atomic_fetch_{add,sub,and,or,xor}()
4edac529eb62 locking/atomic, arch/mips: Implement atomic{,64}_fetch_{add,sub,and,or,xor}()
e898eb27ffd8 locking/atomic, arch/metag: Implement atomic_fetch_{add,sub,and,or,xor}()
e39d88ea3ce4 locking/atomic, arch/m68k: Implement atomic_fetch_{add,sub,and,or,xor}()
f64937052303 locking/atomic, arch/m32r: Implement atomic_fetch_{add,sub,and,or,xor}()
cc102507fac7 locking/atomic, arch/ia64: Implement atomic{,64}_fetch_{add,sub,and,or,xor}()
4be7dd393515 locking/atomic, arch/hexagon: Implement atomic_fetch_{add,sub,and,or,xor}()
0c074cbc3309 locking/atomic, arch/h8300: Implement atomic_fetch_{add,sub,and,or,xor}()
d9c730281617 locking/atomic, arch/frv: Implement atomic{,64}_fetch_{add,sub,and,or,xor}()
e87fc0ec0705 locking/atomic, arch/blackfin: Implement atomic_fetch_{add,sub,and,or,xor}()
1a6eafacd481 locking/atomic, arch/avr32: Implement atomic_fetch_{add,sub,and,or,xor}()
2efe95fe6952 locking/atomic, arch/arm64: Implement atomic{,64}_fetch_{add,sub,and,andnot,or,xor}{,_relaxed,_acquire,_release}() for LSE instructions
6822a84dd4e3 locking/atomic, arch/arm64: Generate LSE non-return cases using common macros
e490f9b1d3b4 locking/atomic, arch/arm64: Implement atomic{,64}_fetch_{add,sub,and,andnot,or,xor}{,_relaxed,_acquire,_release}()
6da068c1beba locking/atomic, arch/arm: Implement atomic{,64}_fetch_{add,sub,and,andnot,or,xor}{,_relaxed,_acquire,_release}()
fbffe892e525 locking/atomic, arch/arc: Implement atomic_fetch_{add,sub,and,andnot,or,xor}()

Re: [RFC PATCH] lib: Introduce generic __cmpxchg_u64() and use it where needed

[Paul E. McKenney]

On Thu, Nov 01, 2018 at 10:38:34PM +0100, Peter Zijlstra wrote:
> On Thu, Nov 01, 2018 at 01:29:10PM -0700, Paul E. McKenney wrote:
> > On Thu, Nov 01, 2018 at 06:27:39PM +0100, Peter Zijlstra wrote:
> > > On Thu, Nov 01, 2018 at 06:14:32PM +0100, Peter Zijlstra wrote:
> > > > > This reminds me of this sooooo silly patch :/
> > > > > 
> > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=adb03115f4590baa280ddc440a8eff08a6be0cb7
> > > 
> > > You'd probably want to write it like so; +- some ordering stuff, that
> > > code didn't look like it really needs the memory barriers implied by
> > > these, but I didn't look too hard.
> > 
> > The atomic_fetch_add() API would need to be propagated out to the other
> > architectures, correct?
> 
> Like these commits I did like 2 years ago ? :-)

Color me blind and stupid!  ;-)

							Thanx, Paul

> $ git log --oneline 6dc25876cdb1...1f51dee7ca74
> 6dc25876cdb1 locking/atomic, arch/xtensa: Implement atomic_fetch_{add,sub,and,or,xor}()
> a8bcccaba162 locking/atomic, arch/x86: Implement atomic{,64}_fetch_{add,sub,and,or,xor}()
> 1af5de9af138 locking/atomic, arch/tile: Implement atomic{,64}_fetch_{add,sub,and,or,xor}()
> 3a1adb23a52c locking/atomic, arch/sparc: Implement atomic{,64}_fetch_{add,sub,and,or,xor}()
> 7d9794e75237 locking/atomic, arch/sh: Implement atomic_fetch_{add,sub,and,or,xor}()
> 56fefbbc3f13 locking/atomic, arch/s390: Implement atomic{,64}_fetch_{add,sub,and,or,xor}()
> a28cc7bbe8e3 locking/atomic, arch/powerpc: Implement atomic{,64}_fetch_{add,sub,and,or,xor}{,_relaxed,_acquire,_release}()
> e5857a6ed600 locking/atomic, arch/parisc: Implement atomic{,64}_fetch_{add,sub,and,or,xor}()
> f8d638e28d7c locking/atomic, arch/mn10300: Implement atomic_fetch_{add,sub,and,or,xor}()
> 4edac529eb62 locking/atomic, arch/mips: Implement atomic{,64}_fetch_{add,sub,and,or,xor}()
> e898eb27ffd8 locking/atomic, arch/metag: Implement atomic_fetch_{add,sub,and,or,xor}()
> e39d88ea3ce4 locking/atomic, arch/m68k: Implement atomic_fetch_{add,sub,and,or,xor}()
> f64937052303 locking/atomic, arch/m32r: Implement atomic_fetch_{add,sub,and,or,xor}()
> cc102507fac7 locking/atomic, arch/ia64: Implement atomic{,64}_fetch_{add,sub,and,or,xor}()
> 4be7dd393515 locking/atomic, arch/hexagon: Implement atomic_fetch_{add,sub,and,or,xor}()
> 0c074cbc3309 locking/atomic, arch/h8300: Implement atomic_fetch_{add,sub,and,or,xor}()
> d9c730281617 locking/atomic, arch/frv: Implement atomic{,64}_fetch_{add,sub,and,or,xor}()
> e87fc0ec0705 locking/atomic, arch/blackfin: Implement atomic_fetch_{add,sub,and,or,xor}()
> 1a6eafacd481 locking/atomic, arch/avr32: Implement atomic_fetch_{add,sub,and,or,xor}()
> 2efe95fe6952 locking/atomic, arch/arm64: Implement atomic{,64}_fetch_{add,sub,and,andnot,or,xor}{,_relaxed,_acquire,_release}() for LSE instructions
> 6822a84dd4e3 locking/atomic, arch/arm64: Generate LSE non-return cases using common macros
> e490f9b1d3b4 locking/atomic, arch/arm64: Implement atomic{,64}_fetch_{add,sub,and,andnot,or,xor}{,_relaxed,_acquire,_release}()
> 6da068c1beba locking/atomic, arch/arm: Implement atomic{,64}_fetch_{add,sub,and,andnot,or,xor}{,_relaxed,_acquire,_release}()
> fbffe892e525 locking/atomic, arch/arc: Implement atomic_fetch_{add,sub,and,andnot,or,xor}()
> 

Re: [RFC PATCH] lib: Introduce generic __cmpxchg_u64() and use it where needed

[Paul E. McKenney]

On Thu, Nov 01, 2018 at 06:14:32PM +0100, Peter Zijlstra wrote:
> On Thu, Nov 01, 2018 at 09:59:38AM -0700, Eric Dumazet wrote:
> > On 11/01/2018 09:32 AM, Peter Zijlstra wrote:
> > 
> > >> Anyhow, if the atomic maintainers are willing to stand up and state for
> > >> the record that the atomic counters are guaranteed to wrap modulo 2^n
> > >> just like unsigned integers, then I'm happy to take Paul's patch.
> > > 
> > > I myself am certainly relying on it.
> > 
> > Could we get uatomic_t support maybe ?
> 
> Whatever for; it'd be the exact identical same functions as for
> atomic_t, except for a giant amount of code duplication to deal with the
> new type.
> 
> That is; today we merged a bunch of scripts that generates most of
> atomic*_t, so we could probably script uatomic*_t wrappers with minimal
> effort, but it would add several thousand lines of code to each compile
> for absolutely no reason what so ever.
> 
> > This reminds me of this sooooo silly patch :/
> > 
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=adb03115f4590baa280ddc440a8eff08a6be0cb7
> 
> Yes, that's stupid. UBSAN is just wrong there.

It would be good for UBSAN to treat atomic operations as guaranteed
2s complement with no UB for signed integer overflow.  After all, if
even the C standard is willing to do this...

Ah, but don't we disable interrupts and fall back to normal arithmetic
for UP systems?  Hmmm...  We do so for atomic_add_return() even on
x86, it turns out:

static __always_inline int arch_atomic_add_return(int i, atomic_t *v)
{
	return i + xadd(&v->counter, i);
}

So UBSAN actually did have a point.  :-(

							Thanx, Paul

Re: [RFC PATCH] lib: Introduce generic __cmpxchg_u64() and use it where needed

[Paul E. McKenney]

On Thu, Nov 01, 2018 at 05:32:12PM +0100, Peter Zijlstra wrote:
> On Thu, Nov 01, 2018 at 03:22:15PM +0000, Trond Myklebust wrote:
> > On Thu, 2018-11-01 at 15:59 +0100, Peter Zijlstra wrote:
> > > On Thu, Nov 01, 2018 at 01:18:46PM +0000, Mark Rutland wrote:
> 
> > > > > My one question (and the reason why I went with cmpxchg() in the
> > > > > first place) would be about the overflow behaviour for
> > > > > atomic_fetch_inc() and friends. I believe those functions should
> > > > > be OK on x86, so that when we overflow the counter, it behaves
> > > > > like an unsigned value and wraps back around.  Is that the case
> > > > > for all architectures?
> > > > > 
> > > > > i.e. are atomic_t/atomic64_t always guaranteed to behave like
> > > > > u32/u64 on increment?
> > > > > 
> > > > > I could not find any documentation that explicitly stated that
> > > > > they should.
> > > > 
> > > > Peter, Will, I understand that the atomic_t/atomic64_t ops are
> > > > required to wrap per 2's-complement. IIUC the refcount code relies
> > > > on this.
> > > > 
> > > > Can you confirm?
> > > 
> > > There is quite a bit of core code that hard assumes 2s-complement.
> > > Not only for atomics but for any signed integer type. Also see the
> > > kernel using -fno-strict-overflow which implies -fwrapv, which
> > > defines signed overflow to behave like 2s-complement (and rids us of
> > > that particular UB).
> > 
> > Fair enough, but there have also been bugfixes to explicitly fix unsafe
> > C standards assumptions for signed integers. See, for instance commit
> > 5a581b367b5d "jiffies: Avoid undefined behavior from signed overflow"
> > from Paul McKenney.
> 
> Yes, I feel Paul has been to too many C/C++ committee meetings and got
> properly paranoid. Which isn't always a bad thing :-)

Even the C standard defines 2s complement for atomics.  Just not for
normal arithmetic, where yes, signed overflow is UB.  And yes, I do
know about -fwrapv, but I would like to avoid at least some copy-pasta
UB from my kernel code to who knows what user-mode environment.  :-/

At least where it is reasonably easy to do so.

And there is a push to define C++ signed arithmetic as 2s complement,
but there are still 1s complement systems with C compilers.  Just not
C++ compilers.  Legacy...

> But for us using -fno-strict-overflow which actually defines signed
> overflow, I myself am really not worried. I'm also not sure if KASAN has
> been taught about this, or if it will still (incorrectly) warn about UB
> for signed types.

UBSAN gave me a signed-overflow warning a few days ago.  Which I have
fixed, even though 2s complement did the right thing.  I am also taking
advantage of the change to use better naming.

> > Anyhow, if the atomic maintainers are willing to stand up and state for
> > the record that the atomic counters are guaranteed to wrap modulo 2^n
> > just like unsigned integers, then I'm happy to take Paul's patch.
> 
> I myself am certainly relying on it.

Color me confused.  My 5a581b367b5d is from 2013.  Or is "Paul" instead
intended to mean Paul Mackerras, who happens to be on CC?

							Thanx, Paul

Re: [RFC PATCH] lib: Introduce generic __cmpxchg_u64() and use it where needed

[Peter Zijlstra]

On Thu, Nov 01, 2018 at 10:01:46AM -0700, Paul E. McKenney wrote:
> On Thu, Nov 01, 2018 at 05:32:12PM +0100, Peter Zijlstra wrote:
> > On Thu, Nov 01, 2018 at 03:22:15PM +0000, Trond Myklebust wrote:
> > > On Thu, 2018-11-01 at 15:59 +0100, Peter Zijlstra wrote:
> > > > On Thu, Nov 01, 2018 at 01:18:46PM +0000, Mark Rutland wrote:
> > 
> > > > > > My one question (and the reason why I went with cmpxchg() in the
> > > > > > first place) would be about the overflow behaviour for
> > > > > > atomic_fetch_inc() and friends. I believe those functions should
> > > > > > be OK on x86, so that when we overflow the counter, it behaves
> > > > > > like an unsigned value and wraps back around.  Is that the case
> > > > > > for all architectures?
> > > > > > 
> > > > > > i.e. are atomic_t/atomic64_t always guaranteed to behave like
> > > > > > u32/u64 on increment?
> > > > > > 
> > > > > > I could not find any documentation that explicitly stated that
> > > > > > they should.
> > > > > 
> > > > > Peter, Will, I understand that the atomic_t/atomic64_t ops are
> > > > > required to wrap per 2's-complement. IIUC the refcount code relies
> > > > > on this.
> > > > > 
> > > > > Can you confirm?
> > > > 
> > > > There is quite a bit of core code that hard assumes 2s-complement.
> > > > Not only for atomics but for any signed integer type. Also see the
> > > > kernel using -fno-strict-overflow which implies -fwrapv, which
> > > > defines signed overflow to behave like 2s-complement (and rids us of
> > > > that particular UB).
> > > 
> > > Fair enough, but there have also been bugfixes to explicitly fix unsafe
> > > C standards assumptions for signed integers. See, for instance commit
> > > 5a581b367b5d "jiffies: Avoid undefined behavior from signed overflow"
> > > from Paul McKenney.
> > 
> > Yes, I feel Paul has been to too many C/C++ committee meetings and got
> > properly paranoid. Which isn't always a bad thing :-)
> 
> Even the C standard defines 2s complement for atomics.  

Ooh good to know.

> Just not for
> normal arithmetic, where yes, signed overflow is UB.  And yes, I do
> know about -fwrapv, but I would like to avoid at least some copy-pasta
> UB from my kernel code to who knows what user-mode environment.  :-/
> 
> At least where it is reasonably easy to do so.

Fair enough I suppose; I just always make sure to include the same
-fknobs for the userspace thing when I lift code.

> And there is a push to define C++ signed arithmetic as 2s complement,
> but there are still 1s complement systems with C compilers.  Just not
> C++ compilers.  Legacy...

*groan*; how about those ancient hardwares keep using ancient compilers
and we all move on to the 70s :-)

> > But for us using -fno-strict-overflow which actually defines signed
> > overflow, I myself am really not worried. I'm also not sure if KASAN has
> > been taught about this, or if it will still (incorrectly) warn about UB
> > for signed types.
> 
> UBSAN gave me a signed-overflow warning a few days ago.  Which I have
> fixed, even though 2s complement did the right thing.  I am also taking
> advantage of the change to use better naming.

Oh too many *SANs I suppose; and yes, if you can make the code better,
why not.

> > > Anyhow, if the atomic maintainers are willing to stand up and state for
> > > the record that the atomic counters are guaranteed to wrap modulo 2^n
> > > just like unsigned integers, then I'm happy to take Paul's patch.
> > 
> > I myself am certainly relying on it.
> 
> Color me confused.  My 5a581b367b5d is from 2013.  Or is "Paul" instead
> intended to mean Paul Mackerras, who happens to be on CC?

Paul Burton I think, on a part of the thread before we joined :-)

Re: [RFC PATCH] lib: Introduce generic __cmpxchg_u64() and use it where needed

[Paul E. McKenney]

On Thu, Nov 01, 2018 at 06:18:46PM +0100, Peter Zijlstra wrote:
> On Thu, Nov 01, 2018 at 10:01:46AM -0700, Paul E. McKenney wrote:
> > On Thu, Nov 01, 2018 at 05:32:12PM +0100, Peter Zijlstra wrote:
> > > On Thu, Nov 01, 2018 at 03:22:15PM +0000, Trond Myklebust wrote:
> > > > On Thu, 2018-11-01 at 15:59 +0100, Peter Zijlstra wrote:
> > > > > On Thu, Nov 01, 2018 at 01:18:46PM +0000, Mark Rutland wrote:
> > > 
> > > > > > > My one question (and the reason why I went with cmpxchg() in the
> > > > > > > first place) would be about the overflow behaviour for
> > > > > > > atomic_fetch_inc() and friends. I believe those functions should
> > > > > > > be OK on x86, so that when we overflow the counter, it behaves
> > > > > > > like an unsigned value and wraps back around.  Is that the case
> > > > > > > for all architectures?
> > > > > > > 
> > > > > > > i.e. are atomic_t/atomic64_t always guaranteed to behave like
> > > > > > > u32/u64 on increment?
> > > > > > > 
> > > > > > > I could not find any documentation that explicitly stated that
> > > > > > > they should.
> > > > > > 
> > > > > > Peter, Will, I understand that the atomic_t/atomic64_t ops are
> > > > > > required to wrap per 2's-complement. IIUC the refcount code relies
> > > > > > on this.
> > > > > > 
> > > > > > Can you confirm?
> > > > > 
> > > > > There is quite a bit of core code that hard assumes 2s-complement.
> > > > > Not only for atomics but for any signed integer type. Also see the
> > > > > kernel using -fno-strict-overflow which implies -fwrapv, which
> > > > > defines signed overflow to behave like 2s-complement (and rids us of
> > > > > that particular UB).
> > > > 
> > > > Fair enough, but there have also been bugfixes to explicitly fix unsafe
> > > > C standards assumptions for signed integers. See, for instance commit
> > > > 5a581b367b5d "jiffies: Avoid undefined behavior from signed overflow"
> > > > from Paul McKenney.
> > > 
> > > Yes, I feel Paul has been to too many C/C++ committee meetings and got
> > > properly paranoid. Which isn't always a bad thing :-)
> > 
> > Even the C standard defines 2s complement for atomics.  
> 
> Ooh good to know.

Must be some mistake, right?  ;-)

> > Just not for
> > normal arithmetic, where yes, signed overflow is UB.  And yes, I do
> > know about -fwrapv, but I would like to avoid at least some copy-pasta
> > UB from my kernel code to who knows what user-mode environment.  :-/
> > 
> > At least where it is reasonably easy to do so.
> 
> Fair enough I suppose; I just always make sure to include the same
> -fknobs for the userspace thing when I lift code.

Agreed!  But when it is other people lifting the code...

> > And there is a push to define C++ signed arithmetic as 2s complement,
> > but there are still 1s complement systems with C compilers.  Just not
> > C++ compilers.  Legacy...
> 
> *groan*; how about those ancient hardwares keep using ancient compilers
> and we all move on to the 70s :-)

Hey!!!  Some of that 70s (and 60s!) 1s-complement hardware helped pay
my way through university the first time around!!!  ;-)

Though where it once filled a room it is now on a single small chip.
Go figure...

> > > But for us using -fno-strict-overflow which actually defines signed
> > > overflow, I myself am really not worried. I'm also not sure if KASAN has
> > > been taught about this, or if it will still (incorrectly) warn about UB
> > > for signed types.
> > 
> > UBSAN gave me a signed-overflow warning a few days ago.  Which I have
> > fixed, even though 2s complement did the right thing.  I am also taking
> > advantage of the change to use better naming.
> 
> Oh too many *SANs I suppose; and yes, if you can make the code better,
> why not.

Yeah, when INT_MIN was confined to a single function, no problem.
But thanks to the RCU flavor consolidation, it has to be spread out a
bit more...  Plus there is now INT_MAX, INT_MAX/2, ...

> > > > Anyhow, if the atomic maintainers are willing to stand up and state for
> > > > the record that the atomic counters are guaranteed to wrap modulo 2^n
> > > > just like unsigned integers, then I'm happy to take Paul's patch.
> > > 
> > > I myself am certainly relying on it.
> > 
> > Color me confused.  My 5a581b367b5d is from 2013.  Or is "Paul" instead
> > intended to mean Paul Mackerras, who happens to be on CC?
> 
> Paul Burton I think, on a part of the thread before we joined :-)

Couldn't be bothered to look up the earlier part of the thread.  Getting
lazy in my old age.  ;-)

							Thanx, Paul

Re: [RFC PATCH] lib: Introduce generic __cmpxchg_u64() and use it where needed

[Dmitry Vyukov]

On Thu, Nov 1, 2018 at 6:18 PM, Peter Zijlstra <peterz@infradead.org> wrote:
>> > > > > > My one question (and the reason why I went with cmpxchg() in the
>> > > > > > first place) would be about the overflow behaviour for
>> > > > > > atomic_fetch_inc() and friends. I believe those functions should
>> > > > > > be OK on x86, so that when we overflow the counter, it behaves
>> > > > > > like an unsigned value and wraps back around.  Is that the case
>> > > > > > for all architectures?
>> > > > > >
>> > > > > > i.e. are atomic_t/atomic64_t always guaranteed to behave like
>> > > > > > u32/u64 on increment?
>> > > > > >
>> > > > > > I could not find any documentation that explicitly stated that
>> > > > > > they should.
>> > > > >
>> > > > > Peter, Will, I understand that the atomic_t/atomic64_t ops are
>> > > > > required to wrap per 2's-complement. IIUC the refcount code relies
>> > > > > on this.
>> > > > >
>> > > > > Can you confirm?
>> > > >
>> > > > There is quite a bit of core code that hard assumes 2s-complement.
>> > > > Not only for atomics but for any signed integer type. Also see the
>> > > > kernel using -fno-strict-overflow which implies -fwrapv, which
>> > > > defines signed overflow to behave like 2s-complement (and rids us of
>> > > > that particular UB).
>> > >
>> > > Fair enough, but there have also been bugfixes to explicitly fix unsafe
>> > > C standards assumptions for signed integers. See, for instance commit
>> > > 5a581b367b5d "jiffies: Avoid undefined behavior from signed overflow"
>> > > from Paul McKenney.
>> >
>> > Yes, I feel Paul has been to too many C/C++ committee meetings and got
>> > properly paranoid. Which isn't always a bad thing :-)
>>
>> Even the C standard defines 2s complement for atomics.
>
> Ooh good to know.
>
>> Just not for
>> normal arithmetic, where yes, signed overflow is UB.  And yes, I do
>> know about -fwrapv, but I would like to avoid at least some copy-pasta
>> UB from my kernel code to who knows what user-mode environment.  :-/
>>
>> At least where it is reasonably easy to do so.
>
> Fair enough I suppose; I just always make sure to include the same
> -fknobs for the userspace thing when I lift code.
>
>> And there is a push to define C++ signed arithmetic as 2s complement,
>> but there are still 1s complement systems with C compilers.  Just not
>> C++ compilers.  Legacy...
>
> *groan*; how about those ancient hardwares keep using ancient compilers
> and we all move on to the 70s :-)
>
>> > But for us using -fno-strict-overflow which actually defines signed
>> > overflow, I myself am really not worried. I'm also not sure if KASAN has
>> > been taught about this, or if it will still (incorrectly) warn about UB
>> > for signed types.
>>
>> UBSAN gave me a signed-overflow warning a few days ago.  Which I have
>> fixed, even though 2s complement did the right thing.  I am also taking
>> advantage of the change to use better naming.
>
> Oh too many *SANs I suppose; and yes, if you can make the code better,
> why not.

If there is a warning that we don't want to see at all, then we can
disable it. It supposed to be a useful tool, rather than a thing in
itself that lives own life. We already I think removed 1 particularly
noisy warning and made another optional via a config.
But the thing with overflows is that, even if it's defined, it's not
necessary the intended behavior. For example, take allocation size
calculation done via unsigned size_t. If it overflows it does not help
if C defines result or not, it still gives a user controlled write
primitive. We've seen similar cases with timeout/deadline calculation
in kernel, we really don't want it to just wrap modulo-2, right. Some
user-space projects even test with unsigned overflow warnings or
implicit truncation warnings, which are formally legal, but frequently
bugs.

Re: [RFC PATCH] lib: Introduce generic __cmpxchg_u64() and use it where needed

[Peter Zijlstra]

On Thu, Nov 01, 2018 at 06:46:50PM +0100, Dmitry Vyukov wrote:
> If there is a warning that we don't want to see at all, then we can
> disable it. It supposed to be a useful tool, rather than a thing in
> itself that lives own life. We already I think removed 1 particularly
> noisy warning and made another optional via a config.

> But the thing with overflows is that, even if it's defined, it's not
> necessary the intended behavior. For example, take allocation size
> calculation done via unsigned size_t. If it overflows it does not help
> if C defines result or not, it still gives a user controlled write
> primitive. We've seen similar cases with timeout/deadline calculation
> in kernel, we really don't want it to just wrap modulo-2, right. Some
> user-space projects even test with unsigned overflow warnings or
> implicit truncation warnings, which are formally legal, but frequently
> bugs.

Sure; but then don't call it UB.

If we want to have an additional integer over/underflow checker (ideally
with a gcc plugin that has explicit annotations like __wrap to make it
go away) that is fine; and it can be done on unsigned and signed.

RE: [RFC PATCH] lib: Introduce generic __cmpxchg_u64() and use it where needed

[David Laight]

From: Paul E. McKenney
> Sent: 01 November 2018 17:02
...
> And there is a push to define C++ signed arithmetic as 2s complement,
> but there are still 1s complement systems with C compilers.  Just not
> C++ compilers.  Legacy...

Hmmm... I've used C compilers for DSPs where signed integer arithmetic
used the 'data registers' and would saturate, unsigned used the 'address
registers' and wrapped.
That was deliberate because it is much better to clip analogue values.

Then there was the annoying cobol run time that didn't update the
result variable if the result wouldn't fit.
Took a while to notice that the sum of a list of values was even wrong!
That would be perfectly valid for C - if unexpected.

> > But for us using -fno-strict-overflow which actually defines signed
> > overflow

I wonder how much real code 'strict-overflow' gets rid of?
IIRC gcc silently turns loops like:
	int i; for (i = 1; i != 0; i *= 2) ...
into infinite ones.
Which is never what is required.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [RFC PATCH] lib: Introduce generic __cmpxchg_u64() and use it where needed

[Peter Zijlstra]

On Fri, Nov 02, 2018 at 10:56:31AM +0000, David Laight wrote:
> From: Paul E. McKenney
> > Sent: 01 November 2018 17:02
> ...
> > And there is a push to define C++ signed arithmetic as 2s complement,
> > but there are still 1s complement systems with C compilers.  Just not
> > C++ compilers.  Legacy...
> 
> Hmmm... I've used C compilers for DSPs where signed integer arithmetic
> used the 'data registers' and would saturate, unsigned used the 'address
> registers' and wrapped.
> That was deliberate because it is much better to clip analogue values.

Seems a dodgy heuristic if you ask me.

> Then there was the annoying cobol run time that didn't update the
> result variable if the result wouldn't fit.
> Took a while to notice that the sum of a list of values was even wrong!
> That would be perfectly valid for C - if unexpected.

That's just insane ;-)

> > > But for us using -fno-strict-overflow which actually defines signed
> > > overflow
> 
> I wonder how much real code 'strict-overflow' gets rid of?
> IIRC gcc silently turns loops like:
> 	int i; for (i = 1; i != 0; i *= 2) ...
> into infinite ones.
> Which is never what is required.

Nobody said C was a 'safe' language. But less UB makes a better language
IMO. Ideally we'd get all UBs filled in -- but I realise C has a few
very 'interesting' ones that might be hard to get rid of.

Re: [RFC PATCH] lib: Introduce generic __cmpxchg_u64() and use it where needed

[Paul E. McKenney]

On Fri, Nov 02, 2018 at 01:23:28PM +0100, Peter Zijlstra wrote:
> On Fri, Nov 02, 2018 at 10:56:31AM +0000, David Laight wrote:
> > From: Paul E. McKenney
> > > Sent: 01 November 2018 17:02
> > ...
> > > And there is a push to define C++ signed arithmetic as 2s complement,
> > > but there are still 1s complement systems with C compilers.  Just not
> > > C++ compilers.  Legacy...
> > 
> > Hmmm... I've used C compilers for DSPs where signed integer arithmetic
> > used the 'data registers' and would saturate, unsigned used the 'address
> > registers' and wrapped.
> > That was deliberate because it is much better to clip analogue values.
> 
> Seems a dodgy heuristic if you ask me.
> 
> > Then there was the annoying cobol run time that didn't update the
> > result variable if the result wouldn't fit.
> > Took a while to notice that the sum of a list of values was even wrong!
> > That would be perfectly valid for C - if unexpected.
> 
> That's just insane ;-)
> 
> > > > But for us using -fno-strict-overflow which actually defines signed
> > > > overflow
> > 
> > I wonder how much real code 'strict-overflow' gets rid of?
> > IIRC gcc silently turns loops like:
> > 	int i; for (i = 1; i != 0; i *= 2) ...
> > into infinite ones.
> > Which is never what is required.
> 
> Nobody said C was a 'safe' language. But less UB makes a better language
> IMO. Ideally we'd get all UBs filled in -- but I realise C has a few
> very 'interesting' ones that might be hard to get rid of.

There has been an effort to reduce UB, but not sure how far they got.

							Thanx, Paul

Re: [RFC PATCH] lib: Introduce generic __cmpxchg_u64() and use it where needed

[Paul E. McKenney]

On Fri, Nov 02, 2018 at 10:56:31AM +0000, David Laight wrote:
> From: Paul E. McKenney
> > Sent: 01 November 2018 17:02
> ...
> > And there is a push to define C++ signed arithmetic as 2s complement,
> > but there are still 1s complement systems with C compilers.  Just not
> > C++ compilers.  Legacy...
> 
> Hmmm... I've used C compilers for DSPs where signed integer arithmetic
> used the 'data registers' and would saturate, unsigned used the 'address
> registers' and wrapped.
> That was deliberate because it is much better to clip analogue values.

There are no C++ compilers for those DSPs, correct?  (Some of the
C++ standards commmittee members believe that they have fully checked,
but they might well have missed something.)

> Then there was the annoying cobol run time that didn't update the
> result variable if the result wouldn't fit.
> Took a while to notice that the sum of a list of values was even wrong!
> That would be perfectly valid for C - if unexpected.

Heh!  COBOL and FORTRAN also helped fund my first pass through university.

> > > But for us using -fno-strict-overflow which actually defines signed
> > > overflow
> 
> I wonder how much real code 'strict-overflow' gets rid of?
> IIRC gcc silently turns loops like:
> 	int i; for (i = 1; i != 0; i *= 2) ...
> into infinite ones.
> Which is never what is required.

The usual response is something like this:

	for (i = 1; i < n; i++)

where the compiler has no idea what range of values "n" might take on.
Can't say that I am convinced by that example, but at least we do have
-fno-strict-overflow.

							Thanx, Paul

Re: [RFC PATCH] lib: Introduce generic __cmpxchg_u64() and use it where needed

[Andrey Ryabinin]

On 11/01/2018 07:32 PM, Peter Zijlstra wrote:
> On Thu, Nov 01, 2018 at 03:22:15PM +0000, Trond Myklebust wrote:
>> On Thu, 2018-11-01 at 15:59 +0100, Peter Zijlstra wrote:
>>> On Thu, Nov 01, 2018 at 01:18:46PM +0000, Mark Rutland wrote:
> 
>>>>> My one question (and the reason why I went with cmpxchg() in the
>>>>> first place) would be about the overflow behaviour for
>>>>> atomic_fetch_inc() and friends. I believe those functions should
>>>>> be OK on x86, so that when we overflow the counter, it behaves
>>>>> like an unsigned value and wraps back around.  Is that the case
>>>>> for all architectures?
>>>>>
>>>>> i.e. are atomic_t/atomic64_t always guaranteed to behave like
>>>>> u32/u64 on increment?
>>>>>
>>>>> I could not find any documentation that explicitly stated that
>>>>> they should.
>>>>
>>>> Peter, Will, I understand that the atomic_t/atomic64_t ops are
>>>> required to wrap per 2's-complement. IIUC the refcount code relies
>>>> on this.
>>>>
>>>> Can you confirm?
>>>
>>> There is quite a bit of core code that hard assumes 2s-complement.
>>> Not only for atomics but for any signed integer type. Also see the
>>> kernel using -fno-strict-overflow which implies -fwrapv, which
>>> defines signed overflow to behave like 2s-complement (and rids us of
>>> that particular UB).
>>
>> Fair enough, but there have also been bugfixes to explicitly fix unsafe
>> C standards assumptions for signed integers. See, for instance commit
>> 5a581b367b5d "jiffies: Avoid undefined behavior from signed overflow"
>> from Paul McKenney.
> 
> Yes, I feel Paul has been to too many C/C++ committee meetings and got
> properly paranoid. Which isn't always a bad thing :-)
> 
> But for us using -fno-strict-overflow which actually defines signed
> overflow, I myself am really not worried. I'm also not sure if KASAN has
> been taught about this, or if it will still (incorrectly) warn about UB
> for signed types.
> 

UBSAN warns about signed overflows despite -fno-strict-overflow if gcc version is < 8.
I have learned recently that UBSAN in GCC 8 ignores signed overflows if -fno-strict-overflow of fwrapv is used.

$ cat signed_overflow.c 
#include <stdio.h>

__attribute__((noinline))
int foo(int a, int b)
{
        return a+b;
}

int main(void)
{
        int a = 0x7fffffff;
        int b = 2;
        printf("%d\n", foo(a,b));
        return 0;
}

$ gcc-8.2.0 -fsanitize=signed-integer-overflow signed_overflow.c && ./a.out 
signed_overflow.c:6:10: runtime error: signed integer overflow: 2147483647 + 2 cannot be represented in type 'int'
-2147483647
$ gcc-8.2.0 -fno-strict-overflow -fsanitize=signed-integer-overflow signed_overflow.c && ./a.out 
-2147483647
$ gcc-7.3.0 -fsanitize=signed-integer-overflow signed_overflow.c && ./a.out
signed_overflow.c:6:10: runtime error: signed integer overflow: 2147483647 + 2 cannot be represented in type 'int'
-2147483647
$ gcc-7.3.0 -fno-strict-overflow -fsanitize=signed-integer-overflow signed_overflow.c && ./a.out
signed_overflow.c:6:10: runtime error: signed integer overflow: 2147483647 + 2 cannot be represented in type 'int'
-2147483647


clang behaves the same way as GCC 8:
$ clang -fsanitize=signed-integer-overflow signed_overflow.c && ./a.out 
signed_overflow.c:6:10: runtime error: signed integer overflow: 2147483647 + 2 cannot be represented in type 'int'
-2147483647
$ clang -fno-strict-overflow -fsanitize=signed-integer-overflow signed_overflow.c && ./a.out 
-2147483647


We can always just drop -fsanitize=signed-integer-overflow if it considered too noisy.
Although it did catch some real bugs.

Re: [RFC PATCH] lib: Introduce generic __cmpxchg_u64() and use it where needed

[Peter Zijlstra]

On Fri, Nov 02, 2018 at 07:19:15PM +0300, Andrey Ryabinin wrote:

> UBSAN warns about signed overflows despite -fno-strict-overflow if gcc
> version is < 8.  I have learned recently that UBSAN in GCC 8 ignores
> signed overflows if -fno-strict-overflow of fwrapv is used.

Ah, good.

> We can always just drop -fsanitize=signed-integer-overflow if it considered too noisy.

I think that is the most consistent beahviour. signed overflow is not UB
in the kernel.

> Although it did catch some real bugs.

If we want an over/under-flow checker, then that should be a separate
plugin and not specific to signed or unsigned.

Re: [RFC PATCH] lib: Introduce generic __cmpxchg_u64() and use it where needed

[Peter Zijlstra]

On Fri, Nov 02, 2018 at 07:19:15PM +0300, Andrey Ryabinin wrote:
> UBSAN warns about signed overflows despite -fno-strict-overflow if gcc version is < 8.
> I have learned recently that UBSAN in GCC 8 ignores signed overflows if -fno-strict-overflow of fwrapv is used.
> 
> $ cat signed_overflow.c 
> #include <stdio.h>
> 
> __attribute__((noinline))
> int foo(int a, int b)
> {
>         return a+b;

 s/+/<</

> }
> 
> int main(void)
> {
>         int a = 0x7fffffff;
>         int b = 2;
>         printf("%d\n", foo(a,b));
>         return 0;
> }

It also seem to affect 'shift':

peterz@hirez:~/tmp$ gcc -fsanitize=signed-integer-overflow,shift overflow.c ; ./a.out
overflow.c:6:11: runtime error: left shift of 2147483647 by 2 places cannot be represented in type 'int'
-4
peterz@hirez:~/tmp$ gcc -fsanitize=signed-integer-overflow,shift -fwrapv overflow.c ; ./a.out
-4

[PATCH] SUNRPC: Use atomic(64)_t for seq_send(64)

[Paul Burton]

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@mips.com>
Fixes: 571ed1fd2390 ("SUNRPC: Replace krb5_seq_lock with a lockless scheme")
Cc: Trond Myklebust <trond.myklebust@hammerspace.com>
Cc: Anna Schumaker <anna.schumaker@netapp.com>
Cc: J. Bruce Fields <bfields@fieldses.org>
Cc: Jeff Layton <jlayton@kernel.org>
Cc: David S. Miller <davem@davemloft.net>
Cc: linux-nfs@vger.kernel.org
Cc: netdev@vger.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(-)

diff --git a/include/linux/sunrpc/gss_krb5.h b/include/linux/sunrpc/gss_krb5.h
index 131424cefc6a..02c0412e368c 100644
--- a/include/linux/sunrpc/gss_krb5.h
+++ b/include/linux/sunrpc/gss_krb5.h
@@ -107,8 +107,8 @@ struct krb5_ctx {
 	u8			Ksess[GSS_KRB5_MAX_KEYLEN]; /* session key */
 	u8			cksum[GSS_KRB5_MAX_KEYLEN];
 	s32			endtime;
-	u32			seq_send;
-	u64			seq_send64;
+	atomic_t		seq_send;
+	atomic64_t		seq_send64;
 	struct xdr_netobj	mech_used;
 	u8			initiator_sign[GSS_KRB5_MAX_KEYLEN];
 	u8			acceptor_sign[GSS_KRB5_MAX_KEYLEN];
@@ -118,9 +118,6 @@ struct krb5_ctx {
 	u8			acceptor_integ[GSS_KRB5_MAX_KEYLEN];
 };
 
-extern u32 gss_seq_send_fetch_and_inc(struct krb5_ctx *ctx);
-extern u64 gss_seq_send64_fetch_and_inc(struct krb5_ctx *ctx);
-
 /* The length of the Kerberos GSS token header */
 #define GSS_KRB5_TOK_HDR_LEN	(16)
 
diff --git a/net/sunrpc/auth_gss/gss_krb5_mech.c b/net/sunrpc/auth_gss/gss_krb5_mech.c
index 7f0424dfa8f6..eab71fc7af3e 100644
--- a/net/sunrpc/auth_gss/gss_krb5_mech.c
+++ b/net/sunrpc/auth_gss/gss_krb5_mech.c
@@ -274,6 +274,7 @@ get_key(const void *p, const void *end,
 static int
 gss_import_v1_context(const void *p, const void *end, struct krb5_ctx *ctx)
 {
+	u32 seq_send;
 	int tmp;
 
 	p = simple_get_bytes(p, end, &ctx->initiate, sizeof(ctx->initiate));
@@ -315,9 +316,10 @@ gss_import_v1_context(const void *p, const void *end, struct krb5_ctx *ctx)
 	p = simple_get_bytes(p, end, &ctx->endtime, sizeof(ctx->endtime));
 	if (IS_ERR(p))
 		goto out_err;
-	p = simple_get_bytes(p, end, &ctx->seq_send, sizeof(ctx->seq_send));
+	p = simple_get_bytes(p, end, &seq_send, sizeof(seq_send));
 	if (IS_ERR(p))
 		goto out_err;
+	atomic_set(&ctx->seq_send, seq_send);
 	p = simple_get_netobj(p, end, &ctx->mech_used);
 	if (IS_ERR(p))
 		goto out_err;
@@ -607,6 +609,7 @@ static int
 gss_import_v2_context(const void *p, const void *end, struct krb5_ctx *ctx,
 		gfp_t gfp_mask)
 {
+	u64 seq_send64;
 	int keylen;
 
 	p = simple_get_bytes(p, end, &ctx->flags, sizeof(ctx->flags));
@@ -617,14 +620,15 @@ gss_import_v2_context(const void *p, const void *end, struct krb5_ctx *ctx,
 	p = simple_get_bytes(p, end, &ctx->endtime, sizeof(ctx->endtime));
 	if (IS_ERR(p))
 		goto out_err;
-	p = simple_get_bytes(p, end, &ctx->seq_send64, sizeof(ctx->seq_send64));
+	p = simple_get_bytes(p, end, &seq_send64, sizeof(seq_send64));
 	if (IS_ERR(p))
 		goto out_err;
+	atomic64_set(&ctx->seq_send64, seq_send64);
 	/* set seq_send for use by "older" enctypes */
-	ctx->seq_send = ctx->seq_send64;
-	if (ctx->seq_send64 != ctx->seq_send) {
-		dprintk("%s: seq_send64 %lx, seq_send %x overflow?\n", __func__,
-			(unsigned long)ctx->seq_send64, ctx->seq_send);
+	atomic_set(&ctx->seq_send, seq_send64);
+	if (seq_send64 != atomic_read(&ctx->seq_send)) {
+		dprintk("%s: seq_send64 %llx, seq_send %x overflow?\n", __func__,
+			seq_send64, atomic_read(&ctx->seq_send));
 		p = ERR_PTR(-EINVAL);
 		goto out_err;
 	}
diff --git a/net/sunrpc/auth_gss/gss_krb5_seal.c b/net/sunrpc/auth_gss/gss_krb5_seal.c
index b4adeb06660b..48fe4a591b54 100644
--- a/net/sunrpc/auth_gss/gss_krb5_seal.c
+++ b/net/sunrpc/auth_gss/gss_krb5_seal.c
@@ -123,30 +123,6 @@ setup_token_v2(struct krb5_ctx *ctx, struct xdr_netobj *token)
 	return krb5_hdr;
 }
 
-u32
-gss_seq_send_fetch_and_inc(struct krb5_ctx *ctx)
-{
-	u32 old, seq_send = READ_ONCE(ctx->seq_send);
-
-	do {
-		old = seq_send;
-		seq_send = cmpxchg(&ctx->seq_send, old, old + 1);
-	} while (old != seq_send);
-	return seq_send;
-}
-
-u64
-gss_seq_send64_fetch_and_inc(struct krb5_ctx *ctx)
-{
-	u64 old, seq_send = READ_ONCE(ctx->seq_send);
-
-	do {
-		old = seq_send;
-		seq_send = cmpxchg64(&ctx->seq_send64, old, old + 1);
-	} while (old != seq_send);
-	return seq_send;
-}
-
 static u32
 gss_get_mic_v1(struct krb5_ctx *ctx, struct xdr_buf *text,
 		struct xdr_netobj *token)
@@ -177,7 +153,7 @@ gss_get_mic_v1(struct krb5_ctx *ctx, struct xdr_buf *text,
 
 	memcpy(ptr + GSS_KRB5_TOK_HDR_LEN, md5cksum.data, md5cksum.len);
 
-	seq_send = gss_seq_send_fetch_and_inc(ctx);
+	seq_send = atomic_fetch_inc(&ctx->seq_send);
 
 	if (krb5_make_seq_num(ctx, ctx->seq, ctx->initiate ? 0 : 0xff,
 			      seq_send, ptr + GSS_KRB5_TOK_HDR_LEN, ptr + 8))
@@ -205,7 +181,7 @@ gss_get_mic_v2(struct krb5_ctx *ctx, struct xdr_buf *text,
 
 	/* Set up the sequence number. Now 64-bits in clear
 	 * text and w/o direction indicator */
-	seq_send_be64 = cpu_to_be64(gss_seq_send64_fetch_and_inc(ctx));
+	seq_send_be64 = cpu_to_be64(atomic64_fetch_inc(&ctx->seq_send64));
 	memcpy(krb5_hdr + 8, (char *) &seq_send_be64, 8);
 
 	if (ctx->initiate) {
diff --git a/net/sunrpc/auth_gss/gss_krb5_wrap.c b/net/sunrpc/auth_gss/gss_krb5_wrap.c
index 962fa84e6db1..5cdde6cb703a 100644
--- a/net/sunrpc/auth_gss/gss_krb5_wrap.c
+++ b/net/sunrpc/auth_gss/gss_krb5_wrap.c
@@ -228,7 +228,7 @@ gss_wrap_kerberos_v1(struct krb5_ctx *kctx, int offset,
 
 	memcpy(ptr + GSS_KRB5_TOK_HDR_LEN, md5cksum.data, md5cksum.len);
 
-	seq_send = gss_seq_send_fetch_and_inc(kctx);
+	seq_send = atomic_fetch_inc(&kctx->seq_send);
 
 	/* XXX would probably be more efficient to compute checksum
 	 * and encrypt at the same time: */
@@ -475,7 +475,7 @@ gss_wrap_kerberos_v2(struct krb5_ctx *kctx, u32 offset,
 	*be16ptr++ = 0;
 
 	be64ptr = (__be64 *)be16ptr;
-	*be64ptr = cpu_to_be64(gss_seq_send64_fetch_and_inc(kctx));
+	*be64ptr = cpu_to_be64(atomic64_fetch_inc(&kctx->seq_send64));
 
 	err = (*kctx->gk5e->encrypt_v2)(kctx, offset, buf, pages);
 	if (err)
-- 
2.19.1

Re: [PATCH] SUNRPC: Use atomic(64)_t for seq_send(64)

[Trond Myklebust]

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@mips.com>
> Fixes: 571ed1fd2390 ("SUNRPC: Replace krb5_seq_lock with a lockless
> scheme")
> Cc: Trond Myklebust <trond.myklebust@hammerspace.com>
> Cc: Anna Schumaker <anna.schumaker@netapp.com>
> Cc: J. Bruce Fields <bfields@fieldses.org>
> Cc: Jeff Layton <jlayton@kernel.org>
> Cc: David S. Miller <davem@davemloft.net>
> Cc: linux-nfs@vger.kernel.org
> Cc: netdev@vger.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@hammerspace.com

Re: [RFC PATCH] lib: Introduce generic __cmpxchg_u64() and use it where needed

[Paul Burton]

Hi Trond,

On Thu, Nov 01, 2018 at 12:17:31AM +0000, Trond Myklebust wrote:
> On Wed, 2018-10-31 at 23:32 +0000, Paul Burton wrote:
> > In this particular case I have no idea why
> > net/sunrpc/auth_gss/gss_krb5_seal.c is using cmpxchg64() at all. It's
> > essentially reinventing atomic64_fetch_inc() which is already
> > provided
> > everywhere via CONFIG_GENERIC_ATOMIC64 & the spinlock approach. At
> > least
> > for atomic64_* functions the assumption that all access will be
> > performed using those same functions seems somewhat reasonable.
> > 
> > So how does the below look? Trond?
> 
> My one question (and the reason why I went with cmpxchg() in the first
> place) would be about the overflow behaviour for atomic_fetch_inc() and
> friends. I believe those functions should be OK on x86, so that when we
> overflow the counter, it behaves like an unsigned value and wraps back
> around.  Is that the case for all architectures?
> 
> i.e. are atomic_t/atomic64_t always guaranteed to behave like u32/u64
> on increment?
> 
> I could not find any documentation that explicitly stated that they
> should.

Based on other replies it seems like it's at least implicitly assumed by
other code, even if not explicitly stated.

From a MIPS perspective where atomics are implemented using load-linked
& store-conditional instructions the actual addition will be performed
using the same addu instruction that a plain integer addition would
generate (regardless of signedness), so there'll be absolutely no
difference in arithmetic between your gss_seq_send64_fetch_and_inc()
function and atomic64_fetch_inc(). I'd expect the same to be true for
other architectures with load-linked & store-conditional style atomics.

In any case, for the benefit of anyone interested who I didn't copy on
the patch submission, here it is:

    https://lore.kernel.org/lkml/20181101175109.8621-1-paul.burton@mips.com/

Thanks,
    Paul

Re: [RFC PATCH] lib: Introduce generic __cmpxchg_u64() and use it where needed

[Guenter Roeck]

On 10/31/18 4:32 PM, Paul Burton wrote:
> (Copying SunRPC & net maintainers.)
> 
> Hi Guenter,
> 
> On Wed, Oct 31, 2018 at 03:02:53PM -0700, Guenter Roeck wrote:
>> The alternatives I can see are
>> - Do not use cmpxchg64() outside architecture code (ie drop its use from
>>    the offending driver, and keep doing the same whenever the problem comes
>>    up again).
>> or
>> - Introduce something like ARCH_HAS_CMPXCHG64 and use it to determine
>>    if cmpxchg64 is supported or not.
>>
>> Any preference ?
> 
> My preference would be option 1 - avoiding cmpxchg64() where possible in
> generic code. I wouldn't be opposed to the Kconfig option if there are
> cases where cmpxchg64() can really help performance though.
> 
> The last time I'm aware of this coming up the affected driver was
> modified to avoid cmpxchg64() [1].
> 
> In this particular case I have no idea why
> net/sunrpc/auth_gss/gss_krb5_seal.c is using cmpxchg64() at all. It's
> essentially reinventing atomic64_fetch_inc() which is already provided
> everywhere via CONFIG_GENERIC_ATOMIC64 & the spinlock approach. At least
> for atomic64_* functions the assumption that all access will be
> performed using those same functions seems somewhat reasonable.
> 
> So how does the below look? Trond?
> 

For my part I agree that this would be a much better solution. The argument
that it is not always absolutely guaranteed that atomics don't wrap doesn't
really hold for me because it looks like they all do. On top of that, there
is an explicit atomic_dec_if_positive() and atomic_fetch_add_unless(),
which to me strongly suggests that they _are_ supposed to wrap.
Given the cost of adding a comparison to each atomic operation to
prevent it from wrapping, anything else would not really make sense to me.

So ... please consider my patch abandoned. Thanks for looking into this!

Guenter

> Thanks,
>      Paul
> 
> [1] https://patchwork.ozlabs.org/cover/891284/
> 
> ---
> diff --git a/include/linux/sunrpc/gss_krb5.h b/include/linux/sunrpc/gss_krb5.h
> index 131424cefc6a..02c0412e368c 100644
> --- a/include/linux/sunrpc/gss_krb5.h
> +++ b/include/linux/sunrpc/gss_krb5.h
> @@ -107,8 +107,8 @@ struct krb5_ctx {
>   	u8			Ksess[GSS_KRB5_MAX_KEYLEN]; /* session key */
>   	u8			cksum[GSS_KRB5_MAX_KEYLEN];
>   	s32			endtime;
> -	u32			seq_send;
> -	u64			seq_send64;
> +	atomic_t		seq_send;
> +	atomic64_t		seq_send64;
>   	struct xdr_netobj	mech_used;
>   	u8			initiator_sign[GSS_KRB5_MAX_KEYLEN];
>   	u8			acceptor_sign[GSS_KRB5_MAX_KEYLEN];
> @@ -118,9 +118,6 @@ struct krb5_ctx {
>   	u8			acceptor_integ[GSS_KRB5_MAX_KEYLEN];
>   };
>   
> -extern u32 gss_seq_send_fetch_and_inc(struct krb5_ctx *ctx);
> -extern u64 gss_seq_send64_fetch_and_inc(struct krb5_ctx *ctx);
> -
>   /* The length of the Kerberos GSS token header */
>   #define GSS_KRB5_TOK_HDR_LEN	(16)
>   
> diff --git a/net/sunrpc/auth_gss/gss_krb5_mech.c b/net/sunrpc/auth_gss/gss_krb5_mech.c
> index 7f0424dfa8f6..eab71fc7af3e 100644
> --- a/net/sunrpc/auth_gss/gss_krb5_mech.c
> +++ b/net/sunrpc/auth_gss/gss_krb5_mech.c
> @@ -274,6 +274,7 @@ get_key(const void *p, const void *end,
>   static int
>   gss_import_v1_context(const void *p, const void *end, struct krb5_ctx *ctx)
>   {
> +	u32 seq_send;
>   	int tmp;
>   
>   	p = simple_get_bytes(p, end, &ctx->initiate, sizeof(ctx->initiate));
> @@ -315,9 +316,10 @@ gss_import_v1_context(const void *p, const void *end, struct krb5_ctx *ctx)
>   	p = simple_get_bytes(p, end, &ctx->endtime, sizeof(ctx->endtime));
>   	if (IS_ERR(p))
>   		goto out_err;
> -	p = simple_get_bytes(p, end, &ctx->seq_send, sizeof(ctx->seq_send));
> +	p = simple_get_bytes(p, end, &seq_send, sizeof(seq_send));
>   	if (IS_ERR(p))
>   		goto out_err;
> +	atomic_set(&ctx->seq_send, seq_send);
>   	p = simple_get_netobj(p, end, &ctx->mech_used);
>   	if (IS_ERR(p))
>   		goto out_err;
> @@ -607,6 +609,7 @@ static int
>   gss_import_v2_context(const void *p, const void *end, struct krb5_ctx *ctx,
>   		gfp_t gfp_mask)
>   {
> +	u64 seq_send64;
>   	int keylen;
>   
>   	p = simple_get_bytes(p, end, &ctx->flags, sizeof(ctx->flags));
> @@ -617,14 +620,15 @@ gss_import_v2_context(const void *p, const void *end, struct krb5_ctx *ctx,
>   	p = simple_get_bytes(p, end, &ctx->endtime, sizeof(ctx->endtime));
>   	if (IS_ERR(p))
>   		goto out_err;
> -	p = simple_get_bytes(p, end, &ctx->seq_send64, sizeof(ctx->seq_send64));
> +	p = simple_get_bytes(p, end, &seq_send64, sizeof(seq_send64));
>   	if (IS_ERR(p))
>   		goto out_err;
> +	atomic64_set(&ctx->seq_send64, seq_send64);
>   	/* set seq_send for use by "older" enctypes */
> -	ctx->seq_send = ctx->seq_send64;
> -	if (ctx->seq_send64 != ctx->seq_send) {
> -		dprintk("%s: seq_send64 %lx, seq_send %x overflow?\n", __func__,
> -			(unsigned long)ctx->seq_send64, ctx->seq_send);
> +	atomic_set(&ctx->seq_send, seq_send64);
> +	if (seq_send64 != atomic_read(&ctx->seq_send)) {
> +		dprintk("%s: seq_send64 %llx, seq_send %x overflow?\n", __func__,
> +			seq_send64, atomic_read(&ctx->seq_send));
>   		p = ERR_PTR(-EINVAL);
>   		goto out_err;
>   	}
> diff --git a/net/sunrpc/auth_gss/gss_krb5_seal.c b/net/sunrpc/auth_gss/gss_krb5_seal.c
> index b4adeb06660b..48fe4a591b54 100644
> --- a/net/sunrpc/auth_gss/gss_krb5_seal.c
> +++ b/net/sunrpc/auth_gss/gss_krb5_seal.c
> @@ -123,30 +123,6 @@ setup_token_v2(struct krb5_ctx *ctx, struct xdr_netobj *token)
>   	return krb5_hdr;
>   }
>   
> -u32
> -gss_seq_send_fetch_and_inc(struct krb5_ctx *ctx)
> -{
> -	u32 old, seq_send = READ_ONCE(ctx->seq_send);
> -
> -	do {
> -		old = seq_send;
> -		seq_send = cmpxchg(&ctx->seq_send, old, old + 1);
> -	} while (old != seq_send);
> -	return seq_send;
> -}
> -
> -u64
> -gss_seq_send64_fetch_and_inc(struct krb5_ctx *ctx)
> -{
> -	u64 old, seq_send = READ_ONCE(ctx->seq_send);
> -
> -	do {
> -		old = seq_send;
> -		seq_send = cmpxchg64(&ctx->seq_send64, old, old + 1);
> -	} while (old != seq_send);
> -	return seq_send;
> -}
> -
>   static u32
>   gss_get_mic_v1(struct krb5_ctx *ctx, struct xdr_buf *text,
>   		struct xdr_netobj *token)
> @@ -177,7 +153,7 @@ gss_get_mic_v1(struct krb5_ctx *ctx, struct xdr_buf *text,
>   
>   	memcpy(ptr + GSS_KRB5_TOK_HDR_LEN, md5cksum.data, md5cksum.len);
>   
> -	seq_send = gss_seq_send_fetch_and_inc(ctx);
> +	seq_send = atomic_fetch_inc(&ctx->seq_send);
>   
>   	if (krb5_make_seq_num(ctx, ctx->seq, ctx->initiate ? 0 : 0xff,
>   			      seq_send, ptr + GSS_KRB5_TOK_HDR_LEN, ptr + 8))
> @@ -205,7 +181,7 @@ gss_get_mic_v2(struct krb5_ctx *ctx, struct xdr_buf *text,
>   
>   	/* Set up the sequence number. Now 64-bits in clear
>   	 * text and w/o direction indicator */
> -	seq_send_be64 = cpu_to_be64(gss_seq_send64_fetch_and_inc(ctx));
> +	seq_send_be64 = cpu_to_be64(atomic64_fetch_inc(&ctx->seq_send64));
>   	memcpy(krb5_hdr + 8, (char *) &seq_send_be64, 8);
>   
>   	if (ctx->initiate) {
> diff --git a/net/sunrpc/auth_gss/gss_krb5_wrap.c b/net/sunrpc/auth_gss/gss_krb5_wrap.c
> index 962fa84e6db1..5cdde6cb703a 100644
> --- a/net/sunrpc/auth_gss/gss_krb5_wrap.c
> +++ b/net/sunrpc/auth_gss/gss_krb5_wrap.c
> @@ -228,7 +228,7 @@ gss_wrap_kerberos_v1(struct krb5_ctx *kctx, int offset,
>   
>   	memcpy(ptr + GSS_KRB5_TOK_HDR_LEN, md5cksum.data, md5cksum.len);
>   
> -	seq_send = gss_seq_send_fetch_and_inc(kctx);
> +	seq_send = atomic_fetch_inc(&kctx->seq_send);
>   
>   	/* XXX would probably be more efficient to compute checksum
>   	 * and encrypt at the same time: */
> @@ -475,7 +475,7 @@ gss_wrap_kerberos_v2(struct krb5_ctx *kctx, u32 offset,
>   	*be16ptr++ = 0;
>   
>   	be64ptr = (__be64 *)be16ptr;
> -	*be64ptr = cpu_to_be64(gss_seq_send64_fetch_and_inc(kctx));
> +	*be64ptr = cpu_to_be64(atomic64_fetch_inc(&kctx->seq_send64));
>   
>   	err = (*kctx->gk5e->encrypt_v2)(kctx, offset, buf, pages);
>   	if (err)
> 

Re: [RFC PATCH] lib: Introduce generic __cmpxchg_u64() and use it where needed

[Trond Myklebust]

On Wed, 2018-10-31 at 18:18 -0700, Guenter Roeck wrote:
> On 10/31/18 4:32 PM, Paul Burton wrote:
> > (Copying SunRPC & net maintainers.)
> > 
> > Hi Guenter,
> > 
> > On Wed, Oct 31, 2018 at 03:02:53PM -0700, Guenter Roeck wrote:
> > > The alternatives I can see are
> > > - Do not use cmpxchg64() outside architecture code (ie drop its
> > > use from
> > >    the offending driver, and keep doing the same whenever the
> > > problem comes
> > >    up again).
> > > or
> > > - Introduce something like ARCH_HAS_CMPXCHG64 and use it to
> > > determine
> > >    if cmpxchg64 is supported or not.
> > > 
> > > Any preference ?
> > 
> > My preference would be option 1 - avoiding cmpxchg64() where
> > possible in
> > generic code. I wouldn't be opposed to the Kconfig option if there
> > are
> > cases where cmpxchg64() can really help performance though.
> > 
> > The last time I'm aware of this coming up the affected driver was
> > modified to avoid cmpxchg64() [1].
> > 
> > In this particular case I have no idea why
> > net/sunrpc/auth_gss/gss_krb5_seal.c is using cmpxchg64() at all.
> > It's
> > essentially reinventing atomic64_fetch_inc() which is already
> > provided
> > everywhere via CONFIG_GENERIC_ATOMIC64 & the spinlock approach. At
> > least
> > for atomic64_* functions the assumption that all access will be
> > performed using those same functions seems somewhat reasonable.
> > 
> > So how does the below look? Trond?
> > 
> 
> For my part I agree that this would be a much better solution. The
> argument
> that it is not always absolutely guaranteed that atomics don't wrap
> doesn't
> really hold for me because it looks like they all do. On top of that,
> there
> is an explicit atomic_dec_if_positive() and
> atomic_fetch_add_unless(),
> which to me strongly suggests that they _are_ supposed to wrap.
> Given the cost of adding a comparison to each atomic operation to
> prevent it from wrapping, anything else would not really make sense
> to me.

That's a hypothesis, not a proven fact. There are architectures out
there that do not wrap signed integers, hence my question.

> So ... please consider my patch abandoned. Thanks for looking into
> this!
> 
> Guenter
> 
> > Thanks,
> >      Paul
> > 
> > [1] https://patchwork.ozlabs.org/cover/891284/
> > 
> > ---
> > diff --git a/include/linux/sunrpc/gss_krb5.h
> > b/include/linux/sunrpc/gss_krb5.h
> > index 131424cefc6a..02c0412e368c 100644
> > --- a/include/linux/sunrpc/gss_krb5.h
> > +++ b/include/linux/sunrpc/gss_krb5.h
> > @@ -107,8 +107,8 @@ struct krb5_ctx {
> >   	u8			Ksess[GSS_KRB5_MAX_KEYLEN]; /* session key
> > */
> >   	u8			cksum[GSS_KRB5_MAX_KEYLEN];
> >   	s32			endtime;
> > -	u32			seq_send;
> > -	u64			seq_send64;
> > +	atomic_t		seq_send;
> > +	atomic64_t		seq_send64;
> >   	struct xdr_netobj	mech_used;
> >   	u8			initiator_sign[GSS_KRB5_MAX_KEYLEN];
> >   	u8			acceptor_sign[GSS_KRB5_MAX_KEYLEN];
> > @@ -118,9 +118,6 @@ struct krb5_ctx {
> >   	u8			acceptor_integ[GSS_KRB5_MAX_KEYLEN];
> >   };
> >   
> > -extern u32 gss_seq_send_fetch_and_inc(struct krb5_ctx *ctx);
> > -extern u64 gss_seq_send64_fetch_and_inc(struct krb5_ctx *ctx);
> > -
> >   /* The length of the Kerberos GSS token header */
> >   #define GSS_KRB5_TOK_HDR_LEN	(16)
> >   
> > diff --git a/net/sunrpc/auth_gss/gss_krb5_mech.c
> > b/net/sunrpc/auth_gss/gss_krb5_mech.c
> > index 7f0424dfa8f6..eab71fc7af3e 100644
> > --- a/net/sunrpc/auth_gss/gss_krb5_mech.c
> > +++ b/net/sunrpc/auth_gss/gss_krb5_mech.c
> > @@ -274,6 +274,7 @@ get_key(const void *p, const void *end,
> >   static int
> >   gss_import_v1_context(const void *p, const void *end, struct
> > krb5_ctx *ctx)
> >   {
> > +	u32 seq_send;
> >   	int tmp;
> >   
> >   	p = simple_get_bytes(p, end, &ctx->initiate, sizeof(ctx-
> > >initiate));
> > @@ -315,9 +316,10 @@ gss_import_v1_context(const void *p, const
> > void *end, struct krb5_ctx *ctx)
> >   	p = simple_get_bytes(p, end, &ctx->endtime, sizeof(ctx-
> > >endtime));
> >   	if (IS_ERR(p))
> >   		goto out_err;
> > -	p = simple_get_bytes(p, end, &ctx->seq_send, sizeof(ctx-
> > >seq_send));
> > +	p = simple_get_bytes(p, end, &seq_send, sizeof(seq_send));
> >   	if (IS_ERR(p))
> >   		goto out_err;
> > +	atomic_set(&ctx->seq_send, seq_send);
> >   	p = simple_get_netobj(p, end, &ctx->mech_used);
> >   	if (IS_ERR(p))
> >   		goto out_err;
> > @@ -607,6 +609,7 @@ static int
> >   gss_import_v2_context(const void *p, const void *end, struct
> > krb5_ctx *ctx,
> >   		gfp_t gfp_mask)
> >   {
> > +	u64 seq_send64;
> >   	int keylen;
> >   
> >   	p = simple_get_bytes(p, end, &ctx->flags, sizeof(ctx->flags));
> > @@ -617,14 +620,15 @@ gss_import_v2_context(const void *p, const
> > void *end, struct krb5_ctx *ctx,
> >   	p = simple_get_bytes(p, end, &ctx->endtime, sizeof(ctx-
> > >endtime));
> >   	if (IS_ERR(p))
> >   		goto out_err;
> > -	p = simple_get_bytes(p, end, &ctx->seq_send64, sizeof(ctx-
> > >seq_send64));
> > +	p = simple_get_bytes(p, end, &seq_send64, sizeof(seq_send64));
> >   	if (IS_ERR(p))
> >   		goto out_err;
> > +	atomic64_set(&ctx->seq_send64, seq_send64);
> >   	/* set seq_send for use by "older" enctypes */
> > -	ctx->seq_send = ctx->seq_send64;
> > -	if (ctx->seq_send64 != ctx->seq_send) {
> > -		dprintk("%s: seq_send64 %lx, seq_send %x overflow?\n",
> > __func__,
> > -			(unsigned long)ctx->seq_send64, ctx->seq_send);
> > +	atomic_set(&ctx->seq_send, seq_send64);
> > +	if (seq_send64 != atomic_read(&ctx->seq_send)) {
> > +		dprintk("%s: seq_send64 %llx, seq_send %x overflow?\n",
> > __func__,
> > +			seq_send64, atomic_read(&ctx->seq_send));
> >   		p = ERR_PTR(-EINVAL);
> >   		goto out_err;
> >   	}
> > diff --git a/net/sunrpc/auth_gss/gss_krb5_seal.c
> > b/net/sunrpc/auth_gss/gss_krb5_seal.c
> > index b4adeb06660b..48fe4a591b54 100644
> > --- a/net/sunrpc/auth_gss/gss_krb5_seal.c
> > +++ b/net/sunrpc/auth_gss/gss_krb5_seal.c
> > @@ -123,30 +123,6 @@ setup_token_v2(struct krb5_ctx *ctx, struct
> > xdr_netobj *token)
> >   	return krb5_hdr;
> >   }
> >   
> > -u32
> > -gss_seq_send_fetch_and_inc(struct krb5_ctx *ctx)
> > -{
> > -	u32 old, seq_send = READ_ONCE(ctx->seq_send);
> > -
> > -	do {
> > -		old = seq_send;
> > -		seq_send = cmpxchg(&ctx->seq_send, old, old + 1);
> > -	} while (old != seq_send);
> > -	return seq_send;
> > -}
> > -
> > -u64
> > -gss_seq_send64_fetch_and_inc(struct krb5_ctx *ctx)
> > -{
> > -	u64 old, seq_send = READ_ONCE(ctx->seq_send);
> > -
> > -	do {
> > -		old = seq_send;
> > -		seq_send = cmpxchg64(&ctx->seq_send64, old, old + 1);
> > -	} while (old != seq_send);
> > -	return seq_send;
> > -}
> > -
> >   static u32
> >   gss_get_mic_v1(struct krb5_ctx *ctx, struct xdr_buf *text,
> >   		struct xdr_netobj *token)
> > @@ -177,7 +153,7 @@ gss_get_mic_v1(struct krb5_ctx *ctx, struct
> > xdr_buf *text,
> >   
> >   	memcpy(ptr + GSS_KRB5_TOK_HDR_LEN, md5cksum.data,
> > md5cksum.len);
> >   
> > -	seq_send = gss_seq_send_fetch_and_inc(ctx);
> > +	seq_send = atomic_fetch_inc(&ctx->seq_send);
> >   
> >   	if (krb5_make_seq_num(ctx, ctx->seq, ctx->initiate ? 0 : 0xff,
> >   			      seq_send, ptr + GSS_KRB5_TOK_HDR_LEN, ptr
> > + 8))
> > @@ -205,7 +181,7 @@ gss_get_mic_v2(struct krb5_ctx *ctx, struct
> > xdr_buf *text,
> >   
> >   	/* Set up the sequence number. Now 64-bits in clear
> >   	 * text and w/o direction indicator */
> > -	seq_send_be64 = cpu_to_be64(gss_seq_send64_fetch_and_inc(ctx));
> > +	seq_send_be64 = cpu_to_be64(atomic64_fetch_inc(&ctx-
> > >seq_send64));
> >   	memcpy(krb5_hdr + 8, (char *) &seq_send_be64, 8);
> >   
> >   	if (ctx->initiate) {
> > diff --git a/net/sunrpc/auth_gss/gss_krb5_wrap.c
> > b/net/sunrpc/auth_gss/gss_krb5_wrap.c
> > index 962fa84e6db1..5cdde6cb703a 100644
> > --- a/net/sunrpc/auth_gss/gss_krb5_wrap.c
> > +++ b/net/sunrpc/auth_gss/gss_krb5_wrap.c
> > @@ -228,7 +228,7 @@ gss_wrap_kerberos_v1(struct krb5_ctx *kctx, int
> > offset,
> >   
> >   	memcpy(ptr + GSS_KRB5_TOK_HDR_LEN, md5cksum.data,
> > md5cksum.len);
> >   
> > -	seq_send = gss_seq_send_fetch_and_inc(kctx);
> > +	seq_send = atomic_fetch_inc(&kctx->seq_send);
> >   
> >   	/* XXX would probably be more efficient to compute checksum
> >   	 * and encrypt at the same time: */
> > @@ -475,7 +475,7 @@ gss_wrap_kerberos_v2(struct krb5_ctx *kctx, u32
> > offset,
> >   	*be16ptr++ = 0;
> >   
> >   	be64ptr = (__be64 *)be16ptr;
> > -	*be64ptr = cpu_to_be64(gss_seq_send64_fetch_and_inc(kctx));
> > +	*be64ptr = cpu_to_be64(atomic64_fetch_inc(&kctx->seq_send64));
> >   
> >   	err = (*kctx->gk5e->encrypt_v2)(kctx, offset, buf, pages);
> >   	if (err)
> > 
-- 
Trond Myklebust
CTO, Hammerspace Inc
4300 El Camino Real, Suite 105
Los Altos, CA 94022
www.hammer.space

Re: [RFC PATCH] lib: Introduce generic __cmpxchg_u64() and use it where needed

[Guenter Roeck]

On Thu, Nov 01, 2018 at 06:30:08AM +0000, Trond Myklebust wrote:
[ ... ]
> > 
> > For my part I agree that this would be a much better solution. The
> > argument
> > that it is not always absolutely guaranteed that atomics don't wrap
> > doesn't
> > really hold for me because it looks like they all do. On top of that,
> > there
> > is an explicit atomic_dec_if_positive() and
> > atomic_fetch_add_unless(),
> > which to me strongly suggests that they _are_ supposed to wrap.
> > Given the cost of adding a comparison to each atomic operation to
> > prevent it from wrapping, anything else would not really make sense
> > to me.
> 
> That's a hypothesis, not a proven fact. There are architectures out
> there that do not wrap signed integers, hence my question.
> 

If what you say is correct, the kernel is in big trouble on those architectures.
atomic_inc_return() is used all over the place in the kernel with the assumption
that each returned value differs from the previous value (ie the value is used
as cookie, session ID, or for similar purposes).

Guenter