On Tue, 12 Nov 2024, Christian Brauner wrote:
> When fixing copy_page_from_iter_atomic() in c749d9b7ebbc ("iov_iter: fix
> copy_page_from_iter_atomic() if KMAP_LOCAL_FORCE_MAP") the check for
> PageHighMem() got moved out of the loop. If copy_page_from_iter_atomic()
> crosses page boundaries it will use a stale PageHighMem() check for an
> earlier page.
>
> Fixes: 908a1ad89466 ("iov_iter: Handle compound highmem pages in copy_page_from_iter_atomic()")
> Fixes: c749d9b7ebbc ("iov_iter: fix copy_page_from_iter_atomic() if KMAP_LOCAL_FORCE_MAP")
> Cc: stable@xxxxxxxxxxxxxxx
> Reviewed-by: David Howells <dhowells@xxxxxxxxxx>
> Signed-off-by: Christian Brauner <brauner@xxxxxxxxxx>
> ---
> Hey Linus,
>
> I think the original fix was buggy but then again my knowledge of
> highmem isn't particularly detailed. Compile tested only. If correct, I
> would ask you to please apply it directly.
I haven't seen whatever discussion led up to this. I don't believe
my commit was buggy (setting uses_kmap once at the top was intentional);
but I haven't looked at the other Fixee, and I've no objection if you all
prefer to add this on.
I imagine you're worried by the idea of a folio getting passed in, and
its first struct page is in a lowmem pageblock, but the folio somehow
spans pageblocks so that a later struct page is in a highmem pageblock.
That does not happen - except perhaps in the case of a hugetlb gigantic
folio, cobbled together from separate pageblocks. But the code here,
before my change and after it and after this mod, does not allow for
that case anyway - the "page += offset / PAGE_SIZE" is assuming that
struct pages are contiguous. If there is a worry here (I assumed not),
I think it would be that.
Cc'ing Matthew who is much more alert to such issues than I am.
Dashing out shortly, back in two hours,
Hugh
>
> Thanks!
> Christian
> ---
> lib/iov_iter.c | 12 ++++++++----
> 1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/lib/iov_iter.c b/lib/iov_iter.c
> index 908e75a28d90..e90a5ababb11 100644
> --- a/lib/iov_iter.c
> +++ b/lib/iov_iter.c
> @@ -457,12 +457,16 @@ size_t iov_iter_zero(size_t bytes, struct iov_iter *i)
> }
> EXPORT_SYMBOL(iov_iter_zero);
>
> +static __always_inline bool iter_atomic_uses_kmap(struct page *page)
> +{
> + return IS_ENABLED(CONFIG_DEBUG_KMAP_LOCAL_FORCE_MAP) ||
> + PageHighMem(page);
> +}
> +
> size_t copy_page_from_iter_atomic(struct page *page, size_t offset,
> size_t bytes, struct iov_iter *i)
> {
> size_t n, copied = 0;
> - bool uses_kmap = IS_ENABLED(CONFIG_DEBUG_KMAP_LOCAL_FORCE_MAP) ||
> - PageHighMem(page);
>
> if (!page_copy_sane(page, offset, bytes))
> return 0;
> @@ -473,7 +477,7 @@ size_t copy_page_from_iter_atomic(struct page *page, size_t offset,
> char *p;
>
> n = bytes - copied;
> - if (uses_kmap) {
> + if (iter_atomic_uses_kmap(page)) {
> page += offset / PAGE_SIZE;
> offset %= PAGE_SIZE;
> n = min_t(size_t, n, PAGE_SIZE - offset);
> @@ -484,7 +488,7 @@ size_t copy_page_from_iter_atomic(struct page *page, size_t offset,
> kunmap_atomic(p);
> copied += n;
> offset += n;
> - } while (uses_kmap && copied != bytes && n > 0);
> + } while (iter_atomic_uses_kmap(page) && copied != bytes && n > 0);
>
> return copied;
> }
> --
> 2.45.2
