[PATCH v2 0/4] nfsd: clean up sparse endianness warnings

[PATCH v2 0/4] nfsd: clean up sparse endianness warnings

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

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(-)

-- 
1.9.3

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

[Jeff Layton]

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.

Signed-off-by: Jeff Layton <jlayton@primarydata.com>
---
 fs/nfsd/nfs4proc.c  | 8 ++++++--
 fs/nfsd/nfs4state.c | 8 ++++++--
 2 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 6851b003f2a4..8904c9cbcb89 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -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));
 }
 
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index c0d45cec9958..0fea1da3dd24 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -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));
 }
 
-- 
1.9.3

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

[Christoph Hellwig]

On Tue, Jun 17, 2014 at 07:44:11AM -0400, Jeff Layton wrote:
> 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.
> 
> Signed-off-by: Jeff Layton <jlayton@primarydata.com>

Looks good,

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

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

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

Signed-off-by: Jeff Layton <jlayton@primarydata.com>
---
 fs/nfsd/nfscache.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/fs/nfsd/nfscache.c b/fs/nfsd/nfscache.c
index 6040da8830ff..ff9567633245 100644
--- a/fs/nfsd/nfscache.c
+++ b/fs/nfsd/nfscache.c
@@ -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)) {
-- 
1.9.3

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

[Christoph Hellwig]

On Tue, Jun 17, 2014 at 07:44:12AM -0400, Jeff Layton wrote:
> 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..

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

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

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

Signed-off-by: Jeff Layton <jlayton@primarydata.com>
---
 fs/nfsd/vfs.c | 7 ++++---
 fs/nfsd/vfs.h | 4 ++--
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index 140c496f612c..960f9e0bb88f 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -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;
diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h
index 91b6ae3f658b..b84aef50f55d 100644
--- a/fs/nfsd/vfs.h
+++ b/fs/nfsd/vfs.h
@@ -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 *);
-- 
1.9.3

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

[Christoph Hellwig]

On Tue, Jun 17, 2014 at 07:44:13AM -0400, Jeff Layton wrote:
> 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.
> 
> Signed-off-by: Jeff Layton <jlayton@primarydata.com>

Looks good,

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

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

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

Signed-off-by: Jeff Layton <jlayton@primarydata.com>
---
 fs/nfsd/nfsfh.c |  9 ++++++++-
 fs/nfsd/nfsfh.h | 15 +++++++++++----
 2 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
index ec8393418154..7e5b2d993372 100644
--- a/fs/nfsd/nfsfh.c
+++ b/fs/nfsd/nfsfh.c
@@ -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;
diff --git a/fs/nfsd/nfsfh.h b/fs/nfsd/nfsfh.h
index 2e89e70ac15c..08236d70c667 100644
--- a/fs/nfsd/nfsfh.h
+++ b/fs/nfsd/nfsfh.h
@@ -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;
 
-- 
1.9.3

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

[Christoph Hellwig]

On Tue, Jun 17, 2014 at 07:44:14AM -0400, Jeff Layton wrote:
> 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.
> 
> Signed-off-by: Jeff Layton <jlayton@primarydata.com>

Looks good,

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

Re: [PATCH v2 0/4] nfsd: clean up sparse endianness warnings

[J. Bruce Fields]

On Tue, Jun 17, 2014 at 07:44:10AM -0400, Jeff Layton wrote:
> 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.

> 
> 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(-)
> 
> -- 
> 1.9.3
>