Skip to contentSkip to conversation

sunrpc: sparse warning cleanups

PATCH Superseded Jeff Layton 4 participants 28 messages Jul 14, 2014 – Jul 30, 2014

Subsystems (3)
Networking NFS client NFS server
lore
Share & tools
lore

Jeff Layton

author
#0 lore raw

cover sunrpc: sparse warning cleanups

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(-)
Signature · 1 line
1.9.3
author
#1 lore raw

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

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 <[email protected]>
Cc Paul McKenney <[email protected]>
Signed-off-by Jeff Layton <[email protected]>
---
 include/linux/sunrpc/auth_gss.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
include/linux/sunrpc/auth_gss.h +1 -1
index cbc6875fb9cf..36eebc451b41 100644
@@ -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;
Signature · 1 line
1.9.3
#2 lore raw

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

Quoted 8 lines · 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 <[email protected]>
Cc Paul McKenney <[email protected]>
Signed-off-by Jeff Layton <[email protected]>

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

#3 lore raw

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

Quoted 6 lines · On Mon, 14 Jul 2014 16:23:53 +0200 Arnd Bergmann <[email protected]> wrote:
Quoted 5 lines · 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.

Quoted 9 lines
Quoted 3 lines
Cc Arnd Bergmann <[email protected]>
Cc Paul McKenney <[email protected]>
Signed-off-by Jeff Layton <[email protected]>

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.

Signature · 1 line
Jeff Layton <[email protected]>
#4 lore raw

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

Quoted 21 lines · On Monday 14 July 2014 11:02:02 Jeff Layton wrote:
Quoted 6 lines · On Mon, 14 Jul 2014 16:23:53 +0200 Arnd Bergmann <[email protected]> wrote:
Quoted 5 lines · 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.

Quoted 9 lines
Quoted 3 lines
Cc Arnd Bergmann <[email protected]>
Cc Paul McKenney <[email protected]>
Signed-off-by Jeff Layton <[email protected]>

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...

Quoted 8 lines

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

author
#5 lore raw

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

In reply to 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 <[email protected]>
Cc Paul McKenney <[email protected]>
Signed-off-by Jeff Layton <[email protected]>
---
 net/sunrpc/auth_gss/auth_gss.c | 52 ++++++++++++++++++++++++++++--------------
 1 file changed, 35 insertions(+), 17 deletions(-)
net/sunrpc/auth_gss/auth_gss.c +35 -17
index 73854314fb85..afb292cd797d 100644
@@ -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:
Signature · 1 line
1.9.3
author
#6 lore raw

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

In reply to 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 <[email protected]>
---
 net/sunrpc/auth_gss/gss_krb5_seal.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
net/sunrpc/auth_gss/gss_krb5_seal.c +4 -4
index 62ae3273186c..94ad57ff7169 100644
@@ -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;
 }
Signature · 1 line
1.9.3
#7 lore raw

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

net/sunrpc/auth_gss/gss_krb5_seal.c setup_token(struct krb5_ctx *ctx, struct xdr_netobj *token) · line 89

Quoted 13 lines · 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.

net/sunrpc/auth_gss/gss_krb5_seal.c +4 -0
@@ -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.

net/sunrpc/auth_gss/gss_krb5_seal.c setup_token(struct krb5_ctx *ctx, struct xdr_netobj *token) · line 91

Quoted 1 line
net/sunrpc/auth_gss/gss_krb5_seal.c +0 -0
 	return (char *)krb5_hdr;

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

#8 lore raw

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

Quoted 8 lines · On Tue, 15 Jul 2014 10:08:06 -0700 Christoph Hellwig <[email protected]> wrote:
Quoted 7 lines · 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.

(quoted diff) +0 -0
@@ -83,10 +83,10 @@ setup_token(struct krb5_ctx *ctx, struct xdr_netobj *token)
Quoted 10 lines

net/sunrpc/auth_gss/gss_krb5_seal.c setup_token(struct krb5_ctx *ctx, struct xdr_netobj *token) · line 89

Quoted 6 lines
net/sunrpc/auth_gss/gss_krb5_seal.c +4 -0
 	/* 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

Quoted 5 lines

net/sunrpc/auth_gss/gss_krb5_seal.c setup_token(struct krb5_ctx *ctx, struct xdr_netobj *token) · line 91

Quoted 1 line
net/sunrpc/auth_gss/gss_krb5_seal.c +0 -0
 	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.

Signature · 1 line
Jeff Layton <[email protected]>
author
#9 lore raw

PATCH 4/7 sunrpc: xdr_get_next_encode_buffer can be static

In reply to Jeff Layton
Signed-off-by Jeff Layton <[email protected]>
---
 net/sunrpc/xdr.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
net/sunrpc/xdr.c +2 -1
index 23fb4e75e245..13b7dd0d38e6 100644
@@ -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;
Signature · 1 line
1.9.3
author
#12 lore raw

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

In reply to Jeff Layton

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

Signed-off-by Jeff Layton <[email protected]>
---
 net/sunrpc/auth_gss/gss_krb5_seal.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
net/sunrpc/auth_gss/gss_krb5_seal.c +4 -4
index 94ad57ff7169..3cf49ea54ca9 100644
@@ -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;
Signature · 1 line
1.9.3
author
#14 lore raw

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

In reply to Jeff Layton
Signed-off-by Jeff Layton <[email protected]>
---
 net/sunrpc/auth_gss/gss_krb5_wrap.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
net/sunrpc/auth_gss/gss_krb5_wrap.c +5 -5
index 42560e55d978..c5cc0270a334 100644
@@ -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;
Signature · 1 line
1.9.3
#15 lore raw

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

Quoted 10 lines · Jeff Layton
Signed-off-by Jeff Layton <[email protected]>
---
 net/sunrpc/auth_gss/gss_krb5_wrap.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
net/sunrpc/auth_gss/gss_krb5_wrap.c +0 -0
index 42560e55d978..c5cc0270a334 100644

net/sunrpc/auth_gss/gss_krb5_wrap.c gss_wrap_kerberos_v1(struct krb5_ctx *kctx, int offset, · line 204

Quoted 4 lines
net/sunrpc/auth_gss/gss_krb5_wrap.c +1 -0
@@ -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.

#16 lore raw

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

Quoted 14 lines · On Tue, 15 Jul 2014 10:04:42 -0700 Christoph Hellwig <[email protected]> wrote:
Quoted 13 lines · Jeff Layton
Signed-off-by Jeff Layton <[email protected]>
---
 net/sunrpc/auth_gss/gss_krb5_wrap.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
net/sunrpc/auth_gss/gss_krb5_wrap.c +0 -0
index 42560e55d978..c5cc0270a334 100644
@@ -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;
Quoted 9 lines

net/sunrpc/auth_gss/gss_krb5_wrap.c gss_wrap_kerberos_v1(struct krb5_ctx *kctx, int offset, · line 204

Quoted 1 line
net/sunrpc/auth_gss/gss_krb5_wrap.c +1 -0
+	*(__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...

Signature · 1 line
Jeff Layton <[email protected]>
#17 lore raw

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

Quoted 7 lines · Jeff Layton
Quoted 7 lines

net/sunrpc/auth_gss/gss_krb5_wrap.c gss_wrap_kerberos_v1(struct krb5_ctx *kctx, int offset, · line 204

Quoted 2 lines
net/sunrpc/auth_gss/gss_krb5_wrap.c +1 -0
+	*(__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);
Quoted 8 lines
Quoted 3 lines

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.

Quoted 1 line

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.

author
#18 lore raw

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

In reply to Jeff Layton

It's always 0.

Signed-off-by Jeff Layton <[email protected]>
---
 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(-)
include/linux/sunrpc/gss_krb5.h +2 -2
index 5af2931cf58d..df02a4188487 100644
@@ -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
net/sunrpc/auth_gss/gss_krb5_crypto.c +2 -7
index 0f43e894bc0a..f5ed9f6ece06 100644
@@ -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;
net/sunrpc/auth_gss/gss_krb5_wrap.c +1 -1
index c5cc0270a334..ccc10321d87d 100644
@@ -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;
Signature · 1 line
1.9.3
#20 lore raw

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

Quoted 6 lines · On Tue, 15 Jul 2014 10:05:41 -0700 Christoph Hellwig <[email protected]> wrote:

Looks good,

Reviewed-by Christoph Hellwig <[email protected]>

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.

Signature · 1 line
Jeff Layton <[email protected]>
author
#21 lore raw

v2 cover sunrpc: sparse warning cleanups

In reply to 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(-)
Signature · 1 line
1.9.3
author
#22 lore raw

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

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 <[email protected]>
Cc Paul McKenney <[email protected]>
Signed-off-by Jeff Layton <[email protected]>
---
 include/linux/sunrpc/auth_gss.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
include/linux/sunrpc/auth_gss.h +1 -1
index cbc6875fb9cf..36eebc451b41 100644
@@ -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;
Signature · 1 line
1.9.3
author
#23 lore raw

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

In reply to 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 <[email protected]>
Cc Paul McKenney <[email protected]>
Signed-off-by Jeff Layton <[email protected]>
---
 net/sunrpc/auth_gss/auth_gss.c | 52 ++++++++++++++++++++++++++++--------------
 1 file changed, 35 insertions(+), 17 deletions(-)
net/sunrpc/auth_gss/auth_gss.c +35 -17
index 73854314fb85..afb292cd797d 100644
@@ -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:
Signature · 1 line
1.9.3
author
#24 lore raw

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

In reply to 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 <[email protected]>
---
 net/sunrpc/auth_gss/gss_krb5_seal.c | 28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)
net/sunrpc/auth_gss/gss_krb5_seal.c +17 -11
index 62ae3273186c..42768e5c3994 100644
@@ -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;
Signature · 1 line
1.9.3
author
#25 lore raw

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

In reply to 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 <[email protected]>
---
 net/sunrpc/auth_gss/gss_krb5_wrap.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)
net/sunrpc/auth_gss/gss_krb5_wrap.c +13 -7
index 42560e55d978..88cd24aacddc 100644
@@ -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;
Signature · 1 line
1.9.3
author
#26 lore raw

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

In reply to Jeff Layton

It's always 0.

Signed-off-by Jeff Layton <[email protected]>
Reviewed-by Christoph Hellwig <[email protected]>
---
 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(-)
include/linux/sunrpc/gss_krb5.h +2 -2
index 5af2931cf58d..df02a4188487 100644
@@ -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
net/sunrpc/auth_gss/gss_krb5_crypto.c +2 -7
index 0f43e894bc0a..f5ed9f6ece06 100644
@@ -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;
net/sunrpc/auth_gss/gss_krb5_wrap.c +1 -1
index 88cd24aacddc..4b614c604fe0 100644
@@ -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;
Signature · 1 line
1.9.3
#27 lore raw
In reply to Jeff Layton
Quoted 27 lines · On Wed, 16 Jul 2014 06:52:17 -0400 Jeff Layton <[email protected]> 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

Quoted 8 lines
 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,

Signature · 1 line
Jeff Layton <[email protected]>