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

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

Posted by Liu Shixin 11 months ago

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

Posted by Baolin Wang 10 months, 4 weeks ago


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

Posted by Andrew Morton 10 months, 4 weeks ago

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

Posted by David Hildenbrand 10 months, 4 weeks ago

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

Posted by Oscar Salvador 11 months ago

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

Posted by David Hildenbrand 11 months ago

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

Posted by Kemeng Shi 11 months ago


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>