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