On Mon, Jul 10, 2023 at 02:02:45PM +0100, Matthew Wilcox (Oracle) wrote:
> copy_page_from_iter_atomic() already handles !highmem compound
> pages correctly, but if we are passed a highmem compound page,
> each base page needs to be mapped & unmapped individually.
>
> Signed-off-by: Matthew Wilcox (Oracle) <willy@xxxxxxxxxxxxx>
> ---
> lib/iov_iter.c | 43 ++++++++++++++++++++++++++++---------------
> 1 file changed, 28 insertions(+), 15 deletions(-)
>
> diff --git a/lib/iov_iter.c b/lib/iov_iter.c
> index b667b1e2f688..c728b6e4fb18 100644
> --- a/lib/iov_iter.c
> +++ b/lib/iov_iter.c
> @@ -566,24 +566,37 @@ size_t iov_iter_zero(size_t bytes, struct iov_iter *i)
> }
> EXPORT_SYMBOL(iov_iter_zero);
>
> -size_t copy_page_from_iter_atomic(struct page *page, unsigned offset, size_t bytes,
> - struct iov_iter *i)
> +size_t copy_page_from_iter_atomic(struct page *page, unsigned offset,
> + size_t bytes, struct iov_iter *i)
> {
> - char *kaddr = kmap_atomic(page), *p = kaddr + offset;
> - if (!page_copy_sane(page, offset, bytes)) {
> - kunmap_atomic(kaddr);
> + size_t n, copied = 0;
> +
> + if (!page_copy_sane(page, offset, bytes))
> return 0;
> - }
> - if (WARN_ON_ONCE(!i->data_source)) {
> - kunmap_atomic(kaddr);
> + if (WARN_ON_ONCE(!i->data_source))
> return 0;
To make it easier to review the split of the kmap_atomic() until later
and the saving of the unwinding would be nice as separate patches.
> - }
> - iterate_and_advance(i, bytes, base, len, off,
> - copyin(p + off, base, len),
> - memcpy_from_iter(i, p + off, base, len)
> - )
> - kunmap_atomic(kaddr);
> - return bytes;
> +
> + do {
> + char *p;
> +
> + n = bytes - copied;
> + if (PageHighMem(page)) {
> + page += offset / PAGE_SIZE;
I don't quite follow here how before the page was not modified
to get to the first kmap_atomic(page) and now immediately if we're
on a PageHighMem(page) we're doing some arithmetic to the page
address to get the first kmap_atomic(). The only thing I could
think of is seems like an implicit assumption here that if its a compound
highmem page then we always start off with offset with a value of
0, is that right? But that seems to not be correct either.
Luis
