[bug report] arm64/mm: Clear PXX_TYPE_MASK and set PXD_TYPE_SECT in [pmd|pud]_mkhuge()

[bug report] arm64/mm: Clear PXX_TYPE_MASK and set PXD_TYPE_SECT in [pmd|pud]_mkhuge()

[Dan Carpenter]

Hello Anshuman Khandual,

Commit 1601df9e366e ("arm64/mm: Clear PXX_TYPE_MASK and set
PXD_TYPE_SECT in [pmd|pud]_mkhuge()") from Feb 21, 2025 (linux-next),
leads to the following (unpublished) Smatch static checker warning:

arch/arm64/include/asm/pgtable.h:587 pmd_mkhuge() warn: odd binop '0x1 & 0xfffffffffffffffe'
arch/arm64/include/asm/pgtable.h:626 pud_mkhuge() warn: odd binop '0x1 & 0xfffffffffffffffe'

arch/arm64/include/asm/pgtable.h
    579 static inline pmd_t pmd_mkhuge(pmd_t pmd)
    580 {
    581         /*
    582          * It's possible that the pmd is present-invalid on entry
    583          * and in that case it needs to remain present-invalid on
    584          * exit. So ensure the VALID bit does not get modified.
    585          */
    586         pmdval_t mask = PMD_TYPE_MASK & ~PTE_VALID;
--> 587         pmdval_t val = PMD_TYPE_SECT & ~PTE_VALID;

This is "1 & ~1".  I see the comment, but I'm too stupid to know even
after reading he comment whether it's intentional or not.  :P

    588 
    589         return __pmd((pmd_val(pmd) & ~mask) | val);
    590 }

regards,
dan carpenter

Re: [bug report] arm64/mm: Clear PXX_TYPE_MASK and set PXD_TYPE_SECT in [pmd|pud]_mkhuge()

[Catalin Marinas]

On Fri, Mar 14, 2025 at 01:54:01PM +0300, Dan Carpenter wrote:
> Hello Anshuman Khandual,
> 
> Commit 1601df9e366e ("arm64/mm: Clear PXX_TYPE_MASK and set
> PXD_TYPE_SECT in [pmd|pud]_mkhuge()") from Feb 21, 2025 (linux-next),
> leads to the following (unpublished) Smatch static checker warning:
> 
> arch/arm64/include/asm/pgtable.h:587 pmd_mkhuge() warn: odd binop '0x1 & 0xfffffffffffffffe'
> arch/arm64/include/asm/pgtable.h:626 pud_mkhuge() warn: odd binop '0x1 & 0xfffffffffffffffe'
> 
> arch/arm64/include/asm/pgtable.h
>     579 static inline pmd_t pmd_mkhuge(pmd_t pmd)
>     580 {
>     581         /*
>     582          * It's possible that the pmd is present-invalid on entry
>     583          * and in that case it needs to remain present-invalid on
>     584          * exit. So ensure the VALID bit does not get modified.
>     585          */
>     586         pmdval_t mask = PMD_TYPE_MASK & ~PTE_VALID;
> --> 587         pmdval_t val = PMD_TYPE_SECT & ~PTE_VALID;
> 
> This is "1 & ~1".  I see the comment, but I'm too stupid to know even
> after reading he comment whether it's intentional or not.  :P
> 
>     588 
>     589         return __pmd((pmd_val(pmd) & ~mask) | val);
>     590 }

Prior to the above commit, the code was:

#define pmd_mkhuge(pmd)			(__pmd(pmd_val(pmd) & ~PMD_TABLE_BIT))

So just clearing a bit. That's why the reworked code has val 0 in the
current configuration. This may change with support for 128-bit PTEs
though, different format.

The comment is about preserving the PTE_VALID bit in the original pmd
value, so it's masked out of both mask and val.

It's good that smatch catches these though. Any way to mark a false
positive?

-- 
Catalin

Re: [bug report] arm64/mm: Clear PXX_TYPE_MASK and set PXD_TYPE_SECT in [pmd|pud]_mkhuge()

[Dan Carpenter]

On Fri, Mar 14, 2025 at 05:52:04PM +0000, Catalin Marinas wrote:
> On Fri, Mar 14, 2025 at 01:54:01PM +0300, Dan Carpenter wrote:
> > Hello Anshuman Khandual,
> > 
> > Commit 1601df9e366e ("arm64/mm: Clear PXX_TYPE_MASK and set
> > PXD_TYPE_SECT in [pmd|pud]_mkhuge()") from Feb 21, 2025 (linux-next),
> > leads to the following (unpublished) Smatch static checker warning:
> > 
> > arch/arm64/include/asm/pgtable.h:587 pmd_mkhuge() warn: odd binop '0x1 & 0xfffffffffffffffe'
> > arch/arm64/include/asm/pgtable.h:626 pud_mkhuge() warn: odd binop '0x1 & 0xfffffffffffffffe'
> > 
> > arch/arm64/include/asm/pgtable.h
> >     579 static inline pmd_t pmd_mkhuge(pmd_t pmd)
> >     580 {
> >     581         /*
> >     582          * It's possible that the pmd is present-invalid on entry
> >     583          * and in that case it needs to remain present-invalid on
> >     584          * exit. So ensure the VALID bit does not get modified.
> >     585          */
> >     586         pmdval_t mask = PMD_TYPE_MASK & ~PTE_VALID;
> > --> 587         pmdval_t val = PMD_TYPE_SECT & ~PTE_VALID;
> > 
> > This is "1 & ~1".  I see the comment, but I'm too stupid to know even
> > after reading he comment whether it's intentional or not.  :P
> > 
> >     588 
> >     589         return __pmd((pmd_val(pmd) & ~mask) | val);
> >     590 }
> 
> Prior to the above commit, the code was:
> 
> #define pmd_mkhuge(pmd)			(__pmd(pmd_val(pmd) & ~PMD_TABLE_BIT))
> 
> So just clearing a bit. That's why the reworked code has val 0 in the
> current configuration. This may change with support for 128-bit PTEs
> though, different format.
> 
> The comment is about preserving the PTE_VALID bit in the original pmd
> value, so it's masked out of both mask and val.
> 
> It's good that smatch catches these though. Any way to mark a false
> positive?

In the kernel, all old warnings are false positives.  We're good at
fixing things.  People can look it up on lore and find this thread
if they have questions.

regards,
dan carpenter