* [PATCH 1/8] nfsd: fix rare symlink decoding bug
[not found] <20140624204418.GG2343@pad.redhat.com>
@ 2014-06-26 1:48 ` J. Bruce Fields
2014-06-26 1:48 ` [PATCH 2/8] nfsd: make NFSv2 null terminate symlink data J. Bruce Fields
` (6 more replies)
0 siblings, 7 replies; 8+ messages in thread
From: J. Bruce Fields @ 2014-06-26 1:48 UTC (permalink / raw)
To: linux-nfs; +Cc: Trond Myklebust, J. Bruce Fields
From: "J. Bruce Fields" <bfields@redhat.com>
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 <jlayton@primarydata.com>
Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
fs/nfsd/nfs4proc.c | 9 ---------
fs/nfsd/nfs4xdr.c | 13 ++++++++++++-
2 files changed, 12 insertions(+), 10 deletions(-)
diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index a6be9d3..2b3795a 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -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,
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index 30913c8..a1c48b4 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -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:
--
1.7.9.5
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/8] nfsd: make NFSv2 null terminate symlink data
2014-06-26 1:48 ` [PATCH 1/8] nfsd: fix rare symlink decoding bug J. Bruce Fields
@ 2014-06-26 1:48 ` J. Bruce Fields
2014-06-26 1:48 ` [PATCH 3/8] nfsd: let nfsd_symlink assume null-terminated data J. Bruce Fields
` (5 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: J. Bruce Fields @ 2014-06-26 1:48 UTC (permalink / raw)
To: linux-nfs; +Cc: Trond Myklebust, J. Bruce Fields
From: "J. Bruce Fields" <bfields@redhat.com>
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 <bfields@redhat.com>
---
fs/nfsd/nfsproc.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c
index 54c6b3d..aebe23c 100644
--- a/fs/nfsd/nfsproc.c
+++ b/fs/nfsd/nfsproc.c
@@ -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);
--
1.7.9.5
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 3/8] nfsd: let nfsd_symlink assume null-terminated data
2014-06-26 1:48 ` [PATCH 1/8] nfsd: fix rare symlink decoding bug J. Bruce Fields
2014-06-26 1:48 ` [PATCH 2/8] nfsd: make NFSv2 null terminate symlink data J. Bruce Fields
@ 2014-06-26 1:48 ` J. Bruce Fields
2014-06-26 1:48 ` [PATCH 4/8] nfsd4: rename cr_linkname->cr_data J. Bruce Fields
` (4 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: J. Bruce Fields @ 2014-06-26 1:48 UTC (permalink / raw)
To: linux-nfs; +Cc: Trond Myklebust, J. Bruce Fields
From: "J. Bruce Fields" <bfields@redhat.com>
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 <bfields@redhat.com>
---
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(-)
diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c
index 61ef42c..19ba233 100644
--- a/fs/nfsd/nfs3proc.c
+++ b/fs/nfsd/nfs3proc.c
@@ -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);
}
diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 2b3795a..7aa83bf 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -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;
diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c
index aebe23c..583ed03 100644
--- a/fs/nfsd/nfsproc.c
+++ b/fs/nfsd/nfsproc.c
@@ -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);
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index 6ffaa70..7518c65 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -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));
diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h
index b84aef5..20e4b66 100644
--- a/fs/nfsd/vfs.h
+++ b/fs/nfsd/vfs.h
@@ -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 *);
--
1.7.9.5
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 4/8] nfsd4: rename cr_linkname->cr_data
2014-06-26 1:48 ` [PATCH 1/8] nfsd: fix rare symlink decoding bug J. Bruce Fields
2014-06-26 1:48 ` [PATCH 2/8] nfsd: make NFSv2 null terminate symlink data J. Bruce Fields
2014-06-26 1:48 ` [PATCH 3/8] nfsd: let nfsd_symlink assume null-terminated data J. Bruce Fields
@ 2014-06-26 1:48 ` J. Bruce Fields
2014-06-26 1:48 ` [PATCH 5/8] nfsd4: remove unused defer_free argument J. Bruce Fields
` (3 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: J. Bruce Fields @ 2014-06-26 1:48 UTC (permalink / raw)
To: linux-nfs; +Cc: Trond Myklebust, J. Bruce Fields
From: "J. Bruce Fields" <bfields@redhat.com>
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 <bfields@redhat.com>
---
fs/nfsd/nfs4proc.c | 2 +-
fs/nfsd/nfs4xdr.c | 15 +++++++--------
fs/nfsd/xdr4.h | 8 ++++----
3 files changed, 12 insertions(+), 13 deletions(-)
diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 7aa83bf..b57c882 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -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;
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index a1c48b4..3d07496 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -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:
diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
index 18cbb6d..b8bf63a 100644
--- a/fs/nfsd/xdr4.h
+++ b/fs/nfsd/xdr4.h
@@ -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
--
1.7.9.5
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 5/8] nfsd4: remove unused defer_free argument
2014-06-26 1:48 ` [PATCH 1/8] nfsd: fix rare symlink decoding bug J. Bruce Fields
` (2 preceding siblings ...)
2014-06-26 1:48 ` [PATCH 4/8] nfsd4: rename cr_linkname->cr_data J. Bruce Fields
@ 2014-06-26 1:48 ` J. Bruce Fields
2014-06-26 1:48 ` [PATCH 6/8] nfsd4: define svcxdr_dupstr to share some common code J. Bruce Fields
` (2 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: J. Bruce Fields @ 2014-06-26 1:48 UTC (permalink / raw)
To: linux-nfs; +Cc: Trond Myklebust, J. Bruce Fields
From: "J. Bruce Fields" <bfields@redhat.com>
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 <bfields@redhat.com>
---
fs/nfsd/nfs4xdr.c | 21 +++++++++------------
fs/nfsd/xdr4.h | 1 -
2 files changed, 9 insertions(+), 13 deletions(-)
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index 3d07496..13f91ce 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -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;
diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
index b8bf63a..4379cc8 100644
--- a/fs/nfsd/xdr4.h
+++ b/fs/nfsd/xdr4.h
@@ -488,7 +488,6 @@ struct nfsd4_compoundargs {
__be32 * tmpp;
struct tmpbuf {
struct tmpbuf *next;
- void (*release)(const void *);
void *buf;
} *to_free;
--
1.7.9.5
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 6/8] nfsd4: define svcxdr_dupstr to share some common code
2014-06-26 1:48 ` [PATCH 1/8] nfsd: fix rare symlink decoding bug J. Bruce Fields
` (3 preceding siblings ...)
2014-06-26 1:48 ` [PATCH 5/8] nfsd4: remove unused defer_free argument J. Bruce Fields
@ 2014-06-26 1:48 ` J. Bruce Fields
2014-06-26 1:48 ` [PATCH 7/8] nfsd4: remove nfs4_acl_new J. Bruce Fields
2014-06-26 1:48 ` [PATCH 8/8] nfsd4: replace defer_free by svcxdr_tmpalloc J. Bruce Fields
6 siblings, 0 replies; 8+ messages in thread
From: J. Bruce Fields @ 2014-06-26 1:48 UTC (permalink / raw)
To: linux-nfs; +Cc: Trond Myklebust, J. Bruce Fields
From: "J. Bruce Fields" <bfields@redhat.com>
Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
fs/nfsd/nfs4xdr.c | 36 +++++++++++++++++++++++-------------
1 file changed, 23 insertions(+), 13 deletions(-)
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index 13f91ce..8fb0f37 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -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:
--
1.7.9.5
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 7/8] nfsd4: remove nfs4_acl_new
2014-06-26 1:48 ` [PATCH 1/8] nfsd: fix rare symlink decoding bug J. Bruce Fields
` (4 preceding siblings ...)
2014-06-26 1:48 ` [PATCH 6/8] nfsd4: define svcxdr_dupstr to share some common code J. Bruce Fields
@ 2014-06-26 1:48 ` J. Bruce Fields
2014-06-26 1:48 ` [PATCH 8/8] nfsd4: replace defer_free by svcxdr_tmpalloc J. Bruce Fields
6 siblings, 0 replies; 8+ messages in thread
From: J. Bruce Fields @ 2014-06-26 1:48 UTC (permalink / raw)
To: linux-nfs; +Cc: Trond Myklebust, J. Bruce Fields
From: "J. Bruce Fields" <bfields@redhat.com>
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 <bfields@redhat.com>
---
fs/nfsd/acl.h | 2 +-
fs/nfsd/nfs4acl.c | 18 ++++++++----------
fs/nfsd/nfs4xdr.c | 2 +-
3 files changed, 10 insertions(+), 12 deletions(-)
diff --git a/fs/nfsd/acl.h b/fs/nfsd/acl.h
index a986ceb..4cd7c69 100644
--- a/fs/nfsd/acl.h
+++ b/fs/nfsd/acl.h
@@ -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);
diff --git a/fs/nfsd/nfs4acl.c b/fs/nfsd/nfs4acl.c
index b0cf00d..acf6974 100644
--- a/fs/nfsd/nfs4acl.c
+++ b/fs/nfsd/nfs4acl.c
@@ -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 {
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index 8fb0f37..be4f19c 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -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;
--
1.7.9.5
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 8/8] nfsd4: replace defer_free by svcxdr_tmpalloc
2014-06-26 1:48 ` [PATCH 1/8] nfsd: fix rare symlink decoding bug J. Bruce Fields
` (5 preceding siblings ...)
2014-06-26 1:48 ` [PATCH 7/8] nfsd4: remove nfs4_acl_new J. Bruce Fields
@ 2014-06-26 1:48 ` J. Bruce Fields
6 siblings, 0 replies; 8+ messages in thread
From: J. Bruce Fields @ 2014-06-26 1:48 UTC (permalink / raw)
To: linux-nfs; +Cc: Trond Myklebust, J. Bruce Fields
From: "J. Bruce Fields" <bfields@redhat.com>
Avoid an extra allocation for the tmpbuf struct itself, and stop
ignoring some allocation failures.
Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
fs/nfsd/nfs4xdr.c | 46 +++++++++++++++++-----------------------------
fs/nfsd/xdr4.h | 13 +++++++++----
2 files changed, 26 insertions(+), 33 deletions(-)
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index be4f19c..46115f2 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -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;
diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
index 4379cc8..efce901 100644
--- a/fs/nfsd/xdr4.h
+++ b/fs/nfsd/xdr4.h
@@ -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;
--
1.7.9.5
^ permalink raw reply related [flat|nested] 8+ messages in thread
end of thread, other threads:[~2014-06-26 1:48 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20140624204418.GG2343@pad.redhat.com>
2014-06-26 1:48 ` [PATCH 1/8] nfsd: fix rare symlink decoding bug J. Bruce Fields
2014-06-26 1:48 ` [PATCH 2/8] nfsd: make NFSv2 null terminate symlink data J. Bruce Fields
2014-06-26 1:48 ` [PATCH 3/8] nfsd: let nfsd_symlink assume null-terminated data J. Bruce Fields
2014-06-26 1:48 ` [PATCH 4/8] nfsd4: rename cr_linkname->cr_data J. Bruce Fields
2014-06-26 1:48 ` [PATCH 5/8] nfsd4: remove unused defer_free argument J. Bruce Fields
2014-06-26 1:48 ` [PATCH 6/8] nfsd4: define svcxdr_dupstr to share some common code J. Bruce Fields
2014-06-26 1:48 ` [PATCH 7/8] nfsd4: remove nfs4_acl_new J. Bruce Fields
2014-06-26 1:48 ` [PATCH 8/8] nfsd4: replace defer_free by svcxdr_tmpalloc J. Bruce Fields
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox