Re: [PATCH] iov_iter: fix copy_page_from_iter_atomic() for highmem

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@vger.kernel.org Reviewed-by: David Howells dhowells@redhat.com Signed-off-by: Christian Brauner brauner@kernel.org

---

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