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 <[email protected]>
Signed-off-by: David Howells <[email protected]>
cc: Alexander Viro <[email protected]>
cc: Dominique Martinet <[email protected]>
cc: Mike Marshall <[email protected]>
cc: Gao Xiang <[email protected]>
cc: [email protected]
cc: [email protected]
cc: [email protected]
cc: [email protected]
cc: [email protected]
cc: [email protected]
---
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,
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 <[email protected]>
> Signed-off-by: David Howells <[email protected]>
> cc: Alexander Viro <[email protected]>
> cc: Dominique Martinet <[email protected]>
> cc: Mike Marshall <[email protected]>
> cc: Gao Xiang <[email protected]>
> cc: [email protected]
> cc: [email protected]
> cc: [email protected]
> cc: [email protected]
> cc: [email protected]
> cc: [email protected]
> ---
>
> 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 <[email protected]>
Tested-by: Jeff Layton <[email protected]>
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 <[email protected]>
> Signed-off-by: David Howells <[email protected]>
> cc: Alexander Viro <[email protected]>
> cc: Dominique Martinet <[email protected]>
> cc: Mike Marshall <[email protected]>
> cc: Gao Xiang <[email protected]>
> cc: [email protected]
> cc: [email protected]
> cc: [email protected]
> cc: [email protected]
> cc: [email protected]
> cc: [email protected]
Looks good to me,
Reviewed-by: Gao Xiang <[email protected]>
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,
>
>
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 <[email protected]>
> Signed-off-by: David Howells <[email protected]>
> cc: Alexander Viro <[email protected]>
> cc: Dominique Martinet <[email protected]>
> cc: Mike Marshall <[email protected]>
> cc: Gao Xiang <[email protected]>
> cc: [email protected]
> cc: [email protected]
> cc: [email protected]
> cc: [email protected]
> cc: [email protected]
> cc: [email protected]
> ---
>
> 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