Re: [PATCH v2 1/3] mm/swapfile: make security_vm_enough_memory_mm() work as expected

[Huang, Ying]

Miaohe Lin <linmiaohe@xxxxxxxxxx> writes:

> On 2022/6/21 9:35, Huang, Ying wrote:
>> Miaohe Lin <linmiaohe@xxxxxxxxxx> writes:
>> 
>>> On 2022/6/20 15:31, Huang, Ying wrote:
>>>> Miaohe Lin <linmiaohe@xxxxxxxxxx> writes:
>>>>
>>>>> security_vm_enough_memory_mm() checks whether a process has enough memory
>>>>> to allocate a new virtual mapping. And total_swap_pages is considered as
>>>>> available memory while swapoff tries to make sure there's enough memory
>>>>> that can hold the swapped out memory. But total_swap_pages contains the
>>>>> swap space that is being swapoff. So security_vm_enough_memory_mm() will
>>>>> success even if there's no memory to hold the swapped out memory because
>>>>> total_swap_pages always greater than or equal to p->pages.
>>>>
>>>> Per my understanding, swapoff will not allocate virtual mapping by
>>>> itself.  But after swapoff, the overcommit limit could be exceeded.
>>>> security_vm_enough_memory_mm() is used to check that.  For example, in a
>>>> system with 4GB memory and 8GB swap, and 10GB is in use,
>>>>
>>>> CommitLimit:    4+8 = 12GB
>>>> Committed_AS:   10GB
>>>>
>>>> security_vm_enough_memory_mm() in swapoff() will fail because
>>>> 10+8 = 18 > 12.  This is expected because after swapoff, the overcommit
>>>> limit will be exceeded.
>>>>
>>>> If 3GB is in use,
>>>>
>>>> CommitLimit:    4+8 = 12GB
>>>> Committed_AS:   3GB
>>>>
>>>> security_vm_enough_memory_mm() in swapoff() will succeed because
>>>> 3+8 = 11 < 12.  This is expected because after swapoff, the overcommit
>>>> limit will not be exceeded.
>>>
>>> In OVERCOMMIT_NEVER scene, I think you're right.
>>>
>>>>
>>>> So, what's the real problem of the original implementation?  Can you
>>>> show it with an example as above?
>>>
>>> In OVERCOMMIT_GUESS scene, in a system with 4GB memory and 8GB swap, and 10GB is in use,
>>> pages below is 8GB, totalram_pages() + total_swap_pages is 12GB, so swapoff() will succeed
>>> instead of expected failure because 8 < 12. The overcommit limit is always *ignored* in the
>>> below case.
>>>
>>> 	if (sysctl_overcommit_memory == OVERCOMMIT_GUESS) {
>>> 		if (pages > totalram_pages() + total_swap_pages)
>>> 			goto error;
>>> 		return 0;
>>> 	}
>>>
>>> Or am I miss something?
>> 
>> Per my understanding, with OVERCOMMIT_GUESS, the number of in-use pages
>> isn't checked at all.  The only restriction is that the size of the
>> virtual mapping created should be less than total RAM + total swap
>
> Do you mean the only restriction is that the size of the virtual mapping
> *created every time* should be less than total RAM + total swap pages but
> *total virtual mapping* is not limited in OVERCOMMIT_GUESS scene? If so,
> the current behavior should be sane and I will drop this patch.

Yes.  This is my understanding.

Best Regards,
Huang, Ying

> Thanks!
>
>> pages.  Because swapoff() will not create virtual mapping, so it's
>> expected that security_vm_enough_memory_mm() in swapoff() always
>> succeeds.
>> 
>> Best Regards,
>> Huang, Ying
>> 
>>>
>>> Thanks!
>>>
>>>>
>>>>> In order to fix it, p->pages should be retracted from total_swap_pages
>>>>> first and then check whether there's enough memory for inuse swap pages.
>>>>>
>>>>> Signed-off-by: Miaohe Lin <linmiaohe@xxxxxxxxxx>
>>>>
>>>> [snip]
>>>>
>>>> .
>>>>
>> 
>> .
>>