[1/3] NFS: Fix off-by-one errors in nfs_readdir
diff mbox series

Message ID 20190706185252.32488-1-trond.myklebust@hammerspace.com
State New
Headers show
Series
  • [1/3] NFS: Fix off-by-one errors in nfs_readdir
Related show

Commit Message

Trond Myklebust July 6, 2019, 6:52 p.m. UTC
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 <zhangliguang@linux.alibaba.com>
Cc: stable@vger.kernel.org # v5.1+
Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
---
 fs/nfs/dir.c | 53 +++++++++++++++++++++++++---------------------------
 1 file changed, 25 insertions(+), 28 deletions(-)

Comments

Tigran Mkrtchyan July 7, 2019, 9:01 p.m. UTC | #1
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" <trondmy@gmail.com>
> To: "linux-nfs" <linux-nfs@vger.kernel.org>
> Cc: "zhangliguang" <zhangliguang@linux.alibaba.com>
> 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 <zhangliguang@linux.alibaba.com>
> Cc: stable@vger.kernel.org # v5.1+
> Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> ---
> 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

Patch
diff mbox series

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);