Re: [PATCH] mm/swapfile: unuse_pte can map random data if swap read fails

[Miaohe Lin <linmiaohe@huawei.com>]

On 2022/4/5 6:53, Andrew Morton wrote:
> On Fri, 1 Apr 2022 15:29:26 +0800 Miaohe Lin <linmiaohe@huawei.com> wrote:
> 
>> There is a bug in unuse_pte(): when swap page happens to be unreadable,
>> page filled with random data is mapped into user address space. The fix
>> is to check for PageUptodate and fail swapoff in case of error.
>>
>> ...
>>
>> --- a/mm/swapfile.c
>> +++ b/mm/swapfile.c
>> @@ -1795,6 +1795,10 @@ static int unuse_pte(struct vm_area_struct *vma, pmd_t *pmd,
>>  		ret = 0;
>>  		goto out;
>>  	}
>> +	if (unlikely(!PageUptodate(page))) {
>> +		ret = -EIO;
>> +		goto out;
>> +	}
>>  
>>  	dec_mm_counter(vma->vm_mm, MM_SWAPENTS);
>>  	inc_mm_counter(vma->vm_mm, MM_ANONPAGES);
> 
> Failing the swapoff after -EIO seems a bit rude.  The user ends up with
> a permanently mounted swap because a sector was bad?
> 

This is really unfortunate. :(

> That would be like failing truncate() or close() or umount after -EIO
> on a regular file.  Somewhat.
> 
> Can we do something better?  Such as shooting down the page anyway and
> permitting the swapoff to proceed?  Worst case, just leak the dang page
> with an apologetic message.
> .
> 

We must have a way to prevent user from accessing the wrong data. One way
is kept the page in the swap cache and kill the user when page is accessed.
But this will end up with a permanently mounted swap.
Another way I can figure out now is that we could set the page table entry
to some special swap entry, such as SWP_EIO like SWP_HWPOISON, we can thus
kill the user when page is accessed while swapoff can proceed. But this makes
the code more complicated... Any suggestions?

Many thanks!