linux-kernel - Re: [PATCH v2] mm/compaction: remove unnecessary detection code.

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

Message-ID: <bd962c73-5362-479f-9e03-6fbda4e3727e@linux.alibaba.com>
Date: Thu, 14 Nov 2024 18:06:18 +0800
From: Baolin Wang <baolin.wang@...ux.alibaba.com>
To: Vlastimil Babka <vbabka@...e.cz>, Qiang Liu <liuq131@...natelecom.cn>,
 Kemeng Shi <shikemeng@...weicloud.com>
Cc: akpm@...ux-foundation.org, linux-mm@...ck.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] mm/compaction: remove unnecessary detection code.



On 2024/11/14 15:52, Vlastimil Babka wrote:
> On 11/14/24 08:44, Vlastimil Babka wrote:
>> On 11/14/24 07:57, Qiang Liu wrote:
>>> It is impossible for the situation where blockpfn > end_pfn to arise,
>>> The if statement here is not only unnecessary, but may also lead to
>>> a misunderstanding that blockpfn > end_pfn could potentially happen.
>>> so these unnecessary checking code should be removed.
>>>
>>> Signed-off-by: Qiang Liu <liuq131@...natelecom.cn>
>>
>> I see that's since 3da0272a4c7d ("mm/compaction: correctly return failure
>> with bogus compound_order in strict mode")
> 
> Hm but we still have:
> 
> for (; blockpfn < end_pfn; blockpfn += stride, page += stride) {
> 
> and this advance by stride can mix up with advance by isolated, initial pfn
> might not be aligned... I don't see any guarantee that the for loop will
> exit with exactly blockpfn == end_pfn, it may easily advance beyond end_pfn
> so we shouldn't remove the check?

Agreed.

>> I think that commit introduced a risk of overflow due to a bogus order
>> (which we read in a racy way), and once blockpfn overflows it will satisfy
>> <= end_pfn and might e.g. end up scanning a completely different zone?
>>
>>                          if (blockpfn + (1UL << order) <= end_pfn) {
>>
>>                                  blockpfn += (1UL << order) - 1;
>>                                  page += (1UL << order) - 1;
>>                                  nr_scanned += (1UL << order) - 1;
>>                          }
>>
>> We should better add back the MAX_ORDER sanity check?
>>
>>> ---
>>>   mm/compaction.c | 6 ------
>>>   1 file changed, 6 deletions(-)
>>>
>>> diff --git a/mm/compaction.c b/mm/compaction.c
>>> index a2b16b08cbbf..baeda7132252 100644
>>> --- a/mm/compaction.c
>>> +++ b/mm/compaction.c
>>> @@ -682,12 +682,6 @@ static unsigned long isolate_freepages_block(struct compact_control *cc,
>>>   	if (locked)
>>>   		spin_unlock_irqrestore(&cc->zone->lock, flags);
>>>   
>>> -	/*
>>> -	 * Be careful to not go outside of the pageblock.
>>> -	 */
>>> -	if (unlikely(blockpfn > end_pfn))
>>> -		blockpfn = end_pfn;
>>> -
>>>   	trace_mm_compaction_isolate_freepages(*start_pfn, blockpfn,
>>>   					nr_scanned, total_isolated);
>>>   
>>
>>