interaction of MADV_PAGEOUT with CoW anonymous mappings?
Subsystems (2)
Jann Horn
reviewerHi!
From looking at the source code, it looks to me as if using MADV_PAGEOUT on a CoW anonymous mapping will page out the page if possible, even if other processes still have the same page mapped. Is that correct?
If so, that's probably bad in environments where many processes (with different privileges) are forked from a single zygote process (like Android and Chrome), I think? If you accidentally call it on a CoW anonymous mapping with shared pages, you'll degrade the performance of other processes. And if an attacker does it intentionally, they could use that to aid with exploiting race conditions or weird microarchitectural stuff (e.g. the new https://lviattack.eu/lvi.pdf
talks about "the assumption that attackers can provoke page faults or microcode assists for (arbitrary) load operations in the victim domain").
Should madvise_cold_or_pageout_pte_range() maybe refuse to operate on pages with mapcount>1, or something like that? Or does it already do that, and I just missed the check?
Quoted 18 lines · Jann Horn
Hi!
Quoted 1 line
From looking at the source code, it looks to me as if using
MADV_PAGEOUTon a CoW anonymous mapping will page out the page if possible, even if other processes still have the same page mapped. Is that correct?If so, that's probably bad in environments where many processes (with different privileges) are forked from a single zygote process (like Android and Chrome), I think? If you accidentally call it on a CoW anonymous mapping with shared pages, you'll degrade the performance of other processes. And if an attacker does it intentionally, they could use that to aid with exploiting race conditions or weird microarchitectural stuff (e.g. the new https://lviattack.eu/lvi.pdf
talks about "the assumption that attackers can provoke page faults or microcode assists for (arbitrary) load operations in the victim domain").
Quoted 3 lines
Should
madvise_cold_or_pageout_pte_range()maybe refuse to operate on pages with mapcount>1, or something like that? Or does it already do that, and I just missed the check?
I have brought up side channel attacks earlier [1] but only in the context of shared page cache pages. I didn't really consider shared anonymous pages to be a real problem. I was under impression that CoW pages shouldn't be a real problem because any security sensible applications shouldn't allow untrusted code to be forked and CoW anything really important. I believe we have made this assumption in other places - IIRC on gup with FOLL_FORCE but I admit I have very happily forgot most details.
[1] http://lkml.kernel.org/r/[email protected]
Signature · 2 lines
Michal Hocko SUSE Labs
Jann Horn
reviewerQuoted 23 lines · Michal Hocko
Quoted 18 lines · Jann Horn
Hi!
Quoted 1 line
From looking at the source code, it looks to me as if using
MADV_PAGEOUTon a CoW anonymous mapping will page out the page if possible, even if other processes still have the same page mapped. Is that correct?If so, that's probably bad in environments where many processes (with different privileges) are forked from a single zygote process (like Android and Chrome), I think? If you accidentally call it on a CoW anonymous mapping with shared pages, you'll degrade the performance of other processes. And if an attacker does it intentionally, they could use that to aid with exploiting race conditions or weird microarchitectural stuff (e.g. the new https://lviattack.eu/lvi.pdf
talks about "the assumption that attackers can provoke page faults or microcode assists for (arbitrary) load operations in the victim domain").Quoted 3 lines
Should
madvise_cold_or_pageout_pte_range()maybe refuse to operate on pages with mapcount>1, or something like that? Or does it already do that, and I just missed the check?
Quoted 8 lines
I have brought up side channel attacks earlier [1] but only in the context of shared page cache pages. I didn't really consider shared anonymous pages to be a real problem. I was under impression that CoW pages shouldn't be a real problem because any security sensible applications shouldn't allow untrusted code to be forked and CoW anything really important. I believe we have made this assumption in other places - IIRC on gup with
FOLL_FORCEbut I admit I have very happily forgot most details.
Android has a "zygote" process that starts up the whole Java environment with a bunch of libraries before entering into a loop that fork()s off a child every time the user wants to launch an app. So all the apps, and even browser renderer processes, on the device share many CoW VMAs. See
<https://developer.android.com/topic/performance/memory-overview#SharingRAM>.
I think Chrome on Linux desktop systems also forks off renderers from a common zygote process after initializing libraries and so on. See
<https://chromium.googlesource.com/chromium/src.git/+/master/docs/linux/zygote.md>.
(But they use a relatively strict seccomp sandbox that e.g. doesn't permit MADV_PAGEOUT.)
Quoted 24 lines · Jann Horn
Quoted 23 lines · Michal Hocko
Quoted 21 lines · Jann Horn
Hi!
Quoted 1 line
From looking at the source code, it looks to me as if using
MADV_PAGEOUTon a CoW anonymous mapping will page out the page if possible, even if other processes still have the same page mapped. Is that correct?If so, that's probably bad in environments where many processes (with different privileges) are forked from a single zygote process (like Android and Chrome), I think? If you accidentally call it on a CoW anonymous mapping with shared pages, you'll degrade the performance of other processes. And if an attacker does it intentionally, they could use that to aid with exploiting race conditions or weird microarchitectural stuff (e.g. the new https://lviattack.eu/lvi.pdf
talks about "the assumption that attackers can provoke page faults or microcode assists for (arbitrary) load operations in the victim domain").Should
madvise_cold_or_pageout_pte_range()maybe refuse to operate on pages with mapcount>1, or something like that? Or does it already do that, and I just missed the check?
Quoted 8 lines
Quoted 8 lines
I have brought up side channel attacks earlier [1] but only in the context of shared page cache pages. I didn't really consider shared anonymous pages to be a real problem. I was under impression that CoW pages shouldn't be a real problem because any security sensible applications shouldn't allow untrusted code to be forked and CoW anything really important. I believe we have made this assumption in other places - IIRC on gup with
FOLL_FORCEbut I admit I have very happily forgot most details.
I have quickly checked FOLL_FORCE and it is careful to break CoW on the write access.
Quoted 6 lines
Android has a "zygote" process that starts up the whole Java environment with a bunch of libraries before entering into a loop that fork()s off a child every time the user wants to launch an app. So all the apps, and even browser renderer processes, on the device share many CoW VMAs. See
<https://developer.android.com/topic/performance/memory-overview#SharingRAM>.
I still have to think about how this could be used for any reasonable attack. But certainly the simplest workaround is to simply back off on pages mapped multiple times as we do for THP already. Something like the following should work but I haven't tested it at all
mm/madvise.c +8 -0
@@ -351,6 +351,10 @@ static int madvise_cold_or_pageout_pte_range(pmd_t *pmd, goto regular_page;return 0 ; } + +/* Do not interfere with other mappings of this page */ +if (page_mapcount (page) !=1 ) + goto huge_unlock;if (pmd_young (orig_pmd)) {pmdp_invalidate (vma, addr, pmd);
@@ -426,6 +430,10 @@ static int madvise_cold_or_pageout_pte_range(pmd_t *pmd,continue ; } +/* Do not interfere with other mappings of this page */ +if (page_mapcount (page) !=1 ) +continue ; +VM_BUG_ON_PAGE (PageTransCompound (page), page);if (pte_young (ptent)) {
Signature · 2 lines
Michal Hocko SUSE Labs
Maybe instead of just punting on MADV_PAGEOUT for map_count>1 we should only let it affect the *local* process. We could still put the page in the swap cache, we just wouldn't go do the rmap walk.
Quoted 2 lines · Michal Hocko
I still have to think about how this could be used for any reasonable attack.
While this sounds theoretically vulnerable, I'm having a hard time convincing myself that it's in any way practical.
Code that's a victim to LVI has to hit a fault or an "assist". The most likely assist in this case is if the victim needs to set the PTE Accessed or Dirty bits.
There would first need to be some secret that an attacker wants to steal, then some *other* data that directly or indirectly protects the secret. The load of the "other" data would be what needs to encounter the fault or the assist, and would also need to be able to speculatively reach a "disclosure gadget".
The attacker would sit there and do MADV_PAGEOUT constantly, inducing lots of faults and assists and then _separately_ poison the microarchitectural buffers that lead to the "other" data getting an injected speculative value.
If an app is vulnerable to this, it's probably going to need to be using the "other" data a *lot*. It's going to have a really hard time doing that because it's going to be handling page faults all the time. It also can't ever write to the data being shared, or it will break the COW.
Quoted 3 lines · Dave Hansen
Maybe instead of just punting on
MADV_PAGEOUTfor map_count>1 we should only let it affect the *local* process. We could still put the page in the swap cache, we just wouldn't go do the rmap walk.
Is it really worth medling with the reclaim code and special case MADV_PAGEOUT there? I mean it is quite reasonable to have an initial implementation that doesn't really touch shared pages because that can lead to all sorts of hard to debug and unexpected problems. So I would much rather go with a simple patch to check map count first and see whether somebody actually cares about those shared pages and go from there.
Minchan, do you want to take my diff and turn it into the proper patch or should I do it.
Signature · 2 lines
Michal Hocko SUSE Labs
Quoted 13 lines · Michal Hocko
Quoted 3 lines · Dave Hansen
Maybe instead of just punting on
MADV_PAGEOUTfor map_count>1 we should only let it affect the *local* process. We could still put the page in the swap cache, we just wouldn't go do the rmap walk.Is it really worth medling with the reclaim code and special case
MADV_PAGEOUTthere? I mean it is quite reasonable to have an initial implementation that doesn't really touch shared pages because that can lead to all sorts of hard to debug and unexpected problems. So I would much rather go with a simple patch to check map count first and see whether somebody actually cares about those shared pages and go from there.
Quoted 2 lines
Minchan, do you want to take my diff and turn it into the proper patch or should I do it.
Hey Michal,
It would be great if you could send it.
Thanks.
Quoted 14 lines · Michal Hocko
Quoted 3 lines · Dave Hansen
Maybe instead of just punting on
MADV_PAGEOUTfor map_count>1 we should only let it affect the *local* process. We could still put the page in the swap cache, we just wouldn't go do the rmap walk.Is it really worth medling with the reclaim code and special case
MADV_PAGEOUTthere? I mean it is quite reasonable to have an initial implementation that doesn't really touch shared pages because that can lead to all sorts of hard to debug and unexpected problems. So I would much rather go with a simple patch to check map count first and see whether somebody actually cares about those shared pages and go from there.
Quoted 3 lines
Minchan, do you want to take my diff and turn it into the proper patch or should I do it.
What about the remote_madvise(MADV_PAGEOUT)? Will your patch disable the pageout from that code path as well for pages with mapcount > 1?
Quoted 15 lines · Shakeel Butt
Quoted 14 lines · Michal Hocko
Quoted 3 lines · Dave Hansen
Maybe instead of just punting on
MADV_PAGEOUTfor map_count>1 we should only let it affect the *local* process. We could still put the page in the swap cache, we just wouldn't go do the rmap walk.Is it really worth medling with the reclaim code and special case
MADV_PAGEOUTthere? I mean it is quite reasonable to have an initial implementation that doesn't really touch shared pages because that can lead to all sorts of hard to debug and unexpected problems. So I would much rather go with a simple patch to check map count first and see whether somebody actually cares about those shared pages and go from there.
Quoted 6 lines
Quoted 3 lines
Minchan, do you want to take my diff and turn it into the proper patch or should I do it.
What about the
remote_madvise(MADV_PAGEOUT)? Will your patch disable the pageout from that code path as well for pages with mapcount > 1?
Maybe, not because process_madvise syscall needs more previliedge(ie, PTRACE_MODE_ATTACH_FSCREDS) so I guess it would be more secure. So in that case, I want to rely on the LRU chance for shared pages.
With that, the manager process could give a hint to several processes and finally makes them paging out.
What do you think?
Quoted 21 lines · Minchan Kim
Quoted 19 lines · Shakeel Butt
Quoted 17 lines · Michal Hocko
Quoted 3 lines · Dave Hansen
Maybe instead of just punting on
MADV_PAGEOUTfor map_count>1 we should only let it affect the *local* process. We could still put the page in the swap cache, we just wouldn't go do the rmap walk.Is it really worth medling with the reclaim code and special case
MADV_PAGEOUTthere? I mean it is quite reasonable to have an initial implementation that doesn't really touch shared pages because that can lead to all sorts of hard to debug and unexpected problems. So I would much rather go with a simple patch to check map count first and see whether somebody actually cares about those shared pages and go from there.Minchan, do you want to take my diff and turn it into the proper patch or should I do it.
Quoted 6 lines
Quoted 2 lines
What about the
remote_madvise(MADV_PAGEOUT)? Will your patch disable the pageout from that code path as well for pages with mapcount > 1?Maybe, not because
process_madvisesyscall needs morepreviliedge(ie, PTRACE_MODE_ATTACH_FSCREDS)so I guess it would be more secure. So in that case, I want to rely on the LRU chance for shared pages.
I don't want the behavior of an madvise command to change depending on *how* the command is invoked. MADV_PAGEOUT should do the same thing regardless. If you want to allow purging of shared pages as well, please add a new MADV_PAGEOUT_ALL or something and require a privilege to use it.
Quoted 2 lines
With that, the manager process could give a hint to several processes and finally makes them paging out.
On many different occasions over the past few years, I've found myself wanting to ask the kernel to do bulk memory management operations. I'd much rather add *one* facility to allow for multiple-target mm operations than add one-off special cases for specific callers cases as they come up. If we had such a bulk operation, the kernel could inspect the bulk operation payload, see whether the number of MADV_PAGEOUT requests for a given page were equal to the share count for that page, and, if so, evict that page despite it being shared.
Quoted 30 lines · Daniel Colascione
Quoted 21 lines · Minchan Kim
Quoted 19 lines · Shakeel Butt
Quoted 17 lines · Michal Hocko
Quoted 3 lines · Dave Hansen
Maybe instead of just punting on
MADV_PAGEOUTfor map_count>1 we should only let it affect the *local* process. We could still put the page in the swap cache, we just wouldn't go do the rmap walk.Is it really worth medling with the reclaim code and special case
MADV_PAGEOUTthere? I mean it is quite reasonable to have an initial implementation that doesn't really touch shared pages because that can lead to all sorts of hard to debug and unexpected problems. So I would much rather go with a simple patch to check map count first and see whether somebody actually cares about those shared pages and go from there.Minchan, do you want to take my diff and turn it into the proper patch or should I do it.
Quoted 6 lines
Quoted 2 lines
What about the
remote_madvise(MADV_PAGEOUT)? Will your patch disable the pageout from that code path as well for pages with mapcount > 1?Maybe, not because
process_madvisesyscall needs morepreviliedge(ie, PTRACE_MODE_ATTACH_FSCREDS)so I guess it would be more secure. So in that case, I want to rely on the LRU chance for shared pages.
Quoted 6 lines
I don't want the behavior of an madvise command to change depending on *how* the command is invoked.
MADV_PAGEOUTshould do the same thing regardless. If you want to allow purging of shared pages as well, please add a newMADV_PAGEOUT_ALLor something and require a privilege to use it.
I would like to have a way to pageout the shared pages and MADV_PAGEOUT_ALL approach looks fine to me.
Quoted 24 lines · Michal Hocko
Quoted 18 lines · Jann Horn
Hi!
Quoted 1 line
From looking at the source code, it looks to me as if using
MADV_PAGEOUTon a CoW anonymous mapping will page out the page if possible, even if other processes still have the same page mapped. Is that correct?If so, that's probably bad in environments where many processes (with different privileges) are forked from a single zygote process (like Android and Chrome), I think? If you accidentally call it on a CoW anonymous mapping with shared pages, you'll degrade the performance of other processes. And if an attacker does it intentionally, they could use that to aid with exploiting race conditions or weird microarchitectural stuff (e.g. the new https://lviattack.eu/lvi.pdf
talks about "the assumption that attackers can provoke page faults or microcode assists for (arbitrary) load operations in the victim domain").Quoted 3 lines
Should
madvise_cold_or_pageout_pte_range()maybe refuse to operate on pages with mapcount>1, or something like that? Or does it already do that, and I just missed the check?
Quoted 8 lines
I have brought up side channel attacks earlier [1] but only in the context of shared page cache pages. I didn't really consider shared anonymous pages to be a real problem. I was under impression that CoW pages shouldn't be a real problem because any security sensible applications shouldn't allow untrusted code to be forked and CoW anything really important. I believe we have made this assumption in other places - IIRC on gup with
FOLL_FORCEbut I admit I have very happily forgot most details.
I'm more worried about the performance implications. Consider libc.so's data section: that's a COW mapping, and we COW it during zygote initialization as we load and relocate libc.so. Child processes shouldn't be dirtying and re-COWing those relocated pages. If I understand Jann's message correctly, MADV_PAGEOUT would force the pages corresponding to the libc.so data segment out to zram just because we MADV_PAGEOUT-ed a single process that happened to use libc. We should leave those pages in memory, IMHO.
Jann Horn
reviewerQuoted 31 lines · Daniel Colascione
Quoted 21 lines · Michal Hocko
Quoted 16 lines · Jann Horn
Quoted 1 line
From looking at the source code, it looks to me as if using
MADV_PAGEOUTon a CoW anonymous mapping will page out the page if possible, even if other processes still have the same page mapped. Is that correct?If so, that's probably bad in environments where many processes (with different privileges) are forked from a single zygote process (like Android and Chrome), I think? If you accidentally call it on a CoW anonymous mapping with shared pages, you'll degrade the performance of other processes. And if an attacker does it intentionally, they could use that to aid with exploiting race conditions or weird microarchitectural stuff (e.g. the new https://lviattack.eu/lvi.pdf
talks about "the assumption that attackers can provoke page faults or microcode assists for (arbitrary) load operations in the victim domain").Quoted 3 lines
Should
madvise_cold_or_pageout_pte_range()maybe refuse to operate on pages with mapcount>1, or something like that? Or does it already do that, and I just missed the check?Quoted 8 lines
I have brought up side channel attacks earlier [1] but only in the context of shared page cache pages. I didn't really consider shared anonymous pages to be a real problem. I was under impression that CoW pages shouldn't be a real problem because any security sensible applications shouldn't allow untrusted code to be forked and CoW anything really important. I believe we have made this assumption in other places - IIRC on gup with
FOLL_FORCEbut I admit I have very happily forgot most details.
Quoted 8 lines
I'm more worried about the performance implications. Consider libc.so's data section: that's a COW mapping, and we COW it during zygote initialization as we load and relocate libc.so. Child processes shouldn't be dirtying and re-COWing those relocated pages. If I understand Jann's message correctly,
MADV_PAGEOUTwould force the pages corresponding to the libc.so data segment out to zram just because we MADV_PAGEOUT-ed a single process that happened to use libc. We should leave those pages in memory, IMHO.
Actually, the libc.so data section is a file mapping, so I think can_do_pageout() would decide whether the caller is allowed to force pageout based on whether the caller is the owner of (or capable over) libc (in other words, root, basically). But I think the bss section, as well as heap memory, could have pageout forced by anyone.
Quoted 42 lines · Jann Horn
Quoted 31 lines · Daniel Colascione
Quoted 21 lines · Michal Hocko
Quoted 16 lines · Jann Horn
Quoted 1 line
From looking at the source code, it looks to me as if using
MADV_PAGEOUTon a CoW anonymous mapping will page out the page if possible, even if other processes still have the same page mapped. Is that correct?If so, that's probably bad in environments where many processes (with different privileges) are forked from a single zygote process (like Android and Chrome), I think? If you accidentally call it on a CoW anonymous mapping with shared pages, you'll degrade the performance of other processes. And if an attacker does it intentionally, they could use that to aid with exploiting race conditions or weird microarchitectural stuff (e.g. the new https://lviattack.eu/lvi.pdf
talks about "the assumption that attackers can provoke page faults or microcode assists for (arbitrary) load operations in the victim domain").Quoted 3 lines
Should
madvise_cold_or_pageout_pte_range()maybe refuse to operate on pages with mapcount>1, or something like that? Or does it already do that, and I just missed the check?Quoted 8 lines
I have brought up side channel attacks earlier [1] but only in the context of shared page cache pages. I didn't really consider shared anonymous pages to be a real problem. I was under impression that CoW pages shouldn't be a real problem because any security sensible applications shouldn't allow untrusted code to be forked and CoW anything really important. I believe we have made this assumption in other places - IIRC on gup with
FOLL_FORCEbut I admit I have very happily forgot most details.Quoted 8 lines
I'm more worried about the performance implications. Consider libc.so's data section: that's a COW mapping, and we COW it during zygote initialization as we load and relocate libc.so. Child processes shouldn't be dirtying and re-COWing those relocated pages. If I understand Jann's message correctly,
MADV_PAGEOUTwould force the pages corresponding to the libc.so data segment out to zram just because we MADV_PAGEOUT-ed a single process that happened to use libc. We should leave those pages in memory, IMHO.
Quoted 5 lines
Actually, the libc.so data section is a file mapping, so I think
can_do_pageout()would decide whether the caller is allowed to force pageout based on whether the caller is the owner of (or capable over) libc (in other words, root, basically). But I think the bss section, as well as heap memory, could have pageout forced by anyone.
lmkd would have that capability though, right? But the point stands regardless. It sounds like both security and performance suggest a behavior change here. Thanks for bringing it up!
Hi Jann,
Quoted 18 lines · Jann Horn
Hi!
From looking at the source code, it looks to me as if using
MADV_PAGEOUTon a CoW anonymous mapping will page out the page if possible, even if other processes still have the same page mapped. Is that correct?If so, that's probably bad in environments where many processes (with different privileges) are forked from a single zygote process (like Android and Chrome), I think? If you accidentally call it on a CoW anonymous mapping with shared pages, you'll degrade the performance of other processes. And if an attacker does it intentionally, they could use that to aid with exploiting race conditions or weird microarchitectural stuff (e.g. the new https://lviattack.eu/lvi.pdf
talks about "the assumption that attackers can provoke page faults or microcode assists for (arbitrary) load operations in the victim domain").
Quoted 3 lines
Should
madvise_cold_or_pageout_pte_range()maybe refuse to operate on pages with mapcount>1, or something like that? Or does it already do that, and I just missed the check?
Originally, patchset had the mapcount check to filer out shared page due to performance concern, not security stuff. However, the code was removed because reviewer asked me "let the shared pages rely on general LRU" because shared pages would have higher chance to be touched compared to private pages so naturally, they will keep in the memory. It did make sense for me.
I am not familiar with the security stuff but if it's really vulnerable and everyone agree on that, it's easy to add mapcount check there.
Thanks.
[Cc akpm]
So what about this?
From eca97990372679c097a88164ff4b3d7879b0e127 Mon Sep 17 00:00:00 2001
From: Michal Hocko <[email protected]>
Date: Thu, 12 Mar 2020 09:04:35 +0100
Subject: [PATCH] mm: do not allow MADV_PAGEOUT for CoW pages
Jann has brought up a very interesting point [1]. While shared pages are excluded from MADV_PAGEOUT normally, CoW pages can be easily reclaimed that way. This can lead to all sorts of hard to debug problems. E.g. performance problems outlined by Daniel [2]. There are runtime environments where there is a substantial memory shared among security domains via CoW memory and a easy to reclaim way of that memory, which MADV_{COLD,PAGEOUT} offers, can lead to either performance degradation in for the parent process which might be more privileged or even open side channel attacks. The feasibility of the later is not really clear to me TBH but there is no real reason for exposure at this stage. It seems there is no real use case to depend on reclaiming CoW memory via madvise at this stage so it is much easier to simply disallow it and this is what this patch does. Put it simply MADV_{PAGEOUT,COLD} can operate only on the exclusively owned memory which is a straightforward semantic.
[1] http://lkml.kernel.org/r/CAG48ez0G3JkMq61gUmyQAaCq=_TwHbi1XKzWRooxZkv08PQKuw@mail.gmail.com
[2] http://lkml.kernel.org/r/CAKOZueua_v8jHCpmEtTB6f3i9e2YnmX4mqdYVWhV4E=Z-n+zRQ@mail.gmail.com
--- mm/madvise.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)
mm/madvise.c +9 -3
@@ -335,12 +335,14 @@ static int madvise_cold_or_pageout_pte_range(pmd_t *pmd, } page =pmd_page (orig_pmd); + +/* Do not interfere with other mappings of this page */ +if (page_mapcount (page) !=1 ) + goto huge_unlock; +if (next - addr !=HPAGE_PMD_SIZE ) {int err; -if (page_mapcount (page) !=1 ) - goto huge_unlock; -get_page (page);spin_unlock (ptl);lock_page (page);
@@ -426,6 +428,10 @@ static int madvise_cold_or_pageout_pte_range(pmd_t *pmd,continue ; } +/* Do not interfere with other mappings of this page */ +if (page_mapcount (page) !=1 ) +continue ; +VM_BUG_ON_PAGE (PageTransCompound (page), page);if (pte_young (ptent)) {
Signature · 5 lines
2.24.1 -- Michal Hocko SUSE Labs
Vlastimil Babka
Acked-byQuoted 25 lines · Michal Hocko
[Cc akpm]
So what about this?
From
eca97990372679c097a88164ff4b3d7879b0e127Mon Sep 17 00:00:00 2001
From: Michal Hocko <[email protected]>
Date: Thu, 12 Mar 2020 09:04:35 +0100
Subject: [PATCH] mm: do not allowMADV_PAGEOUTfor CoW pagesJann has brought up a very interesting point [1]. While shared pages are excluded from
MADV_PAGEOUTnormally, CoW pages can be easily reclaimed that way. This can lead to all sorts of hard to debug problems. E.g. performance problems outlined by Daniel [2]. There are runtime environments where there is a substantial memory shared among security domains via CoW memory and a easy to reclaim way of that memory, which MADV_{COLD,PAGEOUT} offers, can lead to either performance degradation in for the parent process which might be more privileged or even open side channel attacks. The feasibility of the later is not really clear to me TBH but there is no real reason for exposure at this stage. It seems there is no real use case to depend on reclaiming CoW memory via madvise at this stage so it is much easier to simply disallow it and this is what this patch does. Put it simply MADV_{PAGEOUT,COLD} can operate only on the exclusively owned memory which is a straightforward semantic.
Quoted 3 lines
[1] http://lkml.kernel.org/r/CAG48ez0G3JkMq61gUmyQAaCq=_TwHbi1XKzWRooxZkv08PQKuw@mail.gmail.com
[2] http://lkml.kernel.org/r/CAKOZueua_v8jHCpmEtTB6f3i9e2YnmX4mqdYVWhV4E=Z-n+zRQ@mail.gmail.com
Maybe CC stable?
Quoted 1 line
Signed-off-by Michal Hocko <[email protected]>
Quoted 38 lines
--- mm/madvise.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)mm/madvise.c +9
-3@@ -335,12 +335,14 @@ static int madvise_cold_or_pageout_pte_range(pmd_t *pmd, } page =pmd_page (orig_pmd); + +/* Do not interfere with other mappings of this page */ +if (page_mapcount (page) !=1 ) + goto huge_unlock; +if (next - addr !=HPAGE_PMD_SIZE ) {int err; -if (page_mapcount (page) !=1 ) - goto huge_unlock; -get_page (page);spin_unlock (ptl);lock_page (page);@@ -426,6 +428,10 @@ static int madvise_cold_or_pageout_pte_range(pmd_t *pmd,continue ; } +/* Do not interfere with other mappings of this page */ +if (page_mapcount (page) !=1 ) +continue ; +VM_BUG_ON_PAGE (PageTransCompound (page), page);if (pte_young (ptent)) {
Quoted 3 lines · Michal Hocko
[Cc akpm]
So what about this?
Thanks, Michal.
I't likde to wait Jann's reply since Dave gave his opinion about the vulnerability.
https://lore.kernel.org/linux-mm/[email protected]/
Jann, could you give your insigh about that practically it's possible?
A real dumb question to understand vulnerability:
The attacker would be able to trigger heavy memory consumption so that he could make paging them out without MADV_PAGEOUT. I know MADV_PAGEOUT makes it easier but he still could do without MADV_PAGEOUT.
What makes difference here?
To clarify how MADV_PAGEWORK works:
If other process has accessed the page so that his page table has access bit marked, MADV_PAGEOUT couldn't page it out.
Quoted 6 lines
From
eca97990372679c097a88164ff4b3d7879b0e127Mon Sep 17 00:00:00 2001
From: Michal Hocko <[email protected]>
Date: Thu, 12 Mar 2020 09:04:35 +0100
Subject: [PATCH] mm: do not allowMADV_PAGEOUTfor CoW pages
Quoted 9 lines
Jann has brought up a very interesting point [1]. While shared pages are excluded from
MADV_PAGEOUTnormally, CoW pages can be easily reclaimed that way. This can lead to all sorts of hard to debug problems. E.g. performance problems outlined by Daniel [2]. There are runtime environments where there is a substantial memory shared among security domains via CoW memory and a easy to reclaim way of that memory, which MADV_{COLD,PAGEOUT} offers, can lead to either performance degradation in for the parent process which might be more privileged or even open side channel attacks. The feasibility of the later is not really clear
I am not sure it's a good idea to mention performance stuff because it's rather arguble. You and Johannes already pointed it out when I sbumit early draft which had shared page filtering out logic due to performance reason. You guys suggested the shared pages has higher chance to be touched so that if it's really hot pages, that whould keep in the memory. I agree.
I think the only reason at this moment is just vulnerability.
Quoted 20 lines
to me TBH but there is no real reason for exposure at this stage. It seems there is no real use case to depend on reclaiming CoW memory via madvise at this stage so it is much easier to simply disallow it and this is what this patch does. Put it simply MADV_{PAGEOUT,COLD} can operate only on the exclusively owned memory which is a straightforward semantic.
[1] http://lkml.kernel.org/r/CAG48ez0G3JkMq61gUmyQAaCq=_TwHbi1XKzWRooxZkv08PQKuw@mail.gmail.com
[2] http://lkml.kernel.org/r/CAKOZueua_v8jHCpmEtTB6f3i9e2YnmX4mqdYVWhV4E=Z-n+zRQ@mail.gmail.comSigned-off-by Michal Hocko <[email protected]>--- mm/madvise.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)mm/madvise.c +0
-0
mm/madvise.c static int madvise_cold_or_pageout_pte_range(pmd_t *pmd, · line 339
Quoted 5 lines
mm/madvise.c +2
-0@@ -335,12 +335,14 @@ static int madvise_cold_or_pageout_pte_range(pmd_t *pmd, } page =pmd_page (orig_pmd); + +/* Do not interfere with other mappings of this page */
How about this?
/*
* paging out only single mapped private pages for anonymous mapping,
* otherwise, it opens a side channel.
*/
Otherwise, looks good to me.
Quoted 29 lines
mm/madvise.c +7
-3+if (page_mapcount (page) !=1 ) + goto huge_unlock; +if (next - addr !=HPAGE_PMD_SIZE ) {int err; -if (page_mapcount (page) !=1 ) - goto huge_unlock; -get_page (page);spin_unlock (ptl);lock_page (page);@@ -426,6 +428,10 @@ static int madvise_cold_or_pageout_pte_range(pmd_t *pmd,continue ; } +/* Do not interfere with other mappings of this page */ +if (page_mapcount (page) !=1 ) +continue ; +VM_BUG_ON_PAGE (PageTransCompound (page), page);if (pte_young (ptent)) {Signature · 5 lines
2.24.1 -- Michal Hocko SUSE Labs
Quoted 4 lines · Minchan Kim
On Thu, Mar 12, 2020 at 09:22:48AM +0100, Michal Hocko wrote:
I't likde to wait Jann's reply since Dave gave his opinion about the vulnerability.
https://lore.kernel.org/linux-mm/[email protected]/
Jann, could you give your insigh about that practically it's possible?
FWIW, just checking for mapcount>=1 seems like a pretty sane fix to me.
I went looking at doing it another way, but Michal was quite correct.
We'd probably end up having to special-case something underneath shrink_page_list().
Quoted 6 lines
A real dumb question to understand vulnerability:
The attacker would be able to trigger heavy memory consumption so that he could make paging them out without
MADV_PAGEOUT. I knowMADV_PAGEOUTmakes it easier but he still could do withoutMADV_PAGEOUT.
What makes difference here?
Causing memory pressure is quite a bit more disruptive than MADV_PAGEOUT. It's a much more blunt instrument and is likely to result in a lot of collateral damage and a lot of I/O.
MADV_PAGEOUT is *surgical*. You can target one very specific page if, for instance, you think that your victim is reading it in a way that is vulnerable. You can also do it with zero I/O (after the initial pageout).
Quoted 3 lines
To clarify how
MADV_PAGEWORKworks:
If other process has accessed the page so that his page table has access bit marked,MADV_PAGEOUTcouldn't page it out.
The attacker doesn't need to get the victim to get a major fault, it just needs to induce *a* fault. I actually did an experiment to see how this would work in practice.
1. Allocate some memory(), touch it
2. fork()
3. In the parent: Loop reading the memory
4. In the child: loop running MADV_PAGEOUT
The pages stayed in the swap cache and the parent reading the memory saw a constant stream of faults.
Quoted 1 line · Minchan Kim
On Thu, Mar 12, 2020 at 09:22:48AM +0100, Michal Hocko wrote:
[...]
Quoted 15 lines
Quoted 5 lines
From
eca97990372679c097a88164ff4b3d7879b0e127Mon Sep 17 00:00:00 2001
From: Michal Hocko <[email protected]>
Date: Thu, 12 Mar 2020 09:04:35 +0100
Subject: [PATCH] mm: do not allowMADV_PAGEOUTfor CoW pagesQuoted 9 lines
Jann has brought up a very interesting point [1]. While shared pages are excluded from
MADV_PAGEOUTnormally, CoW pages can be easily reclaimed that way. This can lead to all sorts of hard to debug problems. E.g. performance problems outlined by Daniel [2]. There are runtime environments where there is a substantial memory shared among security domains via CoW memory and a easy to reclaim way of that memory, which MADV_{COLD,PAGEOUT} offers, can lead to either performance degradation in for the parent process which might be more privileged or even open side channel attacks. The feasibility of the later is not really clear
Quoted 5 lines
I am not sure it's a good idea to mention performance stuff because it's rather arguble. You and Johannes already pointed it out when I sbumit early draft which had shared page filtering out logic due to performance reason. You guys suggested the shared pages has higher chance to be touched so that if it's really hot pages, that whould keep in the memory. I agree.
Yes, the hot memory is likely to be referenced but the point was an unexpected latency because of the major fault. I have to say that I have underestimated the issue because I was not aware of runtimes mentioned in the referenced links. Essentially a lot of CoW memory shared over security domains.
Quoted 24 lines
I think the only reason at this moment is just vulnerability.
Quoted 22 lines
to me TBH but there is no real reason for exposure at this stage. It seems there is no real use case to depend on reclaiming CoW memory via madvise at this stage so it is much easier to simply disallow it and this is what this patch does. Put it simply MADV_{PAGEOUT,COLD} can operate only on the exclusively owned memory which is a straightforward semantic.
[1] http://lkml.kernel.org/r/CAG48ez0G3JkMq61gUmyQAaCq=_TwHbi1XKzWRooxZkv08PQKuw@mail.gmail.com
[2] http://lkml.kernel.org/r/CAKOZueua_v8jHCpmEtTB6f3i9e2YnmX4mqdYVWhV4E=Z-n+zRQ@mail.gmail.comSigned-off-by Michal Hocko <[email protected]>--- mm/madvise.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)mm/madvise.c +0
-0@@ -335,12 +335,14 @@ static int madvise_cold_or_pageout_pte_range(pmd_t *pmd, }
Quoted 10 lines
mm/madvise.c static int madvise_cold_or_pageout_pte_range(pmd_t *pmd, · line 339
Quoted 3 lines
mm/madvise.c +2
-0page =pmd_page (orig_pmd); + +/* Do not interfere with other mappings of this page */ How about this?
/*
* paging out only single mapped private pages for anonymous mapping,
* otherwise, it opens a side channel.
*/
I am not sure this is much more helpful without a larger context. I would stick with the wording unless you insist.
Quoted 1 line
Otherwise, looks good to me.
Thanks for the review.
Signature · 2 lines
Michal Hocko SUSE Labs
Quoted 18 lines · Michal Hocko
Quoted 1 line · Minchan Kim
On Thu, Mar 12, 2020 at 09:22:48AM +0100, Michal Hocko wrote:
[...]
Quoted 15 lines
Quoted 14 lines
From
eca97990372679c097a88164ff4b3d7879b0e127Mon Sep 17 00:00:00 2001
From: Michal Hocko <[email protected]>
Date: Thu, 12 Mar 2020 09:04:35 +0100
Subject: [PATCH] mm: do not allowMADV_PAGEOUTfor CoW pagesJann has brought up a very interesting point [1]. While shared pages are excluded from
MADV_PAGEOUTnormally, CoW pages can be easily reclaimed that way. This can lead to all sorts of hard to debug problems. E.g. performance problems outlined by Daniel [2]. There are runtime environments where there is a substantial memory shared among security domains via CoW memory and a easy to reclaim way of that memory, which MADV_{COLD,PAGEOUT} offers, can lead to either performance degradation in for the parent process which might be more privileged or even open side channel attacks. The feasibility of the later is not really clear
Quoted 8 lines
Quoted 5 lines
I am not sure it's a good idea to mention performance stuff because it's rather arguble. You and Johannes already pointed it out when I sbumit early draft which had shared page filtering out logic due to performance reason. You guys suggested the shared pages has higher chance to be touched so that if it's really hot pages, that whould keep in the memory. I agree.
Yes, the hot memory is likely to be referenced but the point was an unexpected latency because of the major fault. I have to say that I have
I don't understand your point here. If it's likely to be referenced among several processes, it doesn't have the major fault latency. What's your point here?
Quoted 3 lines
underestimated the issue because I was not aware of runtimes mentioned in the referenced links. Essentially a lot of CoW memory shared over security domains.
I tend to agree about security part in the description, but still don't agree with performance concern in the description so I'd like to remove it in the description. Current situation is caused by security concern unfortunately, not performance reason.
Quoted 36 lines
Quoted 25 lines
I think the only reason at this moment is just vulnerability.
Quoted 22 lines
to me TBH but there is no real reason for exposure at this stage. It seems there is no real use case to depend on reclaiming CoW memory via madvise at this stage so it is much easier to simply disallow it and this is what this patch does. Put it simply MADV_{PAGEOUT,COLD} can operate only on the exclusively owned memory which is a straightforward semantic.
[1] http://lkml.kernel.org/r/CAG48ez0G3JkMq61gUmyQAaCq=_TwHbi1XKzWRooxZkv08PQKuw@mail.gmail.com
[2] http://lkml.kernel.org/r/CAKOZueua_v8jHCpmEtTB6f3i9e2YnmX4mqdYVWhV4E=Z-n+zRQ@mail.gmail.comSigned-off-by Michal Hocko <[email protected]>--- mm/madvise.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)mm/madvise.c +0
-0@@ -335,12 +335,14 @@ static int madvise_cold_or_pageout_pte_range(pmd_t *pmd, }Quoted 10 lines
mm/madvise.c static int madvise_cold_or_pageout_pte_range(pmd_t *pmd, · line 339
Quoted 3 lines
mm/madvise.c +2
-0page =pmd_page (orig_pmd); + +/* Do not interfere with other mappings of this page */ How about this?
/*
* paging out only single mapped private pages for anonymous mapping,
* otherwise, it opens a side channel.
*/
Quoted 2 lines
I am not sure this is much more helpful without a larger context. I would stick with the wording unless you insist.
The comment you provides explain what code does, not *why*. Comment is always lack of explaining the whole story but we try to demonstrate a certain context to make reader careful. Havind said, I will not insist on it.
Thanks.
Quoted 25 lines · Minchan Kim
Quoted 24 lines · Michal Hocko
Quoted 1 line · Minchan Kim
On Thu, Mar 12, 2020 at 09:22:48AM +0100, Michal Hocko wrote:
[...]
Quoted 20 lines
Quoted 5 lines
From
eca97990372679c097a88164ff4b3d7879b0e127Mon Sep 17 00:00:00 2001
From: Michal Hocko <[email protected]>
Date: Thu, 12 Mar 2020 09:04:35 +0100
Subject: [PATCH] mm: do not allowMADV_PAGEOUTfor CoW pagesQuoted 9 lines
Jann has brought up a very interesting point [1]. While shared pages are excluded from
MADV_PAGEOUTnormally, CoW pages can be easily reclaimed that way. This can lead to all sorts of hard to debug problems. E.g. performance problems outlined by Daniel [2]. There are runtime environments where there is a substantial memory shared among security domains via CoW memory and a easy to reclaim way of that memory, which MADV_{COLD,PAGEOUT} offers, can lead to either performance degradation in for the parent process which might be more privileged or even open side channel attacks. The feasibility of the later is not really clearI am not sure it's a good idea to mention performance stuff because it's rather arguble. You and Johannes already pointed it out when I sbumit early draft which had shared page filtering out logic due to performance reason. You guys suggested the shared pages has higher chance to be touched so that if it's really hot pages, that whould keep in the memory. I agree.
Quoted 6 lines
Quoted 2 lines
Yes, the hot memory is likely to be referenced but the point was an unexpected latency because of the major fault. I have to say that I have
I don't understand your point here. If it's likely to be referenced among several processes, it doesn't have the major fault latency. What's your point here?
a) the particular CoW page might be cold enough to be reclaimed and b) nothing really prevents the MADV_PAGEOUT to be called faster than the reference bit being readded.
Quoted 8 lines
Quoted 3 lines
underestimated the issue because I was not aware of runtimes mentioned in the referenced links. Essentially a lot of CoW memory shared over security domains.
I tend to agree about security part in the description, but still don't agree with performance concern in the description so I'd like to remove it in the description. Current situation is caused by security concern unfortunately, not performance reason.
Well, I have to admit that I haven't seen any actual numbers here but considering zygote like workload I would rather not even risk it. Even if the risk is theoretical I would rather put the restriction and mention it in the changelog. If somebody would like to drop this restriction it is at least more clear what to test for.
Signature · 2 lines
Michal Hocko SUSE Labs
Quoted 26 lines · Michal Hocko
Quoted 25 lines · Minchan Kim
Quoted 24 lines · Michal Hocko
Quoted 1 line · Minchan Kim
On Thu, Mar 12, 2020 at 09:22:48AM +0100, Michal Hocko wrote:
[...]
Quoted 20 lines
Quoted 5 lines
From
eca97990372679c097a88164ff4b3d7879b0e127Mon Sep 17 00:00:00 2001
From: Michal Hocko <[email protected]>
Date: Thu, 12 Mar 2020 09:04:35 +0100
Subject: [PATCH] mm: do not allowMADV_PAGEOUTfor CoW pagesQuoted 9 lines
Jann has brought up a very interesting point [1]. While shared pages are excluded from
MADV_PAGEOUTnormally, CoW pages can be easily reclaimed that way. This can lead to all sorts of hard to debug problems. E.g. performance problems outlined by Daniel [2]. There are runtime environments where there is a substantial memory shared among security domains via CoW memory and a easy to reclaim way of that memory, which MADV_{COLD,PAGEOUT} offers, can lead to either performance degradation in for the parent process which might be more privileged or even open side channel attacks. The feasibility of the later is not really clearI am not sure it's a good idea to mention performance stuff because it's rather arguble. You and Johannes already pointed it out when I sbumit early draft which had shared page filtering out logic due to performance reason. You guys suggested the shared pages has higher chance to be touched so that if it's really hot pages, that whould keep in the memory. I agree.
Quoted 8 lines
Quoted 6 lines
Quoted 2 lines
Yes, the hot memory is likely to be referenced but the point was an unexpected latency because of the major fault. I have to say that I have
I don't understand your point here. If it's likely to be referenced among several processes, it doesn't have the major fault latency. What's your point here?
a) the particular CoW page might be cold enough to be reclaimed and b)
If it is, that means it's *cold* so it's really worth to be reclaimed.
Quoted 2 lines
nothing really prevents the
MADV_PAGEOUTto be called faster than the reference bit being readded.
Yeb, that's undesirable. I should admit it was not intended when I implemented PAGEOUT. The thing is page_check_references clears access bit of pte for every process are sharing the page so that two times MADV_PAGEOUT from a process could evict the page. That's the really bug. It shouldn't have cleared the access bit for other processes. What I wanted to do with MADV_PAGEOUT is just check the reference from the other processes without clearing access bit of pte and if it found other process pte has access bit, just bail out.
Okay so you're right. Current implementation could cause performance impact since MADV_PAGEOUT unconditionally clear the access bit of other processes so your patch will fix it.
Thanks.
Quoted 38 lines · Minchan Kim
Quoted 26 lines · Michal Hocko
Quoted 25 lines · Minchan Kim
Quoted 24 lines · Michal Hocko
Quoted 1 line · Minchan Kim
On Thu, Mar 12, 2020 at 09:22:48AM +0100, Michal Hocko wrote:
[...]
Quoted 20 lines
Quoted 5 lines
From
eca97990372679c097a88164ff4b3d7879b0e127Mon Sep 17 00:00:00 2001
From: Michal Hocko <[email protected]>
Date: Thu, 12 Mar 2020 09:04:35 +0100
Subject: [PATCH] mm: do not allowMADV_PAGEOUTfor CoW pagesQuoted 9 lines
Jann has brought up a very interesting point [1]. While shared pages are excluded from
MADV_PAGEOUTnormally, CoW pages can be easily reclaimed that way. This can lead to all sorts of hard to debug problems. E.g. performance problems outlined by Daniel [2]. There are runtime environments where there is a substantial memory shared among security domains via CoW memory and a easy to reclaim way of that memory, which MADV_{COLD,PAGEOUT} offers, can lead to either performance degradation in for the parent process which might be more privileged or even open side channel attacks. The feasibility of the later is not really clearI am not sure it's a good idea to mention performance stuff because it's rather arguble. You and Johannes already pointed it out when I sbumit early draft which had shared page filtering out logic due to performance reason. You guys suggested the shared pages has higher chance to be touched so that if it's really hot pages, that whould keep in the memory. I agree.
Quoted 8 lines
Quoted 6 lines
Quoted 2 lines
Yes, the hot memory is likely to be referenced but the point was an unexpected latency because of the major fault. I have to say that I have
I don't understand your point here. If it's likely to be referenced among several processes, it doesn't have the major fault latency. What's your point here?
a) the particular CoW page might be cold enough to be reclaimed and b)
If it is, that means it's *cold* so it's really worth to be reclaimed.
Quoted 7 lines
Quoted 2 lines
nothing really prevents the
MADV_PAGEOUTto be called faster than the reference bit being readded.Yeb, that's undesirable. I should admit it was not intended when I implemented PAGEOUT. The thing is
page_check_referencesclears access bit of pte for every process are sharing the page so that two timesMADV_PAGEOUTfrom a process could evict the page. That's the really bug.
I do not really think this is a bug. This is a side effect of the reclaim process and we do not really want MADV_{PAGEOUT,COLD} behave differently here because then the behavior would be even harder to understand.
Signature · 2 lines
Michal Hocko SUSE Labs
Quoted 42 lines · Michal Hocko
Quoted 41 lines · Minchan Kim
Quoted 26 lines · Michal Hocko
Quoted 25 lines · Minchan Kim
Quoted 24 lines · Michal Hocko
Quoted 1 line · Minchan Kim
On Thu, Mar 12, 2020 at 09:22:48AM +0100, Michal Hocko wrote:
[...]
Quoted 20 lines
Quoted 5 lines
From
eca97990372679c097a88164ff4b3d7879b0e127Mon Sep 17 00:00:00 2001
From: Michal Hocko <[email protected]>
Date: Thu, 12 Mar 2020 09:04:35 +0100
Subject: [PATCH] mm: do not allowMADV_PAGEOUTfor CoW pagesQuoted 9 lines
Jann has brought up a very interesting point [1]. While shared pages are excluded from
MADV_PAGEOUTnormally, CoW pages can be easily reclaimed that way. This can lead to all sorts of hard to debug problems. E.g. performance problems outlined by Daniel [2]. There are runtime environments where there is a substantial memory shared among security domains via CoW memory and a easy to reclaim way of that memory, which MADV_{COLD,PAGEOUT} offers, can lead to either performance degradation in for the parent process which might be more privileged or even open side channel attacks. The feasibility of the later is not really clearI am not sure it's a good idea to mention performance stuff because it's rather arguble. You and Johannes already pointed it out when I sbumit early draft which had shared page filtering out logic due to performance reason. You guys suggested the shared pages has higher chance to be touched so that if it's really hot pages, that whould keep in the memory. I agree.
Quoted 8 lines
Quoted 6 lines
Quoted 2 lines
Yes, the hot memory is likely to be referenced but the point was an unexpected latency because of the major fault. I have to say that I have
I don't understand your point here. If it's likely to be referenced among several processes, it doesn't have the major fault latency. What's your point here?
a) the particular CoW page might be cold enough to be reclaimed and b)
If it is, that means it's *cold* so it's really worth to be reclaimed.
Quoted 2 lines
nothing really prevents the
MADV_PAGEOUTto be called faster than the reference bit being readded.
Quoted 7 lines
Quoted 4 lines
Yeb, that's undesirable. I should admit it was not intended when I implemented PAGEOUT. The thing is
page_check_referencesclears access bit of pte for every process are sharing the page so that two timesMADV_PAGEOUTfrom a process could evict the page. That's the really bug.I do not really think this is a bug. This is a side effect of the reclaim process and we do not really want MADV_{PAGEOUT,COLD} behave
No, that's the bug since we didn't consider the side effect.
Quoted 1 line
differently here because then the behavior would be even harder to
No, I do want to have difference because it's per-process hint. IOW, what he know is for only his context, not others so it shouldn't clean others' pte. That makes difference between LRU aging and the hint.
Quoted 1 line
understand.
It's not hard to understand.. MADV_PAGEOUT should consider only his context since it's per-process hint(Even, he couldn't know others' context) so it shouldn't bother others.
Actually, Dave's suggestion is correct to fix the issue if there was no isse with side channel attack. However, due to the attack issue, page_mapcount could prevent the problem effectively. That's why I am not against of the patch now since it fixes the bug as well as vulnerability.
Quoted 51 lines · Minchan Kim
Quoted 42 lines · Michal Hocko
Quoted 41 lines · Minchan Kim
Quoted 26 lines · Michal Hocko
Quoted 25 lines · Minchan Kim
Quoted 24 lines · Michal Hocko
Quoted 1 line · Minchan Kim
On Thu, Mar 12, 2020 at 09:22:48AM +0100, Michal Hocko wrote:
[...]
Quoted 20 lines
Quoted 5 lines
From
eca97990372679c097a88164ff4b3d7879b0e127Mon Sep 17 00:00:00 2001
From: Michal Hocko <[email protected]>
Date: Thu, 12 Mar 2020 09:04:35 +0100
Subject: [PATCH] mm: do not allowMADV_PAGEOUTfor CoW pagesQuoted 9 lines
Jann has brought up a very interesting point [1]. While shared pages are excluded from
MADV_PAGEOUTnormally, CoW pages can be easily reclaimed that way. This can lead to all sorts of hard to debug problems. E.g. performance problems outlined by Daniel [2]. There are runtime environments where there is a substantial memory shared among security domains via CoW memory and a easy to reclaim way of that memory, which MADV_{COLD,PAGEOUT} offers, can lead to either performance degradation in for the parent process which might be more privileged or even open side channel attacks. The feasibility of the later is not really clearI am not sure it's a good idea to mention performance stuff because it's rather arguble. You and Johannes already pointed it out when I sbumit early draft which had shared page filtering out logic due to performance reason. You guys suggested the shared pages has higher chance to be touched so that if it's really hot pages, that whould keep in the memory. I agree.
Quoted 8 lines
Quoted 6 lines
Quoted 2 lines
Yes, the hot memory is likely to be referenced but the point was an unexpected latency because of the major fault. I have to say that I have
I don't understand your point here. If it's likely to be referenced among several processes, it doesn't have the major fault latency. What's your point here?
a) the particular CoW page might be cold enough to be reclaimed and b)
If it is, that means it's *cold* so it's really worth to be reclaimed.
Quoted 2 lines
nothing really prevents the
MADV_PAGEOUTto be called faster than the reference bit being readded.Quoted 7 lines
Quoted 4 lines
Yeb, that's undesirable. I should admit it was not intended when I implemented PAGEOUT. The thing is
page_check_referencesclears access bit of pte for every process are sharing the page so that two timesMADV_PAGEOUTfrom a process could evict the page. That's the really bug.I do not really think this is a bug. This is a side effect of the reclaim process and we do not really want MADV_{PAGEOUT,COLD} behave
Quoted 7 lines
No, that's the bug since we didn't consider the side effect.
Quoted 1 line
differently here because then the behavior would be even harder to
No, I do want to have difference because it's per-process hint. IOW, what he know is for only his context, not others so it shouldn't clean others' pte. That makes difference between LRU aging and the hint.
Just to make it clear, are you really suggesting to special case page_check_references for madvise path?
Signature · 2 lines
Michal Hocko SUSE Labs
Quoted 54 lines · Michal Hocko
Quoted 53 lines · Minchan Kim
Quoted 42 lines · Michal Hocko
Quoted 41 lines · Minchan Kim
Quoted 26 lines · Michal Hocko
Quoted 25 lines · Minchan Kim
Quoted 24 lines · Michal Hocko
Quoted 1 line · Minchan Kim
On Thu, Mar 12, 2020 at 09:22:48AM +0100, Michal Hocko wrote:
[...]
Quoted 20 lines
Quoted 5 lines
From
eca97990372679c097a88164ff4b3d7879b0e127Mon Sep 17 00:00:00 2001
From: Michal Hocko <[email protected]>
Date: Thu, 12 Mar 2020 09:04:35 +0100
Subject: [PATCH] mm: do not allowMADV_PAGEOUTfor CoW pagesQuoted 9 lines
Jann has brought up a very interesting point [1]. While shared pages are excluded from
MADV_PAGEOUTnormally, CoW pages can be easily reclaimed that way. This can lead to all sorts of hard to debug problems. E.g. performance problems outlined by Daniel [2]. There are runtime environments where there is a substantial memory shared among security domains via CoW memory and a easy to reclaim way of that memory, which MADV_{COLD,PAGEOUT} offers, can lead to either performance degradation in for the parent process which might be more privileged or even open side channel attacks. The feasibility of the later is not really clearI am not sure it's a good idea to mention performance stuff because it's rather arguble. You and Johannes already pointed it out when I sbumit early draft which had shared page filtering out logic due to performance reason. You guys suggested the shared pages has higher chance to be touched so that if it's really hot pages, that whould keep in the memory. I agree.
Quoted 8 lines
Quoted 6 lines
Quoted 2 lines
Yes, the hot memory is likely to be referenced but the point was an unexpected latency because of the major fault. I have to say that I have
I don't understand your point here. If it's likely to be referenced among several processes, it doesn't have the major fault latency. What's your point here?
a) the particular CoW page might be cold enough to be reclaimed and b)
If it is, that means it's *cold* so it's really worth to be reclaimed.
Quoted 2 lines
nothing really prevents the
MADV_PAGEOUTto be called faster than the reference bit being readded.Quoted 7 lines
Quoted 4 lines
Yeb, that's undesirable. I should admit it was not intended when I implemented PAGEOUT. The thing is
page_check_referencesclears access bit of pte for every process are sharing the page so that two timesMADV_PAGEOUTfrom a process could evict the page. That's the really bug.I do not really think this is a bug. This is a side effect of the reclaim process and we do not really want MADV_{PAGEOUT,COLD} behave
No, that's the bug since we didn't consider the side effect.
Quoted 9 lines
Quoted 5 lines
Quoted 1 line
differently here because then the behavior would be even harder to
No, I do want to have difference because it's per-process hint. IOW, what he know is for only his context, not others so it shouldn't clean others' pte. That makes difference between LRU aging and the hint.
Just to make it clear, are you really suggesting to special case
page_check_referencesfor madvise path?
No, (page_mapcount() > 1) checks *effectively* fixes the performance bug as well as vulnerability issue.
Quoted 1 line · Minchan Kim
On Tue, Mar 17, 2020 at 08:12:39AM +0100, Michal Hocko wrote:
[...]
Quoted 6 lines
Quoted 3 lines
Just to make it clear, are you really suggesting to special case
page_check_referencesfor madvise path?No, (
page_mapcount()> 1) checks *effectively* fixes the performance bug as well as vulnerability issue.
Ahh, ok then we are on the same page. You were replying to the part where I have pointed out that you can control aging by these calls and your response suggested that this is somehow undesirable behavior or even a bug.
Signature · 2 lines
Michal Hocko SUSE Labs
Quoted 7 lines · Michal Hocko
Quoted 1 line · Minchan Kim
On Tue, Mar 17, 2020 at 08:12:39AM +0100, Michal Hocko wrote:
[...]
Quoted 4 lines
Quoted 3 lines
Just to make it clear, are you really suggesting to special case
page_check_referencesfor madvise path?
Quoted 7 lines
Quoted 2 lines
No, (
page_mapcount()> 1) checks *effectively* fixes the performance bug as well as vulnerability issue.Ahh, ok then we are on the same page. You were replying to the part where I have pointed out that you can control aging by these calls and your response suggested that this is somehow undesirable behavior or even a bug.
Sorry about the confusing.
I want to clarify my speaking.
If we don't have vulnerability issue Jann raised, the performance issue Daniel pointed should be fixed by introducing a special flag in page_check_references from madvise path to avoid cleaning of access bit from other processes's pte. With it, we don't need to limit semantic of MADV_PAGEOUT as "exclusive page only" so that MADV_PAGEOUT will work *cold* shared pages as well as exclusive one.
However, since we have the vulnerability issue, *unfortunately*, we need to make MADV_PAGEOUT's semantic working with only exclusive page. Thus, page_mapcount check in madvise patch will fix both issues *effectively*.
Thanks.
One other fun thing. I have a "victim" thread sitting in a loop doing:
sleep(1) memcpy(&garbage, buffer, sz);
The "attacker" is doing
madvise(buffer, sz, MADV_PAGEOUT);
in a loop. That, oddly enough doesn't cause the victim to page fault. But, if I do:
memcpy(&garbage, buffer, sz); madvise(buffer, sz, MADV_PAGEOUT);
It *does* cause the memory to get paged out. The MADV_PAGEOUT code actually has a !pte_present() check. It will punt on a PTE if it sees it. In other words, if a page is in the swap cache but not mapped by a pte_present() PTE, MADV_PAGEOUT won't touch it.
Shouldn't MADV_PAGEOUT be able to find and reclaim those pages? Patch attached.
---
b/mm/madvise.c | 38 +++++++++++++++++++++++++++++++------- 1 file changed, 31 insertions(+), 7 deletions(-)
mm/madvise.c renamed from mm/madvise.c~madv-pageout-find-swap-cache +31 -7
@@ -248,6 +248,36 @@ static void force_shm_swapin_readahead(s#endif /* CONFIG_SWAP */ /* +* Given a PTE, find the corresponding 'struct page'. Also handles +* non-present swap PTEs. +*/ +struct page *pte_to_reclaim_page (struct vm_area_struct *vma, +unsigned long addr,pte_t ptent) +{ +swp_entry_t entry; + +/* Totally empty PTE: */ +if (pte_none (ptent)) +return NULL ; + +/* A normal, present page is mapped: */ +if (pte_present (ptent)) +return vm_normal_page (vma, addr, ptent); + + entry =pte_to_swp_entry (vmf->orig_pte); +/* Is it one of the "swap PTEs" that's not really swap? */ +if (non_swap_entry (entry)) +return false; + +/* +* The PTE was a true swap entry. The page may be in the +* swap cache. If so, find it and return it so it may be +* reclaimed. +*/ +return lookup_swap_cache (entry, vma, addr); +} + +/* * Schedule all required I/O operations. Do not wait for completion. */ static long madvise_willneed(struct vm_area_struct *vma,
@@ -389,13 +419,7 @@ regular_page:for (; addr < end; pte++, addr +=PAGE_SIZE ) { ptent = *pte; -if (pte_none (ptent)) -continue ; - -if (!pte_present (ptent)) -continue ; - - page =vm_normal_page (vma, addr, ptent); + page =pte_to_reclaim_page (vma, addr, ptent);if (!page)continue ;
_
Quoted 43 lines · Dave Hansen
One other fun thing. I have a "victim" thread sitting in a loop doing:
sleep(1) memcpy(&garbage, buffer, sz);The "attacker" is doing
madvise(buffer, sz, MADV_PAGEOUT);in a loop. That, oddly enough doesn't cause the victim to page fault. But, if I do:
memcpy(&garbage, buffer, sz); madvise(buffer, sz, MADV_PAGEOUT);It *does* cause the memory to get paged out. The
MADV_PAGEOUTcode actually has a !pte_present() check. It will punt on a PTE if it sees it. In other words, if a page is in the swap cache but not mapped by apte_present()PTE,MADV_PAGEOUTwon't touch it.Shouldn't
MADV_PAGEOUTbe able to find and reclaim those pages? Patch attached.---
b/mm/madvise.c | 38 +++++++++++++++++++++++++++++++------- 1 file changed, 31 insertions(+), 7 deletions(-)diff -puN
mm/madvise.c~madv-pageout-find-swap-cachemm/madvise.cmm/madvise.c renamed from mm/madvise.c~madv-pageout-find-swap-cache +6
-0@@ -248,6 +248,36 @@ static void force_shm_swapin_readahead(s#endif /* CONFIG_SWAP */ /* +* Given a PTE, find the corresponding 'struct page'. Also handles +* non-present swap PTEs. +*/ +struct page *pte_to_reclaim_page (struct vm_area_struct *vma, +unsigned long addr,pte_t ptent) +{
mm/madvise.c static void force_shm_swapin_readahead(s · line 266
Quoted 10 lines
mm/madvise.c renamed from mm/madvise.c~madv-pageout-find-swap-cache +10
-0+swp_entry_t entry; + +/* Totally empty PTE: */ +if (pte_none (ptent)) +return NULL ; + +/* A normal, present page is mapped: */ +if (pte_present (ptent)) +return vm_normal_page (vma, addr, ptent); +
Please check is_swap_pte first.
mm/madvise.c static void force_shm_swapin_readahead(s · line 277
Quoted 11 lines
mm/madvise.c renamed from mm/madvise.c~madv-pageout-find-swap-cache +11
-0+ entry =pte_to_swp_entry (vmf->orig_pte); +/* Is it one of the "swap PTEs" that's not really swap? */ +if (non_swap_entry (entry)) +return false; + +/* +* The PTE was a true swap entry. The page may be in the +* swap cache. If so, find it and return it so it may be +* reclaimed. +*/ +return lookup_swap_cache (entry, vma, addr);
If we go with handling only exclusived owned page for anon, I think we should apply the rule to swap cache, too.
Do you mind posting it as formal patch?
Thanks for the explain about vulnerability and the patch, Dave!
Quoted 22 lines
mm/madvise.c renamed from mm/madvise.c~madv-pageout-find-swap-cache +4
-7+} + +/* * Schedule all required I/O operations. Do not wait for completion. */ static long madvise_willneed(struct vm_area_struct *vma,@@ -389,13 +419,7 @@ regular_page:for (; addr < end; pte++, addr +=PAGE_SIZE ) { ptent = *pte; -if (pte_none (ptent)) -continue ; - -if (!pte_present (ptent)) -continue ; - - page =vm_normal_page (vma, addr, ptent); + page =pte_to_reclaim_page (vma, addr, ptent);if (!page)continue ;_
Quoted 48 lines · Minchan Kim
Quoted 47 lines · Dave Hansen
One other fun thing. I have a "victim" thread sitting in a loop doing:
sleep(1) memcpy(&garbage, buffer, sz);The "attacker" is doing
madvise(buffer, sz, MADV_PAGEOUT);in a loop. That, oddly enough doesn't cause the victim to page fault. But, if I do:
memcpy(&garbage, buffer, sz); madvise(buffer, sz, MADV_PAGEOUT);It *does* cause the memory to get paged out. The
MADV_PAGEOUTcode actually has a !pte_present() check. It will punt on a PTE if it sees it. In other words, if a page is in the swap cache but not mapped by apte_present()PTE,MADV_PAGEOUTwon't touch it.Shouldn't
MADV_PAGEOUTbe able to find and reclaim those pages? Patch attached.---
b/mm/madvise.c | 38 +++++++++++++++++++++++++++++++------- 1 file changed, 31 insertions(+), 7 deletions(-)diff -puN
mm/madvise.c~madv-pageout-find-swap-cachemm/madvise.cmm/madvise.c renamed from mm/madvise.c~madv-pageout-find-swap-cache +10
-0@@ -248,6 +248,36 @@ static void force_shm_swapin_readahead(s#endif /* CONFIG_SWAP */ /* +* Given a PTE, find the corresponding 'struct page'. Also handles +* non-present swap PTEs. +*/ +struct page *pte_to_reclaim_page (struct vm_area_struct *vma, +unsigned long addr,pte_t ptent) +{ +swp_entry_t entry; + +/* Totally empty PTE: */ +if (pte_none (ptent))
Quoted 8 lines
mm/madvise.c static void force_shm_swapin_readahead(s · line 266
Quoted 6 lines
mm/madvise.c renamed from mm/madvise.c~madv-pageout-find-swap-cache +6
-0+return NULL ; + +/* A normal, present page is mapped: */ +if (pte_present (ptent)) +return vm_normal_page (vma, addr, ptent); +Please check
is_swap_ptefirst.
Why?
is_swap_pte() duplicates the first two checks. But, I need an explicit pte_present() check somewhere because I need to call vm_normal_page() only on present PTEs.
I guess the pte_present() check could be:
if (!is_swap_pte(ptent)) return vm_normal_page(...);
*after* the pte_none() check.
Quoted 6 lines
Quoted 6 lines
mm/madvise.c renamed from mm/madvise.c~madv-pageout-find-swap-cache +6
-0+ entry =pte_to_swp_entry (vmf->orig_pte); +/* Is it one of the "swap PTEs" that's not really swap? */ +if (non_swap_entry (entry)) +return false; + +/*
Quoted 8 lines
mm/madvise.c static void force_shm_swapin_readahead(s · line 277
Quoted 5 lines
mm/madvise.c renamed from mm/madvise.c~madv-pageout-find-swap-cache +5
-0+* The PTE was a true swap entry. The page may be in the +* swap cache. If so, find it and return it so it may be +* reclaimed. +*/ +return lookup_swap_cache (entry, vma, addr);If we go with handling only exclusived owned page for anon, I think we should apply the rule to swap cache, too.
I'm going back and forth on it. If we're just trying to avoid causing faults in other processes, we could add a mapcount>0 check here in addition to the mapcount>1 checks that were added in the other patch.
But, if we want a check for true exclusivity: no other swap entries or mappings, we need to check swap_count() too. It's getting quite a bit uglier as I add that it, but I guess we'll see how it looks in the end.
Quoted 1 line
Do you mind posting it as formal patch?
Yeah, I'll send something out.
Quoted 64 lines · Dave Hansen
Quoted 48 lines · Minchan Kim
Quoted 47 lines · Dave Hansen
One other fun thing. I have a "victim" thread sitting in a loop doing:
sleep(1) memcpy(&garbage, buffer, sz);The "attacker" is doing
madvise(buffer, sz, MADV_PAGEOUT);in a loop. That, oddly enough doesn't cause the victim to page fault. But, if I do:
memcpy(&garbage, buffer, sz); madvise(buffer, sz, MADV_PAGEOUT);It *does* cause the memory to get paged out. The
MADV_PAGEOUTcode actually has a !pte_present() check. It will punt on a PTE if it sees it. In other words, if a page is in the swap cache but not mapped by apte_present()PTE,MADV_PAGEOUTwon't touch it.Shouldn't
MADV_PAGEOUTbe able to find and reclaim those pages? Patch attached.---
b/mm/madvise.c | 38 +++++++++++++++++++++++++++++++------- 1 file changed, 31 insertions(+), 7 deletions(-)diff -puN
mm/madvise.c~madv-pageout-find-swap-cachemm/madvise.cmm/madvise.c renamed from mm/madvise.c~madv-pageout-find-swap-cache +10
-0@@ -248,6 +248,36 @@ static void force_shm_swapin_readahead(s#endif /* CONFIG_SWAP */ /* +* Given a PTE, find the corresponding 'struct page'. Also handles +* non-present swap PTEs. +*/ +struct page *pte_to_reclaim_page (struct vm_area_struct *vma, +unsigned long addr,pte_t ptent) +{ +swp_entry_t entry; + +/* Totally empty PTE: */ +if (pte_none (ptent))Quoted 8 lines
mm/madvise.c static void force_shm_swapin_readahead(s · line 266
Quoted 6 lines
mm/madvise.c renamed from mm/madvise.c~madv-pageout-find-swap-cache +6
-0+return NULL ; + +/* A normal, present page is mapped: */ +if (pte_present (ptent)) +return vm_normal_page (vma, addr, ptent); +Please check
is_swap_ptefirst.Why?
is_swap_pte()duplicates the first two checks. But, I need an explicitpte_present()check somewhere because I need to callvm_normal_page()only on present PTEs.
Quoted 6 lines
I guess the
pte_present()check could be:if (!is_swap_pte(ptent)) return vm_normal_page(...);*after* the
pte_none()check.
Yub, I thought is_swap_pte looks more readable and maintainable for the change of pte encoding in future. Anyway, I am not insisting.
Quoted 16 lines
Quoted 15 lines
mm/madvise.c static void force_shm_swapin_readahead(s · line 277
Quoted 12 lines
mm/madvise.c renamed from mm/madvise.c~madv-pageout-find-swap-cache +11
-0+ entry =pte_to_swp_entry (vmf->orig_pte); +/* Is it one of the "swap PTEs" that's not really swap? */ +if (non_swap_entry (entry)) +return false; + +/* +* The PTE was a true swap entry. The page may be in the +* swap cache. If so, find it and return it so it may be +* reclaimed. +*/ +return lookup_swap_cache (entry, vma, addr);If we go with handling only exclusived owned page for anon, I think we should apply the rule to swap cache, too.
Quoted 7 lines
I'm going back and forth on it. If we're just trying to avoid causing faults in other processes, we could add a mapcount>0 check here in addition to the mapcount>1 checks that were added in the other patch.
But, if we want a check for true exclusivity: no other swap entries or mappings, we need to check
swap_count()too. It's getting quite a bit uglier as I add that it, but I guess we'll see how it looks in the end.
If we go to the map_count > 1 check here and follows the Daniel's suggestion of MADV_PAGEOUT_ALL to make shared page paging out, that means it clearly makes semantic change for MADV_PAGEOUT: "paging out only exclusive owned page" so it would be rather weired if we reclaim swap_count() > 1 of swap cache.
Quoted 4 lines
Quoted 2 lines
Do you mind posting it as formal patch?
Yeah, I'll send something out.
Thanks for bring up the issue, Dave!
Jann Horn
reviewerQuoted 10 lines · Minchan Kim
Quoted 3 lines · Michal Hocko
[Cc akpm]
So what about this?
Thanks, Michal.
I't likde to wait Jann's reply since Dave gave his opinion about the vulnerability.
https://lore.kernel.org/linux-mm/[email protected]/
Jann, could you give your insigh about that practically it's possible?
Since I don't really know much about the whole LVI attack, and in particular don't have any practical experience with it, I don't think I can help with insight on that. So since Dave said that he doesn't think it's practical, I'll defer to him on that.