* [PATCH] uprobes: __replace_page() avoid BUG in munlock_vma_page()
@ 2020-08-16 20:44 ` Hugh Dickins
0 siblings, 0 replies; 8+ messages in thread
From: Hugh Dickins @ 2020-08-16 20:44 UTC (permalink / raw)
To: Andrew Morton
Cc: Song Liu, Kirill A. Shutemov, Srikar Dronamraju, Oleg Nesterov,
linux-kernel, linux-mm
syzbot crashed on the VM_BUG_ON_PAGE(PageTail) in munlock_vma_page(),
when called from uprobes __replace_page(). Which of many ways to fix it?
Settled on not calling when PageCompound (since Head and Tail are equals
in this context, PageCompound the usual check in uprobes.c, and the prior
use of FOLL_SPLIT_PMD will have cleared PageMlocked already).
Reported-by: syzbot <syzkaller@googlegroups.com>
Fixes: 5a52c9df62b4 ("uprobe: use FOLL_SPLIT_PMD instead of FOLL_SPLIT")
Signed-off-by: Hugh Dickins <hughd@google.com>
Cc: stable@vger.kernel.org # v5.4+
---
This one is not a 5.9-rc regression, but still good to fix.
kernel/events/uprobes.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
--- v5.9-rc/kernel/events/uprobes.c 2020-08-12 19:46:50.851196584 -0700
+++ linux/kernel/events/uprobes.c 2020-08-16 13:18:35.292821674 -0700
@@ -205,7 +205,7 @@ static int __replace_page(struct vm_area
try_to_free_swap(old_page);
page_vma_mapped_walk_done(&pvmw);
- if (vma->vm_flags & VM_LOCKED)
+ if ((vma->vm_flags & VM_LOCKED) && !PageCompound(old_page))
munlock_vma_page(old_page);
put_page(old_page);
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] uprobes: __replace_page() avoid BUG in munlock_vma_page()
2020-08-16 20:44 ` Hugh Dickins
(?)
@ 2020-08-17 5:43 ` Song Liu
2020-08-17 7:17 ` Hugh Dickins
-1 siblings, 1 reply; 8+ messages in thread
From: Song Liu @ 2020-08-17 5:43 UTC (permalink / raw)
To: Hugh Dickins
Cc: Andrew Morton, Kirill A. Shutemov, Srikar Dronamraju,
Oleg Nesterov, linux-kernel, linux-mm
> On Aug 16, 2020, at 1:44 PM, Hugh Dickins <hughd@google.com> wrote:
>
> syzbot crashed on the VM_BUG_ON_PAGE(PageTail) in munlock_vma_page(),
> when called from uprobes __replace_page(). Which of many ways to fix it?
> Settled on not calling when PageCompound (since Head and Tail are equals
> in this context, PageCompound the usual check in uprobes.c, and the prior
> use of FOLL_SPLIT_PMD will have cleared PageMlocked already).
>
> Reported-by: syzbot <syzkaller@googlegroups.com>
> Fixes: 5a52c9df62b4 ("uprobe: use FOLL_SPLIT_PMD instead of FOLL_SPLIT")
> Signed-off-by: Hugh Dickins <hughd@google.com>
> Cc: stable@vger.kernel.org # v5.4+
> ---
> This one is not a 5.9-rc regression, but still good to fix.
>
> kernel/events/uprobes.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> --- v5.9-rc/kernel/events/uprobes.c 2020-08-12 19:46:50.851196584 -0700
> +++ linux/kernel/events/uprobes.c 2020-08-16 13:18:35.292821674 -0700
> @@ -205,7 +205,7 @@ static int __replace_page(struct vm_area
> try_to_free_swap(old_page);
> page_vma_mapped_walk_done(&pvmw);
>
> - if (vma->vm_flags & VM_LOCKED)
> + if ((vma->vm_flags & VM_LOCKED) && !PageCompound(old_page))
Do we need munlock_vma_page() for THP page head?
Thanks,
Song
> munlock_vma_page(old_page);
> put_page(old_page);
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] uprobes: __replace_page() avoid BUG in munlock_vma_page()
2020-08-17 5:43 ` Song Liu
@ 2020-08-17 7:17 ` Hugh Dickins
0 siblings, 0 replies; 8+ messages in thread
From: Hugh Dickins @ 2020-08-17 7:17 UTC (permalink / raw)
To: Song Liu
Cc: Hugh Dickins, Andrew Morton, Kirill A. Shutemov,
Srikar Dronamraju, Oleg Nesterov, linux-kernel, linux-mm
On Mon, 17 Aug 2020, Song Liu wrote:
> > On Aug 16, 2020, at 1:44 PM, Hugh Dickins <hughd@google.com> wrote:
> >
> > syzbot crashed on the VM_BUG_ON_PAGE(PageTail) in munlock_vma_page(),
> > when called from uprobes __replace_page(). Which of many ways to fix it?
> > Settled on not calling when PageCompound (since Head and Tail are equals
> > in this context, PageCompound the usual check in uprobes.c, and the prior
> > use of FOLL_SPLIT_PMD will have cleared PageMlocked already).
> >
> > Reported-by: syzbot <syzkaller@googlegroups.com>
> > Fixes: 5a52c9df62b4 ("uprobe: use FOLL_SPLIT_PMD instead of FOLL_SPLIT")
> > Signed-off-by: Hugh Dickins <hughd@google.com>
> > Cc: stable@vger.kernel.org # v5.4+
> > ---
> > This one is not a 5.9-rc regression, but still good to fix.
> >
> > kernel/events/uprobes.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > --- v5.9-rc/kernel/events/uprobes.c 2020-08-12 19:46:50.851196584 -0700
> > +++ linux/kernel/events/uprobes.c 2020-08-16 13:18:35.292821674 -0700
> > @@ -205,7 +205,7 @@ static int __replace_page(struct vm_area
> > try_to_free_swap(old_page);
> > page_vma_mapped_walk_done(&pvmw);
> >
> > - if (vma->vm_flags & VM_LOCKED)
> > + if ((vma->vm_flags & VM_LOCKED) && !PageCompound(old_page))
>
> Do we need munlock_vma_page() for THP page head?
No: as the commit message says "the prior use of FOLL_SPLIT_PMD
will have cleared PageMlocked already" - our THP implementation
has difficulty supporting Mlocked consistently when the huge page is
somewhere mapped by ptes, so one of the things that __split_huge_pmd()
does is clear_page_mlock(), then PageDoubleMap will prevent Mlocked
being set again once GUP has brought the old_page pte back in.
But if you'd prefer us to munlock_vma_page(compound_head(old_page))
instead, I can certainly change the patch: it's one of the options
I considered, but couldn't quite bring myself to do it that way,
knowing that actually it would never find PageMlocked set. (If
PageMlocked were allowed on tail pages, I'd have used a PageMlocked
test instead of the PageCompound one: I spent nearly an hour
bikeshedding the alternatives here!)
(One day I must remind myself of when munlock_vma_page() should be
used, versus when clear_page_mlock() should be used: I think it comes
down to a choice of which stats get incremented, but I may also be
forgetting something more important: anyway, no obvious reason to
depart from the munlock_vma_page() that's always been used here.)
Hugh
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] uprobes: __replace_page() avoid BUG in munlock_vma_page()
@ 2020-08-17 7:17 ` Hugh Dickins
0 siblings, 0 replies; 8+ messages in thread
From: Hugh Dickins @ 2020-08-17 7:17 UTC (permalink / raw)
To: Song Liu
Cc: Hugh Dickins, Andrew Morton, Kirill A. Shutemov,
Srikar Dronamraju, Oleg Nesterov, linux-kernel, linux-mm
On Mon, 17 Aug 2020, Song Liu wrote:
> > On Aug 16, 2020, at 1:44 PM, Hugh Dickins <hughd@google.com> wrote:
> >
> > syzbot crashed on the VM_BUG_ON_PAGE(PageTail) in munlock_vma_page(),
> > when called from uprobes __replace_page(). Which of many ways to fix it?
> > Settled on not calling when PageCompound (since Head and Tail are equals
> > in this context, PageCompound the usual check in uprobes.c, and the prior
> > use of FOLL_SPLIT_PMD will have cleared PageMlocked already).
> >
> > Reported-by: syzbot <syzkaller@googlegroups.com>
> > Fixes: 5a52c9df62b4 ("uprobe: use FOLL_SPLIT_PMD instead of FOLL_SPLIT")
> > Signed-off-by: Hugh Dickins <hughd@google.com>
> > Cc: stable@vger.kernel.org # v5.4+
> > ---
> > This one is not a 5.9-rc regression, but still good to fix.
> >
> > kernel/events/uprobes.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > --- v5.9-rc/kernel/events/uprobes.c 2020-08-12 19:46:50.851196584 -0700
> > +++ linux/kernel/events/uprobes.c 2020-08-16 13:18:35.292821674 -0700
> > @@ -205,7 +205,7 @@ static int __replace_page(struct vm_area
> > try_to_free_swap(old_page);
> > page_vma_mapped_walk_done(&pvmw);
> >
> > - if (vma->vm_flags & VM_LOCKED)
> > + if ((vma->vm_flags & VM_LOCKED) && !PageCompound(old_page))
>
> Do we need munlock_vma_page() for THP page head?
No: as the commit message says "the prior use of FOLL_SPLIT_PMD
will have cleared PageMlocked already" - our THP implementation
has difficulty supporting Mlocked consistently when the huge page is
somewhere mapped by ptes, so one of the things that __split_huge_pmd()
does is clear_page_mlock(), then PageDoubleMap will prevent Mlocked
being set again once GUP has brought the old_page pte back in.
But if you'd prefer us to munlock_vma_page(compound_head(old_page))
instead, I can certainly change the patch: it's one of the options
I considered, but couldn't quite bring myself to do it that way,
knowing that actually it would never find PageMlocked set. (If
PageMlocked were allowed on tail pages, I'd have used a PageMlocked
test instead of the PageCompound one: I spent nearly an hour
bikeshedding the alternatives here!)
(One day I must remind myself of when munlock_vma_page() should be
used, versus when clear_page_mlock() should be used: I think it comes
down to a choice of which stats get incremented, but I may also be
forgetting something more important: anyway, no obvious reason to
depart from the munlock_vma_page() that's always been used here.)
Hugh
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] uprobes: __replace_page() avoid BUG in munlock_vma_page()
2020-08-17 7:17 ` Hugh Dickins
(?)
@ 2020-08-17 16:06 ` Song Liu
-1 siblings, 0 replies; 8+ messages in thread
From: Song Liu @ 2020-08-17 16:06 UTC (permalink / raw)
To: Hugh Dickins
Cc: Andrew Morton, Kirill A. Shutemov, Srikar Dronamraju,
Oleg Nesterov, linux-kernel, linux-mm
> On Aug 17, 2020, at 12:17 AM, Hugh Dickins <hughd@google.com> wrote:
>
> On Mon, 17 Aug 2020, Song Liu wrote:
>>> On Aug 16, 2020, at 1:44 PM, Hugh Dickins <hughd@google.com> wrote:
>>>
>>> syzbot crashed on the VM_BUG_ON_PAGE(PageTail) in munlock_vma_page(),
>>> when called from uprobes __replace_page(). Which of many ways to fix it?
>>> Settled on not calling when PageCompound (since Head and Tail are equals
>>> in this context, PageCompound the usual check in uprobes.c, and the prior
>>> use of FOLL_SPLIT_PMD will have cleared PageMlocked already).
>>>
>>> Reported-by: syzbot <syzkaller@googlegroups.com>
>>> Fixes: 5a52c9df62b4 ("uprobe: use FOLL_SPLIT_PMD instead of FOLL_SPLIT")
>>> Signed-off-by: Hugh Dickins <hughd@google.com>
>>> Cc: stable@vger.kernel.org # v5.4+
>>> ---
>>> This one is not a 5.9-rc regression, but still good to fix.
>>>
>>> kernel/events/uprobes.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> --- v5.9-rc/kernel/events/uprobes.c 2020-08-12 19:46:50.851196584 -0700
>>> +++ linux/kernel/events/uprobes.c 2020-08-16 13:18:35.292821674 -0700
>>> @@ -205,7 +205,7 @@ static int __replace_page(struct vm_area
>>> try_to_free_swap(old_page);
>>> page_vma_mapped_walk_done(&pvmw);
>>>
>>> - if (vma->vm_flags & VM_LOCKED)
>>> + if ((vma->vm_flags & VM_LOCKED) && !PageCompound(old_page))
>>
>> Do we need munlock_vma_page() for THP page head?
>
> No: as the commit message says "the prior use of FOLL_SPLIT_PMD
> will have cleared PageMlocked already" - our THP implementation
> has difficulty supporting Mlocked consistently when the huge page is
> somewhere mapped by ptes, so one of the things that __split_huge_pmd()
> does is clear_page_mlock(), then PageDoubleMap will prevent Mlocked
> being set again once GUP has brought the old_page pte back in.
>
> But if you'd prefer us to munlock_vma_page(compound_head(old_page))
> instead, I can certainly change the patch: it's one of the options
> I considered, but couldn't quite bring myself to do it that way,
> knowing that actually it would never find PageMlocked set. (If
> PageMlocked were allowed on tail pages, I'd have used a PageMlocked
> test instead of the PageCompound one: I spent nearly an hour
> bikeshedding the alternatives here!)
>
> (One day I must remind myself of when munlock_vma_page() should be
> used, versus when clear_page_mlock() should be used: I think it comes
> down to a choice of which stats get incremented, but I may also be
> forgetting something more important: anyway, no obvious reason to
> depart from the munlock_vma_page() that's always been used here.)
Thanks a lot for the explanation!
Acked-by: Song Liu <songliubraving@fb.com>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] uprobes: __replace_page() avoid BUG in munlock_vma_page()
2020-08-16 20:44 ` Hugh Dickins
(?)
(?)
@ 2020-08-17 14:07 ` Oleg Nesterov
-1 siblings, 0 replies; 8+ messages in thread
From: Oleg Nesterov @ 2020-08-17 14:07 UTC (permalink / raw)
To: Hugh Dickins
Cc: Andrew Morton, Song Liu, Kirill A. Shutemov, Srikar Dronamraju,
linux-kernel, linux-mm
On 08/16, Hugh Dickins wrote:
>
> --- v5.9-rc/kernel/events/uprobes.c 2020-08-12 19:46:50.851196584 -0700
> +++ linux/kernel/events/uprobes.c 2020-08-16 13:18:35.292821674 -0700
> @@ -205,7 +205,7 @@ static int __replace_page(struct vm_area
> try_to_free_swap(old_page);
> page_vma_mapped_walk_done(&pvmw);
>
> - if (vma->vm_flags & VM_LOCKED)
> + if ((vma->vm_flags & VM_LOCKED) && !PageCompound(old_page))
> munlock_vma_page(old_page);
> put_page(old_page);
Thanks!
Acked-by: Oleg Nesterov <oleg@redhat.com>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] uprobes: __replace_page() avoid BUG in munlock_vma_page()
2020-08-16 20:44 ` Hugh Dickins
` (2 preceding siblings ...)
(?)
@ 2020-08-17 14:40 ` Srikar Dronamraju
-1 siblings, 0 replies; 8+ messages in thread
From: Srikar Dronamraju @ 2020-08-17 14:40 UTC (permalink / raw)
To: Hugh Dickins
Cc: Andrew Morton, Song Liu, Kirill A. Shutemov, Oleg Nesterov,
linux-kernel, linux-mm
* Hugh Dickins <hughd@google.com> [2020-08-16 13:44:25]:
> syzbot crashed on the VM_BUG_ON_PAGE(PageTail) in munlock_vma_page(),
> when called from uprobes __replace_page(). Which of many ways to fix it?
> Settled on not calling when PageCompound (since Head and Tail are equals
> in this context, PageCompound the usual check in uprobes.c, and the prior
> use of FOLL_SPLIT_PMD will have cleared PageMlocked already).
>
> Reported-by: syzbot <syzkaller@googlegroups.com>
> Fixes: 5a52c9df62b4 ("uprobe: use FOLL_SPLIT_PMD instead of FOLL_SPLIT")
> Signed-off-by: Hugh Dickins <hughd@google.com>
> Cc: stable@vger.kernel.org # v5.4+
> ---
> This one is not a 5.9-rc regression, but still good to fix.
>
> kernel/events/uprobes.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> --- v5.9-rc/kernel/events/uprobes.c 2020-08-12 19:46:50.851196584 -0700
> +++ linux/kernel/events/uprobes.c 2020-08-16 13:18:35.292821674 -0700
> @@ -205,7 +205,7 @@ static int __replace_page(struct vm_area
> try_to_free_swap(old_page);
> page_vma_mapped_walk_done(&pvmw);
>
> - if (vma->vm_flags & VM_LOCKED)
> + if ((vma->vm_flags & VM_LOCKED) && !PageCompound(old_page))
> munlock_vma_page(old_page);
> put_page(old_page);
>
Looks good to me.
Reviewed-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
--
Thanks and Regards
Srikar Dronamraju
^ permalink raw reply [flat|nested] 8+ messages in thread