struct nfs_fattr alignment problem in nfs3proc.c

struct nfs_fattr alignment problem in nfs3proc.c

[Richard Curnow]

In the nfs3_proc_unlink_setup function, there appears to be a bug with
the way kmalloc is used to allocate storage for two structs grouped
together.  The second struct ends up with a non 8-byte aligned pointer,
which can cause trouble later (in xdr_decode_fattr) when stores occur to
the u64 fields inside it.  The following patch (on 2.4.19) fixes this
problem, though I'm not sure if it's the cleanest fix.  (I hit this when
working on the port to SH-5, which is currently baselined on 2.4.19).

--- ../linux-2.4/fs/nfs/nfs3proc.c	Fri Jan  3 15:01:17 2003
+++ fs/nfs/nfs3proc.c	Fri Mar 21 17:42:38 2003
@@ -294,25 +294,28 @@
 	nfs_refresh_inode(dir, &dir_attr);
 	dprintk("NFS reply remove: %d\n", status);
 	return status;
 }
 
 static int
 nfs3_proc_unlink_setup(struct rpc_message *msg, struct dentry *dir, struct qstr *name)
 {
 	struct nfs3_diropargs	*arg;
 	struct nfs_fattr	*res;
+	int arg_size_8, res_size_8;
 
-	arg = (struct nfs3_diropargs *)kmalloc(sizeof(*arg)+sizeof(*res), GFP_KERNEL);
+	arg_size_8 = (sizeof(*arg) + 7) & ~7;
+	res_size_8 = (sizeof(*res) + 7) & ~7;
+	arg = (struct nfs3_diropargs *)kmalloc(arg_size_8 + res_size_8, GFP_KERNEL);
 	if (!arg)
 		return -ENOMEM;
-	res = (struct nfs_fattr*)(arg + 1);
+	res = (struct nfs_fattr*)((char *)arg + arg_size_8);
 	arg->fh = NFS_FH(dir->d_inode);
 	arg->name = name->name;
 	arg->len = name->len;
 	res->valid = 0;
 	msg->rpc_proc = NFS3PROC_REMOVE;
 	msg->rpc_argp = arg;
 	msg->rpc_resp = res;
 	return 0;
 }
 

-- 
Richard \\\ SuperH Core+Debug Architect /// .. At home ..
  P.    /// richard.curnow@superh.com  ///  rc@rc0.org.uk
Curnow  \\\ http://www.superh.com/    ///  www.rc0.org.uk

Re: struct nfs_fattr alignment problem in nfs3proc.c

[Trond Myklebust]

>>>>> " " == Richard Curnow <Richard.Curnow@superh.com> writes:

     > In the nfs3_proc_unlink_setup function, there appears to be a
     > bug with the way kmalloc is used to allocate storage for two
     > structs grouped together.  The second struct ends up with a non
     > 8-byte aligned pointer, which can cause trouble later (in
     > xdr_decode_fattr) when stores occur to the u64 fields inside
     > it.  The following patch (on 2.4.19) fixes this problem, though
     > I'm not sure if it's the cleanest fix.  (I hit this when
     > working on the port to SH-5, which is currently baselined on
     > 2.4.19).

Why not just define

struct {
        struct nfs3_diropargs   arg;
        struct nfs_fattr        res;
} unlinkxdr;

and then kmalloc that?

Cheers,
  Trond

Re: struct nfs_fattr alignment problem in nfs3proc.c

[Richard Curnow]

* Trond Myklebust <trond.myklebust@fys.uio.no> [2003-03-22]:
> Why not just define
> 
> struct {
>         struct nfs3_diropargs   arg;
>         struct nfs_fattr        res;
> } unlinkxdr;
> 
> and then kmalloc that?
> 

Yes, that's much better, since it also avoids bloating the allocation on
architectures that only need 4-byte alignment for later stores into the
result structures to work, plus it's more future-proof.  Here's the
revised patch.

--- ../linux-2.4/fs/nfs/nfs3proc.c	Fri Jan  3 15:01:17 2003
+++ fs/nfs/nfs3proc.c	Mon Mar 24 09:39:21 2003
@@ -294,25 +294,30 @@
 	nfs_refresh_inode(dir, &dir_attr);
 	dprintk("NFS reply remove: %d\n", status);
 	return status;
 }
 
 static int
 nfs3_proc_unlink_setup(struct rpc_message *msg, struct dentry *dir, struct qstr *name)
 {
 	struct nfs3_diropargs	*arg;
 	struct nfs_fattr	*res;
+	struct unlinkxdr {
+		struct nfs3_diropargs	*arg;
+		struct nfs_fattr	*res;
+	} *ptr;
 
-	arg = (struct nfs3_diropargs *)kmalloc(sizeof(*arg)+sizeof(*res), GFP_KERNEL);
-	if (!arg)
+	ptr = (struct unlinkxdr *) kmalloc(sizeof(struct unlinkxdr), GFP_KERNEL);
+	if (!ptr)
 		return -ENOMEM;
-	res = (struct nfs_fattr*)(arg + 1);
+	arg = &ptr->arg;
+	res = &ptr->res;
 	arg->fh = NFS_FH(dir->d_inode);
 	arg->name = name->name;
 	arg->len = name->len;
 	res->valid = 0;
 	msg->rpc_proc = NFS3PROC_REMOVE;
 	msg->rpc_argp = arg;
 	msg->rpc_resp = res;
 	return 0;
 }
 

Cheers
Richard

-- 
Richard \\\ SuperH Core+Debug Architect /// .. At home ..
  P.    /// richard.curnow@superh.com  ///  rc@rc0.org.uk
Curnow  \\\ http://www.superh.com/    ///  www.rc0.org.uk
Speaking for myself, not on behalf of SuperH

Re: struct nfs_fattr alignment problem in nfs3proc.c

[Richard Curnow]

* Richard Curnow <Richard.Curnow@superh.com> [2003-03-24]:
> +	struct unlinkxdr {
> +		struct nfs3_diropargs	*arg;
> +		struct nfs_fattr	*res;
> +	} *ptr;

Sorry, those asterisks are not good.  Instead:

--- ../linux-2.4/fs/nfs/nfs3proc.c	Fri Jan  3 15:01:17 2003
+++ fs/nfs/nfs3proc.c	Mon Mar 24 13:06:36 2003
@@ -294,25 +294,36 @@
 	nfs_refresh_inode(dir, &dir_attr);
 	dprintk("NFS reply remove: %d\n", status);
 	return status;
 }
 
 static int
 nfs3_proc_unlink_setup(struct rpc_message *msg, struct dentry *dir, struct qstr *name)
 {
 	struct nfs3_diropargs	*arg;
 	struct nfs_fattr	*res;
+	struct unlinkxdr {
+		struct nfs3_diropargs	arg;
+		struct nfs_fattr	res;
+	} *ptr;
 
-	arg = (struct nfs3_diropargs *)kmalloc(sizeof(*arg)+sizeof(*res), GFP_KERNEL);
-	if (!arg)
+	ptr = (struct unlinkxdr *) kmalloc(sizeof(struct unlinkxdr), GFP_KERNEL);
+	if (!ptr)
 		return -ENOMEM;
-	res = (struct nfs_fattr*)(arg + 1);
+	arg = &ptr->arg;
+	res = &ptr->res;
+	if (((unsigned long) arg & 0x7) != 0) {
+		printk("nfs3_proc_unlink_setup : arg not 8-byte aligned!\n");
+	}
+	if (((unsigned long) res & 0x7) != 0) {
+		printk("nfs3_proc_unlink_setup : res not 8-byte aligned!\n");
+	}
 	arg->fh = NFS_FH(dir->d_inode);
 	arg->name = name->name;
 	arg->len = name->len;
 	res->valid = 0;
 	msg->rpc_proc = NFS3PROC_REMOVE;
 	msg->rpc_argp = arg;
 	msg->rpc_resp = res;
 	return 0;
 }

-- 
Richard \\\ SuperH Core+Debug Architect /// .. At home ..
  P.    /// richard.curnow@superh.com  ///  rc@rc0.org.uk
Curnow  \\\ http://www.superh.com/    ///  www.rc0.org.uk
Speaking for myself, not on behalf of SuperH

Re: struct nfs_fattr alignment problem in nfs3proc.c

[Trond Myklebust]

>>>>> " " == Richard Curnow <Richard.Curnow@superh.com> writes:

     > + if (((unsigned long) arg & 0x7) != 0) {
     > + printk("nfs3_proc_unlink_setup : arg not 8-byte aligned!\n");
     > + }
     > + if (((unsigned long) res & 0x7) != 0) {
     > + printk("nfs3_proc_unlink_setup : res not 8-byte aligned!\n");
     > + }

Nope...

Cheers,
  Trond

Re: struct nfs_fattr alignment problem in nfs3proc.c

[Richard Curnow]

* Trond Myklebust <trond.myklebust@fys.uio.no> [2003-03-24]:
> >>>>> " " == Richard Curnow <Richard.Curnow@superh.com> writes:
> 
>      > + if (((unsigned long) arg & 0x7) != 0) {
>      > + printk("nfs3_proc_unlink_setup : arg not 8-byte aligned!\n");
>      > + }
>      > + if (((unsigned long) res & 0x7) != 0) {
>      > + printk("nfs3_proc_unlink_setup : res not 8-byte aligned!\n");
>      > + }
> 
> Nope...
> 

Well spotted, ignore those checks :-)  At least they showed that the
change higher up was making a difference.

Once there's general agreement about this fix, I'll generate some
patches for up-to-date 2.4 and 2.5.

Cheers
Richard

-- 
Richard \\\ SuperH Core+Debug Architect /// .. At home ..
  P.    /// richard.curnow@superh.com  ///  rc@rc0.org.uk
Curnow  \\\ http://www.superh.com/    ///  www.rc0.org.uk
Speaking for myself, not on behalf of SuperH