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

["Eva Kurchatova (Virtuozzo)" <eva.kurchatova@virtuozzo.com>]

On 10/22/26 01:57, no-reply@sashiko.virtuozzo.com wrote:
> 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.
>
> [ ... ]

Checked, this is not a problem in practice and there isn't a follow-up 
upstream fix,

because it isn't needed. __GENMASK() folds to zero on its own in this case.

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