[lvc-project] [PATCH] lib: free pagelist on error in iov_iter_extract_pages()

[Caleb Sander Mateos]

On Fri, May 8, 2026 at 4:16 AM Dmitry Antipov <dmantipov at yandex.ru> wrote:
>
> Since 'iov_iter_extract_pages()' may allocate new pagelist if the passed
> one isn't large enough, the worst-case scenario may be:
>
> ...
> struct page *stack_pages[SMALL];
> struct page **pages = stack_pages;
> ...
> if (iov_iter_extract_pages(i..., &pages, ...) <= 0) {
>         /* Even in case of error, new pagelist may be allocated */
>         if (pages != stack_pages)
>                 kvfree(pages);                                  [1]

iov_iter_extract_pages() will only allocate a pages array if the
initial struct page ** passed is NULL (see want_pages_array()). So the
condition pages != stack_pages will never be true. Indeed, it looks
like *all* callers of iov_iter_extract_pages() pass a non-NULL struct
page **. Would it make sense for iov_iter_extract_pages() to require a
pre-allocated pages array and remove support for allocating one?

>         /* The rest of error handling and return */
> }
> /* Regular flow */
> ...
> if (pages != stack_pages)
>         kvfree(pages);
> ...
> return 0;
>
> If you're unlucky so SMALL amount of pages wasn't enough and new
> pagelist was allocated, missing [1] causes the memory leak similar
> to one I've recently observed and fixed for 6.12 in [2]. So adjust
> 'iov_iter_extract_pages()' to make such a cleanup itself rather than
> rely on caller's handling on error paths, thus making [1] not needed.
>
> [2] https://lore.kernel.org/stable/20260505094529.406783-1-dmantipov@yandex.ru/T/#u
>
> Suggested-by: Fedor Pchelkin <pchelkin at ispras.ru>
> Signed-off-by: Dmitry Antipov <dmantipov at yandex.ru>
> ---
>  lib/iov_iter.c | 54 ++++++++++++++++++++++++++++++--------------------
>  1 file changed, 33 insertions(+), 21 deletions(-)
>
> diff --git a/lib/iov_iter.c b/lib/iov_iter.c
> index 243662af1af7..46dd11913df0 100644
> --- a/lib/iov_iter.c
> +++ b/lib/iov_iter.c
> @@ -1807,7 +1807,8 @@ static ssize_t iov_iter_extract_user_pages(struct iov_iter *i,
>   *  (*) Use with ITER_DISCARD is not supported as that has no content.
>   *
>   * On success, the function sets *@pages to the new pagelist, if allocated, and
> - * sets *offset0 to the offset into the first page.
> + * sets *offset0 to the offset into the first page. On error, new pagelist
> + * is freed if was allocated, and *@pages sets back to its original value.
>   *
>   * It may also return -ENOMEM and -EFAULT.
>   */
> @@ -1818,31 +1819,42 @@ ssize_t iov_iter_extract_pages(struct iov_iter *i,
>                                iov_iter_extraction_t extraction_flags,
>                                size_t *offset0)
>  {
> +       struct page **oldpages = *pages;
> +       int ret;

Should be ssize_t to avoid truncation?

> +
>         maxsize = min_t(size_t, min_t(size_t, maxsize, i->count), MAX_RW_COUNT);
>         if (!maxsize)
>                 return 0;
>
>         if (likely(user_backed_iter(i)))
> -               return iov_iter_extract_user_pages(i, pages, maxsize,
> -                                                  maxpages, extraction_flags,
> -                                                  offset0);
> -       if (iov_iter_is_kvec(i))
> -               return iov_iter_extract_kvec_pages(i, pages, maxsize,
> -                                                  maxpages, extraction_flags,
> -                                                  offset0);
> -       if (iov_iter_is_bvec(i))
> -               return iov_iter_extract_bvec_pages(i, pages, maxsize,
> -                                                  maxpages, extraction_flags,
> -                                                  offset0);
> -       if (iov_iter_is_folioq(i))
> -               return iov_iter_extract_folioq_pages(i, pages, maxsize,
> -                                                    maxpages, extraction_flags,
> -                                                    offset0);
> -       if (iov_iter_is_xarray(i))
> -               return iov_iter_extract_xarray_pages(i, pages, maxsize,
> -                                                    maxpages, extraction_flags,
> -                                                    offset0);
> -       return -EFAULT;
> +               ret = iov_iter_extract_user_pages(i, pages, maxsize,
> +                                                 maxpages, extraction_flags,
> +                                                 offset0);
> +       else if (iov_iter_is_kvec(i))
> +               ret = iov_iter_extract_kvec_pages(i, pages, maxsize,
> +                                                 maxpages, extraction_flags,
> +                                                 offset0);
> +       else if (iov_iter_is_bvec(i))
> +               ret = iov_iter_extract_bvec_pages(i, pages, maxsize,
> +                                                 maxpages, extraction_flags,
> +                                                 offset0);
> +       else if (iov_iter_is_folioq(i))
> +               ret = iov_iter_extract_folioq_pages(i, pages, maxsize,
> +                                                   maxpages, extraction_flags,
> +                                                   offset0);
> +       else if (iov_iter_is_xarray(i))
> +               ret = iov_iter_extract_xarray_pages(i, pages, maxsize,
> +                                                   maxpages, extraction_flags,
> +                                                   offset0);
> +       else
> +               ret = -EFAULT;
> +
> +       if (unlikely(ret) && *pages && *pages != oldpages) {

iov_iter_extract_pages() returns a positive value (number of bytes
extracted) on success (even if all maxsize bytes were extracted), so I
don't think this condition is correct. Callers seem to be inconsistent
about whether they treat ret < 0 or ret <= 0 as the error case, which
is another argument not to handle freeing the pages array inside
iov_iter_extract_pages().

Best,
Caleb

> +               kvfree(*pages);
> +               *pages = oldpages;
> +       }
> +
> +       return ret;
>  }
>  EXPORT_SYMBOL_GPL(iov_iter_extract_pages);
>
> --
> 2.54.0
>
>