fix encode_entryplus_baggage stack usage

PATCH In review J. Bruce Fields 2 participants 4 messages Jan 9, 2014 – Jan 23, 2014

Subsystems (2)

NFS client NFS server

lore

Share & tools

lore

J. Bruce Fields

author

2014-01-09 21:33 UTC #0 lore raw

Eric ran across an excessive stack with this (and a bunch of xfs code) to blame. Untested. I don't know whether it's worth the minor extra work here to avoid the alloc/free on each entry.

nfsd4_encode_fattr will need the same treatment.

--b.

commit 5b35a1f99f999555e82420e687e9d079ae796547

Author: J. Bruce Fields <[email protected]>
Date:   Thu Jan 9 16:24:35 2014 -0500

nfsd: fix encode_entryplus_baggage stack usage

We stick an extra svc_fh in nfsd3_readdirres to save the need to kmalloc, though maybe it would be fine to kmalloc instead.

Signed-off-by J. Bruce Fields <[email protected]>

fs/nfsd/nfs3xdr.c +6 -6

index 1ee6bae..de6e39e 100644
@@ -842,21 +842,21 @@ out:
 
 static __be32 *encode_entryplus_baggage(struct nfsd3_readdirres *cd, __be32 *p, const char *name, int namlen)
 {
-	struct svc_fh	fh;
+	struct svc_fh	*fh = &cd->scratch;
 	__be32 err;
 
-	fh_init(&fh, NFS3_FHSIZE);
-	err = compose_entry_fh(cd, &fh, name, namlen);
+	fh_init(fh, NFS3_FHSIZE);
+	err = compose_entry_fh(cd, fh, name, namlen);
 	if (err) {
 		*p++ = 0;
 		*p++ = 0;
 		goto out;
 	}
-	p = encode_post_op_attr(cd->rqstp, p, &fh);
+	p = encode_post_op_attr(cd->rqstp, p, fh);
 	*p++ = xdr_one;			/* yes, a file handle follows */
-	p = encode_fh(p, &fh);
+	p = encode_fh(p, fh);
 out:
-	fh_put(&fh);
+	fh_put(fh);
 	return p;
 }

fs/nfsd/xdr3.h +3 -0

index b6d5542..335e04a 100644
@@ -174,6 +174,9 @@ struct nfsd3_linkres {
 struct nfsd3_readdirres {
 	__be32			status;
 	struct svc_fh		fh;
+	/* Just to save kmalloc on every readdirplus entry (svc_fh is a
+	 * little large for the stack): */
+	struct svc_fh		scratch;
 	int			count;
 	__be32			verf[2];

Jeff Layton

Acked-by

2014-01-10 00:19 UTC #1 lore raw

Quoted 56 lines · On Thu, 9 Jan 2014 16:33:35 -0500 "J. Bruce Fields" <[email protected]>…

Eric ran across an excessive stack with this (and a bunch of xfs code) to blame. Untested. I don't know whether it's worth the minor extra work here to avoid the alloc/free on each entry.
nfsd4_encode_fattr will need the same treatment.
--b.
commit 5b35a1f99f999555e82420e687e9d079ae796547
Author: J. Bruce Fields <[email protected]>
Date:   Thu Jan 9 16:24:35 2014 -0500
nfsd: fix encode_entryplus_baggage stack usage
We stick an extra svc_fh in nfsd3_readdirres to save the need to kmalloc, though maybe it would be fine to kmalloc instead.
Signed-off-by J. Bruce Fields <[email protected]>
fs/nfsd/nfs3xdr.c +6 -6
index 1ee6bae..de6e39e 100644
@@ -842,21 +842,21 @@ out:
 
 static __be32 *encode_entryplus_baggage(struct nfsd3_readdirres *cd, __be32 *p, const char *name, int namlen)
 {
-	struct svc_fh	fh;
+	struct svc_fh	*fh = &cd->scratch;
 	__be32 err;
 
-	fh_init(&fh, NFS3_FHSIZE);
-	err = compose_entry_fh(cd, &fh, name, namlen);
+	fh_init(fh, NFS3_FHSIZE);
+	err = compose_entry_fh(cd, fh, name, namlen);
 	if (err) {
 		*p++ = 0;
 		*p++ = 0;
 		goto out;
 	}
-	p = encode_post_op_attr(cd->rqstp, p, &fh);
+	p = encode_post_op_attr(cd->rqstp, p, fh);
 	*p++ = xdr_one;			/* yes, a file handle follows */
-	p = encode_fh(p, &fh);
+	p = encode_fh(p, fh);
 out:
-	fh_put(&fh);
+	fh_put(fh);
 	return p;
 }
 
fs/nfsd/xdr3.h +0 -0
index b6d5542..335e04a 100644

fs/nfsd/xdr3.h struct nfsd3_linkres { · line 182

Quoted 9 lines

fs/nfsd/xdr3.h +3 -0

@@ -174,6 +174,9 @@ struct nfsd3_linkres {
 struct nfsd3_readdirres {
 	__be32			status;
 	struct svc_fh		fh;
+	/* Just to save kmalloc on every readdirplus entry (svc_fh is a
+	 * little large for the stack): */
+	struct svc_fh		scratch;
 	int			count;
 	__be32			verf[2];

I think this is probably the best solution...

Acked-by Jeff Layton <[email protected]>

J. Bruce Fields

author

2014-01-23 18:53 UTC #2 lore raw

PATCH 1/1 nfsd4: decrease nfsd4_encode_fattr stack usage

From: "J. Bruce Fields" <[email protected]>

A struct svc_fh is 320 bytes on x86_64, it'd be better not to have these on the stack.

kmalloc'ing them probably isn't ideal either, but this is the simplest thing to do. If it turns out to be a problem in the readdir case then we could add a svc_fh to nfsd4_readdir and pass that in.

Signed-off-by J. Bruce Fields <[email protected]>

---
 fs/nfsd/nfs4xdr.c |   16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

Quoted 2 lines · Jeff Layton

On Thu, 9 Jan 2014 16:33:35 -0500
Quoted 1 line
nfsd: fix encode_entryplus_baggage stack usage

...

Quoted 3 lines

I think this is probably the best solution...
Acked-by Jeff Layton <[email protected]>

Thanks. While we're at it, here's the v4 case.

--b.

fs/nfsd/nfs4xdr.c +10 -6

index 8198ecf..63f2395 100644
@@ -2058,7 +2058,7 @@ nfsd4_encode_fattr(struct svc_fh *fhp, struct svc_export *exp,
 	u32 bmval1 = bmval[1];
 	u32 bmval2 = bmval[2];
 	struct kstat stat;
-	struct svc_fh tempfh;
+	struct svc_fh *tempfh = NULL;
 	struct kstatfs statfs;
 	int buflen = count << 2;
 	__be32 *attrlenp;

@@ -2105,11 +2105,15 @@ nfsd4_encode_fattr(struct svc_fh *fhp, struct svc_export *exp,
 			goto out_nfserr;
 	}
 	if ((bmval0 & (FATTR4_WORD0_FILEHANDLE | FATTR4_WORD0_FSID)) && !fhp) {
-		fh_init(&tempfh, NFS4_FHSIZE);
-		status = fh_compose(&tempfh, exp, dentry, NULL);
+		tempfh = kmalloc(sizeof(struct svc_fh), GFP_KERNEL);
+		status = nfserr_jukebox;
+		if (!tempfh)
+			goto out;
+		fh_init(tempfh, NFS4_FHSIZE);
+		status = fh_compose(tempfh, exp, dentry, NULL);
 		if (status)
 			goto out;
-		fhp = &tempfh;
+		fhp = tempfh;
 	}
 	if (bmval0 & (FATTR4_WORD0_ACL | FATTR4_WORD0_ACLSUPPORT
 			| FATTR4_WORD0_SUPPORTED_ATTRS)) {

@@ -2495,8 +2499,8 @@ out:
 		security_release_secctx(context, contextlen);
 #endif /* CONFIG_NFSD_V4_SECURITY_LABEL */
 	kfree(acl);
-	if (fhp == &tempfh)
-		fh_put(&tempfh);
+	if (tempfh)
+		fh_put(tempfh);
 	return status;
 out_nfserr:
 	status = nfserrno(err);

Signature · 1 line

1.7.9.5

Jeff Layton

Acked-by on v1 1/1

2014-01-23 23:07 UTC #3 lore raw

Re: [PATCH] nfsd4: decrease nfsd4_encode_fattr stack usage

Quoted 59 lines · On Thu, 23 Jan 2014 13:53:05 -0500 "J. Bruce Fields" <[email protected]>…

From: "J. Bruce Fields" <[email protected]>
A struct svc_fh is 320 bytes on x86_64, it'd be better not to have these on the stack.
kmalloc'ing them probably isn't ideal either, but this is the simplest thing to do. If it turns out to be a problem in the readdir case then we could add a svc_fh to nfsd4_readdir and pass that in.
Signed-off-by J. Bruce Fields <[email protected]>
---
 fs/nfsd/nfs4xdr.c |   16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)
Quoted 2 lines · Jeff Layton
On Thu, 9 Jan 2014 16:33:35 -0500
Quoted 1 line
nfsd: fix encode_entryplus_baggage stack usage
...
Quoted 3 lines
I think this is probably the best solution...
Acked-by Jeff Layton <[email protected]>
Thanks. While we're at it, here's the v4 case.
--b.
fs/nfsd/nfs4xdr.c +8 -4
index 8198ecf..63f2395 100644
@@ -2058,7 +2058,7 @@ nfsd4_encode_fattr(struct svc_fh *fhp, struct svc_export *exp,
 	u32 bmval1 = bmval[1];
 	u32 bmval2 = bmval[2];
 	struct kstat stat;
-	struct svc_fh tempfh;
+	struct svc_fh *tempfh = NULL;
 	struct kstatfs statfs;
 	int buflen = count << 2;
 	__be32 *attrlenp;
@@ -2105,11 +2105,15 @@ nfsd4_encode_fattr(struct svc_fh *fhp, struct svc_export *exp,
 			goto out_nfserr;
 	}
 	if ((bmval0 & (FATTR4_WORD0_FILEHANDLE | FATTR4_WORD0_FSID)) && !fhp) {
-		fh_init(&tempfh, NFS4_FHSIZE);
-		status = fh_compose(&tempfh, exp, dentry, NULL);
+		tempfh = kmalloc(sizeof(struct svc_fh), GFP_KERNEL);
+		status = nfserr_jukebox;
+		if (!tempfh)
+			goto out;
+		fh_init(tempfh, NFS4_FHSIZE);
+		status = fh_compose(tempfh, exp, dentry, NULL);
 		if (status)
 			goto out;
-		fhp = &tempfh;
+		fhp = tempfh;
 	}
 	if (bmval0 & (FATTR4_WORD0_ACL | FATTR4_WORD0_ACLSUPPORT
 			| FATTR4_WORD0_SUPPORTED_ATTRS)) {

fs/nfsd/nfs4xdr.c out: · line 2506

Quoted 10 lines

fs/nfsd/nfs4xdr.c +2 -2

@@ -2495,8 +2499,8 @@ out:
 		security_release_secctx(context, contextlen);
 #endif /* CONFIG_NFSD_V4_SECURITY_LABEL */
 	kfree(acl);
-	if (fhp == &tempfh)
-		fh_put(&tempfh);
+	if (tempfh)
+		fh_put(tempfh);
 	return status;
 out_nfserr:
 	status = nfserrno(err);

Ok, different approach here. You're kmalloc'ing it separately instead of making it part of the nfsd4_readdir struct. Should be fine though...

Acked-by Jeff Layton <[email protected]>