Re: [Devel] [PATCH hci-8.1] ms/page_pool: Fix PP_MAGIC_MASK to avoid crashing on some 32-bit arches

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] mm: PP_DMA_INDEX_MASK uses an inverted GENMASK range when PP_DMA_INDEX_BITS folds to 0
--

commit 430b5a3ba30b9045b484a6671db44819d46c0d9d
Author: Eva Kurchatova <eva.kurchatova@virtuozzo.com>

ms/page_pool: Fix PP_MAGIC_MASK to avoid crashing on some 32-bit arches

This backport changes PP_DMA_INDEX_BITS so it is derived from the lowest
set bit of PAGE_OFFSET. When PAGE_OFFSET is not a usable compile-time
constant, the bit count falls back to 0 and DMA index tracking is turned
off. The DMA index register and release code in page_pool.c is moved into
helper functions.

Link: https://lore.kernel.org/all/aMNJMFa5fDalFmtn@p100/
Link: https://patch.msgid.link/20250930114331.675412-1-toke@redhat.com

> diff --git a/include/linux/mm.h b/include/linux/mm.h
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h

[ ... ]

>  #define PP_DMA_INDEX_SHIFT (1 + __fls(PP_SIGNATURE - POISON_POINTER_DELTA))
>  #if POISON_POINTER_DELTA > 0

[ ... ]

>  #else
> -/* Always leave out the topmost two; see above. */
> -#define PP_DMA_INDEX_BITS MIN(32, BITS_PER_LONG - PP_DMA_INDEX_SHIFT - 2)
> +/* Use the lowest bit of PAGE_OFFSET if there's at least 8 bits available; see above */
> +#define PP_DMA_INDEX_MIN_OFFSET (1 << (PP_DMA_INDEX_SHIFT + 8))
> +#define PP_DMA_INDEX_BITS ((__builtin_constant_p(PAGE_OFFSET) && \
> +			    PAGE_OFFSET >= PP_DMA_INDEX_MIN_OFFSET && \
> +			    !(PAGE_OFFSET & (PP_DMA_INDEX_MIN_OFFSET - 1))) ? \
> +			      MIN(32, __ffs(PAGE_OFFSET) - PP_DMA_INDEX_SHIFT) : 0)
> +
>  #endif
>  
>  #define PP_DMA_INDEX_MASK GENMASK(PP_DMA_INDEX_BITS + PP_DMA_INDEX_SHIFT - 1, \

[Severity: Medium]
Can PP_DMA_INDEX_MASK break the build when PP_DMA_INDEX_BITS folds to 0?

PP_DMA_INDEX_MASK is unchanged, so with zero bits it expands to:

    GENMASK(0 + PP_DMA_INDEX_SHIFT - 1, PP_DMA_INDEX_SHIFT)

That makes h = SHIFT-1 and l = SHIFT, which is an inverted range.

GENMASK() is defined as:

    GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l)

GENMASK_INPUT_CHECK() is BUILD_BUG_ON_ZERO(const_true((l) > (h))), and
const_true() is true whenever the expression is an integer constant
expression.

On architectures where PAGE_OFFSET is a compile-time constant that is
too small or has low bits set, every operand of the ternary in
PP_DMA_INDEX_BITS is constant. Two examples are MIPS Malta, which has
"#define PAGE_OFFSET _AC(0x0, UL)", and m68k, which has
"PAGE_OFFSET_RAW 0x00000000". NOMMU configs that take PAGE_OFFSET from
CONFIG_ values may also hit this.

In those cases the bit count should fold to a constant 0, and "l > h"
would then be a constant true. BUILD_BUG_ON_ZERO would fire in every
translation unit that expands PP_MAGIC_MASK or PP_DMA_INDEX_MASK. That
includes net/core/page_pool.c, the netmem_priv.h users, and
page_pool_page_is_pp() under CONFIG_PAGE_POOL.

The UML case described in the rebase note is not affected, because
PAGE_OFFSET is a variable there. __builtin_constant_p() is not an ICE
in that case, so const_true() evaluates to false and the runtime mask is
0.

I could not run a compiler, so whether GCC treats this ternary as an
integer constant expression is reasoned from the code and not observed.

The new comment and the commit message describe 0 bits as the general
fallback, but the mask definition was not updated to handle it. Should
PP_DMA_INDEX_MASK be defined as 0 when PP_DMA_INDEX_BITS is 0, or use a
form that tolerates an empty range?

The upstream commit 95920c2ed02b defines the mask the same way, so this
is not specific to the backport.

[ ... ]

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261021225127.2383437-1-eva.kurchatova@virtuozzo.com?part=1

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel