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@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.
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. :)
