[PATCH] mm/compaction: fix UBSAN shift-out-of-bounds warning

[PATCH] mm/compaction: fix UBSAN shift-out-of-bounds warning

[Liu Shixin]

syzkaller reported a UBSAN shift-out-of-bounds warning of (1UL << order)
in isolate_freepages_block(). The bogus compound_order can be any value
because it is union with flags. Add back the MAX_PAGE_ORDER check to fix
the warning.

Fixes: 3da0272a4c7d ("mm/compaction: correctly return failure with bogus compound_order in strict mode")
Signed-off-by: Liu Shixin <liushixin2@huawei.com>
---
 mm/compaction.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/mm/compaction.c b/mm/compaction.c
index a2b16b08cbbff..384e4672998e5 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -630,7 +630,8 @@ static unsigned long isolate_freepages_block(struct compact_control *cc,
 		if (PageCompound(page)) {
 			const unsigned int order = compound_order(page);
 
-			if (blockpfn + (1UL << order) <= end_pfn) {
+			if ((order <= MAX_PAGE_ORDER) &&
+			    (blockpfn + (1UL << order) <= end_pfn)) {
 				blockpfn += (1UL << order) - 1;
 				page += (1UL << order) - 1;
 				nr_scanned += (1UL << order) - 1;
-- 
2.34.1

Re: [PATCH] mm/compaction: fix UBSAN shift-out-of-bounds warning

[Kemeng Shi]

on 1/23/2025 10:10 AM, Liu Shixin wrote:
> syzkaller reported a UBSAN shift-out-of-bounds warning of (1UL << order)
> in isolate_freepages_block(). The bogus compound_order can be any value
> because it is union with flags. Add back the MAX_PAGE_ORDER check to fix
> the warning.
> 
> Fixes: 3da0272a4c7d ("mm/compaction: correctly return failure with bogus compound_order in strict mode")
> Signed-off-by: Liu Shixin <liushixin2@huawei.com>
> ---
>  mm/compaction.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/compaction.c b/mm/compaction.c
> index a2b16b08cbbff..384e4672998e5 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -630,7 +630,8 @@ static unsigned long isolate_freepages_block(struct compact_control *cc,
>  		if (PageCompound(page)) {
>  			const unsigned int order = compound_order(page);
>  
> -			if (blockpfn + (1UL << order) <= end_pfn) {
> +			if ((order <= MAX_PAGE_ORDER) &&
> +			    (blockpfn + (1UL << order) <= end_pfn)) {
>  				blockpfn += (1UL << order) - 1;
>  				page += (1UL << order) - 1;
>  				nr_scanned += (1UL << order) - 1;
> 
Look good to me, feel free to add
Reviewed-by: Kemeng Shi <shikemeng@huaweicloud.com>

Re: [PATCH] mm/compaction: fix UBSAN shift-out-of-bounds warning

[David Hildenbrand]

On 23.01.25 03:10, Liu Shixin wrote:
> syzkaller reported a UBSAN shift-out-of-bounds warning of (1UL << order)
> in isolate_freepages_block(). The bogus compound_order can be any value
> because it is union with flags. Add back the MAX_PAGE_ORDER check to fix
> the warning.
> 
> Fixes: 3da0272a4c7d ("mm/compaction: correctly return failure with bogus compound_order in strict mode")
> Signed-off-by: Liu Shixin <liushixin2@huawei.com>
> ---
>   mm/compaction.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/compaction.c b/mm/compaction.c
> index a2b16b08cbbff..384e4672998e5 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -630,7 +630,8 @@ static unsigned long isolate_freepages_block(struct compact_control *cc,
>   		if (PageCompound(page)) {
>   			const unsigned int order = compound_order(page);
>   
> -			if (blockpfn + (1UL << order) <= end_pfn) {
> +			if ((order <= MAX_PAGE_ORDER) &&

hugetlb can exceed MAX_PAGE_ORDER, but end_pfn cannot. So I think what 
you have here will not change the exiting handling.

Acked-by: David Hildenbrand <david@redhat.com>


Note: the whole "-1" with stride != 1 looks odd ...

-- 
Cheers,

David / dhildenb

Re: [PATCH] mm/compaction: fix UBSAN shift-out-of-bounds warning

[Oscar Salvador]

On Thu, Jan 23, 2025 at 10:10:29AM +0800, Liu Shixin wrote:
> syzkaller reported a UBSAN shift-out-of-bounds warning of (1UL << order)
> in isolate_freepages_block(). The bogus compound_order can be any value
> because it is union with flags. Add back the MAX_PAGE_ORDER check to fix
> the warning.
> 
> Fixes: 3da0272a4c7d ("mm/compaction: correctly return failure with bogus compound_order in strict mode")
> Signed-off-by: Liu Shixin <liushixin2@huawei.com>

Reviewed-by: Oscar Salvador <osalvador@suse.de>


-- 
Oscar Salvador
SUSE Labs

Re: [PATCH] mm/compaction: fix UBSAN shift-out-of-bounds warning

[Andrew Morton]

On Thu, 23 Jan 2025 10:10:29 +0800 Liu Shixin <liushixin2@huawei.com> wrote:

> syzkaller reported a UBSAN shift-out-of-bounds warning of (1UL << order)

A Link: to the syzcaller report would be great, please.

> in isolate_freepages_block(). The bogus compound_order can be any value
> because it is union with flags. Add back the MAX_PAGE_ORDER check to fix
> the warning.

OK, I'd never noticed compound_order()'s restrictions before.  It looks
like a crazy thing - what use is it if it can return "wild return
values"?

Can someone please explain what's going on here and suggest what we can
do about it?

For example, should we have a compound_order_not_wild() which is called
with refcounted pages and which cannot return "wild" numbers?  Or
something else.

> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -630,7 +630,8 @@ static unsigned long isolate_freepages_block(struct compact_control *cc,
>  		if (PageCompound(page)) {
>  			const unsigned int order = compound_order(page);
>  
> -			if (blockpfn + (1UL << order) <= end_pfn) {
> +			if ((order <= MAX_PAGE_ORDER) &&
> +			    (blockpfn + (1UL << order) <= end_pfn)) {
>  				blockpfn += (1UL << order) - 1;
>  				page += (1UL << order) - 1;
>  				nr_scanned += (1UL << order) - 1;

isolate_migratepages_block()'s

		if (skip_isolation_on_order(order, cc->order)) {

doesn't check for "wild" values, but it seems that
skip_isolation_on_order() will handle it.

Re: [PATCH] mm/compaction: fix UBSAN shift-out-of-bounds warning

[David Hildenbrand]

On 24.01.25 07:20, Andrew Morton wrote:
> On Thu, 23 Jan 2025 10:10:29 +0800 Liu Shixin <liushixin2@huawei.com> wrote:
> 
>> syzkaller reported a UBSAN shift-out-of-bounds warning of (1UL << order)
> 
> A Link: to the syzcaller report would be great, please.
> 

Hi,

>> in isolate_freepages_block(). The bogus compound_order can be any value
>> because it is union with flags. Add back the MAX_PAGE_ORDER check to fix
>> the warning.
> 
> OK, I'd never noticed compound_order()'s restrictions before.  It looks
> like a crazy thing - what use is it if it can return "wild return
> values"?

It's perfectly fine to call if we hold a folio reference. There is some code that
wants to avoid that, so we might see the page concurrently get freed and
a compound page dissolved.

Similar to doing a folio_test_large() or folio_nr_pages() etc. without a
folio reference.

> 
> Can someone please explain what's going on here and suggest what we can
> do about it?

Note the comment:

		/*
		 * 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.
		 */

So there is not really anything going wrong. Racy access can result in
skipping too much.

The UBSAN warning is not anything critical.

> 
> For example, should we have a compound_order_not_wild() which is called
> with refcounted pages and which cannot return "wild" numbers?  Or
> something else.

We only have a handful of these racy usages.

Observe another one with a similar MAX_PAGE_ORDER check:

			/*
			 * skip hugetlbfs if we are not compacting for pages
			 * bigger than its order. THPs and other compound pages
			 * are handled below.
			 */
			if (!cc->alloc_contig) {
				const unsigned int order = compound_order(page);

				if (order <= MAX_PAGE_ORDER) {
					low_pfn += (1UL << order) - 1;
					nr_scanned += (1UL << order) - 1;
				}
				goto isolate_fail;
			}


So the common patter is on this racy access in code that operates on pageblock granularity
is to check against MAX_PAGE_ORDER before advancing based on racily obtained value.


Note that we do have folios that have order > MAX_PAGE_ORDER,
it's rather that *this code* only works in pageblock chunks, so <= MAX_PAGE_ORDER is
all it needs to advance.

So having a compact_racy_compound_order() in this file and replacing all instances
here where we sanitize against MAX_PAGE_ORDER might be reasonable, because page
compaction works on pageblocks (which are <= MAX_PAGE_ORDER).

-- 
Cheers,

David / dhildenb

Re: [PATCH] mm/compaction: fix UBSAN shift-out-of-bounds warning

[Baolin Wang]

On 2025/1/23 10:10, Liu Shixin wrote:
> syzkaller reported a UBSAN shift-out-of-bounds warning of (1UL << order)
> in isolate_freepages_block(). The bogus compound_order can be any value
> because it is union with flags. Add back the MAX_PAGE_ORDER check to fix
> the warning.
> 
> Fixes: 3da0272a4c7d ("mm/compaction: correctly return failure with bogus compound_order in strict mode")
> Signed-off-by: Liu Shixin <liushixin2@huawei.com>
> ---

LGTM. And I remember Vlastimil had the same concern before[1].
Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>

[1] 
https://lore.kernel.org/all/6b055821-ce14-4a6d-959c-25ade4a9bfd7@suse.cz/

>   mm/compaction.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/compaction.c b/mm/compaction.c
> index a2b16b08cbbff..384e4672998e5 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -630,7 +630,8 @@ static unsigned long isolate_freepages_block(struct compact_control *cc,
>   		if (PageCompound(page)) {
>   			const unsigned int order = compound_order(page);
>   
> -			if (blockpfn + (1UL << order) <= end_pfn) {
> +			if ((order <= MAX_PAGE_ORDER) &&
> +			    (blockpfn + (1UL << order) <= end_pfn)) {
>   				blockpfn += (1UL << order) - 1;
>   				page += (1UL << order) - 1;
>   				nr_scanned += (1UL << order) - 1;

Re: [PATCH] mm/compaction: fix UBSAN shift-out-of-bounds warning

[Liu Shixin]

[-- Attachment #1: Type: text/plain, Size: 1852 bytes --]



On 2025/1/24 14:20, Andrew Morton wrote:
> On Thu, 23 Jan 2025 10:10:29 +0800 Liu Shixin <liushixin2@huawei.com> wrote:
>
>> syzkaller reported a UBSAN shift-out-of-bounds warning of (1UL << order)
> A Link: to the syzcaller report would be great, please.
The warning arises only once in another version instead of mainline, and the syzkaller log
not reproduce. In that version, compound_orderis still union with lru instead of flags,
so I didn't put it in the log.
See enum pageflags, we can know that the warning need folio setting PG_waiters flags,
which is low probability.
>
>> in isolate_freepages_block(). The bogus compound_order can be any value
>> because it is union with flags. Add back the MAX_PAGE_ORDER check to fix
>> the warning.
> OK, I'd never noticed compound_order()'s restrictions before.  It looks
> like a crazy thing - what use is it if it can return "wild return
> values"?
>
> Can someone please explain what's going on here and suggest what we can
> do about it?
>
> For example, should we have a compound_order_not_wild() which is called
> with refcounted pages and which cannot return "wild" numbers?  Or
> something else.
>
>> --- a/mm/compaction.c
>> +++ b/mm/compaction.c
>> @@ -630,7 +630,8 @@ static unsigned long isolate_freepages_block(struct compact_control *cc,
>>  		if (PageCompound(page)) {
>>  			const unsigned int order = compound_order(page);
>>  
>> -			if (blockpfn + (1UL << order) <= end_pfn) {
>> +			if ((order <= MAX_PAGE_ORDER) &&
>> +			    (blockpfn + (1UL << order) <= end_pfn)) {
>>  				blockpfn += (1UL << order) - 1;
>>  				page += (1UL << order) - 1;
>>  				nr_scanned += (1UL << order) - 1;
> isolate_migratepages_block()'s
>
> 		if (skip_isolation_on_order(order, cc->order)) {
>
> doesn't check for "wild" values, but it seems that
> skip_isolation_on_order() will handle it.
> .
>


[-- Attachment #2: Type: text/html, Size: 4232 bytes --]