On Tue, Nov 12, 2024 at 08:51:04AM -0800, Hugh Dickins wrote:
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.
Thank you for the detailed reply that really cleared a lot of things up for me. I was very confused at first by the change and that's why I thought to just send a patch and see whether I can get a good explanation. :)