iov_iter: Fix iter_xarray_get_pages{,_alloc}()

Commit Message

David Howells June 9, 2022, 8:07 a.m. UTC

The maths at the end of iter_xarray_get_pages() to calculate the actual
size doesn't work under some circumstances, such as when it's been asked to
extract a partial single page.  Various terms of the equation cancel out
and you end up with actual == offset.  The same issue exists in
iter_xarray_get_pages_alloc().

Fix these to just use min() to select the lesser amount from between the
amount of page content transcribed into the buffer, minus the offset, and
the size limit specified.

This doesn't appear to have caused a problem yet upstream because network
filesystems aren't getting the pages from an xarray iterator, but rather
passing it directly to the socket, which just iterates over it.  Cachefiles
*does* do DIO from one to/from ext4/xfs/btrfs/etc. but it always asks for
whole pages to be written or read.

Fixes: 7ff5062079ef ("iov_iter: Add ITER_XARRAY")
Reported-by: Jeff Layton <[e-mail]>
Signed-off-by: David Howells <[e-mail]>
cc: Alexander Viro <[e-mail]>
cc: Dominique Martinet <[e-mail]>
cc: Mike Marshall <[e-mail]>
cc: Gao Xiang <[e-mail]>
cc: [e-mail]
cc: [e-mail]
cc: [e-mail]
cc: [e-mail]
cc: [e-mail]
cc: [e-mail]
---

 lib/iov_iter.c |   20 ++++----------------
 1 file changed, 4 insertions(+), 16 deletions(-)

Comments

David Howells June 9, 2022, 8:16 a.m. UTC | #1

Here's a program that can be used to exercise the iter_xarray_get_pages()
function in userspace.  In the main() function, there are various parameters
that can be adjusted, such as the starting offset (iter.xarray_start), the
size of the content (iter.count), the maximum number of pages to be extracted
(maxpages) and the maximum size to be extracted (maxsize).

David
---
/* SPDX-License-Identifier: GPL-2.0 */
#include <stdbool.h>
#include <stdlib.h>
#include <stdio.h>
#include <string.h>

typedef unsigned long pgoff_t;
#define PAGE_SHIFT 12
#define PAGE_SIZE ((unsigned long)1 << PAGE_SHIFT)
#define PAGE_MASK (~(PAGE_SIZE - 1))

struct page;
struct xarray;

struct iov_iter {
	size_t iov_offset;
	size_t count;
	loff_t xarray_start;
};
#define __is_constexpr(x) \
	(sizeof(int) == sizeof(*(8 ? ((void *)((long)(x) * 0l)) : (int *)8)))
#define __typecheck(x, y) \
	(!!(sizeof((typeof(x) *)1 == (typeof(y) *)1)))

#define __no_side_effects(x, y) \
		(__is_constexpr(x) && __is_constexpr(y))

#define __safe_cmp(x, y) \
		(__typecheck(x, y) && __no_side_effects(x, y))

#define __cmp(x, y, op)	((x) op (y) ? (x) : (y))

#define __cmp_once(x, y, unique_x, unique_y, op) ({	\
		typeof(x) unique_x = (x);		\
		typeof(y) unique_y = (y);		\
		__cmp(unique_x, unique_y, op); })

#define __careful_cmp(x, y, op) \
	__builtin_choose_expr(__safe_cmp(x, y), \
		__cmp(x, y, op), \
		__cmp_once(x, y, __x, __y, op))
#define min(x, y)	__careful_cmp(x, y, <)
#define min_t(type, x, y)	__careful_cmp((type)(x), (type)(y), <)

static int apply_fix;

static ssize_t iter_xarray_populate_pages(pgoff_t index, unsigned int nr_pages)
{
	return nr_pages;
}

static ssize_t iter_xarray_get_pages(struct iov_iter *i, size_t maxsize,
				     unsigned maxpages, size_t *_start_offset)
{
	unsigned nr, offset;
	pgoff_t index, count;
	size_t size = maxsize, head_size, tail_size;
	loff_t pos;

	if (!size || !maxpages)
		return 0;

	pos = i->xarray_start + i->iov_offset;
	index = pos >> PAGE_SHIFT;
	offset = pos & ~PAGE_MASK;
	*_start_offset = offset;

	count = 1;
	tail_size = head_size = PAGE_SIZE - offset;
	if (maxsize > head_size) {
		size -= head_size;
		count += size >> PAGE_SHIFT;
		tail_size = size & ~PAGE_MASK;
		if (tail_size)
			count++;
	}

	if (count > maxpages)
		count = maxpages;

	printf(" %6lx %6lu %6zx |", index, count, tail_size);

	nr = iter_xarray_populate_pages(index, count);
	if (nr == 0)
		return 0;

	if (!apply_fix) {
		size_t actual = PAGE_SIZE * nr;
		actual -= offset;
		if (nr == count && size > 0) {
			unsigned last_offset = (nr > 1) ? 0 : offset;
			actual -= PAGE_SIZE - (last_offset + size);
		}
		return actual;
	} else {
		return min(nr * PAGE_SIZE - offset, maxsize);
	}
}

ssize_t iov_iter_get_pages(struct iov_iter *i,
			   size_t maxsize, unsigned maxpages, size_t *start)
{
	if (maxsize > i->count)
		maxsize = i->count;
	if (!maxsize)
		return 0;
	return iter_xarray_get_pages(i, maxsize, maxpages, start);
}

int main()
{
	struct iov_iter iter;
	ssize_t size;
	size_t i, maxpages, maxsize, offset;

	memset(&iter, 0, sizeof(iter));

	/* Adjustable parameters */
	iter.xarray_start	= 0x11000;
	iter.count		= PAGE_SIZE * 16;
	maxpages		= 15;
	maxsize			= maxpages * PAGE_SIZE;

	printf("X-STRT X-OFFS X-CNT  | INDEX  COUNT  T-SIZE | OFFSET SIZE\n");
	printf("====== ====== ====== | ====== ====== ====== | ====== ======\n");

	for (apply_fix = 0; apply_fix < 2; apply_fix++) {
		i = 0;
		for (;;) {
			iter.iov_offset = i;
			printf("%6lx %6zx %6zx |",
			       iter.xarray_start, iter.iov_offset, iter.count);
			size = iov_iter_get_pages(&iter, maxsize, maxpages,
						  &offset);

			printf(" %6zx %6zx", offset, size);
			if (offset + size > maxsize)
				printf(" ** BIG");
			if (offset + size > iter.iov_offset + iter.count)
				printf(" ** OVER");
			printf("\n");
			if (i > PAGE_SIZE)
				break;
			i += 0x111;
		}

	}
	return 0;
}

Jeffrey Layton June 9, 2022, 5:05 p.m. UTC | #2

On Thu, 2022-06-09 at 09:07 +0100, David Howells wrote:
> The maths at the end of iter_xarray_get_pages() to calculate the actual
> size doesn't work under some circumstances, such as when it's been asked to
> extract a partial single page.  Various terms of the equation cancel out
> and you end up with actual == offset.  The same issue exists in
> iter_xarray_get_pages_alloc().
> 
> Fix these to just use min() to select the lesser amount from between the
> amount of page content transcribed into the buffer, minus the offset, and
> the size limit specified.
> 
> This doesn't appear to have caused a problem yet upstream because network
> filesystems aren't getting the pages from an xarray iterator, but rather
> passing it directly to the socket, which just iterates over it.  Cachefiles
> *does* do DIO from one to/from ext4/xfs/btrfs/etc. but it always asks for
> whole pages to be written or read.
> 
> Fixes: 7ff5062079ef ("iov_iter: Add ITER_XARRAY")
> Reported-by: Jeff Layton <[e-mail]>
> Signed-off-by: David Howells <[e-mail]>
> cc: Alexander Viro <[e-mail]>
> cc: Dominique Martinet <[e-mail]>
> cc: Mike Marshall <[e-mail]>
> cc: Gao Xiang <[e-mail]>
> cc: [e-mail]
> cc: [e-mail]
> cc: [e-mail]
> cc: [e-mail]
> cc: [e-mail]
> cc: [e-mail]
> ---
> 
>  lib/iov_iter.c |   20 ++++----------------
>  1 file changed, 4 insertions(+), 16 deletions(-)
> 
> diff --git a/lib/iov_iter.c b/lib/iov_iter.c
> index 834e1e268eb6..814f65fd0c42 100644
> --- a/lib/iov_iter.c
> +++ b/lib/iov_iter.c
> @@ -1434,7 +1434,7 @@ static ssize_t iter_xarray_get_pages(struct iov_iter *i,
>  {
>  	unsigned nr, offset;
>  	pgoff_t index, count;
> -	size_t size = maxsize, actual;
> +	size_t size = maxsize;
>  	loff_t pos;
>  
>  	if (!size || !maxpages)
> @@ -1461,13 +1461,7 @@ static ssize_t iter_xarray_get_pages(struct iov_iter *i,
>  	if (nr == 0)
>  		return 0;
>  
> -	actual = PAGE_SIZE * nr;
> -	actual -= offset;
> -	if (nr == count && size > 0) {
> -		unsigned last_offset = (nr > 1) ? 0 : offset;
> -		actual -= PAGE_SIZE - (last_offset + size);
> -	}
> -	return actual;
> +	return min(nr * PAGE_SIZE - offset, maxsize);
>  }
>  
>  /* must be done on non-empty ITER_IOVEC one */
> @@ -1602,7 +1596,7 @@ static ssize_t iter_xarray_get_pages_alloc(struct iov_iter *i,
>  	struct page **p;
>  	unsigned nr, offset;
>  	pgoff_t index, count;
> -	size_t size = maxsize, actual;
> +	size_t size = maxsize;
>  	loff_t pos;
>  
>  	if (!size)
> @@ -1631,13 +1625,7 @@ static ssize_t iter_xarray_get_pages_alloc(struct iov_iter *i,
>  	if (nr == 0)
>  		return 0;
>  
> -	actual = PAGE_SIZE * nr;
> -	actual -= offset;
> -	if (nr == count && size > 0) {
> -		unsigned last_offset = (nr > 1) ? 0 : offset;
> -		actual -= PAGE_SIZE - (last_offset + size);
> -	}
> -	return actual;
> +	return min(nr * PAGE_SIZE - offset, maxsize);
>  }
>  
>  ssize_t iov_iter_get_pages_alloc(struct iov_iter *i,
> 
> 

This seems to fix the bug I was hitting. Thanks!

Reviewed-by: Jeff Layton <[e-mail]>
Tested-by: Jeff Layton <[e-mail]>

Gao Xiang June 9, 2022, 7:03 p.m. UTC | #3

On Thu, Jun 09, 2022 at 09:07:01AM +0100, David Howells wrote:
> The maths at the end of iter_xarray_get_pages() to calculate the actual
> size doesn't work under some circumstances, such as when it's been asked to
> extract a partial single page.  Various terms of the equation cancel out
> and you end up with actual == offset.  The same issue exists in
> iter_xarray_get_pages_alloc().
> 
> Fix these to just use min() to select the lesser amount from between the
> amount of page content transcribed into the buffer, minus the offset, and
> the size limit specified.
> 
> This doesn't appear to have caused a problem yet upstream because network
> filesystems aren't getting the pages from an xarray iterator, but rather
> passing it directly to the socket, which just iterates over it.  Cachefiles
> *does* do DIO from one to/from ext4/xfs/btrfs/etc. but it always asks for
> whole pages to be written or read.
> 
> Fixes: 7ff5062079ef ("iov_iter: Add ITER_XARRAY")
> Reported-by: Jeff Layton <[e-mail]>
> Signed-off-by: David Howells <[e-mail]>
> cc: Alexander Viro <[e-mail]>
> cc: Dominique Martinet <[e-mail]>
> cc: Mike Marshall <[e-mail]>
> cc: Gao Xiang <[e-mail]>
> cc: [e-mail]
> cc: [e-mail]
> cc: [e-mail]
> cc: [e-mail]
> cc: [e-mail]
> cc: [e-mail]

Looks good to me,

Reviewed-by: Gao Xiang <[e-mail]>

Thanks,
Gao Xiang

> ---
> 
>  lib/iov_iter.c |   20 ++++----------------
>  1 file changed, 4 insertions(+), 16 deletions(-)
> 
> diff --git a/lib/iov_iter.c b/lib/iov_iter.c
> index 834e1e268eb6..814f65fd0c42 100644
> --- a/lib/iov_iter.c
> +++ b/lib/iov_iter.c
> @@ -1434,7 +1434,7 @@ static ssize_t iter_xarray_get_pages(struct iov_iter *i,
>  {
>  	unsigned nr, offset;
>  	pgoff_t index, count;
> -	size_t size = maxsize, actual;
> +	size_t size = maxsize;
>  	loff_t pos;
>  
>  	if (!size || !maxpages)
> @@ -1461,13 +1461,7 @@ static ssize_t iter_xarray_get_pages(struct iov_iter *i,
>  	if (nr == 0)
>  		return 0;
>  
> -	actual = PAGE_SIZE * nr;
> -	actual -= offset;
> -	if (nr == count && size > 0) {
> -		unsigned last_offset = (nr > 1) ? 0 : offset;
> -		actual -= PAGE_SIZE - (last_offset + size);
> -	}
> -	return actual;
> +	return min(nr * PAGE_SIZE - offset, maxsize);
>  }
>  
>  /* must be done on non-empty ITER_IOVEC one */
> @@ -1602,7 +1596,7 @@ static ssize_t iter_xarray_get_pages_alloc(struct iov_iter *i,
>  	struct page **p;
>  	unsigned nr, offset;
>  	pgoff_t index, count;
> -	size_t size = maxsize, actual;
> +	size_t size = maxsize;
>  	loff_t pos;
>  
>  	if (!size)
> @@ -1631,13 +1625,7 @@ static ssize_t iter_xarray_get_pages_alloc(struct iov_iter *i,
>  	if (nr == 0)
>  		return 0;
>  
> -	actual = PAGE_SIZE * nr;
> -	actual -= offset;
> -	if (nr == count && size > 0) {
> -		unsigned last_offset = (nr > 1) ? 0 : offset;
> -		actual -= PAGE_SIZE - (last_offset + size);
> -	}
> -	return actual;
> +	return min(nr * PAGE_SIZE - offset, maxsize);
>  }
>  
>  ssize_t iov_iter_get_pages_alloc(struct iov_iter *i,
> 
>

Guenter Roeck June 11, 2022, 1:43 p.m. UTC | #4

On Thu, Jun 09, 2022 at 09:07:01AM +0100, David Howells wrote:
> The maths at the end of iter_xarray_get_pages() to calculate the actual
> size doesn't work under some circumstances, such as when it's been asked to
> extract a partial single page.  Various terms of the equation cancel out
> and you end up with actual == offset.  The same issue exists in
> iter_xarray_get_pages_alloc().
> 
> Fix these to just use min() to select the lesser amount from between the
> amount of page content transcribed into the buffer, minus the offset, and
> the size limit specified.
> 
> This doesn't appear to have caused a problem yet upstream because network
> filesystems aren't getting the pages from an xarray iterator, but rather
> passing it directly to the socket, which just iterates over it.  Cachefiles
> *does* do DIO from one to/from ext4/xfs/btrfs/etc. but it always asks for
> whole pages to be written or read.
> 
> Fixes: 7ff5062079ef ("iov_iter: Add ITER_XARRAY")
> Reported-by: Jeff Layton <[e-mail]>
> Signed-off-by: David Howells <[e-mail]>
> cc: Alexander Viro <[e-mail]>
> cc: Dominique Martinet <[e-mail]>
> cc: Mike Marshall <[e-mail]>
> cc: Gao Xiang <[e-mail]>
> cc: [e-mail]
> cc: [e-mail]
> cc: [e-mail]
> cc: [e-mail]
> cc: [e-mail]
> cc: [e-mail]
> ---
> 
>  lib/iov_iter.c |   20 ++++----------------
>  1 file changed, 4 insertions(+), 16 deletions(-)
> 
> diff --git a/lib/iov_iter.c b/lib/iov_iter.c
> index 834e1e268eb6..814f65fd0c42 100644
> --- a/lib/iov_iter.c
> +++ b/lib/iov_iter.c
> @@ -1434,7 +1434,7 @@ static ssize_t iter_xarray_get_pages(struct iov_iter *i,
>  {
>  	unsigned nr, offset;
>  	pgoff_t index, count;
> -	size_t size = maxsize, actual;
> +	size_t size = maxsize;
>  	loff_t pos;
>  
>  	if (!size || !maxpages)
> @@ -1461,13 +1461,7 @@ static ssize_t iter_xarray_get_pages(struct iov_iter *i,
>  	if (nr == 0)
>  		return 0;
>  
> -	actual = PAGE_SIZE * nr;
> -	actual -= offset;
> -	if (nr == count && size > 0) {
> -		unsigned last_offset = (nr > 1) ? 0 : offset;
> -		actual -= PAGE_SIZE - (last_offset + size);
> -	}
> -	return actual;
> +	return min(nr * PAGE_SIZE - offset, maxsize);

This needs min_t to avoid a build error on 32-bit builds.

In file included from include/linux/kernel.h:26,
                 from include/linux/crypto.h:16,
                 from include/crypto/hash.h:11,
                 from lib/iov_iter.c:2:
lib/iov_iter.c: In function 'iter_xarray_get_pages':
include/linux/minmax.h:20:35: error: comparison of distinct pointer types lacks a cast [-Werror]
...
lib/iov_iter.c:1628:16: note: in expansion of macro 'min'
 1628 |         return min(nr * PAGE_SIZE - offset, maxsize);
      |                ^~~

Guenter

Patch

diff --git a/lib/iov_iter.c b/lib/iov_iter.c
index 834e1e268eb6..814f65fd0c42 100644
--- a/lib/iov_iter.c
+++ b/lib/iov_iter.c
@@ -1434,7 +1434,7 @@  static ssize_t iter_xarray_get_pages(struct iov_iter *i,
 {
 	unsigned nr, offset;
 	pgoff_t index, count;
-	size_t size = maxsize, actual;
+	size_t size = maxsize;
 	loff_t pos;
 
 	if (!size || !maxpages)
@@ -1461,13 +1461,7 @@  static ssize_t iter_xarray_get_pages(struct iov_iter *i,
 	if (nr == 0)
 		return 0;
 
-	actual = PAGE_SIZE * nr;
-	actual -= offset;
-	if (nr == count && size > 0) {
-		unsigned last_offset = (nr > 1) ? 0 : offset;
-		actual -= PAGE_SIZE - (last_offset + size);
-	}
-	return actual;
+	return min(nr * PAGE_SIZE - offset, maxsize);
 }
 
 /* must be done on non-empty ITER_IOVEC one */
@@ -1602,7 +1596,7 @@  static ssize_t iter_xarray_get_pages_alloc(struct iov_iter *i,
 	struct page **p;
 	unsigned nr, offset;
 	pgoff_t index, count;
-	size_t size = maxsize, actual;
+	size_t size = maxsize;
 	loff_t pos;
 
 	if (!size)
@@ -1631,13 +1625,7 @@  static ssize_t iter_xarray_get_pages_alloc(struct iov_iter *i,
 	if (nr == 0)
 		return 0;
 
-	actual = PAGE_SIZE * nr;
-	actual -= offset;
-	if (nr == count && size > 0) {
-		unsigned last_offset = (nr > 1) ? 0 : offset;
-		actual -= PAGE_SIZE - (last_offset + size);
-	}
-	return actual;
+	return min(nr * PAGE_SIZE - offset, maxsize);
 }
 
 ssize_t iov_iter_get_pages_alloc(struct iov_iter *i,