| Message ID | 20220131162940.210846-4-david@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm: COW fixes part 1: fix the COW security issue for THP and swap | expand |
On 1/31/22 17:29, David Hildenbrand wrote: > Let's make it clearer that KSM might only have to copy a page > in case we have a page in the swapcache, not if we allocated a fresh > page and bypassed the swapcache. While at it, add a comment why this is > usually necessary and merge the two swapcache conditions. > > Signed-off-by: David Hildenbrand <david@redhat.com> Acked-by: Vlastimil Babka <vbabka@suse.cz> > --- > mm/memory.c | 38 +++++++++++++++++++++++--------------- > 1 file changed, 23 insertions(+), 15 deletions(-) > > diff --git a/mm/memory.c b/mm/memory.c > index 923165b4c27e..3c91294cca98 100644 > --- a/mm/memory.c > +++ b/mm/memory.c > @@ -3615,21 +3615,29 @@ vm_fault_t do_swap_page(struct vm_fault *vmf) > goto out_release; > } > > - /* > - * Make sure try_to_free_swap or reuse_swap_page or swapoff did not > - * release the swapcache from under us. The page pin, and pte_same > - * test below, are not enough to exclude that. Even if it is still > - * swapcache, we need to check that the page's swap has not changed. > - */ > - if (unlikely((!PageSwapCache(page) || > - page_private(page) != entry.val)) && swapcache) > - goto out_page; > - > - page = ksm_might_need_to_copy(page, vma, vmf->address); > - if (unlikely(!page)) { > - ret = VM_FAULT_OOM; > - page = swapcache; > - goto out_page; > + if (swapcache) { > + /* > + * Make sure try_to_free_swap or reuse_swap_page or swapoff did > + * not release the swapcache from under us. The page pin, and > + * pte_same test below, are not enough to exclude that. Even if > + * it is still swapcache, we need to check that the page's swap > + * has not changed. > + */ > + if (unlikely(!PageSwapCache(page) || > + page_private(page) != entry.val)) > + goto out_page; > + > + /* > + * KSM sometimes has to copy on read faults, for example, if > + * page->index of !PageKSM() pages would be nonlinear inside the > + * anon VMA -- PageKSM() is lost on actual swapout. > + */ > + page = ksm_might_need_to_copy(page, vma, vmf->address); > + if (unlikely(!page)) { > + ret = VM_FAULT_OOM; > + page = swapcache; > + goto out_page; > + } > } > > cgroup_throttle_swaprate(page, GFP_KERNEL);
On Mon, Jan 31, 2022 at 8:33 AM David Hildenbrand <david@redhat.com> wrote: > > Let's make it clearer that KSM might only have to copy a page > in case we have a page in the swapcache, not if we allocated a fresh > page and bypassed the swapcache. While at it, add a comment why this is > usually necessary and merge the two swapcache conditions. > > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > mm/memory.c | 38 +++++++++++++++++++++++--------------- > 1 file changed, 23 insertions(+), 15 deletions(-) > > diff --git a/mm/memory.c b/mm/memory.c > index 923165b4c27e..3c91294cca98 100644 > --- a/mm/memory.c > +++ b/mm/memory.c > @@ -3615,21 +3615,29 @@ vm_fault_t do_swap_page(struct vm_fault *vmf) > goto out_release; > } > > - /* > - * Make sure try_to_free_swap or reuse_swap_page or swapoff did not We could remove the reference to "reuse_swap_page", right? > - * release the swapcache from under us. The page pin, and pte_same > - * test below, are not enough to exclude that. Even if it is still > - * swapcache, we need to check that the page's swap has not changed. > - */ > - if (unlikely((!PageSwapCache(page) || > - page_private(page) != entry.val)) && swapcache) > - goto out_page; > - > - page = ksm_might_need_to_copy(page, vma, vmf->address); > - if (unlikely(!page)) { > - ret = VM_FAULT_OOM; > - page = swapcache; > - goto out_page; > + if (swapcache) { > + /* > + * Make sure try_to_free_swap or reuse_swap_page or swapoff did > + * not release the swapcache from under us. The page pin, and > + * pte_same test below, are not enough to exclude that. Even if > + * it is still swapcache, we need to check that the page's swap > + * has not changed. > + */ > + if (unlikely(!PageSwapCache(page) || > + page_private(page) != entry.val)) > + goto out_page; > + > + /* > + * KSM sometimes has to copy on read faults, for example, if > + * page->index of !PageKSM() pages would be nonlinear inside the > + * anon VMA -- PageKSM() is lost on actual swapout. > + */ > + page = ksm_might_need_to_copy(page, vma, vmf->address); > + if (unlikely(!page)) { > + ret = VM_FAULT_OOM; > + page = swapcache; > + goto out_page; > + } > } > > cgroup_throttle_swaprate(page, GFP_KERNEL); > -- > 2.34.1 >
On 09.03.22 19:48, Yang Shi wrote: > On Mon, Jan 31, 2022 at 8:33 AM David Hildenbrand <david@redhat.com> wrote: >> >> Let's make it clearer that KSM might only have to copy a page >> in case we have a page in the swapcache, not if we allocated a fresh >> page and bypassed the swapcache. While at it, add a comment why this is >> usually necessary and merge the two swapcache conditions. >> >> Signed-off-by: David Hildenbrand <david@redhat.com> >> --- >> mm/memory.c | 38 +++++++++++++++++++++++--------------- >> 1 file changed, 23 insertions(+), 15 deletions(-) >> >> diff --git a/mm/memory.c b/mm/memory.c >> index 923165b4c27e..3c91294cca98 100644 >> --- a/mm/memory.c >> +++ b/mm/memory.c >> @@ -3615,21 +3615,29 @@ vm_fault_t do_swap_page(struct vm_fault *vmf) >> goto out_release; >> } >> >> - /* >> - * Make sure try_to_free_swap or reuse_swap_page or swapoff did not > > We could remove the reference to "reuse_swap_page", right? > Yes, I noticed this a couple of days ago as well and already have a patch prepared for that ("mm: adjust stale comment in do_swap_page() mentioning reuse_swap_page()" at https://github.com/davidhildenbrand/linux/commits/cow_fixes_part_3) If Andrew wants, we can fix that up directly before sending upstream or I'll simply include that patch when sending out part2 v2. (I want to avoid sending another series just for this) Thanks!
On Wed, 9 Mar 2022 20:15:54 +0100 David Hildenbrand <david@redhat.com> wrote: > On 09.03.22 19:48, Yang Shi wrote: > > On Mon, Jan 31, 2022 at 8:33 AM David Hildenbrand <david@redhat.com> wrote: > >> > >> Let's make it clearer that KSM might only have to copy a page > >> in case we have a page in the swapcache, not if we allocated a fresh > >> page and bypassed the swapcache. While at it, add a comment why this is > >> usually necessary and merge the two swapcache conditions. > >> > >> Signed-off-by: David Hildenbrand <david@redhat.com> > >> --- > >> mm/memory.c | 38 +++++++++++++++++++++++--------------- > >> 1 file changed, 23 insertions(+), 15 deletions(-) > >> > >> diff --git a/mm/memory.c b/mm/memory.c > >> index 923165b4c27e..3c91294cca98 100644 > >> --- a/mm/memory.c > >> +++ b/mm/memory.c > >> @@ -3615,21 +3615,29 @@ vm_fault_t do_swap_page(struct vm_fault *vmf) > >> goto out_release; > >> } > >> > >> - /* > >> - * Make sure try_to_free_swap or reuse_swap_page or swapoff did not > > > > We could remove the reference to "reuse_swap_page", right? > > > Yes, I noticed this a couple of days ago as well and already have a > patch prepared for that ("mm: adjust stale comment in do_swap_page() > mentioning reuse_swap_page()" at > https://github.com/davidhildenbrand/linux/commits/cow_fixes_part_3) > > If Andrew wants, we can fix that up directly before sending upstream or > I'll simply include that patch when sending out part2 v2. > > (I want to avoid sending another series just for this) Thanks, I did this. The same change plus gratuitous comment reflowing. --- a/mm/memory.c~mm-slightly-clarify-ksm-logic-in-do_swap_page-fix +++ a/mm/memory.c @@ -3609,11 +3609,11 @@ vm_fault_t do_swap_page(struct vm_fault if (swapcache) { /* - * Make sure try_to_free_swap or reuse_swap_page or swapoff did - * not release the swapcache from under us. The page pin, and - * pte_same test below, are not enough to exclude that. Even if - * it is still swapcache, we need to check that the page's swap - * has not changed. + * Make sure try_to_free_swap or swapoff did not release the + * swapcache from under us. The page pin, and pte_same test + * below, are not enough to exclude that. Even if it is still + * swapcache, we need to check that the page's swap has not + * changed. */ if (unlikely(!PageSwapCache(page) || page_private(page) != entry.val))
diff --git a/mm/memory.c b/mm/memory.c index 923165b4c27e..3c91294cca98 100644 --- a/mm/memory.c +++ b/mm/memory.c @@ -3615,21 +3615,29 @@ vm_fault_t do_swap_page(struct vm_fault *vmf) goto out_release; } - /* - * Make sure try_to_free_swap or reuse_swap_page or swapoff did not - * release the swapcache from under us. The page pin, and pte_same - * test below, are not enough to exclude that. Even if it is still - * swapcache, we need to check that the page's swap has not changed. - */ - if (unlikely((!PageSwapCache(page) || - page_private(page) != entry.val)) && swapcache) - goto out_page; - - page = ksm_might_need_to_copy(page, vma, vmf->address); - if (unlikely(!page)) { - ret = VM_FAULT_OOM; - page = swapcache; - goto out_page; + if (swapcache) { + /* + * Make sure try_to_free_swap or reuse_swap_page or swapoff did + * not release the swapcache from under us. The page pin, and + * pte_same test below, are not enough to exclude that. Even if + * it is still swapcache, we need to check that the page's swap + * has not changed. + */ + if (unlikely(!PageSwapCache(page) || + page_private(page) != entry.val)) + goto out_page; + + /* + * KSM sometimes has to copy on read faults, for example, if + * page->index of !PageKSM() pages would be nonlinear inside the + * anon VMA -- PageKSM() is lost on actual swapout. + */ + page = ksm_might_need_to_copy(page, vma, vmf->address); + if (unlikely(!page)) { + ret = VM_FAULT_OOM; + page = swapcache; + goto out_page; + } } cgroup_throttle_swaprate(page, GFP_KERNEL);
Let's make it clearer that KSM might only have to copy a page in case we have a page in the swapcache, not if we allocated a fresh page and bypassed the swapcache. While at it, add a comment why this is usually necessary and merge the two swapcache conditions. Signed-off-by: David Hildenbrand <david@redhat.com> --- mm/memory.c | 38 +++++++++++++++++++++++--------------- 1 file changed, 23 insertions(+), 15 deletions(-)