I botched the b67fbebd4cf9 (VM_PFNMAP TLB flush) stable backport, how do I have to fix it?

I botched the b67fbebd4cf9 (VM_PFNMAP TLB flush) stable backport, how do I have to fix it?

[Jann Horn]

Hi!

Hugh reached out to me and let me know (in nicer words) that I botched
my attempt to re-implement b67fbebd4cf9 for the stable backport; the
backport is an incomplete fix (because I forgot that in
unmap_region(), "vma" is just one of potentially several VMAs).

What should the commit message for fixing that look like? And should
we first revert the botched backport and then do a correct backport on
top of that, or should I write a single fix commit?

Sorry for causing you extra work, Greg...


Regarding how to actually fix it, one of the possible approaches
suggested by Hugh, and what I'd do, is something like this (not yet
tested) - unless someone thinks this is getting too far from upstream
and that we should backport the original fix instead, including the
refactoring?

diff --git a/mm/mmap.c b/mm/mmap.c
index 5ee3c91450de1..cee6593cbdbe3 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -2566,6 +2566,7 @@ static void unmap_region(struct mm_struct *mm,
                unsigned long start, unsigned long end)
 {
        struct vm_area_struct *next = prev ? prev->vm_next : mm->mmap;
+       struct vm_area_struct *cur_vma;
        struct mmu_gather tlb;

        lru_add_drain();
@@ -2581,8 +2582,12 @@ static void unmap_region(struct mm_struct *mm,
         * concurrent flush in this region has to be coming through the rmap,
         * and we synchronize against that using the rmap lock.
         */
-       if ((vma->vm_flags & (VM_PFNMAP|VM_MIXEDMAP)) != 0)
-               tlb_flush_mmu(&tlb);
+       for (cur_vma = vma; cur_vma; cur_vma = cur_vma->next) {
+               if ((cur_vma->vm_flags & (VM_PFNMAP|VM_MIXEDMAP)) != 0) {
+                       tlb_flush_mmu(&tlb);
+                       break;
+               }
+       }

        free_pgtables(&tlb, vma, prev ? prev->vm_end : FIRST_USER_ADDRESS,
                                 next ? next->vm_start : USER_PGTABLES_CEILING);

Re: I botched the b67fbebd4cf9 (VM_PFNMAP TLB flush) stable backport, how do I have to fix it?

[Greg Kroah-Hartman]

On Thu, Sep 15, 2022 at 12:48:44PM +0200, Jann Horn wrote:
> Hi!
> 
> Hugh reached out to me and let me know (in nicer words) that I botched
> my attempt to re-implement b67fbebd4cf9 for the stable backport; the
> backport is an incomplete fix (because I forgot that in
> unmap_region(), "vma" is just one of potentially several VMAs).
> 
> What should the commit message for fixing that look like? And should
> we first revert the botched backport and then do a correct backport on
> top of that, or should I write a single fix commit?

Which every you want is fine with me, I can easily add 1 or 2 patches :)

If you want do the revert now, and get a release out with that, and then
do a "better" patch later, that's fine too, just let me know what you
want to do.

thanks,

greg k-h

Re: I botched the b67fbebd4cf9 (VM_PFNMAP TLB flush) stable backport, how do I have to fix it?

[Jann Horn]

On Thu, Sep 15, 2022 at 1:01 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Thu, Sep 15, 2022 at 12:48:44PM +0200, Jann Horn wrote:
> > Hi!
> >
> > Hugh reached out to me and let me know (in nicer words) that I botched
> > my attempt to re-implement b67fbebd4cf9 for the stable backport; the
> > backport is an incomplete fix (because I forgot that in
> > unmap_region(), "vma" is just one of potentially several VMAs).
> >
> > What should the commit message for fixing that look like? And should
> > we first revert the botched backport and then do a correct backport on
> > top of that, or should I write a single fix commit?
>
> Which every you want is fine with me, I can easily add 1 or 2 patches :)
>
> If you want do the revert now, and get a release out with that, and then
> do a "better" patch later, that's fine too, just let me know what you
> want to do.

Ok, I just sent you a fixup patch that should apply cleanly to the
affected stable trees ("[PATCH stable 4.9-5.15] mm: Fix TLB flush for
not-first PFNMAP mappings in unmap_region()").

@Hugh: Can you maybe take a look and let me know if this looks reasonable now?

Re: I botched the b67fbebd4cf9 (VM_PFNMAP TLB flush) stable backport, how do I have to fix it?

[Hugh Dickins]

On Thu, 15 Sep 2022, Jann Horn wrote:
> On Thu, Sep 15, 2022 at 1:01 PM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Thu, Sep 15, 2022 at 12:48:44PM +0200, Jann Horn wrote:
> > > Hi!
> > >
> > > Hugh reached out to me and let me know (in nicer words) that I botched
> > > my attempt to re-implement b67fbebd4cf9 for the stable backport; the
> > > backport is an incomplete fix (because I forgot that in
> > > unmap_region(), "vma" is just one of potentially several VMAs).
> > >
> > > What should the commit message for fixing that look like? And should
> > > we first revert the botched backport and then do a correct backport on
> > > top of that, or should I write a single fix commit?
> >
> > Which every you want is fine with me, I can easily add 1 or 2 patches :)
> >
> > If you want do the revert now, and get a release out with that, and then
> > do a "better" patch later, that's fine too, just let me know what you
> > want to do.
> 
> Ok, I just sent you a fixup patch that should apply cleanly to the
> affected stable trees ("[PATCH stable 4.9-5.15] mm: Fix TLB flush for
> not-first PFNMAP mappings in unmap_region()").
> 
> @Hugh: Can you maybe take a look and let me know if this looks reasonable now?

Yes, that one looks fine to me, thanks Jann.  I would not have liked it
if Peter had chosen that way for upstream, but there's good reason to
avoid using tlb_end_vma() in these backports, and good reason to avoid
cluttering up free_pgtables().  No way great: this way good enough, thanks.

Hugh