In C, the array size and the maximum index are not the same. In this
case, the field desc->pvec.nr is being used as a cursor but is
occasionally being treated as if it the array size.
This patch renames it to desc->pvec.cursor in order to make clear that
it is tracking an index.
Fixes: be4c2d4723a4 ("NFS: readdirplus optimization by cache mechanism")
Cc: Liguang Zhang <[email protected]>
Cc: [email protected] # v5.1+
Signed-off-by: Trond Myklebust <[email protected]>
---
fs/nfs/dir.c | 53 +++++++++++++++++++++++++---------------------------
1 file changed, 25 insertions(+), 28 deletions(-)
diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index 57b6a45576ad..e44f3c9fad5b 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -134,14 +134,14 @@ struct nfs_cache_array_entry {
};
struct nfs_cache_array {
- int size;
- int eof_index;
u64 last_cookie;
+ unsigned int size;
+ bool eof;
struct nfs_cache_array_entry array[0];
};
struct readdirvec {
- unsigned long nr;
+ unsigned long cursor;
unsigned long index;
struct page *pages[NFS_MAX_READDIR_RAPAGES];
};
@@ -172,7 +172,7 @@ static
void nfs_readdir_clear_array(struct page *page)
{
struct nfs_cache_array *array;
- int i;
+ unsigned int i;
array = kmap_atomic(page);
for (i = 0; i < array->size; i++)
@@ -224,7 +224,7 @@ int nfs_readdir_add_to_array(struct nfs_entry *entry, struct page *page)
array->last_cookie = entry->cookie;
array->size++;
if (entry->eof != 0)
- array->eof_index = array->size;
+ array->eof = true;
out:
kunmap(page);
return ret;
@@ -239,7 +239,7 @@ int nfs_readdir_search_for_pos(struct nfs_cache_array *array, nfs_readdir_descri
if (diff < 0)
goto out_eof;
if (diff >= array->size) {
- if (array->eof_index >= 0)
+ if (array->eof)
goto out_eof;
return -EAGAIN;
}
@@ -265,7 +265,7 @@ nfs_readdir_inode_mapping_valid(struct nfs_inode *nfsi)
static
int nfs_readdir_search_for_cookie(struct nfs_cache_array *array, nfs_readdir_descriptor_t *desc)
{
- int i;
+ unsigned int i;
loff_t new_pos;
int status = -EAGAIN;
@@ -300,7 +300,7 @@ int nfs_readdir_search_for_cookie(struct nfs_cache_array *array, nfs_readdir_des
return 0;
}
}
- if (array->eof_index >= 0) {
+ if (array->eof) {
status = -EBADCOOKIE;
if (*desc->dir_cookie == array->last_cookie)
desc->eof = true;
@@ -532,10 +532,9 @@ int nfs_readdir_page_filler(nfs_readdir_descriptor_t *desc, struct nfs_entry *en
struct nfs_cache_array *array;
unsigned int count = 0;
int status;
- int max_rapages = NFS_MAX_READDIR_RAPAGES;
desc->pvec.index = desc->page_index;
- desc->pvec.nr = 0;
+ desc->pvec.cursor = 0;
scratch = alloc_page(GFP_KERNEL);
if (scratch == NULL)
@@ -560,12 +559,12 @@ int nfs_readdir_page_filler(nfs_readdir_descriptor_t *desc, struct nfs_entry *en
if (desc->plus)
nfs_prime_dcache(file_dentry(desc->file), entry);
- status = nfs_readdir_add_to_array(entry, desc->pvec.pages[desc->pvec.nr]);
+ status = nfs_readdir_add_to_array(entry, desc->pvec.pages[desc->pvec.cursor]);
if (status == -ENOSPC) {
- desc->pvec.nr++;
- if (desc->pvec.nr == max_rapages)
+ if (desc->pvec.cursor == ARRAY_SIZE(desc->pvec.pages) - 1)
break;
- status = nfs_readdir_add_to_array(entry, desc->pvec.pages[desc->pvec.nr]);
+ desc->pvec.cursor++;
+ status = nfs_readdir_add_to_array(entry, desc->pvec.pages[desc->pvec.cursor]);
}
if (status != 0)
break;
@@ -579,8 +578,8 @@ int nfs_readdir_page_filler(nfs_readdir_descriptor_t *desc, struct nfs_entry *en
out_nopages:
if (count == 0 || (status == -EBADCOOKIE && entry->eof != 0)) {
- array = kmap_atomic(desc->pvec.pages[desc->pvec.nr]);
- array->eof_index = array->size;
+ array = kmap_atomic(desc->pvec.pages[desc->pvec.cursor]);
+ array->eof = true;
status = 0;
kunmap_atomic(array);
}
@@ -588,11 +587,11 @@ int nfs_readdir_page_filler(nfs_readdir_descriptor_t *desc, struct nfs_entry *en
put_page(scratch);
/*
- * desc->pvec.nr > 0 means at least one page was completely filled,
+ * desc->pvec.cursor > 0 means at least one page was completely filled,
* we should return -ENOSPC. Otherwise function
* nfs_readdir_xdr_to_array will enter infinite loop.
*/
- if (desc->pvec.nr > 0)
+ if (desc->pvec.cursor > 0)
return -ENOSPC;
return status;
}
@@ -634,13 +633,12 @@ static
void nfs_readdir_rapages_init(nfs_readdir_descriptor_t *desc)
{
struct nfs_cache_array *array;
- int max_rapages = NFS_MAX_READDIR_RAPAGES;
- int index;
+ unsigned int max_rapages = NFS_MAX_READDIR_RAPAGES;
+ unsigned int index;
for (index = 0; index < max_rapages; index++) {
array = kmap_atomic(desc->pvec.pages[index]);
memset(array, 0, sizeof(struct nfs_cache_array));
- array->eof_index = -1;
kunmap_atomic(array);
}
}
@@ -678,7 +676,6 @@ int nfs_readdir_xdr_to_array(nfs_readdir_descriptor_t *desc, struct page *page,
array = kmap(page);
memset(array, 0, sizeof(struct nfs_cache_array));
- array->eof_index = -1;
status = nfs_readdir_alloc_pages(pages, array_size);
if (status < 0)
@@ -696,7 +693,7 @@ int nfs_readdir_xdr_to_array(nfs_readdir_descriptor_t *desc, struct page *page,
status = 0;
break;
}
- } while (array->eof_index < 0);
+ } while (!array->eof);
nfs_readdir_free_pages(pages, array_size);
out_release_array:
@@ -723,10 +720,10 @@ int nfs_readdir_filler(void *data, struct page* page)
/*
* If desc->page_index in range desc->pvec.index and
- * desc->pvec.index + desc->pvec.nr, we get readdir cache hit.
+ * desc->pvec.index + desc->pvec.cursor, we get readdir cache hit.
*/
if (desc->page_index >= desc->pvec.index &&
- desc->page_index < (desc->pvec.index + desc->pvec.nr)) {
+ desc->page_index <= (desc->pvec.index + desc->pvec.cursor)) {
/*
* page and desc->pvec.pages[x] are valid, don't need to check
* whether or not to be NULL.
@@ -809,7 +806,7 @@ static
int nfs_do_filldir(nfs_readdir_descriptor_t *desc)
{
struct file *file = desc->file;
- int i = 0;
+ unsigned int i = 0;
int res = 0;
struct nfs_cache_array *array = NULL;
struct nfs_open_dir_context *ctx = file->private_data;
@@ -832,7 +829,7 @@ int nfs_do_filldir(nfs_readdir_descriptor_t *desc)
if (ctx->duped != 0)
ctx->duped = 1;
}
- if (array->eof_index >= 0)
+ if (array->eof)
desc->eof = true;
kunmap(desc->page);
@@ -903,7 +900,7 @@ static int nfs_readdir(struct file *file, struct dir_context *ctx)
*desc = &my_desc;
struct nfs_open_dir_context *dir_ctx = file->private_data;
int res = 0;
- int max_rapages = NFS_MAX_READDIR_RAPAGES;
+ unsigned int max_rapages = NFS_MAX_READDIR_RAPAGES;
dfprintk(FILE, "NFS: readdir(%pD2) starting at cookie %llu\n",
file, (long long)ctx->pos);
--
2.21.0
64 pointers is 512bytes on a typical 64-bit system.
Signed-off-by: Trond Myklebust <[email protected]>
---
fs/nfs/dir.c | 29 +++++++++++++++++++----------
1 file changed, 19 insertions(+), 10 deletions(-)
diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index e44f3c9fad5b..6ccf0e6c9c84 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -646,12 +646,12 @@ void nfs_readdir_rapages_init(nfs_readdir_descriptor_t *desc)
static
int nfs_readdir_xdr_to_array(nfs_readdir_descriptor_t *desc, struct page *page, struct inode *inode)
{
- struct page *pages[NFS_MAX_READDIR_PAGES];
+ const unsigned int array_size = NFS_MAX_READDIR_PAGES;
+ struct page **pages;
struct nfs_entry entry;
struct file *file = desc->file;
struct nfs_cache_array *array;
int status = -ENOMEM;
- unsigned int array_size = ARRAY_SIZE(pages);
/*
* This means we hit readdir rdpages miss, the preallocated rdpages
@@ -674,12 +674,17 @@ int nfs_readdir_xdr_to_array(nfs_readdir_descriptor_t *desc, struct page *page,
goto out;
}
- array = kmap(page);
- memset(array, 0, sizeof(struct nfs_cache_array));
+ pages = kmalloc_array(array_size, sizeof(*pages), GFP_KERNEL);
+ if (!pages)
+ goto out_release_array;
status = nfs_readdir_alloc_pages(pages, array_size);
if (status < 0)
goto out_release_array;
+
+ array = kmap(page);
+ memset(array, 0, sizeof(struct nfs_cache_array));
+
do {
unsigned int pglen;
status = nfs_readdir_xdr_filler(pages, desc, &entry, file, inode);
@@ -695,9 +700,10 @@ int nfs_readdir_xdr_to_array(nfs_readdir_descriptor_t *desc, struct page *page,
}
} while (!array->eof);
+ kunmap(page);
nfs_readdir_free_pages(pages, array_size);
out_release_array:
- kunmap(page);
+ kfree(pages);
nfs4_label_free(entry.label);
out:
nfs_free_fattr(entry.fattr);
@@ -896,10 +902,9 @@ static int nfs_readdir(struct file *file, struct dir_context *ctx)
{
struct dentry *dentry = file_dentry(file);
struct inode *inode = d_inode(dentry);
- nfs_readdir_descriptor_t my_desc,
- *desc = &my_desc;
struct nfs_open_dir_context *dir_ctx = file->private_data;
- int res = 0;
+ nfs_readdir_descriptor_t *desc;
+ int res = -ENOMEM;
unsigned int max_rapages = NFS_MAX_READDIR_RAPAGES;
dfprintk(FILE, "NFS: readdir(%pD2) starting at cookie %llu\n",
@@ -912,7 +917,9 @@ static int nfs_readdir(struct file *file, struct dir_context *ctx)
* to either find the entry with the appropriate number or
* revalidate the cookie.
*/
- memset(desc, 0, sizeof(*desc));
+ desc = kzalloc(sizeof(*desc), GFP_KERNEL);
+ if (!desc)
+ goto free_desc;
desc->file = file;
desc->ctx = ctx;
@@ -922,7 +929,7 @@ static int nfs_readdir(struct file *file, struct dir_context *ctx)
res = nfs_readdir_alloc_pages(desc->pvec.pages, max_rapages);
if (res < 0)
- return -ENOMEM;
+ goto free_desc;
nfs_readdir_rapages_init(desc);
@@ -964,6 +971,8 @@ static int nfs_readdir(struct file *file, struct dir_context *ctx)
nfs_readdir_free_pages(desc->pvec.pages, max_rapages);
if (res > 0)
res = 0;
+free_desc:
+ kfree(desc);
dfprintk(FILE, "NFS: readdir(%pD2) returns %d\n", file, res);
return res;
}
--
2.21.0
Hi Trond,
Unfortunately, this change doesn't fix another issue introduced
with be4c2d4723a4: en extra READDIR request after server have sent
all entries and indicated it with eol=true:
https://www.spinics.net/lists/linux-nfs/msg73399.html
Tigran.
----- Original Message -----
> From: "Trond Myklebust" <[email protected]>
> To: "linux-nfs" <[email protected]>
> Cc: "zhangliguang" <[email protected]>
> Sent: Saturday, July 6, 2019 8:52:50 PM
> Subject: [PATCH 1/3] NFS: Fix off-by-one errors in nfs_readdir
> In C, the array size and the maximum index are not the same. In this
> case, the field desc->pvec.nr is being used as a cursor but is
> occasionally being treated as if it the array size.
> This patch renames it to desc->pvec.cursor in order to make clear that
> it is tracking an index.
>
> Fixes: be4c2d4723a4 ("NFS: readdirplus optimization by cache mechanism")
> Cc: Liguang Zhang <[email protected]>
> Cc: [email protected] # v5.1+
> Signed-off-by: Trond Myklebust <[email protected]>
> ---
> fs/nfs/dir.c | 53 +++++++++++++++++++++++++---------------------------
> 1 file changed, 25 insertions(+), 28 deletions(-)
>
> diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
> index 57b6a45576ad..e44f3c9fad5b 100644
> --- a/fs/nfs/dir.c
> +++ b/fs/nfs/dir.c
> @@ -134,14 +134,14 @@ struct nfs_cache_array_entry {
> };
>
> struct nfs_cache_array {
> - int size;
> - int eof_index;
> u64 last_cookie;
> + unsigned int size;
> + bool eof;
> struct nfs_cache_array_entry array[0];
> };
>
> struct readdirvec {
> - unsigned long nr;
> + unsigned long cursor;
> unsigned long index;
> struct page *pages[NFS_MAX_READDIR_RAPAGES];
> };
> @@ -172,7 +172,7 @@ static
> void nfs_readdir_clear_array(struct page *page)
> {
> struct nfs_cache_array *array;
> - int i;
> + unsigned int i;
>
> array = kmap_atomic(page);
> for (i = 0; i < array->size; i++)
> @@ -224,7 +224,7 @@ int nfs_readdir_add_to_array(struct nfs_entry *entry, struct
> page *page)
> array->last_cookie = entry->cookie;
> array->size++;
> if (entry->eof != 0)
> - array->eof_index = array->size;
> + array->eof = true;
> out:
> kunmap(page);
> return ret;
> @@ -239,7 +239,7 @@ int nfs_readdir_search_for_pos(struct nfs_cache_array
> *array, nfs_readdir_descri
> if (diff < 0)
> goto out_eof;
> if (diff >= array->size) {
> - if (array->eof_index >= 0)
> + if (array->eof)
> goto out_eof;
> return -EAGAIN;
> }
> @@ -265,7 +265,7 @@ nfs_readdir_inode_mapping_valid(struct nfs_inode *nfsi)
> static
> int nfs_readdir_search_for_cookie(struct nfs_cache_array *array,
> nfs_readdir_descriptor_t *desc)
> {
> - int i;
> + unsigned int i;
> loff_t new_pos;
> int status = -EAGAIN;
>
> @@ -300,7 +300,7 @@ int nfs_readdir_search_for_cookie(struct nfs_cache_array
> *array, nfs_readdir_des
> return 0;
> }
> }
> - if (array->eof_index >= 0) {
> + if (array->eof) {
> status = -EBADCOOKIE;
> if (*desc->dir_cookie == array->last_cookie)
> desc->eof = true;
> @@ -532,10 +532,9 @@ int nfs_readdir_page_filler(nfs_readdir_descriptor_t *desc,
> struct nfs_entry *en
> struct nfs_cache_array *array;
> unsigned int count = 0;
> int status;
> - int max_rapages = NFS_MAX_READDIR_RAPAGES;
>
> desc->pvec.index = desc->page_index;
> - desc->pvec.nr = 0;
> + desc->pvec.cursor = 0;
>
> scratch = alloc_page(GFP_KERNEL);
> if (scratch == NULL)
> @@ -560,12 +559,12 @@ int nfs_readdir_page_filler(nfs_readdir_descriptor_t
> *desc, struct nfs_entry *en
> if (desc->plus)
> nfs_prime_dcache(file_dentry(desc->file), entry);
>
> - status = nfs_readdir_add_to_array(entry, desc->pvec.pages[desc->pvec.nr]);
> + status = nfs_readdir_add_to_array(entry,
> desc->pvec.pages[desc->pvec.cursor]);
> if (status == -ENOSPC) {
> - desc->pvec.nr++;
> - if (desc->pvec.nr == max_rapages)
> + if (desc->pvec.cursor == ARRAY_SIZE(desc->pvec.pages) - 1)
> break;
> - status = nfs_readdir_add_to_array(entry, desc->pvec.pages[desc->pvec.nr]);
> + desc->pvec.cursor++;
> + status = nfs_readdir_add_to_array(entry,
> desc->pvec.pages[desc->pvec.cursor]);
> }
> if (status != 0)
> break;
> @@ -579,8 +578,8 @@ int nfs_readdir_page_filler(nfs_readdir_descriptor_t *desc,
> struct nfs_entry *en
>
> out_nopages:
> if (count == 0 || (status == -EBADCOOKIE && entry->eof != 0)) {
> - array = kmap_atomic(desc->pvec.pages[desc->pvec.nr]);
> - array->eof_index = array->size;
> + array = kmap_atomic(desc->pvec.pages[desc->pvec.cursor]);
> + array->eof = true;
> status = 0;
> kunmap_atomic(array);
> }
> @@ -588,11 +587,11 @@ int nfs_readdir_page_filler(nfs_readdir_descriptor_t
> *desc, struct nfs_entry *en
> put_page(scratch);
>
> /*
> - * desc->pvec.nr > 0 means at least one page was completely filled,
> + * desc->pvec.cursor > 0 means at least one page was completely filled,
> * we should return -ENOSPC. Otherwise function
> * nfs_readdir_xdr_to_array will enter infinite loop.
> */
> - if (desc->pvec.nr > 0)
> + if (desc->pvec.cursor > 0)
> return -ENOSPC;
> return status;
> }
> @@ -634,13 +633,12 @@ static
> void nfs_readdir_rapages_init(nfs_readdir_descriptor_t *desc)
> {
> struct nfs_cache_array *array;
> - int max_rapages = NFS_MAX_READDIR_RAPAGES;
> - int index;
> + unsigned int max_rapages = NFS_MAX_READDIR_RAPAGES;
> + unsigned int index;
>
> for (index = 0; index < max_rapages; index++) {
> array = kmap_atomic(desc->pvec.pages[index]);
> memset(array, 0, sizeof(struct nfs_cache_array));
> - array->eof_index = -1;
> kunmap_atomic(array);
> }
> }
> @@ -678,7 +676,6 @@ int nfs_readdir_xdr_to_array(nfs_readdir_descriptor_t *desc,
> struct page *page,
>
> array = kmap(page);
> memset(array, 0, sizeof(struct nfs_cache_array));
> - array->eof_index = -1;
>
> status = nfs_readdir_alloc_pages(pages, array_size);
> if (status < 0)
> @@ -696,7 +693,7 @@ int nfs_readdir_xdr_to_array(nfs_readdir_descriptor_t *desc,
> struct page *page,
> status = 0;
> break;
> }
> - } while (array->eof_index < 0);
> + } while (!array->eof);
>
> nfs_readdir_free_pages(pages, array_size);
> out_release_array:
> @@ -723,10 +720,10 @@ int nfs_readdir_filler(void *data, struct page* page)
>
> /*
> * If desc->page_index in range desc->pvec.index and
> - * desc->pvec.index + desc->pvec.nr, we get readdir cache hit.
> + * desc->pvec.index + desc->pvec.cursor, we get readdir cache hit.
> */
> if (desc->page_index >= desc->pvec.index &&
> - desc->page_index < (desc->pvec.index + desc->pvec.nr)) {
> + desc->page_index <= (desc->pvec.index + desc->pvec.cursor)) {
> /*
> * page and desc->pvec.pages[x] are valid, don't need to check
> * whether or not to be NULL.
> @@ -809,7 +806,7 @@ static
> int nfs_do_filldir(nfs_readdir_descriptor_t *desc)
> {
> struct file *file = desc->file;
> - int i = 0;
> + unsigned int i = 0;
> int res = 0;
> struct nfs_cache_array *array = NULL;
> struct nfs_open_dir_context *ctx = file->private_data;
> @@ -832,7 +829,7 @@ int nfs_do_filldir(nfs_readdir_descriptor_t *desc)
> if (ctx->duped != 0)
> ctx->duped = 1;
> }
> - if (array->eof_index >= 0)
> + if (array->eof)
> desc->eof = true;
>
> kunmap(desc->page);
> @@ -903,7 +900,7 @@ static int nfs_readdir(struct file *file, struct dir_context
> *ctx)
> *desc = &my_desc;
> struct nfs_open_dir_context *dir_ctx = file->private_data;
> int res = 0;
> - int max_rapages = NFS_MAX_READDIR_RAPAGES;
> + unsigned int max_rapages = NFS_MAX_READDIR_RAPAGES;
>
> dfprintk(FILE, "NFS: readdir(%pD2) starting at cookie %llu\n",
> file, (long long)ctx->pos);
> --
> 2.21.0