mm: Fixes on pte markers
Subsystems (5)
cover mm: Fixes on pte markers
Patch 1 resolves the syzkiller report from Pengfei.
Patch 2 further harden pte markers when used with the recent swapin error markers. The major case is we should persist a swapin error marker after fork(), so child shouldn't read a corrupted page.
No report so far with patch 2, but it can be somehow tested with things like:
https://github.com/xzpeter/clibs/blob/master/misc/pageout.c
Plus some hacks.
Please have a look, thanks.
Peter Xu (2):
mm/uffd: Fix pte marker when fork() without fork event
mm: Fix a few rare cases of using swapin error pte marker
mm/hugetlb.c | 3 +++ mm/memory.c | 14 +++++++------- mm/mprotect.c | 8 +++++++- 3 files changed, 17 insertions(+), 8 deletions(-)
Signature · 1 line
2.37.3
PATCH 1/2 mm/uffd: Fix pte marker when fork() without fork event
When fork(), dst_vma is not guaranteed to have VM_UFFD_WP even if src may have it and has pte marker installed. The warning is improper along with the comment. The right thing is to inherit the pte marker when needed, or keep the dst pte empty.
A vague guess is this happened by an accident when there's the prior patch to introduce src/dst vma into this helper during the uffd-wp feature got developed and I probably messed up in the rebase, since if we replace dst_vma with src_vma the warning & comment it all makes sense too.
Hugetlb did exactly the right here (copy_hugetlb_page_range()). Fix the general path.
Reproducer:
https://github.com/xupengfe/syzkaller_logs/blob/main/221208_115556_copy_page_range/repro.c
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=216808
--- mm/memory.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)
mm/memory.c +2 -6
@@ -828,12 +828,8 @@ copy_nonpresent_pte(struct mm_struct *dst_mm, struct mm_struct *src_mm,return -EBUSY ;return -ENOENT ; }else if (is_pte_marker_entry (entry)) { -/* -* We're copying the pgtable should only because dst_vma has -* uffd-wp enabled, do sanity check. -*/ -WARN_ON_ONCE (!userfaultfd_wp (dst_vma)); -set_pte_at (dst_mm, addr, dst_pte, pte); +if (userfaultfd_wp (dst_vma)) +set_pte_at (dst_mm, addr, dst_pte, pte);return 0 ; }if (!userfaultfd_wp (dst_vma))
Signature · 1 line
2.37.3
Re: [PATCH 1/2] mm/uffd: Fix pte marker when fork() without fork event
Quoted 35 lines · Peter Xu
When
fork(),dst_vmais not guaranteed to haveVM_UFFD_WPeven if src may have it and has pte marker installed. The warning is improper along with the comment. The right thing is to inherit the pte marker when needed, or keep the dst pte empty.A vague guess is this happened by an accident when there's the prior patch to introduce src/dst vma into this helper during the uffd-wp feature got developed and I probably messed up in the rebase, since if we replace
dst_vmawithsrc_vmathe warning & comment it all makes sense too.Hugetlb did exactly the right here (
copy_hugetlb_page_range()). Fix the general path.Reproducer:
https://github.com/xupengfe/syzkaller_logs/blob/main/221208_115556_copy_page_range/repro.c
Cc <[email protected]> # 5.19+Fixes c56d1b62cce8 ("mm/shmem: handle uffd-wp during fork()")Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=216808
Reported-by Pengfei Xu <[email protected]>Signed-off-by Peter Xu <[email protected]>--- mm/memory.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)mm/memory.c +0
-1@@ -828,12 +828,8 @@ copy_nonpresent_pte(struct mm_struct *dst_mm, struct mm_struct *src_mm,return -EBUSY ;return -ENOENT ; }else if (is_pte_marker_entry (entry)) { -/*
mm/memory.c copy_nonpresent_pte(struct mm_struct *dst_mm, struct mm_struct *src_mm, · line 835
Quoted 10 lines
mm/memory.c +2
-5-* We're copying the pgtable should only because dst_vma has -* uffd-wp enabled, do sanity check. -*/ -WARN_ON_ONCE (!userfaultfd_wp (dst_vma)); -set_pte_at (dst_mm, addr, dst_pte, pte); +if (userfaultfd_wp (dst_vma)) +set_pte_at (dst_mm, addr, dst_pte, pte);return 0 ; }if (!userfaultfd_wp (dst_vma))
Staring at the code first made me go "what about other PTE markers". I then looked into the discussion in patch #2. The fix as is is suboptimal, because it
1) Removes the warning which is good, but
2) Silently drops swapin errors now
So it silently breaks something else temporarily ...
I remember, that theoretically we could have multiple markers stored in a single PTE marker.
Wouldn't it be cleaner to be able to "clean" specific markers from a PTE marker without having to special case on each and everyone? I mean, only uffd-wp is really special such that it might disappear for the target.
Something like (pseudocode):
if (!userfaultfd_wp(dst_vma))
pte_marker_clear_uff_wp(entry);
if (!pte_marker_empty(entry)) {
pte = make_pte_marker(pte_marker_get(entry));
set_pte_at(dst_mm, addr, dst_pte, pte);
}Then this fix would be correct and backport-able even without #2. And it would work for new types of markers :)
I'd prefer a fix that doesn't break something else temporarily, even if the stable backport might require 5 additional minutes to do. So squashing #2 into #1 would also work.
Signature · 3 lines
Thanks, David / dhildenb
Re: [PATCH 1/2] mm/uffd: Fix pte marker when fork() without fork event
Quoted 57 lines · David Hildenbrand
Quoted 35 lines · Peter Xu
When
fork(),dst_vmais not guaranteed to haveVM_UFFD_WPeven if src may have it and has pte marker installed. The warning is improper along with the comment. The right thing is to inherit the pte marker when needed, or keep the dst pte empty.A vague guess is this happened by an accident when there's the prior patch to introduce src/dst vma into this helper during the uffd-wp feature got developed and I probably messed up in the rebase, since if we replace
dst_vmawithsrc_vmathe warning & comment it all makes sense too.Hugetlb did exactly the right here (
copy_hugetlb_page_range()). Fix the general path.Reproducer:
https://github.com/xupengfe/syzkaller_logs/blob/main/221208_115556_copy_page_range/repro.c
Cc <[email protected]> # 5.19+Fixes c56d1b62cce8 ("mm/shmem: handle uffd-wp during fork()")Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=216808
Reported-by Pengfei Xu <[email protected]>Signed-off-by Peter Xu <[email protected]>--- mm/memory.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)mm/memory.c +0
-1@@ -828,12 +828,8 @@ copy_nonpresent_pte(struct mm_struct *dst_mm, struct mm_struct *src_mm,return -EBUSY ;return -ENOENT ; }else if (is_pte_marker_entry (entry)) { -/* mm/memory.c copy_nonpresent_pte(struct mm_struct *dst_mm, struct mm_struct *src_mm, · line 835
Quoted 10 lines
mm/memory.c +2
-5-* We're copying the pgtable should only because dst_vma has -* uffd-wp enabled, do sanity check. -*/ -WARN_ON_ONCE (!userfaultfd_wp (dst_vma)); -set_pte_at (dst_mm, addr, dst_pte, pte); +if (userfaultfd_wp (dst_vma)) +set_pte_at (dst_mm, addr, dst_pte, pte);return 0 ; }if (!userfaultfd_wp (dst_vma))Staring at the code first made me go "what about other PTE markers". I then looked into the discussion in patch #2. The fix as is is suboptimal, because it
1) Removes the warning which is good, but
2) Silently drops swapin errors nowSo it silently breaks something else temporarily ...
Quoted 6 lines
I remember, that theoretically we could have multiple markers stored in a single PTE marker.
Wouldn't it be cleaner to be able to "clean" specific markers from a PTE marker without having to special case on each and everyone? I mean, only uffd-wp is really special such that it might disappear for the target.
Quotting the commit message in patch 2:
Currently there is a priority difference between the uffd-wp bit and the swapin error entry, in which the swapin error always has higher priority (e.g. we don't need to wr-protect a swapin error pte marker).
If there will be a 3rd bit introduced, we'll probably need to consider a more involved approach so we may need to start operate on the bits. Let's leave that for later.
I actually started the fix with something like that, but I noticed it's not needed to add more code if there's no 3rd bit introduced so I dropped that. I decided to go the simpler change approach and leave that for later.
Quoted 4 lines
Something like (pseudocode):
if (!userfaultfd_wp(dst_vma))
Quoted 8 lines
pte_marker_clear_uff_wp(entry); if (!pte_marker_empty(entry)) { pte = make_pte_marker(pte_marker_get(entry)); set_pte_at(dst_mm, addr, dst_pte, pte); }Then this fix would be correct and backport-able even without #2. And it would work for new types of markers :)
When that comes, we may need one set_pte_marker_at() taking care of empty pte markers, otherwise there can be a lot of such check.
Quoted 5 lines
I'd prefer a fix that doesn't break something else temporarily, even if the stable backport might require 5 additional minutes to do. So squashing #2 into #1 would also work.
The thing is whether do we care about someone: (1) explicitly checkout at the commit of patch 1, then (2) runs the kernel, hit a swapnin error, (3) fork(), and (4) access the swapin error page in the child.
To me I don't care even starting from (1).. because it really shouldn't happen at all in any serious environment.
The other reason is these are indeed two issues to solve. Even if by accident we kept the swapin error in old code we'll probably dump an warning which is not wanted either. It's not something someone will really get benefit from..
So like many other places, I don't have a strong opinion, but personally I prefer the current approach.
Andrew, let me know if you want me to repost with a squashed version.
Thanks,
Signature · 1 line
Peter Xu
Re: [PATCH 1/2] mm/uffd: Fix pte marker when fork() without fork event
Quoted 11 lines
Quoted 4 lines
Wouldn't it be cleaner to be able to "clean" specific markers from a PTE marker without having to special case on each and everyone? I mean, only uffd-wp is really special such that it might disappear for the target.
Quotting the commit message in patch 2:
Currently there is a priority difference between the uffd-wp bit and the swapin error entry, in which the swapin error always has higher priority (e.g. we don't need to wr-protect a swapin error pte marker).
Quoted 7 lines
If there will be a 3rd bit introduced, we'll probably need to consider a more involved approach so we may need to start operate on the bits. Let's leave that for later.
I actually started the fix with something like that, but I noticed it's not needed to add more code if there's no 3rd bit introduced so I dropped that. I decided to go the simpler change approach and leave that for later.
Okay, makes sense.
Quoted 11 lines
Quoted 11 lines
Something like (pseudocode):
if (!userfaultfd_wp(dst_vma))
pte_marker_clear_uff_wp(entry); if (!pte_marker_empty(entry)) { pte = make_pte_marker(pte_marker_get(entry)); set_pte_at(dst_mm, addr, dst_pte, pte); }
Quoted 5 lines
Quoted 2 lines
Then this fix would be correct and backport-able even without #2. And it would work for new types of markers :)
When that comes, we may need one
set_pte_marker_at()taking care of empty pte markers, otherwise there can be a lot of such check.
Right. In the future it might be cleaner.
Quoted 10 lines
Quoted 6 lines
I'd prefer a fix that doesn't break something else temporarily, even if the stable backport might require 5 additional minutes to do. So squashing #2 into #1 would also work.
The thing is whether do we care about someone: (1) explicitly checkout at the commit of patch 1, then (2) runs the kernel, hit a swapnin error, (3)
fork(), and (4) access the swapin error page in the child.
I'm more concerned about backports, when one backports #1 but not #2. In theory, patch #2 fixes patch #1, because that introduced IMHO a real regression -- a possible memory corruption when discarding a hwpoison marker. Warnings are not nice but at least indicate that something needs a second look.
Quoted 11 lines
To me I don't care even starting from (1).. because it really shouldn't happen at all in any serious environment.
The other reason is these are indeed two issues to solve. Even if by accident we kept the swapin error in old code we'll probably dump an warning which is not wanted either. It's not something someone will really get benefit from..
So like many other places, I don't have a strong opinion, but personally I prefer the current approach.
Me neither, two patches just felt more complicated than it should be.
Anyhow, the final code change LGTM.
Signature · 3 lines
Thanks, David / dhildenb
Re: [PATCH 1/2] mm/uffd: Fix pte marker when fork() without fork event
Quoted 5 lines · David Hildenbrand
I'm more concerned about backports, when one backports #1 but not #2. In theory, patch #2 fixes patch #1, because that introduced IMHO a real regression -- a possible memory corruption when discarding a hwpoison marker. Warnings are not nice but at least indicate that something needs a second look.
Note that backporting patch 1 only is exactly what I wanted to do here - it means his/her tree should not have the swapin error pte markers at all.
The swapin error pte marker change only existed for a few days in Linus's tree, so no one should be backporting patch 2.
Thanks,
Signature · 1 line
Peter Xu
David Hildenbrand
Acked-byRe: [PATCH 1/2] mm/uffd: Fix pte marker when fork() without fork event
Quoted 7 lines · Peter Xu
Quoted 5 lines · David Hildenbrand
I'm more concerned about backports, when one backports #1 but not #2. In theory, patch #2 fixes patch #1, because that introduced IMHO a real regression -- a possible memory corruption when discarding a hwpoison marker. Warnings are not nice but at least indicate that something needs a second look.
Quoted 5 lines
Note that backporting patch 1 only is exactly what I wanted to do here - it means his/her tree should not have the swapin error pte markers at all.
The swapin error pte marker change only existed for a few days in Linus's tree, so no one should be backporting patch 2.
Right, and these patches are supposed to land in 6.2 as well. Makes sense to me then.
Especially, the other parts in patch #2 are worth being in a separate patch.
Signature · 3 lines
Thanks, David / dhildenb
Re: [PATCH 1/2] mm/uffd: Fix pte marker when fork() without fork event
In reply to Peter XuQuoted 15 lines · Peter Xu
When
fork(),dst_vmais not guaranteed to haveVM_UFFD_WPeven if src may have it and has pte marker installed. The warning is improper along with the comment. The right thing is to inherit the pte marker when needed, or keep the dst pte empty.A vague guess is this happened by an accident when there's the prior patch to introduce src/dst vma into this helper during the uffd-wp feature got developed and I probably messed up in the rebase, since if we replace
dst_vmawithsrc_vmathe warning & comment it all makes sense too.Hugetlb did exactly the right here (
copy_hugetlb_page_range()). Fix the general path.Reproducer:
Quoted 7 lines
https://github.com/xupengfe/syzkaller_logs/blob/main/221208_115556_copy_page_range/repro.c
Cc <[email protected]> # 5.19+Fixes c56d1b62cce8 ("mm/shmem: handle uffd-wp during fork()")Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=216808
Reported-by Pengfei Xu <[email protected]>Signed-off-by Peter Xu <[email protected]>
Looks good to me. Thanks.
Thanks,
Miaohe Lin
PATCH 2/2 mm: Fix a few rare cases of using swapin error pte marker
In reply to Peter XuThis patch should harden commit 15520a3f0469 ("mm: use pte markers for swap errors") on using pte markers for swapin errors on a few corner cases.
1. Propagate swapin errors across fork()s: if there're swapin errors in the parent mm, after fork()s the child should sigbus too when an error page is accessed.
2. Fix a rare condition race in pte_marker_clear() where a uffd-wp pte marker can be quickly switched to a swapin error.
3. Explicitly ignore swapin error pte markers in change_protection().
I mostly don't worry on (2) or (3) at all, but we should still have them. Case (1) is special because it can potentially cause silent data corrupt on child when parent has swapin error triggered with swapoff, but since swapin error is rare itself already it's probably not easy to trigger either.
Currently there is a priority difference between the uffd-wp bit and the swapin error entry, in which the swapin error always has higher priority (e.g. we don't need to wr-protect a swapin error pte marker).
If there will be a 3rd bit introduced, we'll probably need to consider a more involved approach so we may need to start operate on the bits. Let's leave that for later.
This patch is tested with case (1) explicitly where we'll get corrupted data before in the child if there's existing swapin error pte markers, and after patch applied the child can be rightfully killed.
We don't need to copy stable for this one since 15520a3f0469 just landed as part of v6.2-rc1, only "Fixes" applied.
--- mm/hugetlb.c | 3 +++ mm/memory.c | 8 ++++++-- mm/mprotect.c | 8 +++++++- 3 files changed, 16 insertions(+), 3 deletions(-)
mm/hugetlb.c +3 -0
@@ -4884,6 +4884,9 @@ int copy_hugetlb_page_range(struct mm_struct *dst, struct mm_struct *src, entry =huge_pte_clear_uffd_wp (entry);set_huge_pte_at (dst, addr, dst_pte, entry); }else if (unlikely (is_pte_marker (entry))) { +/* No swap on hugetlb */ +WARN_ON_ONCE ( +is_swapin_error_entry (pte_to_swp_entry (entry)));/* * We copy the pte marker only if the dst vma has * uffd-wp enabled.
mm/memory.c +6 -2
@@ -828,7 +828,7 @@ copy_nonpresent_pte(struct mm_struct *dst_mm, struct mm_struct *src_mm,return -EBUSY ;return -ENOENT ; }else if (is_pte_marker_entry (entry)) { -if (userfaultfd_wp (dst_vma)) +if (is_swapin_error_entry (entry) ||userfaultfd_wp (dst_vma))set_pte_at (dst_mm, addr, dst_pte, pte);return 0 ; }
@@ -3625,8 +3625,12 @@ static vm_fault_t pte_marker_clear(struct vm_fault *vmf)/* * Be careful so that we will only recover a special uffd-wp pte into a * none pte. Otherwise it means the pte could have changed, so retry. +* +* This should also cover the case where e.g. the pte changed +* quickly from a PTE_MARKER_UFFD_WP into PTE_MARKER_SWAPIN_ERROR. +* So is_pte_marker() check is not enough to safely drop the pte. */ -if (is_pte_marker (*vmf->pte)) +if (pte_same (vmf->orig_pte, *vmf->pte))pte_clear (vmf->vma->vm_mm, vmf->address, vmf->pte);pte_unmap_unlock (vmf->pte, vmf->ptl);return 0 ;
mm/mprotect.c +7 -1
@@ -245,7 +245,13 @@ static unsigned long change_pte_range(struct mmu_gather *tlb, newpte =pte_swp_mksoft_dirty (newpte);if (pte_swp_uffd_wp (oldpte)) newpte =pte_swp_mkuffd_wp (newpte); - }else if (pte_marker_entry_uffd_wp (entry)) { + }else if (is_pte_marker_entry (entry)) { +/* +* Ignore swapin errors unconditionally, +* because any access should sigbus anyway. +*/ +if (is_swapin_error_entry (entry)) +continue ;/* * If this is uffd-wp pte marker and we'd like * to unprotect it, drop it; the next page
Signature · 1 line
2.37.3
Re: [PATCH 2/2] mm: Fix a few rare cases of using swapin error pte marker
Quoted 59 lines · Peter Xu
This patch should harden commit
15520a3f0469("mm: use pte markers for swap errors") on using pte markers for swapin errors on a few corner cases.1. Propagate swapin errors across fork()s: if there're swapin errors in the parent mm, after fork()s the child should sigbus too when an error page is accessed.
2. Fix a rare condition race in
pte_marker_clear()where a uffd-wp pte marker can be quickly switched to a swapin error.3. Explicitly ignore swapin error pte markers in
change_protection().I mostly don't worry on (2) or (3) at all, but we should still have them. Case (1) is special because it can potentially cause silent data corrupt on child when parent has swapin error triggered with swapoff, but since swapin error is rare itself already it's probably not easy to trigger either.
Currently there is a priority difference between the uffd-wp bit and the swapin error entry, in which the swapin error always has higher priority (e.g. we don't need to wr-protect a swapin error pte marker).
If there will be a 3rd bit introduced, we'll probably need to consider a more involved approach so we may need to start operate on the bits. Let's leave that for later.
This patch is tested with case (1) explicitly where we'll get corrupted data before in the child if there's existing swapin error pte markers, and after patch applied the child can be rightfully killed.
We don't need to copy stable for this one since
15520a3f0469just landed as part of v6.2-rc1, only "Fixes" applied.Fixes 15520a3f0469 ("mm: use pte markers for swap errors")Signed-off-by Peter Xu <[email protected]>--- mm/hugetlb.c | 3 +++ mm/memory.c | 8 ++++++-- mm/mprotect.c | 8 +++++++- 3 files changed, 16 insertions(+), 3 deletions(-)mm/hugetlb.c +3
-0@@ -4884,6 +4884,9 @@ int copy_hugetlb_page_range(struct mm_struct *dst, struct mm_struct *src, entry =huge_pte_clear_uffd_wp (entry);set_huge_pte_at (dst, addr, dst_pte, entry); }else if (unlikely (is_pte_marker (entry))) { +/* No swap on hugetlb */ +WARN_ON_ONCE ( +is_swapin_error_entry (pte_to_swp_entry (entry)));/* * We copy the pte marker only if the dst vma has * uffd-wp enabled. mm/memory.c +0
-0
mm/memory.c copy_nonpresent_pte(struct mm_struct *dst_mm, struct mm_struct *src_mm, · line 831
Quoted 5 lines
mm/memory.c +1
-1@@ -828,7 +828,7 @@ copy_nonpresent_pte(struct mm_struct *dst_mm, struct mm_struct *src_mm,return -EBUSY ;return -ENOENT ; }else if (is_pte_marker_entry (entry)) { -if (userfaultfd_wp (dst_vma)) +if (is_swapin_error_entry (entry) ||userfaultfd_wp (dst_vma))
Should we do this in [1/2]? It appears that we introduce an issue in [1/2] and fix it in [2/2]?
Best Regards,
Huang, Ying
Quoted 36 lines
mm/memory.c +5
-1set_pte_at (dst_mm, addr, dst_pte, pte);return 0 ; }@@ -3625,8 +3625,12 @@ static vm_fault_t pte_marker_clear(struct vm_fault *vmf)/* * Be careful so that we will only recover a special uffd-wp pte into a * none pte. Otherwise it means the pte could have changed, so retry. +* +* This should also cover the case where e.g. the pte changed +* quickly from a PTE_MARKER_UFFD_WP into PTE_MARKER_SWAPIN_ERROR. +* So is_pte_marker() check is not enough to safely drop the pte. */ -if (is_pte_marker (*vmf->pte)) +if (pte_same (vmf->orig_pte, *vmf->pte))pte_clear (vmf->vma->vm_mm, vmf->address, vmf->pte);pte_unmap_unlock (vmf->pte, vmf->ptl);return 0 ;mm/mprotect.c +7
-1@@ -245,7 +245,13 @@ static unsigned long change_pte_range(struct mmu_gather *tlb, newpte =pte_swp_mksoft_dirty (newpte);if (pte_swp_uffd_wp (oldpte)) newpte =pte_swp_mkuffd_wp (newpte); - }else if (pte_marker_entry_uffd_wp (entry)) { + }else if (is_pte_marker_entry (entry)) { +/* +* Ignore swapin errors unconditionally, +* because any access should sigbus anyway. +*/ +if (is_swapin_error_entry (entry)) +continue ;/* * If this is uffd-wp pte marker and we'd like * to unprotect it, drop it; the next page
Re: [PATCH 2/2] mm: Fix a few rare cases of using swapin error pte marker
Quoted 61 lines · Ying
Quoted 59 lines · Peter Xu
This patch should harden commit
15520a3f0469("mm: use pte markers for swap errors") on using pte markers for swapin errors on a few corner cases.1. Propagate swapin errors across fork()s: if there're swapin errors in the parent mm, after fork()s the child should sigbus too when an error page is accessed.
2. Fix a rare condition race in
pte_marker_clear()where a uffd-wp pte marker can be quickly switched to a swapin error.3. Explicitly ignore swapin error pte markers in
change_protection().I mostly don't worry on (2) or (3) at all, but we should still have them. Case (1) is special because it can potentially cause silent data corrupt on child when parent has swapin error triggered with swapoff, but since swapin error is rare itself already it's probably not easy to trigger either.
Currently there is a priority difference between the uffd-wp bit and the swapin error entry, in which the swapin error always has higher priority (e.g. we don't need to wr-protect a swapin error pte marker).
If there will be a 3rd bit introduced, we'll probably need to consider a more involved approach so we may need to start operate on the bits. Let's leave that for later.
This patch is tested with case (1) explicitly where we'll get corrupted data before in the child if there's existing swapin error pte markers, and after patch applied the child can be rightfully killed.
We don't need to copy stable for this one since
15520a3f0469just landed as part of v6.2-rc1, only "Fixes" applied.Fixes 15520a3f0469 ("mm: use pte markers for swap errors")Signed-off-by Peter Xu <[email protected]>--- mm/hugetlb.c | 3 +++ mm/memory.c | 8 ++++++-- mm/mprotect.c | 8 +++++++- 3 files changed, 16 insertions(+), 3 deletions(-)mm/hugetlb.c +3
-0@@ -4884,6 +4884,9 @@ int copy_hugetlb_page_range(struct mm_struct *dst, struct mm_struct *src, entry =huge_pte_clear_uffd_wp (entry);set_huge_pte_at (dst, addr, dst_pte, entry); }else if (unlikely (is_pte_marker (entry))) { +/* No swap on hugetlb */ +WARN_ON_ONCE ( +is_swapin_error_entry (pte_to_swp_entry (entry)));/* * We copy the pte marker only if the dst vma has * uffd-wp enabled. mm/memory.c +0
-0@@ -828,7 +828,7 @@ copy_nonpresent_pte(struct mm_struct *dst_mm, struct mm_struct *src_mm,
Quoted 8 lines
mm/memory.c copy_nonpresent_pte(struct mm_struct *dst_mm, struct mm_struct *src_mm, · line 831
Quoted 5 lines
mm/memory.c +1
-1return -EBUSY ;return -ENOENT ; }else if (is_pte_marker_entry (entry)) { -if (userfaultfd_wp (dst_vma)) +if (is_swapin_error_entry (entry) ||userfaultfd_wp (dst_vma))Should we do this in [1/2]? It appears that we introduce an issue in [1/2] and fix it in [2/2]?
Patch 1 copied stable with 5.19+, this one is not.
So if we want to squash, we may want to squash both patches into one, then we'll need an explicit follow up on stable branch with something like patch
1. The current way works easier for stable, but I can also do the other.
Thanks,
Signature · 1 line
Peter Xu
Re: [PATCH 2/2] mm: Fix a few rare cases of using swapin error pte marker
Quoted 71 lines · Peter Xu
Quoted 61 lines · Ying
Quoted 59 lines · Peter Xu
This patch should harden commit
15520a3f0469("mm: use pte markers for swap errors") on using pte markers for swapin errors on a few corner cases.1. Propagate swapin errors across fork()s: if there're swapin errors in the parent mm, after fork()s the child should sigbus too when an error page is accessed.
2. Fix a rare condition race in
pte_marker_clear()where a uffd-wp pte marker can be quickly switched to a swapin error.3. Explicitly ignore swapin error pte markers in
change_protection().I mostly don't worry on (2) or (3) at all, but we should still have them. Case (1) is special because it can potentially cause silent data corrupt on child when parent has swapin error triggered with swapoff, but since swapin error is rare itself already it's probably not easy to trigger either.
Currently there is a priority difference between the uffd-wp bit and the swapin error entry, in which the swapin error always has higher priority (e.g. we don't need to wr-protect a swapin error pte marker).
If there will be a 3rd bit introduced, we'll probably need to consider a more involved approach so we may need to start operate on the bits. Let's leave that for later.
This patch is tested with case (1) explicitly where we'll get corrupted data before in the child if there's existing swapin error pte markers, and after patch applied the child can be rightfully killed.
We don't need to copy stable for this one since
15520a3f0469just landed as part of v6.2-rc1, only "Fixes" applied.Fixes 15520a3f0469 ("mm: use pte markers for swap errors")Signed-off-by Peter Xu <[email protected]>--- mm/hugetlb.c | 3 +++ mm/memory.c | 8 ++++++-- mm/mprotect.c | 8 +++++++- 3 files changed, 16 insertions(+), 3 deletions(-)mm/hugetlb.c +3
-0@@ -4884,6 +4884,9 @@ int copy_hugetlb_page_range(struct mm_struct *dst, struct mm_struct *src, entry =huge_pte_clear_uffd_wp (entry);set_huge_pte_at (dst, addr, dst_pte, entry); }else if (unlikely (is_pte_marker (entry))) { +/* No swap on hugetlb */ +WARN_ON_ONCE ( +is_swapin_error_entry (pte_to_swp_entry (entry)));/* * We copy the pte marker only if the dst vma has * uffd-wp enabled. mm/memory.c +0
-0@@ -828,7 +828,7 @@ copy_nonpresent_pte(struct mm_struct *dst_mm, struct mm_struct *src_mm,Quoted 8 lines
mm/memory.c copy_nonpresent_pte(struct mm_struct *dst_mm, struct mm_struct *src_mm, · line 831
Quoted 5 lines
mm/memory.c +1
-1return -EBUSY ;return -ENOENT ; }else if (is_pte_marker_entry (entry)) { -if (userfaultfd_wp (dst_vma)) +if (is_swapin_error_entry (entry) ||userfaultfd_wp (dst_vma))Should we do this in [1/2]? It appears that we introduce an issue in [1/2] and fix it in [2/2]?
Quoted 5 lines
Patch 1 copied stable with 5.19+, this one is not.
So if we want to squash, we may want to squash both patches into one, then we'll need an explicit follow up on stable branch with something like patch
1. The current way works easier for stable, but I can also do the other.
Got it. Thanks for explanation. It's OK for me to keep them in current way.
Best Regards,
Huang, Ying
Re: [PATCH 2/2] mm: Fix a few rare cases of using swapin error pte marker
In reply to Peter Xumm/memory.c copy_nonpresent_pte(struct mm_struct *dst_mm, struct mm_struct *src_mm, · line 832
Quoted 11 lines
mm/memory.c +1
-1@@ -828,7 +828,7 @@ copy_nonpresent_pte(struct mm_struct *dst_mm, struct mm_struct *src_mm,return -EBUSY ;return -ENOENT ; }else if (is_pte_marker_entry (entry)) { -if (userfaultfd_wp (dst_vma)) +if (is_swapin_error_entry (entry) ||userfaultfd_wp (dst_vma))set_pte_at (dst_mm, addr, dst_pte, pte);
As discussed, it would be great if we could in the future avoid such allow lists and instead have default allow and only handle problematic cases. Maybe we can do that in the future with selective clearing of markers.
Signature · 3 lines
Thanks, David / dhildenb
Re: [PATCH 2/2] mm: Fix a few rare cases of using swapin error pte marker
In reply to Peter XuQuoted 29 lines · Peter Xu
This patch should harden commit
15520a3f0469("mm: use pte markers for swap errors") on using pte markers for swapin errors on a few corner cases.1. Propagate swapin errors across fork()s: if there're swapin errors in the parent mm, after fork()s the child should sigbus too when an error page is accessed.
2. Fix a rare condition race in
pte_marker_clear()where a uffd-wp pte marker can be quickly switched to a swapin error.3. Explicitly ignore swapin error pte markers in
change_protection().I mostly don't worry on (2) or (3) at all, but we should still have them. Case (1) is special because it can potentially cause silent data corrupt on child when parent has swapin error triggered with swapoff, but since swapin error is rare itself already it's probably not easy to trigger either.
Currently there is a priority difference between the uffd-wp bit and the swapin error entry, in which the swapin error always has higher priority (e.g. we don't need to wr-protect a swapin error pte marker).
If there will be a 3rd bit introduced, we'll probably need to consider a more involved approach so we may need to start operate on the bits. Let's leave that for later.
This patch is tested with case (1) explicitly where we'll get corrupted data before in the child if there's existing swapin error pte markers, and after patch applied the child can be rightfully killed.
Quoted 5 lines
We don't need to copy stable for this one since
15520a3f0469just landed as part of v6.2-rc1, only "Fixes" applied.Fixes 15520a3f0469 ("mm: use pte markers for swap errors")Signed-off-by Peter Xu <[email protected]>
Signature · 3 lines
Thanks, David / dhildenb
Re: [PATCH 2/2] mm: Fix a few rare cases of using swapin error pte marker
In reply to Peter XuQuoted 29 lines · Peter Xu
This patch should harden commit
15520a3f0469("mm: use pte markers for swap errors") on using pte markers for swapin errors on a few corner cases.1. Propagate swapin errors across fork()s: if there're swapin errors in the parent mm, after fork()s the child should sigbus too when an error page is accessed.
2. Fix a rare condition race in
pte_marker_clear()where a uffd-wp pte marker can be quickly switched to a swapin error.3. Explicitly ignore swapin error pte markers in
change_protection().I mostly don't worry on (2) or (3) at all, but we should still have them. Case (1) is special because it can potentially cause silent data corrupt on child when parent has swapin error triggered with swapoff, but since swapin error is rare itself already it's probably not easy to trigger either.
Currently there is a priority difference between the uffd-wp bit and the swapin error entry, in which the swapin error always has higher priority (e.g. we don't need to wr-protect a swapin error pte marker).
If there will be a 3rd bit introduced, we'll probably need to consider a more involved approach so we may need to start operate on the bits. Let's leave that for later.
This patch is tested with case (1) explicitly where we'll get corrupted data before in the child if there's existing swapin error pte markers, and after patch applied the child can be rightfully killed.
Quoted 5 lines
We don't need to copy stable for this one since
15520a3f0469just landed as part of v6.2-rc1, only "Fixes" applied.Fixes 15520a3f0469 ("mm: use pte markers for swap errors")Signed-off-by Peter Xu <[email protected]>
Looks good to me. Thanks.
Thanks,
Miaohe Lin