mm/swap: abort swapoff after disk error
Subsystems (2)
PATCH 1/1 mm/swap: abort swapoff after disk error
Content of non-uptodate pages completely random, we cannot expose them into userspace. This leads to information leak and will crash userspace for sure. Probably we can reuse hwpoison entries here, but tmpfs already too complex.
--- mm/swapfile.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
mm/swapfile.c +16 -0
@@ -1127,6 +1127,22 @@ int try_to_unuse(unsigned int type, bool frontswap,wait_on_page_writeback (page);/* +* If read failed we cannot map not-uptodate page to +* user space. Actually, we are in serious troubles, +* we do not even know what process to kill. So, the only +* variant remains: to stop swapoff() and allow someone +* to kill processes to zap invalid pages. +* +* TODO replace page with hwpoison entry in pte and shmem. +*/ +if (unlikely (!PageUptodate (page))) { +unlock_page (page); +page_cache_release (page); + retval = -EIO ; +break ; + } + +/* * Remove all references to entry. */ swcount = *swap_map;
Quoted 2 lines · Konstantin Khlebnikov
Content of non-uptodate pages completely random, we cannot expose them into userspace. This leads to information leak and will crash userspace for sure.
Good find, yes, it's very wrong as is. But, sorry, I don't like your fix
- better than ignoring the issue as at present, but not the right answer.
Quoted 1 line
Probably we can reuse hwpoison entries here, but tmpfs already too complex.
HWpoison entries? They're for when that page of RAM is bad, but this is quite a different case: the page is fine and can perfectly well be freed and reused - what's bad is the data currently in it.
Quoted 15 lines
Signed-off-by Konstantin Khlebnikov <[email protected]>Original-patch-by Alexey Kuznetsov <[email protected]>Cc Andrew Morton <[email protected]>Cc Hugh Dickins <[email protected]>Cc Andi Kleen <[email protected]>--- mm/swapfile.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)mm/swapfile.c +0
-0
mm/swapfile.c int try_to_unuse(unsigned int type, bool frontswap, · line 1132
Quoted 6 lines
mm/swapfile.c +3
-0@@ -1127,6 +1127,22 @@ int try_to_unuse(unsigned int type, bool frontswap,wait_on_page_writeback (page);/* +* If read failed we cannot map not-uptodate page to +* user space. Actually, we are in serious troubles, +* we do not even know what process to kill. So, the only
try_to_unuse() is all about locating exactly where this page belongs; and if the user is lucky, the page in question won't even be needed again before the process exits, so nothing should be killed at this point.
mm/swapfile.c int try_to_unuse(unsigned int type, bool frontswap, · line 1134
Quoted 2 lines
mm/swapfile.c +2
-0+* variant remains: to stop swapoff() and allow someone +* to kill processes to zap invalid pages.
No, we should not abort swapoff: there's every reason to continue, to make sure that this unreliable area can be taken out of service.
mm/swapfile.c int try_to_unuse(unsigned int type, bool frontswap, · line 1136
Quoted 2 lines
mm/swapfile.c +2
-0+* +* TODO replace page with hwpoison entry in pte and shmem.
Instead of blindly going ahead and inserting ptes pointing to the !PageUptodate page, unuse_pte() and shmem_unuse_inode() should insert a substitute bad swapentry, to generate SIGBUS if it's accessed.
swp_entry(1, 0) might serve, but there's probably a few mods needed here and there; and getting the details right (e.g. memcg charges) will need care.
Not as straightforward as your block below, I admit. I wonder if you posted that just to stir me to do better: or can you take it further?
Thanks,
Hugh
Quoted 12 lines
mm/swapfile.c +9
-0+*/ +if (unlikely (!PageUptodate (page))) { +unlock_page (page); +page_cache_release (page); + retval = -EIO ; +break ; + } + +/* * Remove all references to entry. */ swcount = *swap_map;
Quoted 54 lines · Hugh Dickins
Quoted 2 lines · Konstantin Khlebnikov
Content of non-uptodate pages completely random, we cannot expose them into userspace. This leads to information leak and will crash userspace for sure.
Good find, yes, it's very wrong as is. But, sorry, I don't like your fix
- better than ignoring the issue as at present, but not the right answer.Quoted 1 line
Probably we can reuse hwpoison entries here, but tmpfs already too complex.
HWpoison entries? They're for when that page of RAM is bad, but this is quite a different case: the page is fine and can perfectly well be freed and reused - what's bad is the data currently in it.
Quoted 15 lines
Signed-off-by Konstantin Khlebnikov<[email protected]>Original-patch-by Alexey Kuznetsov<[email protected]>Cc Andrew Morton<[email protected]>Cc Hugh Dickins<[email protected]>Cc Andi Kleen<[email protected]>--- mm/swapfile.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)mm/swapfile.c +0
-0mm/swapfile.c int try_to_unuse(unsigned int type, bool frontswap, · line 1132
Quoted 6 lines
mm/swapfile.c +3
-0@@ -1127,6 +1127,22 @@ int try_to_unuse(unsigned int type, bool frontswap,wait_on_page_writeback (page);/* +* If read failed we cannot map not-uptodate page to +* user space. Actually, we are in serious troubles, +* we do not even know what process to kill. So, the only
try_to_unuse()is all about locating exactly where this page belongs; and if the user is lucky, the page in question won't even be needed again before the process exits, so nothing should be killed at this point.mm/swapfile.c int try_to_unuse(unsigned int type, bool frontswap, · line 1134
Quoted 2 lines
mm/swapfile.c +2
-0+* variant remains: to stop swapoff() and allow someone +* to kill processes to zap invalid pages. No, we should not abort swapoff: there's every reason to continue, to make sure that this unreliable area can be taken out of service.
mm/swapfile.c int try_to_unuse(unsigned int type, bool frontswap, · line 1136
Quoted 2 lines
mm/swapfile.c +2
-0+* +* TODO replace page with hwpoison entry in pte and shmem. Instead of blindly going ahead and inserting ptes pointing to the !PageUptodate page,
unuse_pte()andshmem_unuse_inode()should insert a substitute bad swapentry, to generate SIGBUS if it's accessed.
Quoted 6 lines
swp_entry(1, 0)might serve, but there's probably a few mods needed here and there; and getting the details right (e.g. memcg charges) will need care.Not as straightforward as your block below, I admit. I wonder if you posted that just to stir me to do better: or can you take it further?
I found this patch in our kernel tree. For some reason it wasn't sent to mainline. So I decided to send it as is to not lose it for a few more years. Using here hwpoison was just a guess. Your bad-swap-entry is much more accurate solution. Seems like here is no rush, this bug was here from the beginning, so I'll handle it. Thanks for your advice.
Quoted 16 lines
Thanks,
HughQuoted 12 lines
mm/swapfile.c +9
-0+*/ +if (unlikely (!PageUptodate (page))) { +unlock_page (page); +page_cache_release (page); + retval = -EIO ; +break ; + } + +/* * Remove all references to entry. */ swcount = *swap_map;