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

[Liu Shixin <liushixin2@xxxxxxxxxx>]

On 2025/1/24 14:20, Andrew Morton wrote:

On Thu, 23 Jan 2025 10:10:29 +0800 Liu Shixin <liushixin2@xxxxxxxxxx> 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_order is 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.
.