Skip to contentSkip to conversation

nfsd: clean up sparse endianness warnings

PATCH In review Jeff Layton 3 participants 10 messages Jun 17, 2014 – Jun 18, 2014

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

v2 cover nfsd: clean up sparse endianness warnings

This patchset does a number of cleanups to remove sparse warnings that appear when -D__CHECK_ENDIAN__ is specified. Mostly, they just add __force in the appropriate places to make it clear that casting directly to a different endianness is intentional. I've also added comments to clarify why we're doing this in the same places.

Note that there are still some sparse warnings that are not addressed. This one looks like a bug in sparse to me. We're initializing different slots in an array:

fs/nfsd/nfssvc.c:120:10: warning: Initializer entry defined twice
fs/nfsd/nfssvc.c:121:10: also defined here

This warning is also not addressed in this set:

fs/nfsd/auth.c:31:38: warning: incorrect type in argument 1 (different address spaces)
fs/nfsd/auth.c:31:38: expected struct cred const *cred
fs/nfsd/auth.c:31:38: got struct cred const [noderef] <asn:4>*real_cred

I think sparse is complaining that we're casting away __rcu when handling creds. I'm not quite sure whether that's a real bug or not. I've left it alone for now until I (or someone else) has some time to look more closely.

Jeff Layton (4):
nfsd: add __force to opaque verifier field casts
nfsd: clean up sparse endianness warnings in nfscache.c
nfsd: nfsd_splice_read and nfsd_readv should return __be32
nfsd: add appropriate __force directives to filehandle generation code

 fs/nfsd/nfs4proc.c  |  8 ++++++--
 fs/nfsd/nfs4state.c |  8 ++++++--
 fs/nfsd/nfscache.c  | 13 +++++++++++--
 fs/nfsd/nfsfh.c     |  9 ++++++++-
 fs/nfsd/nfsfh.h     | 15 +++++++++++----
 fs/nfsd/vfs.c       |  7 ++++---
 fs/nfsd/vfs.h       |  4 ++--
 7 files changed, 48 insertions(+), 16 deletions(-)
Signature · 1 line
1.9.3
#1 lore raw

PATCH v2 1/4 nfsd: add __force to opaque verifier field casts

sparse complains that we're stuffing non-byte-swapped values into __be32's here. Since they're supposed to be opaque, it doesn't matter much. Just add __force to make sparse happy.

---
 fs/nfsd/nfs4proc.c  | 8 ++++++--
 fs/nfsd/nfs4state.c | 8 ++++++--
 2 files changed, 12 insertions(+), 4 deletions(-)
fs/nfsd/nfs4proc.c +6 -2
index 6851b003f2a4..8904c9cbcb89 100644
@@ -581,8 +581,12 @@ static void gen_boot_verifier(nfs4_verifier *verifier, struct net *net)
 	__be32 verf[2];
 	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
 
-	verf[0] = (__be32)nn->nfssvc_boot.tv_sec;
-	verf[1] = (__be32)nn->nfssvc_boot.tv_usec;
+	/*
+	 * This is opaque to client, so no need to byte-swap. Use
+	 * __force to keep sparse happy
+	 */
+	verf[0] = (__force __be32)nn->nfssvc_boot.tv_sec;
+	verf[1] = (__force __be32)nn->nfssvc_boot.tv_usec;
 	memcpy(verifier->data, verf, sizeof(verifier->data));
 }
 
fs/nfsd/nfs4state.c +6 -2
index c0d45cec9958..0fea1da3dd24 100644
@@ -1345,8 +1345,12 @@ static void gen_confirm(struct nfs4_client *clp)
 	__be32 verf[2];
 	static u32 i;
 
-	verf[0] = (__be32)get_seconds();
-	verf[1] = (__be32)i++;
+	/*
+	 * This is opaque to client, so no need to byte-swap. Use
+	 * __force to keep sparse happy
+	 */
+	verf[0] = (__force __be32)get_seconds();
+	verf[1] = (__force __be32)i++;
 	memcpy(clp->cl_confirm.data, verf, sizeof(clp->cl_confirm.data));
 }
 
Signature · 1 line
1.9.3
#3 lore raw

PATCH v2 2/4 nfsd: clean up sparse endianness warnings in nfscache.c

In reply to Jeff Layton

We currently hash the XID to determine a hash bucket to use for the reply cache entry, which is fed into hash_32 without byte-swapping it. Add __force to make sparse happy, and add some comments to explain why.

---
 fs/nfsd/nfscache.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)
fs/nfsd/nfscache.c +11 -2
index 6040da8830ff..ff9567633245 100644
@@ -221,7 +221,12 @@ static void
 hash_refile(struct svc_cacherep *rp)
 {
 	hlist_del_init(&rp->c_hash);
-	hlist_add_head(&rp->c_hash, cache_hash + hash_32(rp->c_xid, maskbits));
+	/*
+	 * No point in byte swapping c_xid since we're just using it to pick
+	 * a hash bucket.
+	 */
+	hlist_add_head(&rp->c_hash, cache_hash +
+			hash_32((__force u32)rp->c_xid, maskbits));
 }
 
 /*
@@ -356,7 +361,11 @@ nfsd_cache_search(struct svc_rqst *rqstp, __wsum csum)
 	struct hlist_head 	*rh;
 	unsigned int		entries = 0;
 
-	rh = &cache_hash[hash_32(rqstp->rq_xid, maskbits)];
+	/*
+	 * No point in byte swapping rq_xid since we're just using it to pick
+	 * a hash bucket.
+	 */
+	rh = &cache_hash[hash_32((__force u32)rqstp->rq_xid, maskbits)];
 	hlist_for_each_entry(rp, rh, c_hash) {
 		++entries;
 		if (nfsd_cache_match(rqstp, csum, rp)) {
Signature · 1 line
1.9.3
Reviewed-by on v2 2/4
#4 lore raw

Re: [PATCH v2 2/4] nfsd: clean up sparse endianness warnings in nfscache.c

Quoted 4 lines · Jeff Layton

We currently hash the XID to determine a hash bucket to use for the reply cache entry, which is fed into hash_32 without byte-swapping it. Add __force to make sparse happy, and add some comments to explain why.

Would seem simpler to me to simply do the byteswap, but..

#5 lore raw

PATCH v2 3/4 nfsd: nfsd_splice_read and nfsd_readv should return __be32

In reply to Jeff Layton

The callers expect a __be32 return and the functions they call return __be32, so having these return int is just wrong. Also, nfsd_finish_read can be made static.

---
 fs/nfsd/vfs.c | 7 ++++---
 fs/nfsd/vfs.h | 4 ++--
 2 files changed, 6 insertions(+), 5 deletions(-)
fs/nfsd/vfs.c +4 -3
index 140c496f612c..960f9e0bb88f 100644
@@ -820,7 +820,8 @@ static int nfsd_direct_splice_actor(struct pipe_inode_info *pipe,
 	return __splice_from_pipe(pipe, sd, nfsd_splice_actor);
 }
 
-__be32 nfsd_finish_read(struct file *file, unsigned long *count, int host_err)
+static __be32
+nfsd_finish_read(struct file *file, unsigned long *count, int host_err)
 {
 	if (host_err >= 0) {
 		nfsdstats.io_read += host_err;
@@ -831,7 +832,7 @@ __be32 nfsd_finish_read(struct file *file, unsigned long *count, int host_err)
 		return nfserrno(host_err);
 }
 
-int nfsd_splice_read(struct svc_rqst *rqstp,
+__be32 nfsd_splice_read(struct svc_rqst *rqstp,
 		     struct file *file, loff_t offset, unsigned long *count)
 {
 	struct splice_desc sd = {
@@ -847,7 +848,7 @@ int nfsd_splice_read(struct svc_rqst *rqstp,
 	return nfsd_finish_read(file, count, host_err);
 }
 
-int nfsd_readv(struct file *file, loff_t offset, struct kvec *vec, int vlen,
+__be32 nfsd_readv(struct file *file, loff_t offset, struct kvec *vec, int vlen,
 		unsigned long *count)
 {
 	mm_segment_t oldfs;
fs/nfsd/vfs.h +2 -2
index 91b6ae3f658b..b84aef50f55d 100644
@@ -74,9 +74,9 @@ struct raparms;
 __be32		nfsd_get_tmp_read_open(struct svc_rqst *, struct svc_fh *,
 				struct file **, struct raparms **);
 void		nfsd_put_tmp_read_open(struct file *, struct raparms *);
-int		nfsd_splice_read(struct svc_rqst *,
+__be32		nfsd_splice_read(struct svc_rqst *,
 				struct file *, loff_t, unsigned long *);
-int		nfsd_readv(struct file *, loff_t, struct kvec *, int,
+__be32		nfsd_readv(struct file *, loff_t, struct kvec *, int,
 				unsigned long *);
 __be32 		nfsd_read(struct svc_rqst *, struct svc_fh *,
 				loff_t, struct kvec *, int, unsigned long *);
Signature · 1 line
1.9.3
#7 lore raw

PATCH v2 4/4 nfsd: add appropriate __force directives to filehandle generation code

In reply to Jeff Layton

The filehandle structs all use host-endian values, but will sometimes stuff big-endian values into those fields. This is OK since these values are opaque to the client, but it confuses sparse. Add __force to make it clear that we are doing this intentionally.

---
 fs/nfsd/nfsfh.c |  9 ++++++++-
 fs/nfsd/nfsfh.h | 15 +++++++++++----
 2 files changed, 19 insertions(+), 5 deletions(-)
fs/nfsd/nfsfh.c +8 -1
index ec8393418154..7e5b2d993372 100644
@@ -162,7 +162,14 @@ static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct svc_fh *fhp)
 			/* deprecated, convert to type 3 */
 			len = key_len(FSID_ENCODE_DEV)/4;
 			fh->fh_fsid_type = FSID_ENCODE_DEV;
-			fh->fh_fsid[0] = new_encode_dev(MKDEV(ntohl(fh->fh_fsid[0]), ntohl(fh->fh_fsid[1])));
+			/*
+			 * struct knfsd_fh uses host-endian fields, which are
+			 * sometimes used to hold net-endian values. This
+			 * confuses sparse, so we must use __force here to
+			 * keep it from complaining.
+			 */
+			fh->fh_fsid[0] = new_encode_dev(MKDEV(ntohl((__force __be32)fh->fh_fsid[0]),
+							ntohl((__force __be32)fh->fh_fsid[1])));
 			fh->fh_fsid[1] = fh->fh_fsid[2];
 		}
 		data_left -= len;
fs/nfsd/nfsfh.h +11 -4
index 2e89e70ac15c..08236d70c667 100644
@@ -73,8 +73,15 @@ enum fsid_source {
 extern enum fsid_source fsid_source(struct svc_fh *fhp);
 
 
-/* This might look a little large to "inline" but in all calls except
+/*
+ * This might look a little large to "inline" but in all calls except
  * one, 'vers' is constant so moste of the function disappears.
+ *
+ * In some cases the values are considered to be host endian and in
+ * others, net endian. fsidv is always considered to be u32 as the
+ * callers don't know which it will be. So we must use __force to keep
+ * sparse from complaining. Since these values are opaque to the
+ * client, that shouldn't be a problem.
  */
 static inline void mk_fsid(int vers, u32 *fsidv, dev_t dev, ino_t ino,
 			   u32 fsid, unsigned char *uuid)
@@ -82,7 +89,7 @@ static inline void mk_fsid(int vers, u32 *fsidv, dev_t dev, ino_t ino,
 	u32 *up;
 	switch(vers) {
 	case FSID_DEV:
-		fsidv[0] = htonl((MAJOR(dev)<<16) |
+		fsidv[0] = (__force __u32)htonl((MAJOR(dev)<<16) |
 				 MINOR(dev));
 		fsidv[1] = ino_t_to_u32(ino);
 		break;
@@ -90,8 +97,8 @@ static inline void mk_fsid(int vers, u32 *fsidv, dev_t dev, ino_t ino,
 		fsidv[0] = fsid;
 		break;
 	case FSID_MAJOR_MINOR:
-		fsidv[0] = htonl(MAJOR(dev));
-		fsidv[1] = htonl(MINOR(dev));
+		fsidv[0] = (__force __u32)htonl(MAJOR(dev));
+		fsidv[1] = (__force __u32)htonl(MINOR(dev));
 		fsidv[2] = ino_t_to_u32(ino);
 		break;
 
Signature · 1 line
1.9.3
Reviewed-by on v2 4/4
#8 lore raw

Re: [PATCH v2 4/4] nfsd: add appropriate __force directives to filehandle generation code

Quoted 6 lines · Jeff Layton

The filehandle structs all use host-endian values, but will sometimes stuff big-endian values into those fields. This is OK since these values are opaque to the client, but it confuses sparse. Add __force to make it clear that we are doing this intentionally.

Looks good,

#9 lore raw
In reply to Jeff Layton
Quoted 5 lines · Jeff Layton

This patchset does a number of cleanups to remove sparse warnings that appear when -D__CHECK_ENDIAN__ is specified. Mostly, they just add __force in the appropriate places to make it clear that casting directly to a different endianness is intentional. I've also added comments to clarify why we're doing this in the same places.

Applying, thanks for sorting through the sparse output. May be a few days before I get a new branch pushed out.

--b.

Quoted 36 lines

Note that there are still some sparse warnings that are not addressed. This one looks like a bug in sparse to me. We're initializing different slots in an array:

fs/nfsd/nfssvc.c:120:10: warning: Initializer entry defined twice
fs/nfsd/nfssvc.c:121:10: also defined here

This warning is also not addressed in this set:

fs/nfsd/auth.c:31:38: warning: incorrect type in argument 1 (different address spaces)
fs/nfsd/auth.c:31:38: expected struct cred const *cred
fs/nfsd/auth.c:31:38: got struct cred const [noderef] <asn:4>*real_cred

I think sparse is complaining that we're casting away __rcu when handling creds. I'm not quite sure whether that's a real bug or not. I've left it alone for now until I (or someone else) has some time to look more closely.

Jeff Layton (4):
nfsd: add __force to opaque verifier field casts
nfsd: clean up sparse endianness warnings in nfscache.c
nfsd: nfsd_splice_read and nfsd_readv should return __be32
nfsd: add appropriate __force directives to filehandle generation code

 fs/nfsd/nfs4proc.c  |  8 ++++++--
 fs/nfsd/nfs4state.c |  8 ++++++--
 fs/nfsd/nfscache.c  | 13 +++++++++++--
 fs/nfsd/nfsfh.c     |  9 ++++++++-
 fs/nfsd/nfsfh.h     | 15 +++++++++++----
 fs/nfsd/vfs.c       |  7 ++++---
 fs/nfsd/vfs.h       |  4 ++--
 7 files changed, 48 insertions(+), 16 deletions(-)
Signature · 1 line
1.9.3