nfsd: clean up sparse endianness warnings
Subsystems (2)
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
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
@@ -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
@@ -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
Re: [PATCH v2 1/4] nfsd: add __force to opaque verifier field casts
Quoted 5 lines · 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
__forceto make sparse happy.Signed-off-by Jeff Layton <[email protected]>
Looks good,
PATCH v2 2/4 nfsd: clean up sparse endianness warnings in nfscache.c
In reply to Jeff LaytonWe 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
@@ -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
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_32without byte-swapping it. Add__forceto make sparse happy, and add some comments to explain why.
Would seem simpler to me to simply do the byteswap, but..
PATCH v2 3/4 nfsd: nfsd_splice_read and nfsd_readv should return __be32
In reply to Jeff LaytonThe 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
@@ -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
@@ -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
Re: [PATCH v2 3/4] nfsd: nfsd_splice_read and nfsd_readv should return __be32
Quoted 5 lines · Jeff Layton
The callers expect a
__be32return and the functions they call return__be32, so having these return int is just wrong. Also,nfsd_finish_readcan be made static.Signed-off-by Jeff Layton <[email protected]>
Looks good,
PATCH v2 4/4 nfsd: add appropriate __force directives to filehandle generation code
In reply to Jeff LaytonThe 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
@@ -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
@@ -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
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
__forceto make it clear that we are doing this intentionally.Signed-off-by Jeff Layton <[email protected]>
Looks good,
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
__forcein 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 hereThis 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_credI think sparse is complaining that we're casting away
__rcuwhen 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__forceto opaque verifier field casts
nfsd: clean up sparse endianness warnings innfscache.c
nfsd:nfsd_splice_readandnfsd_readvshould return__be32
nfsd: add appropriate__forcedirectives to filehandle generation codefs/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