[PATCH] mm/swap: abort swapoff after disk error

[PATCH] mm/swap: abort swapoff after disk error

[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.
Probably we can reuse hwpoison entries here, but tmpfs already too complex.

Signed-off-by: Konstantin Khlebnikov <khlebnikov@openvz.org>
Original-patch-by: Alexey Kuznetsov <kuznet@ms2.inr.ac.ru>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Hugh Dickins <hughd@google.com>
Cc: Andi Kleen <andi@firstfloor.org>
---
 mm/swapfile.c |   16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/mm/swapfile.c b/mm/swapfile.c
index e97a0e5..98fc2fd 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -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;

Re: [PATCH] mm/swap: abort swapoff after disk error

[Hugh Dickins]

On Fri, 14 Dec 2012, Konstantin Khlebnikov wrote:

> 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.

> 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.

> 
> Signed-off-by: Konstantin Khlebnikov <khlebnikov@openvz.org>
> Original-patch-by: Alexey Kuznetsov <kuznet@ms2.inr.ac.ru>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Hugh Dickins <hughd@google.com>
> Cc: Andi Kleen <andi@firstfloor.org>
> ---
>  mm/swapfile.c |   16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index e97a0e5..98fc2fd 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -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.


> +		 * 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.

> +		 *
> +		 * 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

> +		 */
> +		if (unlikely(!PageUptodate(page))) {
> +			unlock_page(page);
> +			page_cache_release(page);
> +			retval = -EIO;
> +			break;
> +		}
> +
> +		/*
>  		 * Remove all references to entry.
>  		 */
>  		swcount = *swap_map;

Re: [PATCH] mm/swap: abort swapoff after disk error

[Konstantin Khlebnikov]

Hugh Dickins wrote:
> On Fri, 14 Dec 2012, Konstantin Khlebnikov wrote:
>
>> 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.
>
>> 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.
>
>>
>> Signed-off-by: Konstantin Khlebnikov<khlebnikov@openvz.org>
>> Original-patch-by: Alexey Kuznetsov<kuznet@ms2.inr.ac.ru>
>> Cc: Andrew Morton<akpm@linux-foundation.org>
>> Cc: Hugh Dickins<hughd@google.com>
>> Cc: Andi Kleen<andi@firstfloor.org>
>> ---
>>   mm/swapfile.c |   16 ++++++++++++++++
>>   1 file changed, 16 insertions(+)
>>
>> diff --git a/mm/swapfile.c b/mm/swapfile.c
>> index e97a0e5..98fc2fd 100644
>> --- a/mm/swapfile.c
>> +++ b/mm/swapfile.c
>> @@ -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.
>
>
>> +		 * 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.
>
>> +		 *
>> +		 * 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?

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.

>
> Thanks,
> Hugh
>
>> +		 */
>> +		if (unlikely(!PageUptodate(page))) {
>> +			unlock_page(page);
>> +			page_cache_release(page);
>> +			retval = -EIO;
>> +			break;
>> +		}
>> +
>> +		/*
>>   		 * Remove all references to entry.
>>   		 */
>>   		swcount = *swap_map;