mainline build failure due to 6c77676645ad ("iov_iter: Fix iter_xarray_get_pages{,_alloc}()")

mainline build failure due to 6c77676645ad ("iov_iter: Fix iter_xarray_get_pages{,_alloc}()")

[Sudip Mukherjee]

Hi All,

The latest mainline kernel branch fails to build for "arm allmodconfig",
"xtensa allmodconfig" and "csky allmodconfig" with the error:

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]
   20 |         (!!(sizeof((typeof(x) *)1 == (typeof(y) *)1)))
      |                                   ^~
./include/linux/minmax.h:26:18: note: in expansion of macro '__typecheck'
   26 |                 (__typecheck(x, y) && __no_side_effects(x, y))
      |                  ^~~~~~~~~~~
./include/linux/minmax.h:36:31: note: in expansion of macro '__safe_cmp'
   36 |         __builtin_choose_expr(__safe_cmp(x, y), \
      |                               ^~~~~~~~~~
./include/linux/minmax.h:45:25: note: in expansion of macro '__careful_cmp'
   45 | #define min(x, y)       __careful_cmp(x, y, <)
      |                         ^~~~~~~~~~~~~
lib/iov_iter.c:1464:16: note: in expansion of macro 'min'
 1464 |         return min(nr * PAGE_SIZE - offset, maxsize);
      |                ^~~
lib/iov_iter.c: In function 'iter_xarray_get_pages_alloc':
./include/linux/minmax.h:20:35: error: comparison of distinct pointer types lacks a cast [-Werror]
   20 |         (!!(sizeof((typeof(x) *)1 == (typeof(y) *)1)))
      |                                   ^~
./include/linux/minmax.h:26:18: note: in expansion of macro '__typecheck'
   26 |                 (__typecheck(x, y) && __no_side_effects(x, y))
      |                  ^~~~~~~~~~~
./include/linux/minmax.h:36:31: note: in expansion of macro '__safe_cmp'
   36 |         __builtin_choose_expr(__safe_cmp(x, y), \
      |                               ^~~~~~~~~~
./include/linux/minmax.h:45:25: note: in expansion of macro '__careful_cmp'
   45 | #define min(x, y)       __careful_cmp(x, y, <)
      |                         ^~~~~~~~~~~~~
lib/iov_iter.c:1628:16: note: in expansion of macro 'min'
 1628 |         return min(nr * PAGE_SIZE - offset, maxsize);


git bisect pointed to 6c77676645ad ("iov_iter: Fix iter_xarray_get_pages{,_alloc}()")

And, reverting it on top of mainline branch has fixed the build failure.

--
Regards
Sudip

Re: mainline build failure due to 6c77676645ad ("iov_iter: Fix iter_xarray_get_pages{,_alloc}()")

[Sudip Mukherjee]

On Sat, Jun 11, 2022 at 11:45 AM Sudip Mukherjee
<sudipm.mukherjee@gmail.com> wrote:
>
> Hi All,
>
> The latest mainline kernel branch fails to build for "arm allmodconfig",
> "xtensa allmodconfig" and "csky allmodconfig" with the error:

missed adding "mips allmodconfig".



-- 
Regards
Sudip

Re: mainline build failure due to 6c77676645ad ("iov_iter: Fix iter_xarray_get_pages{,_alloc}()")

[Al Viro]

On Sat, Jun 11, 2022 at 11:45:03AM +0100, Sudip Mukherjee wrote:
> Hi All,
> 
> The latest mainline kernel branch fails to build for "arm allmodconfig",
> "xtensa allmodconfig" and "csky allmodconfig" with the error:
> 
> 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]
>    20 |         (!!(sizeof((typeof(x) *)1 == (typeof(y) *)1)))
>       |                                   ^~
> ./include/linux/minmax.h:26:18: note: in expansion of macro '__typecheck'
>    26 |                 (__typecheck(x, y) && __no_side_effects(x, y))
>       |                  ^~~~~~~~~~~
> ./include/linux/minmax.h:36:31: note: in expansion of macro '__safe_cmp'
>    36 |         __builtin_choose_expr(__safe_cmp(x, y), \
>       |                               ^~~~~~~~~~
> ./include/linux/minmax.h:45:25: note: in expansion of macro '__careful_cmp'
>    45 | #define min(x, y)       __careful_cmp(x, y, <)
>       |                         ^~~~~~~~~~~~~
> lib/iov_iter.c:1464:16: note: in expansion of macro 'min'
>  1464 |         return min(nr * PAGE_SIZE - offset, maxsize);
>       |                ^~~
> lib/iov_iter.c: In function 'iter_xarray_get_pages_alloc':
> ./include/linux/minmax.h:20:35: error: comparison of distinct pointer types lacks a cast [-Werror]
>    20 |         (!!(sizeof((typeof(x) *)1 == (typeof(y) *)1)))
>       |                                   ^~
> ./include/linux/minmax.h:26:18: note: in expansion of macro '__typecheck'
>    26 |                 (__typecheck(x, y) && __no_side_effects(x, y))
>       |                  ^~~~~~~~~~~
> ./include/linux/minmax.h:36:31: note: in expansion of macro '__safe_cmp'
>    36 |         __builtin_choose_expr(__safe_cmp(x, y), \
>       |                               ^~~~~~~~~~
> ./include/linux/minmax.h:45:25: note: in expansion of macro '__careful_cmp'
>    45 | #define min(x, y)       __careful_cmp(x, y, <)
>       |                         ^~~~~~~~~~~~~
> lib/iov_iter.c:1628:16: note: in expansion of macro 'min'
>  1628 |         return min(nr * PAGE_SIZE - offset, maxsize);
> 
> 
> git bisect pointed to 6c77676645ad ("iov_iter: Fix iter_xarray_get_pages{,_alloc}()")

At a guess, should be
	return min((size_t)nr * PAGE_SIZE - offset, maxsize);

in both places.  I'm more than half-asleep right now; could you verify that it
(as the last lines of both iter_xarray_get_pages() and iter_xarray_get_pages_alloc())
builds correctly?

Re: mainline build failure due to 6c77676645ad ("iov_iter: Fix iter_xarray_get_pages{,_alloc}()")

[Al Viro]

On Sat, Jun 11, 2022 at 12:12:47PM +0000, Al Viro wrote:


> At a guess, should be
> 	return min((size_t)nr * PAGE_SIZE - offset, maxsize);
> 
> in both places.  I'm more than half-asleep right now; could you verify that it
> (as the last lines of both iter_xarray_get_pages() and iter_xarray_get_pages_alloc())
> builds correctly?

No, I'm misreading it - it's unsigned * unsigned long - unsigned vs. size_t.
On arm it ends up with unsigned long vs. unsigned int; functionally it *is*
OK (both have the same range there), but it triggers the tests.  Try 

	return min_t(size_t, nr * PAGE_SIZE - offset, maxsize);

there (both places).

Al, going back to sleep - 4 hours is not enough...

Re: mainline build failure due to 6c77676645ad ("iov_iter: Fix iter_xarray_get_pages{,_alloc}()")

[Al Viro]

On Sat, Jun 11, 2022 at 12:37:44PM +0000, Al Viro wrote:
> On Sat, Jun 11, 2022 at 12:12:47PM +0000, Al Viro wrote:
> 
> 
> > At a guess, should be
> > 	return min((size_t)nr * PAGE_SIZE - offset, maxsize);
> > 
> > in both places.  I'm more than half-asleep right now; could you verify that it
> > (as the last lines of both iter_xarray_get_pages() and iter_xarray_get_pages_alloc())
> > builds correctly?
> 
> No, I'm misreading it - it's unsigned * unsigned long - unsigned vs. size_t.
> On arm it ends up with unsigned long vs. unsigned int; functionally it *is*
> OK (both have the same range there), but it triggers the tests.  Try 
> 
> 	return min_t(size_t, nr * PAGE_SIZE - offset, maxsize);
> 
> there (both places).

The reason we can't overflow on multiplication there, BTW, is that we have
nr <= count, and count has come from weirdly open-coded
	DIV_ROUND_UP(size + offset, PAGE_SIZE)
IMO we'd better make it explicit, so how about the following:

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
diff --git a/lib/iov_iter.c b/lib/iov_iter.c
index dda6d5f481c1..150dbd314d25 100644
--- a/lib/iov_iter.c
+++ b/lib/iov_iter.c
@@ -1445,15 +1445,7 @@ static ssize_t iter_xarray_get_pages(struct iov_iter *i,
 	offset = pos & ~PAGE_MASK;
 	*_start_offset = offset;
 
-	count = 1;
-	if (size > PAGE_SIZE - offset) {
-		size -= PAGE_SIZE - offset;
-		count += size >> PAGE_SHIFT;
-		size &= ~PAGE_MASK;
-		if (size)
-			count++;
-	}
-
+	count = DIV_ROUND_UP(size + offset, PAGE_SIZE);
 	if (count > maxpages)
 		count = maxpages;
 
@@ -1461,7 +1453,7 @@ static ssize_t iter_xarray_get_pages(struct iov_iter *i,
 	if (nr == 0)
 		return 0;
 
-	return min(nr * PAGE_SIZE - offset, maxsize);
+	return min_t(size_t, nr * PAGE_SIZE - offset, maxsize);
 }
 
 /* must be done on non-empty ITER_IOVEC one */
@@ -1607,15 +1599,7 @@ static ssize_t iter_xarray_get_pages_alloc(struct iov_iter *i,
 	offset = pos & ~PAGE_MASK;
 	*_start_offset = offset;
 
-	count = 1;
-	if (size > PAGE_SIZE - offset) {
-		size -= PAGE_SIZE - offset;
-		count += size >> PAGE_SHIFT;
-		size &= ~PAGE_MASK;
-		if (size)
-			count++;
-	}
-
+	count = DIV_ROUND_UP(size + offset, PAGE_SIZE);
 	p = get_pages_array(count);
 	if (!p)
 		return -ENOMEM;
@@ -1625,7 +1609,7 @@ static ssize_t iter_xarray_get_pages_alloc(struct iov_iter *i,
 	if (nr == 0)
 		return 0;
 
-	return min(nr * PAGE_SIZE - offset, maxsize);
+	return min_t(size_t, nr * PAGE_SIZE - offset, maxsize);
 }
 
 ssize_t iov_iter_get_pages_alloc(struct iov_iter *i,

Re: mainline build failure due to 6c77676645ad ("iov_iter: Fix iter_xarray_get_pages{,_alloc}()")

[Guenter Roeck]

On Sat, Jun 11, 2022 at 12:56:27PM +0000, Al Viro wrote:
> On Sat, Jun 11, 2022 at 12:37:44PM +0000, Al Viro wrote:
> > On Sat, Jun 11, 2022 at 12:12:47PM +0000, Al Viro wrote:
> > 
> > 
> > > At a guess, should be
> > > 	return min((size_t)nr * PAGE_SIZE - offset, maxsize);
> > > 
> > > in both places.  I'm more than half-asleep right now; could you verify that it
> > > (as the last lines of both iter_xarray_get_pages() and iter_xarray_get_pages_alloc())
> > > builds correctly?
> > 
> > No, I'm misreading it - it's unsigned * unsigned long - unsigned vs. size_t.
> > On arm it ends up with unsigned long vs. unsigned int; functionally it *is*
> > OK (both have the same range there), but it triggers the tests.  Try 
> > 
> > 	return min_t(size_t, nr * PAGE_SIZE - offset, maxsize);
> > 
> > there (both places).
> 
> The reason we can't overflow on multiplication there, BTW, is that we have
> nr <= count, and count has come from weirdly open-coded
> 	DIV_ROUND_UP(size + offset, PAGE_SIZE)

That is often done to avoid possible overflows. Is size + offset
guaranteed to be smaller than ULONG_MAX ?

Guenter

> IMO we'd better make it explicit, so how about the following:
> 
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---
> diff --git a/lib/iov_iter.c b/lib/iov_iter.c
> index dda6d5f481c1..150dbd314d25 100644
> --- a/lib/iov_iter.c
> +++ b/lib/iov_iter.c
> @@ -1445,15 +1445,7 @@ static ssize_t iter_xarray_get_pages(struct iov_iter *i,
>  	offset = pos & ~PAGE_MASK;
>  	*_start_offset = offset;
>  
> -	count = 1;
> -	if (size > PAGE_SIZE - offset) {
> -		size -= PAGE_SIZE - offset;
> -		count += size >> PAGE_SHIFT;
> -		size &= ~PAGE_MASK;
> -		if (size)
> -			count++;
> -	}
> -
> +	count = DIV_ROUND_UP(size + offset, PAGE_SIZE);
>  	if (count > maxpages)
>  		count = maxpages;
>  
> @@ -1461,7 +1453,7 @@ static ssize_t iter_xarray_get_pages(struct iov_iter *i,
>  	if (nr == 0)
>  		return 0;
>  
> -	return min(nr * PAGE_SIZE - offset, maxsize);
> +	return min_t(size_t, nr * PAGE_SIZE - offset, maxsize);
>  }
>  
>  /* must be done on non-empty ITER_IOVEC one */
> @@ -1607,15 +1599,7 @@ static ssize_t iter_xarray_get_pages_alloc(struct iov_iter *i,
>  	offset = pos & ~PAGE_MASK;
>  	*_start_offset = offset;
>  
> -	count = 1;
> -	if (size > PAGE_SIZE - offset) {
> -		size -= PAGE_SIZE - offset;
> -		count += size >> PAGE_SHIFT;
> -		size &= ~PAGE_MASK;
> -		if (size)
> -			count++;
> -	}
> -
> +	count = DIV_ROUND_UP(size + offset, PAGE_SIZE);
>  	p = get_pages_array(count);
>  	if (!p)
>  		return -ENOMEM;
> @@ -1625,7 +1609,7 @@ static ssize_t iter_xarray_get_pages_alloc(struct iov_iter *i,
>  	if (nr == 0)
>  		return 0;
>  
> -	return min(nr * PAGE_SIZE - offset, maxsize);
> +	return min_t(size_t, nr * PAGE_SIZE - offset, maxsize);
>  }
>  
>  ssize_t iov_iter_get_pages_alloc(struct iov_iter *i,

Re: mainline build failure due to 6c77676645ad ("iov_iter: Fix iter_xarray_get_pages{,_alloc}()")

[Al Viro]

On Sat, Jun 11, 2022 at 07:00:52AM -0700, Guenter Roeck wrote:
> On Sat, Jun 11, 2022 at 12:56:27PM +0000, Al Viro wrote:
> > On Sat, Jun 11, 2022 at 12:37:44PM +0000, Al Viro wrote:
> > > On Sat, Jun 11, 2022 at 12:12:47PM +0000, Al Viro wrote:
> > > 
> > > 
> > > > At a guess, should be
> > > > 	return min((size_t)nr * PAGE_SIZE - offset, maxsize);
> > > > 
> > > > in both places.  I'm more than half-asleep right now; could you verify that it
> > > > (as the last lines of both iter_xarray_get_pages() and iter_xarray_get_pages_alloc())
> > > > builds correctly?
> > > 
> > > No, I'm misreading it - it's unsigned * unsigned long - unsigned vs. size_t.
> > > On arm it ends up with unsigned long vs. unsigned int; functionally it *is*
> > > OK (both have the same range there), but it triggers the tests.  Try 
> > > 
> > > 	return min_t(size_t, nr * PAGE_SIZE - offset, maxsize);
> > > 
> > > there (both places).
> > 
> > The reason we can't overflow on multiplication there, BTW, is that we have
> > nr <= count, and count has come from weirdly open-coded
> > 	DIV_ROUND_UP(size + offset, PAGE_SIZE)
> 
> That is often done to avoid possible overflows. Is size + offset
> guaranteed to be smaller than ULONG_MAX ?

You'd need iter->count and maxsize argument to be within PAGE_SIZE of
ULONG_MAX.  How would you populate that xarray, anyway?

Re: mainline build failure due to 6c77676645ad ("iov_iter: Fix iter_xarray_get_pages{,_alloc}()")

[Al Viro]

On Sat, Jun 11, 2022 at 03:01:16PM +0000, Al Viro wrote:
> On Sat, Jun 11, 2022 at 07:00:52AM -0700, Guenter Roeck wrote:
> > On Sat, Jun 11, 2022 at 12:56:27PM +0000, Al Viro wrote:
> > > On Sat, Jun 11, 2022 at 12:37:44PM +0000, Al Viro wrote:
> > > > On Sat, Jun 11, 2022 at 12:12:47PM +0000, Al Viro wrote:
> > > > 
> > > > 
> > > > > At a guess, should be
> > > > > 	return min((size_t)nr * PAGE_SIZE - offset, maxsize);
> > > > > 
> > > > > in both places.  I'm more than half-asleep right now; could you verify that it
> > > > > (as the last lines of both iter_xarray_get_pages() and iter_xarray_get_pages_alloc())
> > > > > builds correctly?
> > > > 
> > > > No, I'm misreading it - it's unsigned * unsigned long - unsigned vs. size_t.
> > > > On arm it ends up with unsigned long vs. unsigned int; functionally it *is*
> > > > OK (both have the same range there), but it triggers the tests.  Try 
> > > > 
> > > > 	return min_t(size_t, nr * PAGE_SIZE - offset, maxsize);
> > > > 
> > > > there (both places).
> > > 
> > > The reason we can't overflow on multiplication there, BTW, is that we have
> > > nr <= count, and count has come from weirdly open-coded
> > > 	DIV_ROUND_UP(size + offset, PAGE_SIZE)
> > 
> > That is often done to avoid possible overflows. Is size + offset
> > guaranteed to be smaller than ULONG_MAX ?
> 
> You'd need iter->count and maxsize argument to be within PAGE_SIZE of
> ULONG_MAX.  How would you populate that xarray, anyway?

	FWIW, it probably would be a good idea to truncate maxsize to LONG_MAX
in iov_iter_get_pages()/iov_iter_get_pages_alloc(), just to avoid that kind
of crap in the future.  Check that maxpages is not zero on the top level,
while we are at it...

	Any caller of iov_iter_get_pages{,_alloc}() must be ready to handle
getting less than what they'd asked for - if nothing else, get_user_pages_fast()
might refuse to give you more than this many pages, etc.  All in-tree callers
do, AFAICS.  And if anyone comes with "pin me more than LONG_MAX bytes of RAM
in one call, I can't accept anything less than that", well...  ISO9000-compliant
response per Dilbert would be called for.

Re: mainline build failure due to 6c77676645ad ("iov_iter: Fix iter_xarray_get_pages{,_alloc}()")

[Sudip Mukherjee]

On Sat, Jun 11, 2022 at 1:56 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Sat, Jun 11, 2022 at 12:37:44PM +0000, Al Viro wrote:
> > On Sat, Jun 11, 2022 at 12:12:47PM +0000, Al Viro wrote:
> >
> >
> > > At a guess, should be
> > >     return min((size_t)nr * PAGE_SIZE - offset, maxsize);
> > >
> > > in both places.  I'm more than half-asleep right now; could you verify that it
> > > (as the last lines of both iter_xarray_get_pages() and iter_xarray_get_pages_alloc())
> > > builds correctly?
> >
> > No, I'm misreading it - it's unsigned * unsigned long - unsigned vs. size_t.
> > On arm it ends up with unsigned long vs. unsigned int; functionally it *is*
> > OK (both have the same range there), but it triggers the tests.  Try
> >
> >       return min_t(size_t, nr * PAGE_SIZE - offset, maxsize);
> >
> > there (both places).
>
> The reason we can't overflow on multiplication there, BTW, is that we have
> nr <= count, and count has come from weirdly open-coded
>         DIV_ROUND_UP(size + offset, PAGE_SIZE)
> IMO we'd better make it explicit, so how about the following:

Thanks. Fixed the build for me.


-- 
Regards
Sudip

Re: mainline build failure due to 6c77676645ad ("iov_iter: Fix iter_xarray_get_pages{,_alloc}()")

[David Howells]

Al Viro <viro@zeniv.linux.org.uk> wrote:

> The reason we can't overflow on multiplication there, BTW, is that we have
> nr <= count, and count has come from weirdly open-coded
> 	DIV_ROUND_UP(size + offset, PAGE_SIZE)
> IMO we'd better make it explicit, so how about the following:
> 
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

It seems reasonable.

Reviewed-by: David Howells <dhowells@redhat.com>