Skip to contentSkip to conversation

nfsd: fix rare symlink decoding bug

PATCH New J. Bruce Fields 1 participant 8 messages Jun 26, 2014 1 message not in the archive

Subsystems (2)
NFS client NFS server
lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

J. Bruce Fields

author
#1 lore raw

PATCH 1/8 nfsd: fix rare symlink decoding bug

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

An NFS operation that creates a new symlink includes the symlink data, which is xdr-encoded as a length followed by the data plus 0 to 3 bytes of zero-padding as required to reach a 4-byte boundary.

The vfs, on the other hand, wants null-terminated data.

The simple way to handle this would be by copying the data into a newly allocated buffer with space for the final null.

The current nfsd_symlink code tries to be more clever by skipping that step in the (likely) case where the byte following the string is already 0.

But that assumes that the byte following the string is ours to look at. In fact, it might be the first byte of a page that we can't read, or of some object that another task might modify.

Worse, the NFSv4 code tries to fix the problem by actually writing to that byte.

In the NFSv2/v3 cases this actually appears to be safe:

- nfs3svc_decode_symlinkargs explicitly null-terminates the data (after first checking its length and copying it to a new page).
- NFSv2 limits symlinks to 1k. The buffer holding the rpc request is always at least a page, and the link data (and previous fields) have maximum lengths that prevent the request from reaching the end of a page.

In the NFSv4 case the CREATE op is potentially just one part of a long compound so can end up on the end of a page if you're unlucky.

The minimal fix here is to copy and null-terminate in the NFSv4 case. The nfsd_symlink() interface here seems too fragile, though. It should really either do the copy itself every time or just require a null-terminated string.

Reported-by Jeff Layton <[email protected]>
Signed-off-by J. Bruce Fields <[email protected]>
---
 fs/nfsd/nfs4proc.c |    9 ---------
 fs/nfsd/nfs4xdr.c  |   13 ++++++++++++-
 2 files changed, 12 insertions(+), 10 deletions(-)
fs/nfsd/nfs4proc.c +0 -9
index a6be9d3..2b3795a 100644
@@ -621,15 +621,6 @@ nfsd4_create(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 
 	switch (create->cr_type) {
 	case NF4LNK:
-		/* ugh! we have to null-terminate the linktext, or
-		 * vfs_symlink() will choke.  it is always safe to
-		 * null-terminate by brute force, since at worst we
-		 * will overwrite the first byte of the create namelen
-		 * in the XDR buffer, which has already been extracted
-		 * during XDR decode.
-		 */
-		create->cr_linkname[create->cr_linklen] = 0;
-
 		status = nfsd_symlink(rqstp, &cstate->current_fh,
 				      create->cr_name, create->cr_namelen,
 				      create->cr_linkname, create->cr_linklen,
fs/nfsd/nfs4xdr.c +12 -1
index 30913c8..a1c48b4 100644
@@ -600,7 +600,18 @@ nfsd4_decode_create(struct nfsd4_compoundargs *argp, struct nfsd4_create *create
 		READ_BUF(4);
 		create->cr_linklen = be32_to_cpup(p++);
 		READ_BUF(create->cr_linklen);
-		SAVEMEM(create->cr_linkname, create->cr_linklen);
+		/*
+		 * The VFS will want a null-terminated string, and
+		 * null-terminating in place isn't safe since this might
+		 * end on a page boundary:
+		 */
+		create->cr_linkname =
+				kmalloc(create->cr_linklen + 1, GFP_KERNEL);
+		if (!create->cr_linkname)
+			return nfserr_jukebox;
+		memcpy(create->cr_linkname, p, create->cr_linklen);
+		create->cr_linkname[create->cr_linklen] = '\0';
+		defer_free(argp, kfree, create->cr_linkname);
 		break;
 	case NF4BLK:
 	case NF4CHR:
Signature · 1 line
1.7.9.5
author
#2 lore raw

PATCH 2/8 nfsd: make NFSv2 null terminate symlink data

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

It's simple enough for NFSv2 to null-terminate the symlink data.

A bit weird (it depends on knowing that we've already read the following byte, which is either padding or part of the mode), but no worse than the conditional kstrdup it otherwise relies on in nfsd_symlink().

Signed-off-by J. Bruce Fields <[email protected]>
---
 fs/nfsd/nfsproc.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
fs/nfsd/nfsproc.c +4 -1
index 54c6b3d..aebe23c 100644
@@ -403,8 +403,11 @@ nfsd_proc_symlink(struct svc_rqst *rqstp, struct nfsd_symlinkargs *argp,
 
 	fh_init(&newfh, NFS_FHSIZE);
 	/*
-	 * Create the link, look up new file and set attrs.
+	 * Crazy hack: the request fits in a page, and already-decoded
+	 * attributes follow argp->tname, so it's safe to just write a
+	 * null to ensure it's null-terminated:
 	 */
+	argp->tname[argp->tlen] = '\0';
 	nfserr = nfsd_symlink(rqstp, &argp->ffh, argp->fname, argp->flen,
 						 argp->tname, argp->tlen,
 				 		 &newfh, &argp->attrs);
Signature · 1 line
1.7.9.5
author
#3 lore raw

PATCH 3/8 nfsd: let nfsd_symlink assume null-terminated data

In reply to J. Bruce Fields

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

Currently nfsd_symlink has a weird hack to serve callers who don't null-terminate symlink data: it looks ahead at the next byte to see if it's zero, and copies it to a new buffer to null-terminate if not.

That means callers don't have to null-terminate, but they *do* have to ensure that the byte following the end of the data is theirs to read.

That's a bit subtle, and the NFSv4 code actually got this wrong.

So let's just throw out that code and let callers pass null-terminated strings; we've already fixed them to do that.

Signed-off-by J. Bruce Fields <[email protected]>
---
 fs/nfsd/nfs3proc.c |    2 +-
 fs/nfsd/nfs4proc.c |    2 +-
 fs/nfsd/nfsproc.c  |    2 +-
 fs/nfsd/vfs.c      |   17 +++--------------
 fs/nfsd/vfs.h      |    2 +-
 5 files changed, 7 insertions(+), 18 deletions(-)
fs/nfsd/nfs3proc.c +1 -1
index 61ef42c..19ba233 100644
@@ -282,7 +282,7 @@ nfsd3_proc_symlink(struct svc_rqst *rqstp, struct nfsd3_symlinkargs *argp,
 	fh_copy(&resp->dirfh, &argp->ffh);
 	fh_init(&resp->fh, NFS3_FHSIZE);
 	nfserr = nfsd_symlink(rqstp, &resp->dirfh, argp->fname, argp->flen,
-						   argp->tname, argp->tlen,
+						   argp->tname,
 						   &resp->fh, &argp->attrs);
 	RETURN_STATUS(nfserr);
 }
fs/nfsd/nfs4proc.c +1 -1
index 2b3795a..7aa83bf 100644
@@ -623,7 +623,7 @@ nfsd4_create(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	case NF4LNK:
 		status = nfsd_symlink(rqstp, &cstate->current_fh,
 				      create->cr_name, create->cr_namelen,
-				      create->cr_linkname, create->cr_linklen,
+				      create->cr_linkname,
 				      &resfh, &create->cr_iattr);
 		break;
fs/nfsd/nfsproc.c +1 -1
index aebe23c..583ed03 100644
@@ -409,7 +409,7 @@ nfsd_proc_symlink(struct svc_rqst *rqstp, struct nfsd_symlinkargs *argp,
 	 */
 	argp->tname[argp->tlen] = '\0';
 	nfserr = nfsd_symlink(rqstp, &argp->ffh, argp->fname, argp->flen,
-						 argp->tname, argp->tlen,
+						 argp->tname,
 				 		 &newfh, &argp->attrs);
fs/nfsd/vfs.c +3 -14
index 6ffaa70..7518c65 100644
@@ -1504,7 +1504,7 @@ out_nfserr:
 __be32
 nfsd_symlink(struct svc_rqst *rqstp, struct svc_fh *fhp,
 				char *fname, int flen,
-				char *path,  int plen,
+				char *path,
 				struct svc_fh *resfhp,
 				struct iattr *iap)
 {

@@ -1513,7 +1513,7 @@ nfsd_symlink(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	int		host_err;
 
 	err = nfserr_noent;
-	if (!flen || !plen)
+	if (!flen || path[0] == '\0')
 		goto out;
 	err = nfserr_exist;
 	if (isdotent(fname, flen))

@@ -1534,18 +1534,7 @@ nfsd_symlink(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	if (IS_ERR(dnew))
 		goto out_nfserr;
 
-	if (unlikely(path[plen] != 0)) {
-		char *path_alloced = kmalloc(plen+1, GFP_KERNEL);
-		if (path_alloced == NULL)
-			host_err = -ENOMEM;
-		else {
-			strncpy(path_alloced, path, plen);
-			path_alloced[plen] = 0;
-			host_err = vfs_symlink(dentry->d_inode, dnew, path_alloced);
-			kfree(path_alloced);
-		}
-	} else
-		host_err = vfs_symlink(dentry->d_inode, dnew, path);
+	host_err = vfs_symlink(dentry->d_inode, dnew, path);
 	err = nfserrno(host_err);
 	if (!err)
 		err = nfserrno(commit_metadata(fhp));
fs/nfsd/vfs.h +1 -1
index b84aef5..20e4b66 100644
@@ -85,7 +85,7 @@ __be32 		nfsd_write(struct svc_rqst *, struct svc_fh *,struct file *,
 __be32		nfsd_readlink(struct svc_rqst *, struct svc_fh *,
 				char *, int *);
 __be32		nfsd_symlink(struct svc_rqst *, struct svc_fh *,
-				char *name, int len, char *path, int plen,
+				char *name, int len, char *path,
 				struct svc_fh *res, struct iattr *);
 __be32		nfsd_link(struct svc_rqst *, struct svc_fh *,
 				char *, int, struct svc_fh *);
Signature · 1 line
1.7.9.5
author
#4 lore raw

PATCH 4/8 nfsd4: rename cr_linkname->cr_data

In reply to J. Bruce Fields

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

The name of a link is currently stored in cr_name and cr_namelen, and the content in cr_linkname and cr_linklen. That's confusing.

Signed-off-by J. Bruce Fields <[email protected]>
---
 fs/nfsd/nfs4proc.c |    2 +-
 fs/nfsd/nfs4xdr.c  |   15 +++++++--------
 fs/nfsd/xdr4.h     |    8 ++++----
 3 files changed, 12 insertions(+), 13 deletions(-)
fs/nfsd/nfs4proc.c +1 -1
index 7aa83bf..b57c882 100644
@@ -623,7 +623,7 @@ nfsd4_create(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	case NF4LNK:
 		status = nfsd_symlink(rqstp, &cstate->current_fh,
 				      create->cr_name, create->cr_namelen,
-				      create->cr_linkname,
+				      create->cr_data,
 				      &resfh, &create->cr_iattr);
 		break;
fs/nfsd/nfs4xdr.c +7 -8
index a1c48b4..3d07496 100644
@@ -598,20 +598,19 @@ nfsd4_decode_create(struct nfsd4_compoundargs *argp, struct nfsd4_create *create
 	switch (create->cr_type) {
 	case NF4LNK:
 		READ_BUF(4);
-		create->cr_linklen = be32_to_cpup(p++);
-		READ_BUF(create->cr_linklen);
+		create->cr_datalen = be32_to_cpup(p++);
+		READ_BUF(create->cr_datalen);
 		/*
 		 * The VFS will want a null-terminated string, and
 		 * null-terminating in place isn't safe since this might
 		 * end on a page boundary:
 		 */
-		create->cr_linkname =
-				kmalloc(create->cr_linklen + 1, GFP_KERNEL);
-		if (!create->cr_linkname)
+		create->cr_data = kmalloc(create->cr_datalen + 1, GFP_KERNEL);
+		if (!create->cr_data)
 			return nfserr_jukebox;
-		memcpy(create->cr_linkname, p, create->cr_linklen);
-		create->cr_linkname[create->cr_linklen] = '\0';
-		defer_free(argp, kfree, create->cr_linkname);
+		memcpy(create->cr_data, p, create->cr_datalen);
+		create->cr_data[create->cr_datalen] = '\0';
+		defer_free(argp, kfree, create->cr_data);
 		break;
 	case NF4BLK:
 	case NF4CHR:
fs/nfsd/xdr4.h +4 -4
index 18cbb6d..b8bf63a 100644
@@ -107,8 +107,8 @@ struct nfsd4_create {
 	u32		cr_type;            /* request */
 	union {                             /* request */
 		struct {
-			u32 namelen;
-			char *name;
+			u32 datalen;
+			char *data;
 		} link;   /* NF4LNK */
 		struct {
 			u32 specdata1;

@@ -121,8 +121,8 @@ struct nfsd4_create {
 	struct nfs4_acl *cr_acl;
 	struct xdr_netobj cr_label;
 };
-#define cr_linklen	u.link.namelen
-#define cr_linkname	u.link.name
+#define cr_datalen	u.link.datalen
+#define cr_data		u.link.data
 #define cr_specdata1	u.dev.specdata1
 #define cr_specdata2	u.dev.specdata2
Signature · 1 line
1.7.9.5
author
#5 lore raw

PATCH 5/8 nfsd4: remove unused defer_free argument

In reply to J. Bruce Fields

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

28e05dd8457c "knfsd: nfsd4: represent nfsv4 acl with array instead of linked list" removed the last user that wanted a custom free function.

Signed-off-by J. Bruce Fields <[email protected]>
---
 fs/nfsd/nfs4xdr.c |   21 +++++++++------------
 fs/nfsd/xdr4.h    |    1 -
 2 files changed, 9 insertions(+), 13 deletions(-)
fs/nfsd/nfs4xdr.c +9 -12
index 3d07496..13f91ce 100644
@@ -182,16 +182,14 @@ static int zero_clientid(clientid_t *clid)
 
 /**
  * defer_free - mark an allocation as deferred freed
- * @argp: NFSv4 compound argument structure to be freed with
- * @release: release callback to free @p, typically kfree()
- * @p: pointer to be freed
+ * @argp: NFSv4 compound argument structure
+ * @p: pointer to be freed (with kfree())
  *
  * Marks @p to be freed when processing the compound operation
  * described in @argp finishes.
  */
 static int
-defer_free(struct nfsd4_compoundargs *argp,
-		void (*release)(const void *), void *p)
+defer_free(struct nfsd4_compoundargs *argp, void *p)
 {
 	struct tmpbuf *tb;
 

@@ -199,7 +197,6 @@ defer_free(struct nfsd4_compoundargs *argp,
 	if (!tb)
 		return -ENOMEM;
 	tb->buf = p;
-	tb->release = release;
 	tb->next = argp->to_free;
 	argp->to_free = tb;
 	return 0;

@@ -225,7 +222,7 @@ static char *savemem(struct nfsd4_compoundargs *argp, __be32 *p, int nbytes)
 		BUG_ON(p != argp->tmpp);
 		argp->tmpp = NULL;
 	}
-	if (defer_free(argp, kfree, p)) {
+	if (defer_free(argp, p)) {
 		kfree(p);
 		return NULL;
 	} else

@@ -296,7 +293,7 @@ nfsd4_decode_fattr(struct nfsd4_compoundargs *argp, u32 *bmval,
 		if (*acl == NULL)
 			return nfserr_jukebox;
 
-		defer_free(argp, kfree, *acl);
+		defer_free(argp, *acl);
 
 		(*acl)->naces = nace;
 		for (ace = (*acl)->aces; ace < (*acl)->aces + nace; ace++) {

@@ -422,7 +419,7 @@ nfsd4_decode_fattr(struct nfsd4_compoundargs *argp, u32 *bmval,
 		if (!label->data)
 			return nfserr_jukebox;
 		label->len = dummy32;
-		defer_free(argp, kfree, label->data);
+		defer_free(argp, label->data);
 		memcpy(label->data, buf, dummy32);
 	}
 #endif

@@ -610,7 +607,7 @@ nfsd4_decode_create(struct nfsd4_compoundargs *argp, struct nfsd4_create *create
 			return nfserr_jukebox;
 		memcpy(create->cr_data, p, create->cr_datalen);
 		create->cr_data[create->cr_datalen] = '\0';
-		defer_free(argp, kfree, create->cr_data);
+		defer_free(argp, create->cr_data);
 		break;
 	case NF4BLK:
 	case NF4CHR:

@@ -1486,7 +1483,7 @@ nfsd4_decode_test_stateid(struct nfsd4_compoundargs *argp, struct nfsd4_test_sta
 			goto out;
 		}
 
-		defer_free(argp, kfree, stateid);
+		defer_free(argp, stateid);
 		INIT_LIST_HEAD(&stateid->ts_id_list);
 		list_add_tail(&stateid->ts_id_list, &test_stateid->ts_stateid_list);
 

@@ -3972,7 +3969,7 @@ int nfsd4_release_compoundargs(void *rq, __be32 *p, void *resp)
 	while (args->to_free) {
 		struct tmpbuf *tb = args->to_free;
 		args->to_free = tb->next;
-		tb->release(tb->buf);
+		kfree(tb->buf);
 		kfree(tb);
 	}
 	return 1;
fs/nfsd/xdr4.h +0 -1
index b8bf63a..4379cc8 100644
@@ -488,7 +488,6 @@ struct nfsd4_compoundargs {
 	__be32 *			tmpp;
 	struct tmpbuf {
 		struct tmpbuf *next;
-		void (*release)(const void *);
 		void *buf;
 	}				*to_free;
Signature · 1 line
1.7.9.5
author
#6 lore raw

PATCH 6/8 nfsd4: define svcxdr_dupstr to share some common code

In reply to J. Bruce Fields

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

Signed-off-by J. Bruce Fields <[email protected]>
---
 fs/nfsd/nfs4xdr.c |   36 +++++++++++++++++++++++-------------
 1 file changed, 23 insertions(+), 13 deletions(-)
fs/nfsd/nfs4xdr.c +23 -13
index 13f91ce..8fb0f37 100644
@@ -202,6 +202,26 @@ defer_free(struct nfsd4_compoundargs *argp, void *p)
 	return 0;
 }
 
+/*
+ * For xdr strings that need to be passed to other kernel api's
+ * as null-terminated strings.
+ *
+ * Note null-terminating in place usually isn't safe since the
+ * buffer might end on a page boundary.
+ */
+static char *
+svcxdr_dupstr(struct nfsd4_compoundargs *argp, void *buf, u32 len)
+{
+	char *p = kmalloc(len + 1, GFP_KERNEL);
+
+	if (!p)
+		return NULL;
+	memcpy(p, buf, len);
+	p[len] = '\0';
+	defer_free(argp, p);
+	return p;
+}
+
 /**
  * savemem - duplicate a chunk of memory for later processing
  * @argp: NFSv4 compound argument structure to be freed with

@@ -415,12 +435,10 @@ nfsd4_decode_fattr(struct nfsd4_compoundargs *argp, u32 *bmval,
 			return nfserr_badlabel;
 		len += (XDR_QUADLEN(dummy32) << 2);
 		READMEM(buf, dummy32);
-		label->data = kzalloc(dummy32 + 1, GFP_KERNEL);
+		label->len = dummy32;
+		label->data = svcxdr_dupstr(argp, buf, dummy32);
 		if (!label->data)
 			return nfserr_jukebox;
-		label->len = dummy32;
-		defer_free(argp, label->data);
-		memcpy(label->data, buf, dummy32);
 	}
 #endif
 

@@ -597,17 +615,9 @@ nfsd4_decode_create(struct nfsd4_compoundargs *argp, struct nfsd4_create *create
 		READ_BUF(4);
 		create->cr_datalen = be32_to_cpup(p++);
 		READ_BUF(create->cr_datalen);
-		/*
-		 * The VFS will want a null-terminated string, and
-		 * null-terminating in place isn't safe since this might
-		 * end on a page boundary:
-		 */
-		create->cr_data = kmalloc(create->cr_datalen + 1, GFP_KERNEL);
+		create->cr_data = svcxdr_dupstr(argp, p, create->cr_datalen);
 		if (!create->cr_data)
 			return nfserr_jukebox;
-		memcpy(create->cr_data, p, create->cr_datalen);
-		create->cr_data[create->cr_datalen] = '\0';
-		defer_free(argp, create->cr_data);
 		break;
 	case NF4BLK:
 	case NF4CHR:
Signature · 1 line
1.7.9.5
author
#7 lore raw

PATCH 7/8 nfsd4: remove nfs4_acl_new

In reply to J. Bruce Fields

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

This is a not-that-useful kmalloc wrapper. And I'd like one of the callers to actually use something other than kmalloc.

Signed-off-by J. Bruce Fields <[email protected]>
---
 fs/nfsd/acl.h     |    2 +-
 fs/nfsd/nfs4acl.c |   18 ++++++++----------
 fs/nfsd/nfs4xdr.c |    2 +-
 3 files changed, 10 insertions(+), 12 deletions(-)
fs/nfsd/acl.h +1 -1
index a986ceb..4cd7c69 100644
@@ -47,7 +47,7 @@ struct svc_rqst;
 #define NFS4_ACL_MAX ((PAGE_SIZE - sizeof(struct nfs4_acl)) \
 			/ sizeof(struct nfs4_ace))
 
-struct nfs4_acl *nfs4_acl_new(int);
+int nfs4_acl_bytes(int entries);
 int nfs4_acl_get_whotype(char *, u32);
 __be32 nfs4_acl_write_who(struct xdr_stream *xdr, int who);
fs/nfsd/nfs4acl.c +8 -10
index b0cf00d..acf6974 100644
@@ -161,11 +161,12 @@ nfsd4_get_nfs4_acl(struct svc_rqst *rqstp, struct dentry *dentry,
 			size += 2 * dpacl->a_count;
 	}
 
-	*acl = nfs4_acl_new(size);
+	*acl = kmalloc(nfs4_acl_bytes(size), GFP_KERNEL);
 	if (*acl == NULL) {
 		error = -ENOMEM;
 		goto out;
 	}
+	(*acl)->naces = 0;
 
 	_posix_to_nfsv4_one(pacl, *acl, flags & ~NFS4_ACL_TYPE_DEFAULT);
 

@@ -872,16 +873,13 @@ ace2type(struct nfs4_ace *ace)
 	return -1;
 }
 
-struct nfs4_acl *
-nfs4_acl_new(int n)
+/*
+ * return the size of the struct nfs4_acl required to represent an acl
+ * with @entries entries.
+ */
+int nfs4_acl_bytes(int entries)
 {
-	struct nfs4_acl *acl;
-
-	acl = kmalloc(sizeof(*acl) + n*sizeof(struct nfs4_ace), GFP_KERNEL);
-	if (acl == NULL)
-		return NULL;
-	acl->naces = 0;
-	return acl;
+	return sizeof(struct nfs4_acl) + entries * sizeof(struct nfs4_ace);
 }
 
 static struct {
fs/nfsd/nfs4xdr.c +1 -1
index 8fb0f37..be4f19c 100644
@@ -309,7 +309,7 @@ nfsd4_decode_fattr(struct nfsd4_compoundargs *argp, u32 *bmval,
 		if (nace > NFS4_ACL_MAX)
 			return nfserr_fbig;
 
-		*acl = nfs4_acl_new(nace);
+		*acl = kmalloc(nfs4_acl_size(nace), GFP_KERNEL);
 		if (*acl == NULL)
 			return nfserr_jukebox;
Signature · 1 line
1.7.9.5
author
#8 lore raw

PATCH 8/8 nfsd4: replace defer_free by svcxdr_tmpalloc

In reply to J. Bruce Fields

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

Avoid an extra allocation for the tmpbuf struct itself, and stop ignoring some allocation failures.

Signed-off-by J. Bruce Fields <[email protected]>
---
 fs/nfsd/nfs4xdr.c |   46 +++++++++++++++++-----------------------------
 fs/nfsd/xdr4.h    |   13 +++++++++----
 2 files changed, 26 insertions(+), 33 deletions(-)
fs/nfsd/nfs4xdr.c +17 -29
index be4f19c..46115f2 100644
@@ -181,25 +181,24 @@ static int zero_clientid(clientid_t *clid)
 }
 
 /**
- * defer_free - mark an allocation as deferred freed
+ * svcxdr_tmpalloc - allocate memory to be freed after compound processing
  * @argp: NFSv4 compound argument structure
  * @p: pointer to be freed (with kfree())
  *
  * Marks @p to be freed when processing the compound operation
  * described in @argp finishes.
  */
-static int
-defer_free(struct nfsd4_compoundargs *argp, void *p)
+static void *
+svcxdr_tmpalloc(struct nfsd4_compoundargs *argp, u32 len)
 {
-	struct tmpbuf *tb;
+	struct svcxdr_tmpbuf *tb;
 
-	tb = kmalloc(sizeof(*tb), GFP_KERNEL);
+	tb = kmalloc(sizeof(*tb) + len, GFP_KERNEL);
 	if (!tb)
-		return -ENOMEM;
-	tb->buf = p;
+		return NULL;
 	tb->next = argp->to_free;
 	argp->to_free = tb;
-	return 0;
+	return tb->buf;
 }
 
 /*

@@ -212,13 +211,12 @@ defer_free(struct nfsd4_compoundargs *argp, void *p)
 static char *
 svcxdr_dupstr(struct nfsd4_compoundargs *argp, void *buf, u32 len)
 {
-	char *p = kmalloc(len + 1, GFP_KERNEL);
+	char *p = svcxdr_tmpalloc(argp, len + 1);
 
 	if (!p)
 		return NULL;
 	memcpy(p, buf, len);
 	p[len] = '\0';
-	defer_free(argp, p);
 	return p;
 }
 

@@ -234,19 +232,13 @@ svcxdr_dupstr(struct nfsd4_compoundargs *argp, void *buf, u32 len)
  */
 static char *savemem(struct nfsd4_compoundargs *argp, __be32 *p, int nbytes)
 {
-	if (p == argp->tmp) {
-		p = kmemdup(argp->tmp, nbytes, GFP_KERNEL);
-		if (!p)
-			return NULL;
-	} else {
-		BUG_ON(p != argp->tmpp);
-		argp->tmpp = NULL;
-	}
-	if (defer_free(argp, p)) {
-		kfree(p);
+	void *ret;
+
+	ret = svcxdr_tmpalloc(argp, nbytes);
+	if (!ret)
 		return NULL;
-	} else
-		return (char *)p;
+	memcpy(ret, p, nbytes);
+	return ret;
 }
 
 static __be32

@@ -309,12 +301,10 @@ nfsd4_decode_fattr(struct nfsd4_compoundargs *argp, u32 *bmval,
 		if (nace > NFS4_ACL_MAX)
 			return nfserr_fbig;
 
-		*acl = kmalloc(nfs4_acl_size(nace), GFP_KERNEL);
+		*acl = svcxdr_tmpalloc(argp, nfs4_acl_bytes(nace));
 		if (*acl == NULL)
 			return nfserr_jukebox;
 
-		defer_free(argp, *acl);
-
 		(*acl)->naces = nace;
 		for (ace = (*acl)->aces; ace < (*acl)->aces + nace; ace++) {
 			READ_BUF(16); len += 16;

@@ -1487,13 +1477,12 @@ nfsd4_decode_test_stateid(struct nfsd4_compoundargs *argp, struct nfsd4_test_sta
 	INIT_LIST_HEAD(&test_stateid->ts_stateid_list);
 
 	for (i = 0; i < test_stateid->ts_num_ids; i++) {
-		stateid = kmalloc(sizeof(struct nfsd4_test_stateid_id), GFP_KERNEL);
+		stateid = svcxdr_tmpalloc(argp, sizeof(*stateid));
 		if (!stateid) {
 			status = nfserrno(-ENOMEM);
 			goto out;
 		}
 
-		defer_free(argp, stateid);
 		INIT_LIST_HEAD(&stateid->ts_id_list);
 		list_add_tail(&stateid->ts_id_list, &test_stateid->ts_stateid_list);
 

@@ -3977,9 +3966,8 @@ int nfsd4_release_compoundargs(void *rq, __be32 *p, void *resp)
 	kfree(args->tmpp);
 	args->tmpp = NULL;
 	while (args->to_free) {
-		struct tmpbuf *tb = args->to_free;
+		struct svcxdr_tmpbuf *tb = args->to_free;
 		args->to_free = tb->next;
-		kfree(tb->buf);
 		kfree(tb);
 	}
 	return 1;
fs/nfsd/xdr4.h +9 -4
index 4379cc8..efce901 100644
@@ -478,6 +478,14 @@ struct nfsd4_op {
 
 bool nfsd4_cache_this_op(struct nfsd4_op *);
 
+/*
+ * Memory needed just for the duration of processing one compound:
+ */
+struct svcxdr_tmpbuf {
+	struct svcxdr_tmpbuf *next;
+	char buf[];
+};
+
 struct nfsd4_compoundargs {
 	/* scratch variables for XDR decode */
 	__be32 *			p;

@@ -486,10 +494,7 @@ struct nfsd4_compoundargs {
 	int				pagelen;
 	__be32				tmp[8];
 	__be32 *			tmpp;
-	struct tmpbuf {
-		struct tmpbuf *next;
-		void *buf;
-	}				*to_free;
+	struct svcxdr_tmpbuf		*to_free;
 
 	struct svc_rqst			*rqstp;
Signature · 1 line
1.7.9.5