Revert "mm/khugepaged: remove redundant transhuge_vma_suitable() check"
Subsystems (2)
PATCH 1/1 Revert "mm/khugepaged: remove redundant transhuge_vma_suitable() check"
A pmd should not cross a VMA boundary, which is normally enforced by vma_adjust_trans_huge(), and assumed by e.g. __split_huge_pmd_locked().
In this regard, the transhuge_vma_suitable() check in hugepage_vma_check() is not redundant with the transhuge_vma_suitable() check previously in hugepage_vma_revalidate().
The former validates the VMA itself, and checks that *some* memory in the VMA is suitable to collapse while the latter validates if collapsing at a specific address is suitable. By removing the check on the faulting address, we've inadvertently allowed collapse of a pmd that can cross vma->vm_end. Revert this change.
---
Apologies, Andrew. I think you've put the series description into this first patch (thank you). Do you mind moving it into the next patch in the series,
"mm: khugepaged: don't carry huge page to the next loop for !CONFIG_NUMA"? Note that the "mm: userspace hugepage collapse, v7" series doesn't actually depend on this patch, it was just a cleanup (and thus perhaps I shouldn't have included it in the series in the first place).
--- mm/khugepaged.c | 2 ++ 1 file changed, 2 insertions(+)
mm/khugepaged.c +2 -0
@@ -855,6 +855,8 @@ static int hugepage_vma_revalidate(struct mm_struct *mm, unsigned long address,if (!vma)return SCAN_VMA_NULL ; +if (!transhuge_vma_suitable (vma, address)) +return SCAN_ADDRESS_RANGE ;if (!hugepage_vma_check (vma, vma->vm_flags, false, false, cc->is_khugepaged))return SCAN_VMA_CHECK ;
Signature · 1 line
2.37.0.170.g444d1eabd0-goog
Quoted 8 lines · Zach O'Keefe
A pmd should not cross a VMA boundary, which is normally enforced by
vma_adjust_trans_huge(), and assumed by e.g.__split_huge_pmd_locked().In this regard, the
transhuge_vma_suitable()check inhugepage_vma_check()is not redundant with thetranshuge_vma_suitable()check previously inhugepage_vma_revalidate().
Quoted 5 lines
The former validates the VMA itself, and checks that *some* memory in the VMA is suitable to collapse while the latter validates if collapsing at a specific address is suitable. By removing the check on the faulting address, we've inadvertently allowed collapse of a pmd that can cross
vma->vm_end. Revert this change.
Aha, yeah, nice catch.
Quoted 31 lines
Fixes 143776e7512e ("mm/khugepaged: remove redundant transhuge_vma_suitable() check")Signed-off-by Zach O'Keefe <[email protected]>---
Apologies, Andrew. I think you've put the series description into this first patch (thank you). Do you mind moving it into the next patch in the series,
"mm: khugepaged: don't carry huge page to the next loop for !CONFIG_NUMA"? Note that the "mm: userspace hugepage collapse, v7" series doesn't actually depend on this patch, it was just a cleanup (and thus perhaps I shouldn't have included it in the series in the first place).--- mm/khugepaged.c | 2 ++ 1 file changed, 2 insertions(+)mm/khugepaged.c +2
-1@@ -855,6 +855,8 @@ static int hugepage_vma_revalidate(struct mm_struct *mm, unsigned long address,if (!vma)return SCAN_VMA_NULL ; +if (!transhuge_vma_suitable (vma, address)) +return SCAN_ADDRESS_RANGE ;if (!hugepage_vma_check (vma, vma->vm_flags, false, false, cc->is_khugepaged))return SCAN_VMA_CHECK ; --2.37.0.170.g444d1eabd0-goog
Quoted 16 lines · Yang Shi
Quoted 8 lines · Zach O'Keefe
A pmd should not cross a VMA boundary, which is normally enforced by
vma_adjust_trans_huge(), and assumed by e.g.__split_huge_pmd_locked().In this regard, the
transhuge_vma_suitable()check inhugepage_vma_check()is not redundant with thetranshuge_vma_suitable()check previously inhugepage_vma_revalidate().Quoted 5 lines
The former validates the VMA itself, and checks that *some* memory in the VMA is suitable to collapse while the latter validates if collapsing at a specific address is suitable. By removing the check on the faulting address, we've inadvertently allowed collapse of a pmd that can cross
vma->vm_end. Revert this change.
Quoted 4 lines
Aha, yeah, nice catch.
Reviewed-by Yang Shi <[email protected]>
Thanks Yang. Also, hughd found it :) In hindsight, I think it's actually customary to add a "Reported-by: Hugh Dickins <[email protected]>" - but since the previous patch will just be dropped and never see the light of day, I guess the value there is diminished. Anyways - credit goes to Hugh :)
Thanks,
Zach
Quoted 31 lines
Quoted 31 lines
Fixes 143776e7512e ("mm/khugepaged: remove redundant transhuge_vma_suitable() check")Signed-off-by Zach O'Keefe <[email protected]>---
Apologies, Andrew. I think you've put the series description into this first patch (thank you). Do you mind moving it into the next patch in the series,
"mm: khugepaged: don't carry huge page to the next loop for !CONFIG_NUMA"? Note that the "mm: userspace hugepage collapse, v7" series doesn't actually depend on this patch, it was just a cleanup (and thus perhaps I shouldn't have included it in the series in the first place).--- mm/khugepaged.c | 2 ++ 1 file changed, 2 insertions(+)mm/khugepaged.c +2
-1@@ -855,6 +855,8 @@ static int hugepage_vma_revalidate(struct mm_struct *mm, unsigned long address,if (!vma)return SCAN_VMA_NULL ; +if (!transhuge_vma_suitable (vma, address)) +return SCAN_ADDRESS_RANGE ;if (!hugepage_vma_check (vma, vma->vm_flags, false, false, cc->is_khugepaged))return SCAN_VMA_CHECK ; --2.37.0.170.g444d1eabd0-goog
Quoted 18 lines · Zach O'Keefe
Quoted 17 lines · Yang Shi
Quoted 8 lines · Zach O'Keefe
A pmd should not cross a VMA boundary, which is normally enforced by
vma_adjust_trans_huge(), and assumed by e.g.__split_huge_pmd_locked().In this regard, the
transhuge_vma_suitable()check inhugepage_vma_check()is not redundant with thetranshuge_vma_suitable()check previously inhugepage_vma_revalidate().Quoted 5 lines
The former validates the VMA itself, and checks that *some* memory in the VMA is suitable to collapse while the latter validates if collapsing at a specific address is suitable. By removing the check on the faulting address, we've inadvertently allowed collapse of a pmd that can cross
vma->vm_end. Revert this change.Aha, yeah, nice catch.
Quoted 8 lines
Quoted 2 lines
Reviewed-by Yang Shi <[email protected]>Thanks Yang. Also, hughd found it :) In hindsight, I think it's actually customary to add a "Reported-by: Hugh Dickins <[email protected]>" - but since the previous patch will just be dropped and never see the light of day, I guess the value there is diminished. Anyways - credit goes to Hugh :)
Thanks Zach, no probs, and as you say it would have vanished anyway.
It was something I hit in testing maple tree, and at first thought a consequence of maple tree's (previous) brk handling:
https://lore.kernel.org/linux-mm/[email protected]/
(the "coincident" paragraph).
But a similar crash occurred when I took that out of the picture:
maple tree not to blame at all - apology to Liam.
Hugh