[2/3] NFSD fix nfserro errno mismatch
diff mbox series

Message ID 20191204201354.17557-3-olga.kornievskaia@gmail.com
State New
Headers show
Series
  • NFSD copy offload fixes
Related show

Commit Message

Olga Kornievskaia Dec. 4, 2019, 8:13 p.m. UTC
There is mismatch between __be32 and u32 in nfserr and errno.

Reported-by: kbuild test robot <lkp@intel.com>
Fixes: d5e54eeb0e3d ("NFSD add nfs4 inter ssc to nfsd4_copy")
Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
---
 fs/nfsd/nfs4proc.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

Comments

J. Bruce Fields Dec. 5, 2019, 9:39 p.m. UTC | #1
On Wed, Dec 04, 2019 at 03:13:53PM -0500, Olga Kornievskaia wrote:
> There is mismatch between __be32 and u32 in nfserr and errno.
> 
...
> @@ -1280,7 +1279,7 @@ extern struct file *nfs42_ssc_open(struct vfsmount *ss_mnt,
>  
>  	copy->c_fh.size = s_fh->fh_handle.fh_size;
>  	memcpy(copy->c_fh.data, &s_fh->fh_handle.fh_base, copy->c_fh.size);
> -	copy->stateid.seqid = s_stid->si_generation;
> +	copy->stateid.seqid = cpu_to_be32(s_stid->si_generation);

This one isn't an errno, and should really be its own patch.  I've split
it out as follows.--b.

commit a1f3cb8bb088
Author: Olga Kornievskaia <olga.kornievskaia@gmail.com>
Date:   Wed Dec 4 15:13:53 2019 -0500

    NFSD: fix seqid in copy stateid
    
    s_stid->si_generation is a u32, copy->stateid.seqid is a __be32, so we
    should be byte-swapping here if necessary.
    
    This effectively undoes the byte-swap performed when reading
    s_stid->s_generation in nfsd4_decode_copy().  Without this second swap,
    the stateid we sent to the source in READ could be different from the
    one the client provided us in the COPY.  We didn't spot this in testing
    since our implementation always uses a 0 in the seqid field.  But other
    implementations might not do that.
    
    You'd think we should just skip the byte-swapping entirely, but the
    s_stid field can be used for either our own stateids (in the
    intra-server case) or foreign stateids (in the inter-server case), and
    the former are interpreted by us and need byte-swapping.
    
    Reported-by: kbuild test robot <lkp@intel.com>
    Fixes: d5e54eeb0e3d ("NFSD add nfs4 inter ssc to nfsd4_copy")
    Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
    Signed-off-by: J. Bruce Fields <bfields@redhat.com>

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index ec4f79c8f71e..9a8debc0d725 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -1280,7 +1280,7 @@ nfsd4_setup_inter_ssc(struct svc_rqst *rqstp,
 
 	copy->c_fh.size = s_fh->fh_handle.fh_size;
 	memcpy(copy->c_fh.data, &s_fh->fh_handle.fh_base, copy->c_fh.size);
-	copy->stateid.seqid = s_stid->si_generation;
+	copy->stateid.seqid = cpu_to_be32(s_stid->si_generation);
 	memcpy(copy->stateid.other, (void *)&s_stid->si_opaque,
 	       sizeof(stateid_opaque_t));
J. Bruce Fields Dec. 5, 2019, 9:43 p.m. UTC | #2
On Thu, Dec 05, 2019 at 04:39:30PM -0500, bfields wrote:
> On Wed, Dec 04, 2019 at 03:13:53PM -0500, Olga Kornievskaia wrote:
> > There is mismatch between __be32 and u32 in nfserr and errno.
> > 
> ...
> > @@ -1280,7 +1279,7 @@ extern struct file *nfs42_ssc_open(struct vfsmount *ss_mnt,
> >  
> >  	copy->c_fh.size = s_fh->fh_handle.fh_size;
> >  	memcpy(copy->c_fh.data, &s_fh->fh_handle.fh_base, copy->c_fh.size);
> > -	copy->stateid.seqid = s_stid->si_generation;
> > +	copy->stateid.seqid = cpu_to_be32(s_stid->si_generation);
> 
> This one isn't an errno, and should really be its own patch.  I've split
> it out as follows.--b.

(And applied the others, thanks.)

> 
> commit a1f3cb8bb088
> Author: Olga Kornievskaia <olga.kornievskaia@gmail.com>
> Date:   Wed Dec 4 15:13:53 2019 -0500
> 
>     NFSD: fix seqid in copy stateid
>     
>     s_stid->si_generation is a u32, copy->stateid.seqid is a __be32, so we
>     should be byte-swapping here if necessary.
>     
>     This effectively undoes the byte-swap performed when reading
>     s_stid->s_generation in nfsd4_decode_copy().  Without this second swap,
>     the stateid we sent to the source in READ could be different from the
>     one the client provided us in the COPY.  We didn't spot this in testing
>     since our implementation always uses a 0 in the seqid field.  But other
>     implementations might not do that.
>     
>     You'd think we should just skip the byte-swapping entirely, but the
>     s_stid field can be used for either our own stateids (in the
>     intra-server case) or foreign stateids (in the inter-server case), and
>     the former are interpreted by us and need byte-swapping.
>     
>     Reported-by: kbuild test robot <lkp@intel.com>
>     Fixes: d5e54eeb0e3d ("NFSD add nfs4 inter ssc to nfsd4_copy")
>     Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
>     Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> 
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index ec4f79c8f71e..9a8debc0d725 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -1280,7 +1280,7 @@ nfsd4_setup_inter_ssc(struct svc_rqst *rqstp,
>  
>  	copy->c_fh.size = s_fh->fh_handle.fh_size;
>  	memcpy(copy->c_fh.data, &s_fh->fh_handle.fh_base, copy->c_fh.size);
> -	copy->stateid.seqid = s_stid->si_generation;
> +	copy->stateid.seqid = cpu_to_be32(s_stid->si_generation);
>  	memcpy(copy->stateid.other, (void *)&s_stid->si_opaque,
>  	       sizeof(stateid_opaque_t));
>

Patch
diff mbox series

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index ec4f79c8..187cef6 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -1169,7 +1169,8 @@  extern struct file *nfs42_ssc_open(struct vfsmount *ss_mnt,
 	size_t tmp_addrlen, match_netid_len = 3;
 	char *startsep = "", *endsep = "", *match_netid = "tcp";
 	char *ipaddr, *dev_name, *raw_data;
-	int len, raw_len, status = -EINVAL;
+	int len, raw_len;
+	__be32 status = nfserr_inval;
 
 	naddr = &nss->u.nl4_addr;
 	tmp_addrlen = rpc_uaddr2sockaddr(SVC_NET(rqstp), naddr->addr,
@@ -1207,7 +1208,7 @@  extern struct file *nfs42_ssc_open(struct vfsmount *ss_mnt,
 
 	snprintf(raw_data, raw_len, NFSD42_INTERSSC_MOUNTOPS, ipaddr);
 
-	status = -ENODEV;
+	status = nfserr_nodev;
 	type = get_fs_type("nfs");
 	if (!type)
 		goto out_free_rawdata;
@@ -1253,8 +1254,6 @@  extern struct file *nfs42_ssc_open(struct vfsmount *ss_mnt,
  * Called with COPY cstate:
  *    SAVED_FH: source filehandle
  *    CURRENT_FH: destination filehandle
- *
- * Returns errno (not nfserrxxx)
  */
 static __be32
 nfsd4_setup_inter_ssc(struct svc_rqst *rqstp,
@@ -1263,7 +1262,7 @@  extern struct file *nfs42_ssc_open(struct vfsmount *ss_mnt,
 {
 	struct svc_fh *s_fh = NULL;
 	stateid_t *s_stid = &copy->cp_src_stateid;
-	__be32 status = -EINVAL;
+	__be32 status = nfserr_inval;
 
 	/* Verify the destination stateid and set dst struct file*/
 	status = nfs4_preprocess_stateid_op(rqstp, cstate, &cstate->current_fh,
@@ -1280,7 +1279,7 @@  extern struct file *nfs42_ssc_open(struct vfsmount *ss_mnt,
 
 	copy->c_fh.size = s_fh->fh_handle.fh_size;
 	memcpy(copy->c_fh.data, &s_fh->fh_handle.fh_base, copy->c_fh.size);
-	copy->stateid.seqid = s_stid->si_generation;
+	copy->stateid.seqid = cpu_to_be32(s_stid->si_generation);
 	memcpy(copy->stateid.other, (void *)&s_stid->si_opaque,
 	       sizeof(stateid_opaque_t));
 
@@ -1308,7 +1307,7 @@  extern struct file *nfs42_ssc_open(struct vfsmount *ss_mnt,
 		      struct vfsmount **mount)
 {
 	*mount = NULL;
-	return -EINVAL;
+	return nfserr_inval;
 }
 
 static void