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

Message-ID: <6b055821-ce14-4a6d-959c-25ade4a9bfd7@suse.cz>
Date: Thu, 14 Nov 2024 10:37:01 +0100
From: Vlastimil Babka <vbabka@...e.cz>
To: Kemeng Shi <shikemeng@...weicloud.com>,
 Qiang Liu <liuq131@...natelecom.cn>, baolin.wang@...ux.alibaba.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 11/14/24 10:21, Kemeng Shi wrote:
> 
> Hello
> on 11/14/2024 3:44 PM, 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>
>> 
> As stride could 32, if isolate_freepages_range() is called with start_pfn not
> aligned with 32, we could bail out look with blockpfn > end_pfn in
> isolate_freepages_block(). Please correct if I miss something.
>> I see that's since 3da0272a4c7d ("mm/compaction: correctly return failure
>> with bogus compound_order in strict mode")
>> 
>> 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?
> As order of pageblock is <= MAX_ORDER, if bogus order is > MAX_ORDER, then
> blockpfn + (1UL << order) must be > end_pfn, I think the sanity check is
> not needed.
Hm I guess we could only overflow with blockpfn being initially >= 1UL << 63
and reading a bogus order of 63.
So it can't realistically happen.

> Thanks.
> Kemeng
>> 
>>> ---
>>>  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);
>>>  
>> 
>