[PATCH 0/7] sunrpc: sparse warning cleanups

[PATCH 0/7] sunrpc: sparse warning cleanups

[Jeff Layton]

This fixes up all of the sparse warnings that I see when building
net/sunrpc. The only real substantive change is the second patch which
should fix the RCU handling for the gc_ctx field. That looks quite wrong
right now, though it may be that the refcounting and lifecycle of the
thing helps paper over it today.

There are still a few warnings that come from generic ipv6.h inlines.
I'll send a separate patch to the netdev folks to address those.

Trond, if these look OK then they should probably go via your tree as it
looks like you've already merged the gc_acceptor patches.

Jeff Layton (7):
  sunrpc: remove __rcu annotation from struct gss_cl_ctx->gc_gss_ctx
  sunrpc: fix RCU handling of gc_ctx field
  sunrpc: clean up endianness warnings in setup_token
  sunrpc: xdr_get_next_encode_buffer can be static
  sunrpc: clean up sparse endianness warnings in gss_krb5_seal.c
  sunrpc: clean up sparse endianness warnings in gss_krb5_wrap.c
  sunrpc: remove "ec" argument from encrypt_v2 operation

 include/linux/sunrpc/auth_gss.h       |  2 +-
 include/linux/sunrpc/gss_krb5.h       |  4 +--
 net/sunrpc/auth_gss/auth_gss.c        | 52 +++++++++++++++++++++++------------
 net/sunrpc/auth_gss/gss_krb5_crypto.c |  9 ++----
 net/sunrpc/auth_gss/gss_krb5_seal.c   | 16 +++++------
 net/sunrpc/auth_gss/gss_krb5_wrap.c   | 10 +++----
 net/sunrpc/xdr.c                      |  3 +-
 7 files changed, 55 insertions(+), 41 deletions(-)

-- 
1.9.3

[PATCH 1/7] sunrpc: remove __rcu annotation from struct gss_cl_ctx->gc_gss_ctx

[Jeff Layton]

Commit 5b22216e11f7 (nfs: __rcu annotations) added a __rcu annotation to
the gc_gss_ctx field. I see no rationale for adding that though, as that
field does not seem to be managed via RCU at all.

Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Paul McKenney <paulmck@linux.vnet.ibm.com>
Signed-off-by: Jeff Layton <jlayton@primarydata.com>
---
 include/linux/sunrpc/auth_gss.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/sunrpc/auth_gss.h b/include/linux/sunrpc/auth_gss.h
index cbc6875fb9cf..36eebc451b41 100644
--- a/include/linux/sunrpc/auth_gss.h
+++ b/include/linux/sunrpc/auth_gss.h
@@ -69,7 +69,7 @@ struct gss_cl_ctx {
 	enum rpc_gss_proc	gc_proc;
 	u32			gc_seq;
 	spinlock_t		gc_seq_lock;
-	struct gss_ctx __rcu	*gc_gss_ctx;
+	struct gss_ctx		*gc_gss_ctx;
 	struct xdr_netobj	gc_wire_ctx;
 	struct xdr_netobj	gc_acceptor;
 	u32			gc_win;
-- 
1.9.3

[PATCH 2/7] sunrpc: fix RCU handling of gc_ctx field

[Jeff Layton]

The handling of the gc_ctx pointer only seems to be partially RCU-safe.
The assignment and freeing are done using RCU, but many places in the
code seem to dereference that pointer without proper RCU safeguards.

Fix them to use rcu_dereference and to rcu_read_lock/unlock, and to
properly handle the case where the pointer is NULL.

Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Paul McKenney <paulmck@linux.vnet.ibm.com>
Signed-off-by: Jeff Layton <jlayton@primarydata.com>
---
 net/sunrpc/auth_gss/auth_gss.c | 52 ++++++++++++++++++++++++++++--------------
 1 file changed, 35 insertions(+), 17 deletions(-)

diff --git a/net/sunrpc/auth_gss/auth_gss.c b/net/sunrpc/auth_gss/auth_gss.c
index 73854314fb85..afb292cd797d 100644
--- a/net/sunrpc/auth_gss/auth_gss.c
+++ b/net/sunrpc/auth_gss/auth_gss.c
@@ -183,8 +183,9 @@ gss_cred_get_ctx(struct rpc_cred *cred)
 	struct gss_cl_ctx *ctx = NULL;
 
 	rcu_read_lock();
-	if (gss_cred->gc_ctx)
-		ctx = gss_get_ctx(gss_cred->gc_ctx);
+	ctx = rcu_dereference(gss_cred->gc_ctx);
+	if (ctx)
+		gss_get_ctx(ctx);
 	rcu_read_unlock();
 	return ctx;
 }
@@ -1207,13 +1208,13 @@ gss_destroying_context(struct rpc_cred *cred)
 {
 	struct gss_cred *gss_cred = container_of(cred, struct gss_cred, gc_base);
 	struct gss_auth *gss_auth = container_of(cred->cr_auth, struct gss_auth, rpc_auth);
+	struct gss_cl_ctx *ctx = rcu_dereference_protected(gss_cred->gc_ctx, 1);
 	struct rpc_task *task;
 
-	if (gss_cred->gc_ctx == NULL ||
-	    test_bit(RPCAUTH_CRED_UPTODATE, &cred->cr_flags) == 0)
+	if (test_bit(RPCAUTH_CRED_UPTODATE, &cred->cr_flags) == 0)
 		return 0;
 
-	gss_cred->gc_ctx->gc_proc = RPC_GSS_PROC_DESTROY;
+	ctx->gc_proc = RPC_GSS_PROC_DESTROY;
 	cred->cr_ops = &gss_nullops;
 
 	/* Take a reference to ensure the cred will be destroyed either
@@ -1274,7 +1275,7 @@ gss_destroy_nullcred(struct rpc_cred *cred)
 {
 	struct gss_cred *gss_cred = container_of(cred, struct gss_cred, gc_base);
 	struct gss_auth *gss_auth = container_of(cred->cr_auth, struct gss_auth, rpc_auth);
-	struct gss_cl_ctx *ctx = gss_cred->gc_ctx;
+	struct gss_cl_ctx *ctx = rcu_dereference_protected(gss_cred->gc_ctx, 1);
 
 	RCU_INIT_POINTER(gss_cred->gc_ctx, NULL);
 	call_rcu(&cred->cr_rcu, gss_free_cred_callback);
@@ -1349,20 +1350,30 @@ gss_cred_init(struct rpc_auth *auth, struct rpc_cred *cred)
 static char *
 gss_stringify_acceptor(struct rpc_cred *cred)
 {
-	char *string;
+	char *string = NULL;
 	struct gss_cred *gss_cred = container_of(cred, struct gss_cred, gc_base);
-	struct xdr_netobj *acceptor = &gss_cred->gc_ctx->gc_acceptor;
+	struct gss_cl_ctx *ctx;
+	struct xdr_netobj *acceptor;
+
+	rcu_read_lock();
+	ctx = rcu_dereference(gss_cred->gc_ctx);
+	if (!ctx)
+		goto out;
+
+	acceptor = &ctx->gc_acceptor;
 
 	/* no point if there's no string */
 	if (!acceptor->len)
-		return NULL;
+		goto out;
 
 	string = kmalloc(acceptor->len + 1, GFP_KERNEL);
 	if (!string)
-		return string;
+		goto out;
 
 	memcpy(string, acceptor->data, acceptor->len);
 	string[acceptor->len] = '\0';
+out:
+	rcu_read_unlock();
 	return string;
 }
 
@@ -1374,15 +1385,16 @@ static int
 gss_key_timeout(struct rpc_cred *rc)
 {
 	struct gss_cred *gss_cred = container_of(rc, struct gss_cred, gc_base);
+	struct gss_cl_ctx *ctx;
 	unsigned long now = jiffies;
 	unsigned long expire;
 
-	if (gss_cred->gc_ctx == NULL)
-		return -EACCES;
-
-	expire = gss_cred->gc_ctx->gc_expiry - (gss_key_expire_timeo * HZ);
-
-	if (time_after(now, expire))
+	rcu_read_lock();
+	ctx = rcu_dereference(gss_cred->gc_ctx);
+	if (ctx)
+		expire = ctx->gc_expiry - (gss_key_expire_timeo * HZ);
+	rcu_read_unlock();
+	if (!ctx || time_after(now, expire))
 		return -EACCES;
 	return 0;
 }
@@ -1391,13 +1403,19 @@ static int
 gss_match(struct auth_cred *acred, struct rpc_cred *rc, int flags)
 {
 	struct gss_cred *gss_cred = container_of(rc, struct gss_cred, gc_base);
+	struct gss_cl_ctx *ctx;
 	int ret;
 
 	if (test_bit(RPCAUTH_CRED_NEW, &rc->cr_flags))
 		goto out;
 	/* Don't match with creds that have expired. */
-	if (time_after(jiffies, gss_cred->gc_ctx->gc_expiry))
+	rcu_read_lock();
+	ctx = rcu_dereference(gss_cred->gc_ctx);
+	if (!ctx || time_after(jiffies, ctx->gc_expiry)) {
+		rcu_read_unlock();
 		return 0;
+	}
+	rcu_read_unlock();
 	if (!test_bit(RPCAUTH_CRED_UPTODATE, &rc->cr_flags))
 		return 0;
 out:
-- 
1.9.3

[PATCH 3/7] sunrpc: clean up endianness warnings in setup_token

[Jeff Layton]

This is safe since KG_TOK_MIC_MSG and SEAL_ALG_NONE are both endian
palindromes. I'm also making the assumption that the signalg field
really should be in little-endian. That looks odd, but looking at the
spec I guess it's correct.

Signed-off-by: Jeff Layton <jlayton@primarydata.com>
---
 net/sunrpc/auth_gss/gss_krb5_seal.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/sunrpc/auth_gss/gss_krb5_seal.c b/net/sunrpc/auth_gss/gss_krb5_seal.c
index 62ae3273186c..94ad57ff7169 100644
--- a/net/sunrpc/auth_gss/gss_krb5_seal.c
+++ b/net/sunrpc/auth_gss/gss_krb5_seal.c
@@ -83,10 +83,10 @@ setup_token(struct krb5_ctx *ctx, struct xdr_netobj *token)
 
 	/* ptr now at start of header described in rfc 1964, section 1.2.1: */
 	krb5_hdr = ptr;
-	*ptr++ = KG_TOK_MIC_MSG;
-	*ptr++ = cpu_to_le16(ctx->gk5e->signalg);
-	*ptr++ = SEAL_ALG_NONE;
-	*ptr++ = 0xffff;
+	*ptr++ = cpu_to_be16(KG_TOK_MIC_MSG);
+	*ptr++ = (__force __be16)cpu_to_le16(ctx->gk5e->signalg);
+	*ptr++ = cpu_to_be16(SEAL_ALG_NONE);
+	*ptr++ = cpu_to_be16(0xffff);
 
 	return (char *)krb5_hdr;
 }
-- 
1.9.3

[PATCH 4/7] sunrpc: xdr_get_next_encode_buffer can be static

[Jeff Layton]

Signed-off-by: Jeff Layton <jlayton@primarydata.com>
---
 net/sunrpc/xdr.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
index 23fb4e75e245..13b7dd0d38e6 100644
--- a/net/sunrpc/xdr.c
+++ b/net/sunrpc/xdr.c
@@ -509,7 +509,8 @@ void xdr_commit_encode(struct xdr_stream *xdr)
 }
 EXPORT_SYMBOL_GPL(xdr_commit_encode);
 
-__be32 *xdr_get_next_encode_buffer(struct xdr_stream *xdr, size_t nbytes)
+static __be32 *
+xdr_get_next_encode_buffer(struct xdr_stream *xdr, size_t nbytes)
 {
 	static __be32 *p;
 	int space_left;
-- 
1.9.3

[PATCH 5/7] sunrpc: clean up sparse endianness warnings in gss_krb5_seal.c

[Jeff Layton]

KG2_TOK_MIC is a "palindrome" so byte swapping is a no-op here.

Signed-off-by: Jeff Layton <jlayton@primarydata.com>
---
 net/sunrpc/auth_gss/gss_krb5_seal.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/sunrpc/auth_gss/gss_krb5_seal.c b/net/sunrpc/auth_gss/gss_krb5_seal.c
index 94ad57ff7169..3cf49ea54ca9 100644
--- a/net/sunrpc/auth_gss/gss_krb5_seal.c
+++ b/net/sunrpc/auth_gss/gss_krb5_seal.c
@@ -106,13 +106,13 @@ setup_token_v2(struct krb5_ctx *ctx, struct xdr_netobj *token)
 	 * just start the token */
 	krb5_hdr = ptr = (__be16 *)token->data;
 
-	*ptr++ = KG2_TOK_MIC;
+	*ptr++ = cpu_to_be16(KG2_TOK_MIC);
 	p = (u8 *)ptr;
 	*p++ = flags;
 	*p++ = 0xff;
 	ptr = (__be16 *)p;
-	*ptr++ = 0xffff;
-	*ptr++ = 0xffff;
+	*ptr++ = cpu_to_be16(0xffff);
+	*ptr++ = cpu_to_be16(0xffff);
 
 	token->len = GSS_KRB5_TOK_HDR_LEN + ctx->gk5e->cksumlength;
 	return krb5_hdr;
@@ -181,7 +181,7 @@ gss_get_mic_v2(struct krb5_ctx *ctx, struct xdr_buf *text,
 	spin_lock(&krb5_seq_lock);
 	seq_send = ctx->seq_send64++;
 	spin_unlock(&krb5_seq_lock);
-	*((u64 *)(krb5_hdr + 8)) = cpu_to_be64(seq_send);
+	*((__be64 *)(krb5_hdr + 8)) = cpu_to_be64(seq_send);
 
 	if (ctx->initiate) {
 		cksumkey = ctx->initiator_sign;
-- 
1.9.3

[PATCH 6/7] sunrpc: clean up sparse endianness warnings in gss_krb5_wrap.c

[Jeff Layton]

Signed-off-by: Jeff Layton <jlayton@primarydata.com>
---
 net/sunrpc/auth_gss/gss_krb5_wrap.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/net/sunrpc/auth_gss/gss_krb5_wrap.c b/net/sunrpc/auth_gss/gss_krb5_wrap.c
index 42560e55d978..c5cc0270a334 100644
--- a/net/sunrpc/auth_gss/gss_krb5_wrap.c
+++ b/net/sunrpc/auth_gss/gss_krb5_wrap.c
@@ -201,9 +201,9 @@ gss_wrap_kerberos_v1(struct krb5_ctx *kctx, int offset,
 
 	msg_start = ptr + GSS_KRB5_TOK_HDR_LEN + kctx->gk5e->cksumlength;
 
-	*(__be16 *)(ptr + 2) = cpu_to_le16(kctx->gk5e->signalg);
+	*(__be16 *)(ptr + 2) = (__force __be16)cpu_to_le16(kctx->gk5e->signalg);
 	memset(ptr + 4, 0xff, 4);
-	*(__be16 *)(ptr + 4) = cpu_to_le16(kctx->gk5e->sealalg);
+	*(__be16 *)(ptr + 4) = (__force __be16)cpu_to_le16(kctx->gk5e->sealalg);
 
 	gss_krb5_make_confounder(msg_start, conflen);
 
@@ -438,7 +438,7 @@ gss_wrap_kerberos_v2(struct krb5_ctx *kctx, u32 offset,
 	u8		*ptr, *plainhdr;
 	s32		now;
 	u8		flags = 0x00;
-	__be16		*be16ptr, ec = 0;
+	__be16		*be16ptr;
 	__be64		*be64ptr;
 	u32		err;
 
@@ -468,7 +468,7 @@ gss_wrap_kerberos_v2(struct krb5_ctx *kctx, u32 offset,
 	be16ptr = (__be16 *)ptr;
 
 	blocksize = crypto_blkcipher_blocksize(kctx->acceptor_enc);
-	*be16ptr++ = cpu_to_be16(ec);
+	*be16ptr++ = cpu_to_be16(0);
 	/* "inner" token header always uses 0 for RRC */
 	*be16ptr++ = cpu_to_be16(0);
 
@@ -477,7 +477,7 @@ gss_wrap_kerberos_v2(struct krb5_ctx *kctx, u32 offset,
 	*be64ptr = cpu_to_be64(kctx->seq_send64++);
 	spin_unlock(&krb5_seq_lock);
 
-	err = (*kctx->gk5e->encrypt_v2)(kctx, offset, buf, ec, pages);
+	err = (*kctx->gk5e->encrypt_v2)(kctx, offset, buf, 0, pages);
 	if (err)
 		return err;
 
-- 
1.9.3

[PATCH 7/7] sunrpc: remove "ec" argument from encrypt_v2 operation

[Jeff Layton]

It's always 0.

Signed-off-by: Jeff Layton <jlayton@primarydata.com>
---
 include/linux/sunrpc/gss_krb5.h       | 4 ++--
 net/sunrpc/auth_gss/gss_krb5_crypto.c | 9 ++-------
 net/sunrpc/auth_gss/gss_krb5_wrap.c   | 2 +-
 3 files changed, 5 insertions(+), 10 deletions(-)

diff --git a/include/linux/sunrpc/gss_krb5.h b/include/linux/sunrpc/gss_krb5.h
index 5af2931cf58d..df02a4188487 100644
--- a/include/linux/sunrpc/gss_krb5.h
+++ b/include/linux/sunrpc/gss_krb5.h
@@ -81,7 +81,7 @@ struct gss_krb5_enctype {
 		       struct xdr_netobj *in,
 		       struct xdr_netobj *out);	/* complete key generation */
 	u32 (*encrypt_v2) (struct krb5_ctx *kctx, u32 offset,
-			   struct xdr_buf *buf, int ec,
+			   struct xdr_buf *buf,
 			   struct page **pages); /* v2 encryption function */
 	u32 (*decrypt_v2) (struct krb5_ctx *kctx, u32 offset,
 			   struct xdr_buf *buf, u32 *headskip,
@@ -310,7 +310,7 @@ gss_krb5_aes_make_key(const struct gss_krb5_enctype *gk5e,
 
 u32
 gss_krb5_aes_encrypt(struct krb5_ctx *kctx, u32 offset,
-		     struct xdr_buf *buf, int ec,
+		     struct xdr_buf *buf,
 		     struct page **pages);
 
 u32
diff --git a/net/sunrpc/auth_gss/gss_krb5_crypto.c b/net/sunrpc/auth_gss/gss_krb5_crypto.c
index 0f43e894bc0a..f5ed9f6ece06 100644
--- a/net/sunrpc/auth_gss/gss_krb5_crypto.c
+++ b/net/sunrpc/auth_gss/gss_krb5_crypto.c
@@ -641,7 +641,7 @@ out:
 
 u32
 gss_krb5_aes_encrypt(struct krb5_ctx *kctx, u32 offset,
-		     struct xdr_buf *buf, int ec, struct page **pages)
+		     struct xdr_buf *buf, struct page **pages)
 {
 	u32 err;
 	struct xdr_netobj hmac;
@@ -684,13 +684,8 @@ gss_krb5_aes_encrypt(struct krb5_ctx *kctx, u32 offset,
 		ecptr = buf->tail[0].iov_base;
 	}
 
-	memset(ecptr, 'X', ec);
-	buf->tail[0].iov_len += ec;
-	buf->len += ec;
-
 	/* copy plaintext gss token header after filler (if any) */
-	memcpy(ecptr + ec, buf->head[0].iov_base + offset,
-						GSS_KRB5_TOK_HDR_LEN);
+	memcpy(ecptr, buf->head[0].iov_base + offset, GSS_KRB5_TOK_HDR_LEN);
 	buf->tail[0].iov_len += GSS_KRB5_TOK_HDR_LEN;
 	buf->len += GSS_KRB5_TOK_HDR_LEN;
 
diff --git a/net/sunrpc/auth_gss/gss_krb5_wrap.c b/net/sunrpc/auth_gss/gss_krb5_wrap.c
index c5cc0270a334..ccc10321d87d 100644
--- a/net/sunrpc/auth_gss/gss_krb5_wrap.c
+++ b/net/sunrpc/auth_gss/gss_krb5_wrap.c
@@ -477,7 +477,7 @@ gss_wrap_kerberos_v2(struct krb5_ctx *kctx, u32 offset,
 	*be64ptr = cpu_to_be64(kctx->seq_send64++);
 	spin_unlock(&krb5_seq_lock);
 
-	err = (*kctx->gk5e->encrypt_v2)(kctx, offset, buf, 0, pages);
+	err = (*kctx->gk5e->encrypt_v2)(kctx, offset, buf, pages);
 	if (err)
 		return err;
 
-- 
1.9.3

Re: [PATCH 1/7] sunrpc: remove __rcu annotation from struct gss_cl_ctx->gc_gss_ctx

[Arnd Bergmann]

On Sunday 13 July 2014 21:57:38 Jeff Layton wrote:
> 
> Commit 5b22216e11f7 (nfs: __rcu annotations) added a __rcu annotation to
> the gc_gss_ctx field. I see no rationale for adding that though, as that
> field does not seem to be managed via RCU at all.
> 
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Paul McKenney <paulmck@linux.vnet.ibm.com>
> Signed-off-by: Jeff Layton <jlayton@primarydata.com>

Unfortunately, it's been too long ago for me to remember what led to me
adding it. I also don't see a reason for it in today's code, but I don't
know if the code has changed, if I made a mistake then, or if it's actually
needed for some reason I don't see.

	Arnd

Re: [PATCH 1/7] sunrpc: remove __rcu annotation from struct gss_cl_ctx->gc_gss_ctx

[Jeff Layton]

On Mon, 14 Jul 2014 16:23:53 +0200
Arnd Bergmann <arnd@arndb.de> wrote:

> On Sunday 13 July 2014 21:57:38 Jeff Layton wrote:
> > 
> > Commit 5b22216e11f7 (nfs: __rcu annotations) added a __rcu annotation to
> > the gc_gss_ctx field. I see no rationale for adding that though, as that
> > field does not seem to be managed via RCU at all.
> > 
> > Cc: Arnd Bergmann <arnd@arndb.de>
> > Cc: Paul McKenney <paulmck@linux.vnet.ibm.com>
> > Signed-off-by: Jeff Layton <jlayton@primarydata.com>
> 
> Unfortunately, it's been too long ago for me to remember what led to me
> adding it. I also don't see a reason for it in today's code, but I don't
> know if the code has changed, if I made a mistake then, or if it's actually
> needed for some reason I don't see.
> 

Darn, I was hoping you would remember... ;)

I don't think the code that manipulates this field has changed
substantially since then, but I'm far from certain either. This field
does live in a structure that is RCU managed, so maybe that was part of
the rationale?

In any case, we're just removing the annotation here so this shouldn't
materially harm anything, AFAICT.

-- 
Jeff Layton <jlayton@primarydata.com>

Re: [PATCH 1/7] sunrpc: remove __rcu annotation from struct gss_cl_ctx->gc_gss_ctx

[Arnd Bergmann]

On Monday 14 July 2014 11:02:02 Jeff Layton wrote:
> On Mon, 14 Jul 2014 16:23:53 +0200
> Arnd Bergmann <arnd@arndb.de> wrote:
> 
> > On Sunday 13 July 2014 21:57:38 Jeff Layton wrote:
> > > 
> > > Commit 5b22216e11f7 (nfs: __rcu annotations) added a __rcu annotation to
> > > the gc_gss_ctx field. I see no rationale for adding that though, as that
> > > field does not seem to be managed via RCU at all.
> > > 
> > > Cc: Arnd Bergmann <arnd@arndb.de>
> > > Cc: Paul McKenney <paulmck@linux.vnet.ibm.com>
> > > Signed-off-by: Jeff Layton <jlayton@primarydata.com>
> > 
> > Unfortunately, it's been too long ago for me to remember what led to me
> > adding it. I also don't see a reason for it in today's code, but I don't
> > know if the code has changed, if I made a mistake then, or if it's actually
> > needed for some reason I don't see.
> > 
> 
> Darn, I was hoping you would remember... 
> 
> I don't think the code that manipulates this field has changed
> substantially since then, but I'm far from certain either. This field
> does live in a structure that is RCU managed, so maybe that was part of
> the rationale?
> 
> In any case, we're just removing the annotation here so this shouldn't
> materially harm anything, AFAICT.
> 

All the annotations I did were the results of building with sparse and
looking at the warnings. If you see fewer warnings after your patch
than before, than it's moving into the right direction ;-)

	Arnd

Re: [PATCH 4/7] sunrpc: xdr_get_next_encode_buffer can be static

[Christoph Hellwig]

Trond just sent a patch for this as well..

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 5/7] sunrpc: clean up sparse endianness warnings in gss_krb5_seal.c

[Christoph Hellwig]

On Sun, Jul 13, 2014 at 09:57:42PM -0400, Jeff Layton wrote:
> KG2_TOK_MIC is a "palindrome" so byte swapping is a no-op here.

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 6/7] sunrpc: clean up sparse endianness warnings in gss_krb5_wrap.c

[Christoph Hellwig]

On Sun, Jul 13, 2014 at 09:57:43PM -0400, Jeff Layton wrote:
> Signed-off-by: Jeff Layton <jlayton@primarydata.com>
> ---
>  net/sunrpc/auth_gss/gss_krb5_wrap.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/net/sunrpc/auth_gss/gss_krb5_wrap.c b/net/sunrpc/auth_gss/gss_krb5_wrap.c
> index 42560e55d978..c5cc0270a334 100644
> --- a/net/sunrpc/auth_gss/gss_krb5_wrap.c
> +++ b/net/sunrpc/auth_gss/gss_krb5_wrap.c
> @@ -201,9 +201,9 @@ gss_wrap_kerberos_v1(struct krb5_ctx *kctx, int offset,
>  
>  	msg_start = ptr + GSS_KRB5_TOK_HDR_LEN + kctx->gk5e->cksumlength;
>  
> +	*(__be16 *)(ptr + 2) = (__force __be16)cpu_to_le16(kctx->gk5e->signalg);

This looks silly.  This should be:

	*(__le16 *)(ptr + 2) = cpu_to_le16(kctx->gk5e->signalg);

Maybe with a comment somewhere explaining why we're doing little endian
encoding here if it's not obvious from the surrounding code.

Re: [PATCH 7/7] sunrpc: remove "ec" argument from encrypt_v2 operation

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

I wonder what hat non-trivial memset and add to buffer code was doing
before it bitrotted, though..

Re: [PATCH 3/7] sunrpc: clean up endianness warnings in setup_token

[Christoph Hellwig]

On Sun, Jul 13, 2014 at 09:57:40PM -0400, Jeff Layton wrote:
> This is safe since KG_TOK_MIC_MSG and SEAL_ALG_NONE are both endian
> palindromes. I'm also making the assumption that the signalg field
> really should be in little-endian. That looks odd, but looking at the
> spec I guess it's correct.

> @@ -83,10 +83,10 @@ setup_token(struct krb5_ctx *ctx, struct xdr_netobj *token)
>  
>  	/* ptr now at start of header described in rfc 1964, section 1.2.1: */
>  	krb5_hdr = ptr;
> +	*ptr++ = cpu_to_be16(KG_TOK_MIC_MSG);
> +	*ptr++ = (__force __be16)cpu_to_le16(ctx->gk5e->signalg);
> +	*ptr++ = cpu_to_be16(SEAL_ALG_NONE);
> +	*ptr++ = cpu_to_be16(0xffff);

If signalg is little endian and all others are palindromes it seems
like ptr should be defines as __le16.

>  	return (char *)krb5_hdr;

Seems like krb5_hdr should simply be defined as void *, together with
the return value?

Re: [PATCH 4/7] sunrpc: xdr_get_next_encode_buffer can be static

[Jeff Layton]

On Tue, 15 Jul 2014 10:02:48 -0700
Christoph Hellwig <hch@infradead.org> wrote:

> Trond just sent a patch for this as well..
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>

Ok, cool. I'll drop this one then.

Thanks,
-- 
Jeff Layton <jlayton@primarydata.com>

Re: [PATCH 3/7] sunrpc: clean up endianness warnings in setup_token

[Jeff Layton]

On Tue, 15 Jul 2014 10:08:06 -0700
Christoph Hellwig <hch@infradead.org> wrote:

> On Sun, Jul 13, 2014 at 09:57:40PM -0400, Jeff Layton wrote:
> > This is safe since KG_TOK_MIC_MSG and SEAL_ALG_NONE are both endian
> > palindromes. I'm also making the assumption that the signalg field
> > really should be in little-endian. That looks odd, but looking at the
> > spec I guess it's correct.
> 
> > @@ -83,10 +83,10 @@ setup_token(struct krb5_ctx *ctx, struct xdr_netobj *token)
> >  
> >  	/* ptr now at start of header described in rfc 1964, section 1.2.1: */
> >  	krb5_hdr = ptr;
> > +	*ptr++ = cpu_to_be16(KG_TOK_MIC_MSG);
> > +	*ptr++ = (__force __be16)cpu_to_le16(ctx->gk5e->signalg);
> > +	*ptr++ = cpu_to_be16(SEAL_ALG_NONE);
> > +	*ptr++ = cpu_to_be16(0xffff);
> 
> If signalg is little endian and all others are palindromes it seems
> like ptr should be defines as __le16.
> 

The spec doesn't really define it as little-endian. It's just an opaque series of bytes that happens to be a little-endian representation of 


> >  	return (char *)krb5_hdr;
> 
> Seems like krb5_hdr should simply be defined as void *, together with
> the return value?
> 

Yeah, that may make more sense.

-- 
Jeff Layton <jlayton@primarydata.com>

Re: [PATCH 7/7] sunrpc: remove "ec" argument from encrypt_v2 operation

[Jeff Layton]

On Tue, 15 Jul 2014 10:05:41 -0700
Christoph Hellwig <hch@infradead.org> wrote:

> Looks good,
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> 
> I wonder what hat non-trivial memset and add to buffer code was doing
> before it bitrotted, though..

AFAICT, it was never used. I guess you could (in principle) have
callers that did pass a non-zero value there, we just never did in the
kernel. I suspect Kevin just added it in there for completeness sake
when he did the implementation initially.

-- 
Jeff Layton <jlayton@primarydata.com>

Re: [PATCH 6/7] sunrpc: clean up sparse endianness warnings in gss_krb5_wrap.c

[Jeff Layton]

On Tue, 15 Jul 2014 10:04:42 -0700
Christoph Hellwig <hch@infradead.org> wrote:

> On Sun, Jul 13, 2014 at 09:57:43PM -0400, Jeff Layton wrote:
> > Signed-off-by: Jeff Layton <jlayton@primarydata.com>
> > ---
> >  net/sunrpc/auth_gss/gss_krb5_wrap.c | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/net/sunrpc/auth_gss/gss_krb5_wrap.c b/net/sunrpc/auth_gss/gss_krb5_wrap.c
> > index 42560e55d978..c5cc0270a334 100644
> > --- a/net/sunrpc/auth_gss/gss_krb5_wrap.c
> > +++ b/net/sunrpc/auth_gss/gss_krb5_wrap.c
> > @@ -201,9 +201,9 @@ gss_wrap_kerberos_v1(struct krb5_ctx *kctx, int offset,
> >  
> >  	msg_start = ptr + GSS_KRB5_TOK_HDR_LEN + kctx->gk5e->cksumlength;
> >  
> > +	*(__be16 *)(ptr + 2) = (__force __be16)cpu_to_le16(kctx->gk5e->signalg);
> 
> This looks silly.  This should be:
> 
> 	*(__le16 *)(ptr + 2) = cpu_to_le16(kctx->gk5e->signalg);
> 
> Maybe with a comment somewhere explaining why we're doing little endian
> encoding here if it's not obvious from the surrounding code.
> 

The spec doesn't really define these as little-endian values, it's just
an opaque series of bytes that the kernel implementation happens to
handle as little-endian (see RFC 1964, section 1.2.1). Maybe I should
reverse the bytes and we can just make that cpu_to_be16 instead?

So the code is correct, AFAICT -- it's just odd...

-- 
Jeff Layton <jlayton@primarydata.com>

Re: [PATCH 6/7] sunrpc: clean up sparse endianness warnings in gss_krb5_wrap.c

[Christoph Hellwig]

On Tue, Jul 15, 2014 at 01:36:14PM -0400, Jeff Layton wrote:
> > >  
> > > +	*(__be16 *)(ptr + 2) = (__force __be16)cpu_to_le16(kctx->gk5e->signalg);
> > 
> > This looks silly.  This should be:
> > 
> > 	*(__le16 *)(ptr + 2) = cpu_to_le16(kctx->gk5e->signalg);
> > 
> > Maybe with a comment somewhere explaining why we're doing little endian
> > encoding here if it's not obvious from the surrounding code.
> > 
> 
> The spec doesn't really define these as little-endian values, it's just
> an opaque series of bytes that the kernel implementation happens to
> handle as little-endian (see RFC 1964, section 1.2.1). Maybe I should
> reverse the bytes and we can just make that cpu_to_be16 instead?

Sounds okay to me.

> So the code is correct, AFAICT -- it's just odd...

It might be correct, but with the added cast it's simply too ugly to
live.

[PATCH v2 0/5] sunrpc: sparse warning cleanups

[Jeff Layton]

v2:
- reworked the gss_krb5_seal.c and gss_krb5_wrap.c patches to be less
  ugly
- squashed the setup_token patch into the gss_krb5_seal.c patch
- dropped patch to make xdr_get_next_encode_buffer static since Trond
  proposed the same fix

This fixes up all of the sparse warnings that I see when building
net/sunrpc. The only real substantive change is the second patch which
should fix the RCU handling for the gc_ctx field. That looks quite wrong
right now, though it may be that the refcounting and lifecycle of the
thing helps paper over it today.

There are still a few warnings that come from generic ipv6.h inlines.
I'll send a separate patch to the netdev folks to address those.

Trond, if these look OK then they should probably go via your tree as it
looks like you've already merged the gc_acceptor patches and the RCU
fixes in patch #2 should apply on top of those.

Jeff Layton (5):
  sunrpc: remove __rcu annotation from struct gss_cl_ctx->gc_gss_ctx
  sunrpc: fix RCU handling of gc_ctx field
  sunrpc: clean up sparse endianness warnings in gss_krb5_seal.c
  sunrpc: clean up sparse endianness warnings in gss_krb5_wrap.c
  sunrpc: remove "ec" argument from encrypt_v2 operation

 include/linux/sunrpc/auth_gss.h       |  2 +-
 include/linux/sunrpc/gss_krb5.h       |  4 +--
 net/sunrpc/auth_gss/auth_gss.c        | 52 +++++++++++++++++++++++------------
 net/sunrpc/auth_gss/gss_krb5_crypto.c |  9 ++----
 net/sunrpc/auth_gss/gss_krb5_seal.c   | 28 +++++++++++--------
 net/sunrpc/auth_gss/gss_krb5_wrap.c   | 20 +++++++++-----
 6 files changed, 70 insertions(+), 45 deletions(-)

-- 
1.9.3

[PATCH v2 1/5] sunrpc: remove __rcu annotation from struct gss_cl_ctx->gc_gss_ctx

[Jeff Layton]

Commit 5b22216e11f7 (nfs: __rcu annotations) added a __rcu annotation to
the gc_gss_ctx field. I see no rationale for adding that though, as that
field does not seem to be managed via RCU at all.

Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Paul McKenney <paulmck@linux.vnet.ibm.com>
Signed-off-by: Jeff Layton <jlayton@primarydata.com>
---
 include/linux/sunrpc/auth_gss.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/sunrpc/auth_gss.h b/include/linux/sunrpc/auth_gss.h
index cbc6875fb9cf..36eebc451b41 100644
--- a/include/linux/sunrpc/auth_gss.h
+++ b/include/linux/sunrpc/auth_gss.h
@@ -69,7 +69,7 @@ struct gss_cl_ctx {
 	enum rpc_gss_proc	gc_proc;
 	u32			gc_seq;
 	spinlock_t		gc_seq_lock;
-	struct gss_ctx __rcu	*gc_gss_ctx;
+	struct gss_ctx		*gc_gss_ctx;
 	struct xdr_netobj	gc_wire_ctx;
 	struct xdr_netobj	gc_acceptor;
 	u32			gc_win;
-- 
1.9.3

[PATCH v2 2/5] sunrpc: fix RCU handling of gc_ctx field

[Jeff Layton]

The handling of the gc_ctx pointer only seems to be partially RCU-safe.
The assignment and freeing are done using RCU, but many places in the
code seem to dereference that pointer without proper RCU safeguards.

Fix them to use rcu_dereference and to rcu_read_lock/unlock, and to
properly handle the case where the pointer is NULL.

Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Paul McKenney <paulmck@linux.vnet.ibm.com>
Signed-off-by: Jeff Layton <jlayton@primarydata.com>
---
 net/sunrpc/auth_gss/auth_gss.c | 52 ++++++++++++++++++++++++++++--------------
 1 file changed, 35 insertions(+), 17 deletions(-)

diff --git a/net/sunrpc/auth_gss/auth_gss.c b/net/sunrpc/auth_gss/auth_gss.c
index 73854314fb85..afb292cd797d 100644
--- a/net/sunrpc/auth_gss/auth_gss.c
+++ b/net/sunrpc/auth_gss/auth_gss.c
@@ -183,8 +183,9 @@ gss_cred_get_ctx(struct rpc_cred *cred)
 	struct gss_cl_ctx *ctx = NULL;
 
 	rcu_read_lock();
-	if (gss_cred->gc_ctx)
-		ctx = gss_get_ctx(gss_cred->gc_ctx);
+	ctx = rcu_dereference(gss_cred->gc_ctx);
+	if (ctx)
+		gss_get_ctx(ctx);
 	rcu_read_unlock();
 	return ctx;
 }
@@ -1207,13 +1208,13 @@ gss_destroying_context(struct rpc_cred *cred)
 {
 	struct gss_cred *gss_cred = container_of(cred, struct gss_cred, gc_base);
 	struct gss_auth *gss_auth = container_of(cred->cr_auth, struct gss_auth, rpc_auth);
+	struct gss_cl_ctx *ctx = rcu_dereference_protected(gss_cred->gc_ctx, 1);
 	struct rpc_task *task;
 
-	if (gss_cred->gc_ctx == NULL ||
-	    test_bit(RPCAUTH_CRED_UPTODATE, &cred->cr_flags) == 0)
+	if (test_bit(RPCAUTH_CRED_UPTODATE, &cred->cr_flags) == 0)
 		return 0;
 
-	gss_cred->gc_ctx->gc_proc = RPC_GSS_PROC_DESTROY;
+	ctx->gc_proc = RPC_GSS_PROC_DESTROY;
 	cred->cr_ops = &gss_nullops;
 
 	/* Take a reference to ensure the cred will be destroyed either
@@ -1274,7 +1275,7 @@ gss_destroy_nullcred(struct rpc_cred *cred)
 {
 	struct gss_cred *gss_cred = container_of(cred, struct gss_cred, gc_base);
 	struct gss_auth *gss_auth = container_of(cred->cr_auth, struct gss_auth, rpc_auth);
-	struct gss_cl_ctx *ctx = gss_cred->gc_ctx;
+	struct gss_cl_ctx *ctx = rcu_dereference_protected(gss_cred->gc_ctx, 1);
 
 	RCU_INIT_POINTER(gss_cred->gc_ctx, NULL);
 	call_rcu(&cred->cr_rcu, gss_free_cred_callback);
@@ -1349,20 +1350,30 @@ gss_cred_init(struct rpc_auth *auth, struct rpc_cred *cred)
 static char *
 gss_stringify_acceptor(struct rpc_cred *cred)
 {
-	char *string;
+	char *string = NULL;
 	struct gss_cred *gss_cred = container_of(cred, struct gss_cred, gc_base);
-	struct xdr_netobj *acceptor = &gss_cred->gc_ctx->gc_acceptor;
+	struct gss_cl_ctx *ctx;
+	struct xdr_netobj *acceptor;
+
+	rcu_read_lock();
+	ctx = rcu_dereference(gss_cred->gc_ctx);
+	if (!ctx)
+		goto out;
+
+	acceptor = &ctx->gc_acceptor;
 
 	/* no point if there's no string */
 	if (!acceptor->len)
-		return NULL;
+		goto out;
 
 	string = kmalloc(acceptor->len + 1, GFP_KERNEL);
 	if (!string)
-		return string;
+		goto out;
 
 	memcpy(string, acceptor->data, acceptor->len);
 	string[acceptor->len] = '\0';
+out:
+	rcu_read_unlock();
 	return string;
 }
 
@@ -1374,15 +1385,16 @@ static int
 gss_key_timeout(struct rpc_cred *rc)
 {
 	struct gss_cred *gss_cred = container_of(rc, struct gss_cred, gc_base);
+	struct gss_cl_ctx *ctx;
 	unsigned long now = jiffies;
 	unsigned long expire;
 
-	if (gss_cred->gc_ctx == NULL)
-		return -EACCES;
-
-	expire = gss_cred->gc_ctx->gc_expiry - (gss_key_expire_timeo * HZ);
-
-	if (time_after(now, expire))
+	rcu_read_lock();
+	ctx = rcu_dereference(gss_cred->gc_ctx);
+	if (ctx)
+		expire = ctx->gc_expiry - (gss_key_expire_timeo * HZ);
+	rcu_read_unlock();
+	if (!ctx || time_after(now, expire))
 		return -EACCES;
 	return 0;
 }
@@ -1391,13 +1403,19 @@ static int
 gss_match(struct auth_cred *acred, struct rpc_cred *rc, int flags)
 {
 	struct gss_cred *gss_cred = container_of(rc, struct gss_cred, gc_base);
+	struct gss_cl_ctx *ctx;
 	int ret;
 
 	if (test_bit(RPCAUTH_CRED_NEW, &rc->cr_flags))
 		goto out;
 	/* Don't match with creds that have expired. */
-	if (time_after(jiffies, gss_cred->gc_ctx->gc_expiry))
+	rcu_read_lock();
+	ctx = rcu_dereference(gss_cred->gc_ctx);
+	if (!ctx || time_after(jiffies, ctx->gc_expiry)) {
+		rcu_read_unlock();
 		return 0;
+	}
+	rcu_read_unlock();
 	if (!test_bit(RPCAUTH_CRED_UPTODATE, &rc->cr_flags))
 		return 0;
 out:
-- 
1.9.3

[PATCH v2 3/5] sunrpc: clean up sparse endianness warnings in gss_krb5_seal.c

[Jeff Layton]

Use u16 pointer in setup_token and setup_token_v2. None of the fields
are actually handled as __be16, so this simplifies the code a bit. Also
get rid of some unneeded pointer increments.

Signed-off-by: Jeff Layton <jlayton@primarydata.com>
---
 net/sunrpc/auth_gss/gss_krb5_seal.c | 28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)

diff --git a/net/sunrpc/auth_gss/gss_krb5_seal.c b/net/sunrpc/auth_gss/gss_krb5_seal.c
index 62ae3273186c..42768e5c3994 100644
--- a/net/sunrpc/auth_gss/gss_krb5_seal.c
+++ b/net/sunrpc/auth_gss/gss_krb5_seal.c
@@ -70,31 +70,37 @@
 
 DEFINE_SPINLOCK(krb5_seq_lock);
 
-static char *
+static void *
 setup_token(struct krb5_ctx *ctx, struct xdr_netobj *token)
 {
-	__be16 *ptr, *krb5_hdr;
+	u16 *ptr;
+	void *krb5_hdr;
 	int body_size = GSS_KRB5_TOK_HDR_LEN + ctx->gk5e->cksumlength;
 
 	token->len = g_token_size(&ctx->mech_used, body_size);
 
-	ptr = (__be16 *)token->data;
+	ptr = (u16 *)token->data;
 	g_make_token_header(&ctx->mech_used, body_size, (unsigned char **)&ptr);
 
 	/* ptr now at start of header described in rfc 1964, section 1.2.1: */
 	krb5_hdr = ptr;
 	*ptr++ = KG_TOK_MIC_MSG;
-	*ptr++ = cpu_to_le16(ctx->gk5e->signalg);
+	/*
+	 * signalg is stored as if it were converted from LE to host endian, even
+	 * though it's an opaque pair of bytes according to the RFC.
+	 */
+	*ptr++ = (__force u16)cpu_to_le16(ctx->gk5e->signalg);
 	*ptr++ = SEAL_ALG_NONE;
-	*ptr++ = 0xffff;
+	*ptr = 0xffff;
 
-	return (char *)krb5_hdr;
+	return krb5_hdr;
 }
 
 static void *
 setup_token_v2(struct krb5_ctx *ctx, struct xdr_netobj *token)
 {
-	__be16 *ptr, *krb5_hdr;
+	u16 *ptr;
+	void *krb5_hdr;
 	u8 *p, flags = 0x00;
 
 	if ((ctx->flags & KRB5_CTX_FLAG_INITIATOR) == 0)
@@ -104,15 +110,15 @@ setup_token_v2(struct krb5_ctx *ctx, struct xdr_netobj *token)
 
 	/* Per rfc 4121, sec 4.2.6.1, there is no header,
 	 * just start the token */
-	krb5_hdr = ptr = (__be16 *)token->data;
+	krb5_hdr = ptr = (u16 *)token->data;
 
 	*ptr++ = KG2_TOK_MIC;
 	p = (u8 *)ptr;
 	*p++ = flags;
 	*p++ = 0xff;
-	ptr = (__be16 *)p;
-	*ptr++ = 0xffff;
+	ptr = (u16 *)p;
 	*ptr++ = 0xffff;
+	*ptr = 0xffff;
 
 	token->len = GSS_KRB5_TOK_HDR_LEN + ctx->gk5e->cksumlength;
 	return krb5_hdr;
@@ -181,7 +187,7 @@ gss_get_mic_v2(struct krb5_ctx *ctx, struct xdr_buf *text,
 	spin_lock(&krb5_seq_lock);
 	seq_send = ctx->seq_send64++;
 	spin_unlock(&krb5_seq_lock);
-	*((u64 *)(krb5_hdr + 8)) = cpu_to_be64(seq_send);
+	*((__be64 *)(krb5_hdr + 8)) = cpu_to_be64(seq_send);
 
 	if (ctx->initiate) {
 		cksumkey = ctx->initiator_sign;
-- 
1.9.3

[PATCH v2 4/5] sunrpc: clean up sparse endianness warnings in gss_krb5_wrap.c

[Jeff Layton]

Fix the endianness handling in gss_wrap_kerberos_v1 and drop the memset
call there in favor of setting the filler bytes directly.

In gss_wrap_kerberos_v2, get rid of the "ec" variable which is always
zero, and drop the endianness conversion of 0. Sparse handles 0 as a
special case, so it's not necessary.

Signed-off-by: Jeff Layton <jlayton@primarydata.com>
---
 net/sunrpc/auth_gss/gss_krb5_wrap.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/net/sunrpc/auth_gss/gss_krb5_wrap.c b/net/sunrpc/auth_gss/gss_krb5_wrap.c
index 42560e55d978..88cd24aacddc 100644
--- a/net/sunrpc/auth_gss/gss_krb5_wrap.c
+++ b/net/sunrpc/auth_gss/gss_krb5_wrap.c
@@ -201,9 +201,15 @@ gss_wrap_kerberos_v1(struct krb5_ctx *kctx, int offset,
 
 	msg_start = ptr + GSS_KRB5_TOK_HDR_LEN + kctx->gk5e->cksumlength;
 
-	*(__be16 *)(ptr + 2) = cpu_to_le16(kctx->gk5e->signalg);
-	memset(ptr + 4, 0xff, 4);
-	*(__be16 *)(ptr + 4) = cpu_to_le16(kctx->gk5e->sealalg);
+	/*
+	 * signalg and sealalg are stored as if they were converted from LE
+	 * to host endian, even though they're opaque pairs of bytes according
+	 * to the RFC.
+	 */
+	*(__le16 *)(ptr + 2) = cpu_to_le16(kctx->gk5e->signalg);
+	*(__le16 *)(ptr + 4) = cpu_to_le16(kctx->gk5e->sealalg);
+	ptr[6] = 0xff;
+	ptr[7] = 0xff;
 
 	gss_krb5_make_confounder(msg_start, conflen);
 
@@ -438,7 +444,7 @@ gss_wrap_kerberos_v2(struct krb5_ctx *kctx, u32 offset,
 	u8		*ptr, *plainhdr;
 	s32		now;
 	u8		flags = 0x00;
-	__be16		*be16ptr, ec = 0;
+	__be16		*be16ptr;
 	__be64		*be64ptr;
 	u32		err;
 
@@ -468,16 +474,16 @@ gss_wrap_kerberos_v2(struct krb5_ctx *kctx, u32 offset,
 	be16ptr = (__be16 *)ptr;
 
 	blocksize = crypto_blkcipher_blocksize(kctx->acceptor_enc);
-	*be16ptr++ = cpu_to_be16(ec);
+	*be16ptr++ = 0;
 	/* "inner" token header always uses 0 for RRC */
-	*be16ptr++ = cpu_to_be16(0);
+	*be16ptr++ = 0;
 
 	be64ptr = (__be64 *)be16ptr;
 	spin_lock(&krb5_seq_lock);
 	*be64ptr = cpu_to_be64(kctx->seq_send64++);
 	spin_unlock(&krb5_seq_lock);
 
-	err = (*kctx->gk5e->encrypt_v2)(kctx, offset, buf, ec, pages);
+	err = (*kctx->gk5e->encrypt_v2)(kctx, offset, buf, 0, pages);
 	if (err)
 		return err;
 
-- 
1.9.3

[PATCH v2 5/5] sunrpc: remove "ec" argument from encrypt_v2 operation

[Jeff Layton]

It's always 0.

Signed-off-by: Jeff Layton <jlayton@primarydata.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 include/linux/sunrpc/gss_krb5.h       | 4 ++--
 net/sunrpc/auth_gss/gss_krb5_crypto.c | 9 ++-------
 net/sunrpc/auth_gss/gss_krb5_wrap.c   | 2 +-
 3 files changed, 5 insertions(+), 10 deletions(-)

diff --git a/include/linux/sunrpc/gss_krb5.h b/include/linux/sunrpc/gss_krb5.h
index 5af2931cf58d..df02a4188487 100644
--- a/include/linux/sunrpc/gss_krb5.h
+++ b/include/linux/sunrpc/gss_krb5.h
@@ -81,7 +81,7 @@ struct gss_krb5_enctype {
 		       struct xdr_netobj *in,
 		       struct xdr_netobj *out);	/* complete key generation */
 	u32 (*encrypt_v2) (struct krb5_ctx *kctx, u32 offset,
-			   struct xdr_buf *buf, int ec,
+			   struct xdr_buf *buf,
 			   struct page **pages); /* v2 encryption function */
 	u32 (*decrypt_v2) (struct krb5_ctx *kctx, u32 offset,
 			   struct xdr_buf *buf, u32 *headskip,
@@ -310,7 +310,7 @@ gss_krb5_aes_make_key(const struct gss_krb5_enctype *gk5e,
 
 u32
 gss_krb5_aes_encrypt(struct krb5_ctx *kctx, u32 offset,
-		     struct xdr_buf *buf, int ec,
+		     struct xdr_buf *buf,
 		     struct page **pages);
 
 u32
diff --git a/net/sunrpc/auth_gss/gss_krb5_crypto.c b/net/sunrpc/auth_gss/gss_krb5_crypto.c
index 0f43e894bc0a..f5ed9f6ece06 100644
--- a/net/sunrpc/auth_gss/gss_krb5_crypto.c
+++ b/net/sunrpc/auth_gss/gss_krb5_crypto.c
@@ -641,7 +641,7 @@ out:
 
 u32
 gss_krb5_aes_encrypt(struct krb5_ctx *kctx, u32 offset,
-		     struct xdr_buf *buf, int ec, struct page **pages)
+		     struct xdr_buf *buf, struct page **pages)
 {
 	u32 err;
 	struct xdr_netobj hmac;
@@ -684,13 +684,8 @@ gss_krb5_aes_encrypt(struct krb5_ctx *kctx, u32 offset,
 		ecptr = buf->tail[0].iov_base;
 	}
 
-	memset(ecptr, 'X', ec);
-	buf->tail[0].iov_len += ec;
-	buf->len += ec;
-
 	/* copy plaintext gss token header after filler (if any) */
-	memcpy(ecptr + ec, buf->head[0].iov_base + offset,
-						GSS_KRB5_TOK_HDR_LEN);
+	memcpy(ecptr, buf->head[0].iov_base + offset, GSS_KRB5_TOK_HDR_LEN);
 	buf->tail[0].iov_len += GSS_KRB5_TOK_HDR_LEN;
 	buf->len += GSS_KRB5_TOK_HDR_LEN;
 
diff --git a/net/sunrpc/auth_gss/gss_krb5_wrap.c b/net/sunrpc/auth_gss/gss_krb5_wrap.c
index 88cd24aacddc..4b614c604fe0 100644
--- a/net/sunrpc/auth_gss/gss_krb5_wrap.c
+++ b/net/sunrpc/auth_gss/gss_krb5_wrap.c
@@ -483,7 +483,7 @@ gss_wrap_kerberos_v2(struct krb5_ctx *kctx, u32 offset,
 	*be64ptr = cpu_to_be64(kctx->seq_send64++);
 	spin_unlock(&krb5_seq_lock);
 
-	err = (*kctx->gk5e->encrypt_v2)(kctx, offset, buf, 0, pages);
+	err = (*kctx->gk5e->encrypt_v2)(kctx, offset, buf, pages);
 	if (err)
 		return err;
 
-- 
1.9.3

Re: [PATCH v2 0/5] sunrpc: sparse warning cleanups

[Jeff Layton]

On Wed, 16 Jul 2014 06:52:17 -0400
Jeff Layton <jlayton@primarydata.com> wrote:

> v2:
> - reworked the gss_krb5_seal.c and gss_krb5_wrap.c patches to be less
>   ugly
> - squashed the setup_token patch into the gss_krb5_seal.c patch
> - dropped patch to make xdr_get_next_encode_buffer static since Trond
>   proposed the same fix
> 
> This fixes up all of the sparse warnings that I see when building
> net/sunrpc. The only real substantive change is the second patch which
> should fix the RCU handling for the gc_ctx field. That looks quite wrong
> right now, though it may be that the refcounting and lifecycle of the
> thing helps paper over it today.
> 
> There are still a few warnings that come from generic ipv6.h inlines.
> I'll send a separate patch to the netdev folks to address those.
> 
> Trond, if these look OK then they should probably go via your tree as it
> looks like you've already merged the gc_acceptor patches and the RCU
> fixes in patch #2 should apply on top of those.
> 
> Jeff Layton (5):
>   sunrpc: remove __rcu annotation from struct gss_cl_ctx->gc_gss_ctx
>   sunrpc: fix RCU handling of gc_ctx field
>   sunrpc: clean up sparse endianness warnings in gss_krb5_seal.c
>   sunrpc: clean up sparse endianness warnings in gss_krb5_wrap.c
>   sunrpc: remove "ec" argument from encrypt_v2 operation
> 
>  include/linux/sunrpc/auth_gss.h       |  2 +-
>  include/linux/sunrpc/gss_krb5.h       |  4 +--
>  net/sunrpc/auth_gss/auth_gss.c        | 52 +++++++++++++++++++++++------------
>  net/sunrpc/auth_gss/gss_krb5_crypto.c |  9 ++----
>  net/sunrpc/auth_gss/gss_krb5_seal.c   | 28 +++++++++++--------
>  net/sunrpc/auth_gss/gss_krb5_wrap.c   | 20 +++++++++-----
>  6 files changed, 70 insertions(+), 45 deletions(-)
> 

Trond, I noticed that you hadn't yet picked these patches up into your
tree. I'd like to get these in since they do make it easier to spot
real bugs with sparse. Are these better fed via your or Bruce's tree?

Thanks,
-- 
Jeff Layton <jlayton@primarydata.com>