I botched the b67fbebd4cf9 (VM_PFNMAP TLB flush) stable backport, how do I have to fix it?
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);
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
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?
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
linux-stable-mirror@lists.linaro.org
-
Greg Kroah-Hartman -
Hugh Dickins -
Jann Horn