linux-kernel - Re: [PATCH] mm/compaction: fix the total

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-next>] [day] [month] [year] [list]

Message-Id: <2503b955-79a9-4d21-9a25-34a6c33e688d@linux.alibaba.com>
Date: Thu, 14 Nov 2024 10:10:44 +0800
From: Qiang Liu <liuq131@...natelecom.cn>
To: liuq131@...natelecom.cn,
	akpm <akpm@...ux-foundation.org>
Cc: linux-mm <linux-mm@...ck.org>,
	linux-kernel <linux-kernel@...r.kernel.org>,
	Baolin Wang <baolin.wang@...ux.alibaba.com>
Subject: Re: [PATCH] mm/compaction: fix the total_isolated in strict mode

From: Baolin Wang <baolin.wang@...ux.alibaba.com>


On 2024/11/12  17:47, baolin.wang@...ux.alibaba.com wrote:
>On 2024/11/12 10:16, liuq131@...natelecom.cn wrote:
>> "We assume that the block we are currently processing is distributed as follows:
>> 0   1   2                                                            511
>> --------------------------------------------------
>> |    |    |                                                              |
>> ---------------------------------------------------
>> Index 0 and 1 are both pages with an order of 0.
>> Index 2 has a bogus order (let's assume the order is 9).
>> When the for loop reaches index 2, it will enter the following code:
>> /*
>>   * For compound pages such as THP and hugetlbfs, we can save
>>   * potentially a lot of iterations if we skip them at once.
>>   * The check is racy, but we can consider only valid values
>>   * and the only danger is skipping too much.
>>   */
>> if (PageCompound(page)) {
>>      const unsigned int order = compound_order(page);
>>      if (blockpfn + (1UL << order) <= end_pfn) {
>>          blockpfn += (1UL << order) - 1;
>>          page += (1UL << order) - 1;
>>          nr_scanned += (1UL << order) - 1;
>>      }
>>      goto isolate_fail;
>> }
>> 
>> After exiting the for loop:
>> blockpfn =basepfn+ 2+2^9 = basepfn+514
>> endpfn  = basepfn +512
>> total_isolated = 2
>> nr_scanned = 514
>
>In your case, the 'blockpfn' will not be updated to 'basepfn+514', 
>because 'blockpfn + (1UL << order) > end_pfn', right? And remember the 
>'end_pfn' is the end of the pageblock.
>
>So I'm still confused about your case. Is this from code inspection?
You're right, the situation where blockpfn > end_pfn would not actually occur here.
I encountered this issue in the 4.19 kernel, which did not have this check.
I didn't carefully examine this scenario later. Sorry about that.

However, when blockpfn == end_pfn, I believe the patch is still applicable,
but the git log needs to be updated. Is there still an opportunity to submit
a revised version of the patch?
>> /*
>> * Be careful to not go outside of the pageblock.
>> */
>> if (unlikely(blockpfn > end_pfn))
>> blockpfn = end_pfn;
>>   
>> So this can happen
>> 
>> /*
>>   * If strict isolation is requested by CMA then check that all the
>>   * pages requested were isolated. If there were any failures, 0 is
>>   * returned and CMA will fail.
>>   */
>> if (strict && blockpfn < end_pfn)
>> total_isolated = 0;
>> 
>> If processed according to the old code, it will not enter the if statement to reset total_isolated, but the correct handling is to reset total_isolated to 0.
>
>Please do not top-posting:
>
>"
>- Use interleaved ("inline") replies, which makes your response easier 
>to read. (i.e. avoid top-posting -- the practice of putting your answer 
>above the quoted text you are responding to.) For more details, see
>   :ref:`Documentation/process/submitting-patches.rst 
><interleaved_replies>`.
>"
>