linux-kernel - Re: [PATCH] mm/damon: Validate if the pmd entry is present before accessing

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <c9cde43f-d169-0a65-e69a-7b77c75322fd@linux.alibaba.com>
Date:   Thu, 18 Aug 2022 10:39:54 +0800
From:   Baolin Wang <baolin.wang@...ux.alibaba.com>
To:     SeongJae Park <sj@...nel.org>
Cc:     akpm@...ux-foundation.org, damon@...ts.linux.dev,
        linux-mm@...ck.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] mm/damon: Validate if the pmd entry is present before
 accessing



On 8/18/2022 10:29 AM, SeongJae Park wrote:
> Hi Baolin,
> 
> On Thu, 18 Aug 2022 09:05:58 +0800 Baolin Wang <baolin.wang@...ux.alibaba.com> wrote:
> 
>>
>>
>> On 8/18/2022 12:09 AM, SeongJae Park wrote:
>>> Hi Baolin,
>>>
>>>
>>> Thank you always for your great patch!
>>>
>>> On Wed, 17 Aug 2022 14:21:12 +0800 Baolin Wang <baolin.wang@...ux.alibaba.com> wrote:
>>>
>>>> The pmd_huge() is used to validate if the pmd entry is mapped by a huge
>>>> page, also including the case of non-present (migration or hwpoisoned)
>>>> pmd entry on arm64 or x86 architectures. Thus we should validate if it
>>>> is present before making the pmd entry old or getting young state,
>>>> otherwise we can not get the correct corresponding page.
>>>
>>> Maybe I'm missing something, but... I'm unsure if the page is present or not
>>> really matters from the perspective of access checking.  In the case, DAMON
>>> could simply report the page has accessed once for the first check after the
>>> page being non-present if it really accessed before, and then report the page
>>> as not accessed, which is true.
>>
>> Yes, that's the patch's goal to make the accesses correct. However if
>> the PMD entry is not present, we can not get the correct page object by
>> pmd_pfn(*pmd), since the non-present pmd entry will contain swap type
>> and swap offset with below format on ARM64, that means the pfn number is
>> saved in bits 8-57 in a migration or poisoned entry, but pmd_pfn() still
>> treat bits 12-47 as the pfn number on ARM64, which may get an incorrect
>> page struct (also maybe is NULL by pfn_to_online_page()) to make the
>> access statistics incorrect.
>>
>> /*
>>    * Encode and decode a swap entry:
>>    *	bits 0-1:	present (must be zero)
>>    *	bits 2:		remember PG_anon_exclusive
>>    *	bits 3-7:	swap type
>>    *	bits 8-57:	swap offset
>>    *	bit  58:	PTE_PROT_NONE (must be zero)
>>    */
>>
>>
>> Moreoever I don't think we should still waste time to get the page of
>> the non-present entry, just treat it as not-accessed and skip it, that
>> keeps consistent with non-present pte level entry.
>>
>> Does that make sense for you? Thanks.
> 
> Yes, that totally makes sense.  Thank you very much for the kind answer.  I
> think it would be great if we could put the detailed explanation in the commit
> message.  Could you please update the commit message and post v2 of the patch?

Sure, will update the commit message to make it more clear and I think 
that can also answer Andrew's concern.

> 
> Reviewed-by: SeongJae Park <sj@...nel.org>

Thanks.