Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <0fa49d1f-a05d-bdb1-df60-800ff5999d8e@redhat.com>
Date:   Tue, 3 May 2022 20:35:59 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.8.0
Subject: Re: [PATCH] mm/mprotect: reduce Committed_AS if memory protection is
 changed to PROT_NONE
Content-Language: en-US
To:     Andrew Morton <akpm@linux-foundation.org>,
        liusongtang <liusongtang@huawei.com>
Cc:     linux-mm@kvack.org, linux-kernel@vger.kernel.org,
        nixiaoming@huawei.com, young.liuyang@huawei.com,
        wangkefeng@huawei.com
References: <20220426112705.3323-1-liusongtang@huawei.com>
 <20220426133401.9404190c5a5c6ff58b694c59@linux-foundation.org>
From:   David Hildenbrand <david@redhat.com>
Organization: Red Hat
In-Reply-To: <20220426133401.9404190c5a5c6ff58b694c59@linux-foundation.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 26.04.22 22:34, Andrew Morton wrote:
> On Tue, 26 Apr 2022 19:27:05 +0800 liusongtang <liusongtang@huawei.com> wrote:
> 
>> If PROT_WRITE is set, the size of vm area will be added to Committed_AS.
>> However, if memory protection is changed to PROT_NONE,
>> the corresponding physical memory will not be used, but Committed_AS still
>> count the size of the PROT_NONE memory.
>>
>> This patch reduce Committed_AS and free the corresponding memory if
>> memory protection is changed to PROT_NONE.
>>
>> ...
>>
>> --- a/mm/mprotect.c
>> +++ b/mm/mprotect.c
>> @@ -497,6 +497,12 @@ mprotect_fixup(struct vm_area_struct *vma, struct vm_area_struct **pprev,
>>  	}
>>  
>>  success:
>> +	if ((newflags & (VM_READ | VM_WRITE | VM_EXEC | VM_LOCKED | VM_ACCOUNT)) == VM_ACCOUNT) {
>> +		zap_page_range(vma, start, end - start);
>> +		newflags &= ~VM_ACCOUNT;
>> +		vm_unacct_memory((end - start) >> PAGE_SHIFT);
>> +	}
>> +
>>  	/*
>>  	 * vm_flags and vm_page_prot are protected by the mmap_lock
>>  	 * held in write mode.
> 
> Surprised.  If userspace does mprotect(addr, len.  PROT_NONE) then
> mprotect(addr, len.  PROT_READ), what is now at *addr?  Zeroes?
> 

I don't think so. I don't see any pages getting zapped at my quick test
(unless it's wrong) shows that data is maintained. Further, it could
violate POSIX semantics.

So this patch is wrong, there might have been anonymous pages populated.

-- 
Thanks,

David / dhildenb